Re: [PATCH 4/6] virtio block driver

[Jens Axboe <jens.axboe-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On Fri, Sep 21 2007, Rusty Russell wrote:
> On Thu, 2007-09-20 at 15:05 +0200, Jens Axboe wrote:
> > On Thu, Sep 20 2007, Rusty Russell wrote:
> > > +static void end_dequeued_request(struct request *req,
> > > +				 struct request_queue *q, int uptodate)
> > > +{
> > > +	/* And so the insanity of the block layer infects us here. */
> > > +	int nsectors = req->hard_nr_sectors;
> > > +
> > > +	if (blk_pc_request(req)) {
> > > +		nsectors = (req->data_len + 511) >> 9;
> > > +		if (!nsectors)
> > > +			nsectors = 1;
> > > +	}
> > > +	if (end_that_request_first(req, uptodate, nsectors))
> > > +		BUG();
> > > +	add_disk_randomness(req->rq_disk);
> > > +	end_that_request_last(req, uptodate);
> > > +}
> > 
> > We have end_queued_request(), lets add end_dequeued_request(). Below.
> 
> OK, thanks, I'll throw that in the mix and test...

I've queued it for 2.6.24 as well, so should be in mainline in that time
frame.

> > > +	vblk->sg[0].page = virt_to_page(&vbr->out_hdr);
> > > +	vblk->sg[0].offset = offset_in_page(&vbr->out_hdr);
> > > +	vblk->sg[0].length = sizeof(vbr->out_hdr);
> > > +	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);
> > 
> > This wont work for chained sglists, but I gather (I'm so funny) that it
> > wont be an issue for you. How large are your sglists?
> 
> Hmm, potentially extremely large.  What do I need to do for chained
> sglists?

Not a lot, actually. You snipped the problematic part in your reply,
though:

        vblk->sg[num+1].page = virt_to_page(&vbr->in_hdr);

which assumes that sg is a contigious piece of memory, for chained sg
lists that isn't true. sg chaining will be in 2.6.24, so if you really
do need large sglists, then that's the way to go.

blk_rq_map_sg() maps correctly for you, no changes needed there. But you
want to use sg_last() for adding to the end of the sglist. And then use
sg_next() to retrieve the next sg segment instead of sg + 1, and
for_each_sg() to loop over all segments.

Just something to keep in mind, if you plan on merging this post 2.6.23.

> > > +		if (!do_req(q, vblk, req)) {
> > > +			/* Queue full?  Wait. */
> > > +			blk_stop_queue(q);
> > > +			break;
> > > +		}
> > 
> > Personally I think this bool stuff is foul. You return false/true, but
> > still use ! to test. That is just more confusing than the canonical 0/1
> > good/bad return imho.
> 
> Except "0/1" is not canonical in the kernel.  Arguably, "-errno/0" is
> canonical.  OTOH, bool is clear.

-errno/0, then. 1 is typically used for 'failure without specific error
number' when -Exx doesn't apply. Like the above :-)

But lets just agree to disagree on the bool.

> if do_req() fails, we assume the queue is full.  I shall change the
> comment to that effect.

Thanks!

-- 
Jens Axboe


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/