On Mon, 2007-02-12 at 05:43 +0100, Jens Axboe wrote:
> On Mon, Feb 12 2007, Rusty Russell wrote:
> > + end_request(bd->req, bd->lb_page->result == 1);
>
> 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:
> > + 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.
I'm confused. That code looks like end_request:
void end_request(struct request *req, int uptodate)
{
if (!end_that_request_first(req, uptodate, req->hard_cur_sectors)) {
add_disk_randomness(req->rq_disk);
blkdev_dequeue_request(req);
end_that_request_last(req, uptodate);
}
}
Rusty.
