Re: [RFC PATCH 2/2] dm: add support for splitting discard requests

[Mike Snitzer <snitzer@redhat.com>]

On Sun, Jun 27 2010 at  5:47am -0400,
Christoph Hellwig <hch@lst.de> wrote:

> On Sat, Jun 26, 2010 at 04:31:25PM -0400, Mike Snitzer wrote:
> > Enable the striped target to support discard requests by splitting a
> > single discard into N discards on a stripe chunk size boundary.
> > 
> > Follow on core block layer work to merge discards would be helpful.
> > 
> > This work relies on DM's clone_bio() always having BIO_RW_BARRIER set
> > for discard requests.  Without BIO_RW_BARRIER the block layer will spew
> > "blk: request botched" warnings for discards that were split by DM.
> > - this clearly needs further investigation!
> 
> Btw, you can get discard requests from the upper layer that do not
> have BIO_RW_BARRIER set, currently from the BLKDISCARD ioctl used by
> various mkfs tools, and also from the not yet merged xfs discard
> support.

Yes, I'm aware of the BLKDISCARD ioctl.  I added the 'else if' clause
which adds the BIO_RW_BARRIER flag specifically for that case.  Testing
showed the mkfs.ext4 uses the BLKDISCARD ioctl and it would result in
"blk: request botched" when operated against a striped DM target (that
was performing the discrad splitting).

> Can I assume it works fine with those?

Yes it works, with or without DM adding the BIO_RW_BARRIER flag, but
without the flag it'll spew "blk: request botched" and the block layer
will fix things up with this at the end of blk_update_request():

        /*
         * If total number of sectors is less than the first segment
         * size, something has gone terribly wrong.
         */
        if (blk_rq_bytes(req) < blk_rq_cur_bytes(req)) {
                printk(KERN_ERR "blk: request botched\n");
                req->__data_len = blk_rq_cur_bytes(req);
        }

Additional tracing showed blk_rq_bytes(req) was always 0 if DM didn't
set BIO_RW_BARRIER for split discard requests.  It could be that if
BIO_RW_BARRIER is set we'll exit early from blk_update_request to
avoid its "blk: request botched"?  I'll look closer.

> > -	clone->bi_rw &= ~(1 << BIO_RW_BARRIER);
> > +	if (!bio_rw_flagged(bio, BIO_RW_DISCARD))
> > +		clone->bi_rw &= ~(1 << BIO_RW_BARRIER);
> > +	else if (!bio_rw_flagged(bio, BIO_RW_BARRIER)) {
> > +		/* discard w/o barrier results in "blk: request botched" */
> > +		clone->bi_rw |= (1 << BIO_RW_BARRIER);
> > +	}
> 
> So previously we unconditionally cleared the BIO_RW_BARRIER bit in the clone. 

Yes.  I'd like to understand why from Mikulas.

> Maybe to make it clear reorder the if a bit and also just set the
> barrier bit unconditionally for discards, similar to how we
> unconditionally clear it otherwise:
> 
> 	if (bio_rw_flagged(bio, BIO_RW_DISCARD)) {
> 		/* discard w/o barrier results in "blk: request botched" */
> 		clone->bi_rw |= (1 << BIO_RW_BARRIER);
> 	} else {
> 		clone->bi_rw &= ~(1 << BIO_RW_BARRIER);
> 	}
> 
> maybe even with a slightly longer comment explaining what's actually
> going on here, and a FIXME.

OK, your reorder is clearer.  As for a FIXME describing what's going on
here: I'll work to sort that out.. it is still unclear to me why the
BIO_RW_BARRIER flag silences the "blk: request botched" spew from
blk_update_request().

Thanks,
Mike