On Tue, Oct 02, 2012 at 04:22:01PM +1000, NeilBrown wrote: > On Fri, 28 Sep 2012 09:23:43 -0700 Kent Overstreet <koverstr...@google.com> > wrote: > > > On Mon, Sep 24, 2012 at 02:56:39PM +1000, NeilBrown wrote: > > > > > > Hi Jens, > > > this patch has been sitting in my -next tree for a little while and I was > > > hoping for it to go in for the next merge window. > > > It simply allows bio_split() to be used on bios without a payload, such > > > as > > > 'discard'. > > > > Thing is, at some point in the stack a discard bio is going to have data > > - see blk_add_rquest_payload(), and it used to be the single page was > > added to discard bios above generic_make_request(), in > > blkdev_issue_discard() or whatever it's called. > > > > So while I'm sure your code works, it's just a fragile way of doing it. > > > > There's also other types of bios where bi_size has nothing to do with > > the amount of data in the bi_io_vec - actually I think this is a new > > thing, since Martin Petersen just added REQ_WRITE_SAME and I don't think > > there were any other instances besides REQ_DISCARD before. > > > > So my preference would be defining a mask (REQ_DISCARD|REQ_WRITE_SAME), > > and if bio->bi_rw & that mask is true, just duplicate the bvec or > > whatever. > > Hi Kent, > I'm afraid I don't see the relevance of your comments to the patch. > > The current bio_split code can successfully split a bio with zero or one > bi_vec entry. If there are more than that, we cannot split. > > How does it matter whether the bio is a DISCARD or a WRITE_SAME or a DATA or > whatever?
Hrm, I think I didn't explain very well. After your change, if bio->bi_vcnt != 0, then it splits the bvec. The trouble is that discard bios do under certain circumstances have bio->bi_vcnt != 0, in which case splitting the bvec is the wrong thing to do - first_sectors will quite likely be bigger than the bvec. In practice this isn't currently a problem for discard bios, because since Christoph added blk_add_request_payload(), discard bios won't have that bvec added until they hit the scsi layer which will be after any splitting. But this is a fairly recent and unrelated change, and IMO not the kind of behaviour I'd want to rely on. WRITE_SAME is a problem for the same reason - bio_sectors(bio) may be large, but the bio will always have a single bvec and splitting the bvec is always the wrong thing to do for WRITE_SAME. So, I think it makes more sense to make the splitting conditional on !(bio->bi_rw & (REQ_DISCARD|REQ_WRITE_SAME)), in addition to bio->bi_vcnt == 1. ..That make more sense? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/