Re: [PATCH 7/8] lguest: trivial guest block driver

[Jens Axboe]

On Mon, Feb 12 2007, Rusty Russell wrote:
> +static irqreturn_t lgb_irq(int irq, void *_bd)
> +{
> +	struct blockdev *bd = _bd;
> +	unsigned long flags;
> +
> +	if (!bd->req) {
> +		pr_debug("No work!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	if (!bd->lb_page->result) {
> +		pr_debug("No result!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	spin_lock_irqsave(&bd->lock, flags);
> +	end_request(bd->req, bd->lb_page->result == 1);
> +	bd->req = NULL;
> +	bd->dma.used_len = 0;
> +	blk_start_queue(bd->disk->queue);
> +	spin_unlock_irqrestore(&bd->lock, flags);
> +	return IRQ_HANDLED;
> +}

You are using the old-style end request handling. So while I generally
discourage use of end_request(), you seem to have a bigger problem here:

> +static unsigned int req_to_dma(struct request *req, struct lguest_dma *dma)
> +{
> +	unsigned int i = 0, idx, len = 0;
> +	struct bio *bio;
> +
> +	rq_for_each_bio(bio, req) {
> +		struct bio_vec *bvec;
> +		bio_for_each_segment(bvec, bio, idx) {
> +			BUG_ON(i == LGUEST_MAX_DMA_SECTIONS);
> +			BUG_ON(!bvec->bv_len);
> +			dma->addr[i] = page_to_phys(bvec->bv_page)
> +				+ bvec->bv_offset;
> +			dma->len[i] = bvec->bv_len;
> +			len += bvec->bv_len;
> +			i++;
> +		}
> +	}
> +	if (i < LGUEST_MAX_DMA_SECTIONS)
> +		dma->len[i] = 0;
> +	return len;
> +}

Here you map the entire request (lets call that segment A..Z), but
end_request() only completes the first chunk of the request. So
elv_next_request() will retrieve the same request again, and you'll then
map B..Z and repeat that transfer. So unless I'm missing some other part
here (just read it over quickly), you are re-doing large parts of a
merged request several times.

So: don't use end_request(). Add some driver helper that does:

static void lgb_end_request(struct blockdev *bd)
{
        int uptodate = bd->lb_page->result == 1;
        struct request *rq = bd->req;

        end_that_request_first(rq, uptodate, req->hard_nr_sectors);
        add_disk_randomness(rq->rq_disk);
        blkdev_dequeue_request(rq);
        end_that_request_last(rq, uptodate);
}

We could probably even make that a block layer helper, I'm sure others
could be cleaned up with that as well. You want to use that helper in
do_lgb_request() as well.

-- 
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/