Re: [PATCH 4/6] virtio block driver

Rusty Russell
2007-09-21T02:06:07Z

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

> > + 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?

> > + 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.

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

Thanks!
Rusty.