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

[Jens Axboe]

On Mon, Feb 12 2007, Jens Axboe wrote:
> On Mon, Feb 12 2007, Rusty Russell wrote:
> > 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);
> > 	}
> > }
> 
> Note hard_cur_sectors vs hard_nr_sectors. The former refers to the first
> segment sector count, the latter to the total sector count in the
> request. Hence the difference!

Also, that is why my example code doesn't check the
end_that_request_first() return value, it must be 0 or it would be
buggy. Alternatively,

        if (end_that_request_first(rq, uptodate, rq->hard_nr_sectors))
                BUG()

would make that more clear. Or just a nice little comment. Anyway, if
you fixup lguest I'll add a kernel helper that we can switch it to once
merged.

-- 
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/