| Message ID | 20180614200808.GA46373@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 14 2018, Mike Snitzer wrote: > On Thu, Jun 14 2018 at 2:12P -0400, > Mike Snitzer <snitzer@redhat.com> wrote: > >> On Thu, Jun 14 2018 at 4:19am -0400, >> Christoph Hellwig <hch@infradead.org> wrote: >> >> > Hi Neil, >> > >> > In commit 18a25da8 ("dm: ensure bio submission follows a depth-first >> > tree walk") you've added a call to bio_clone_bioset to >> > __split_and_process_bio. Unlike all other bio splitting code this >> > actually allocates a new bio_vec array instead of just splitting the bio >> > and the iterator. I can't actually find a good reason for that either >> > in a cursory review of the code, the commit or the comments. >> > >> > Do you remember why this can't just use bio_clone_fast? Good question. I don't remember having a good reason to choose it, and if there was one I suspect I would have mentioned it in the commit message. So it was most likely an oversight. Looking at the code now, I can see no justification for not using bio_clone_fast() or similar. Thanks for looking into this - I guess the next step is to get rid of bio_clone_bioset() completely. Nice. >> >> Your question caused me to revisit this code and it is suspect for a >> couple reasons: >> >> 1) I'm also not seeing why we need bio_clone_bioset() > > The patch below seems to work fine (given quick testing).. It also has a > side-effect of not breaking integrity support (which commit 18a25da8 > appears to do because it isn't accounting for any of the integrity stuff > bio_split, or dm.c:clone_bio, does). > > FYI, my other concerns in my my previous reply were unfounded and due to > misreading the existing code. > > Neil, please still feel free to have a look at this to see if you can > recall why you used bio_clone_bioset(). > > If in the end you agree that the following patch is fine please let me > know and I'll get a proper fix staged. I agree with the patch. Reviewed-by: NeilBrown <neilb@suse.com> Thanks! NeilBrown > > Thanks, > Mike > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 20a8d63..dfb4783 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -1582,10 +1582,9 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md, > * the usage of io->orig_bio in dm_remap_zone_report() > * won't be affected by this reassignment. > */ > - struct bio *b = bio_clone_bioset(bio, GFP_NOIO, > - &md->queue->bio_split); > + struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count, > + GFP_NOIO, &md->queue->bio_split); > ci.io->orig_bio = b; > - bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9); > bio_chain(b, bio); > ret = generic_make_request(bio); > break; > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Jun 14, 2018 at 04:08:09PM -0400, Mike Snitzer wrote: > The patch below seems to work fine (given quick testing).. It also has a > side-effect of not breaking integrity support (which commit 18a25da8 > appears to do because it isn't accounting for any of the integrity stuff > bio_split, or dm.c:clone_bio, does). Looks sensible to me. Are you going to queue this up for 4.18?
On Fri, Jun 15 2018 at 3:38am -0400, Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Jun 14, 2018 at 04:08:09PM -0400, Mike Snitzer wrote: > > The patch below seems to work fine (given quick testing).. It also has a > > side-effect of not breaking integrity support (which commit 18a25da8 > > appears to do because it isn't accounting for any of the integrity stuff > > bio_split, or dm.c:clone_bio, does). > > Looks sensible to me. Are you going to queue this up for 4.18? Yes. Already queued, likely won't send to Linus until end of next week though.. unless you want it upstream sooner? https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.18&id=bbb683cf88f58742233c3551d1b9839a5fe5bbf8
On Fri, Jun 15, 2018 at 02:26:31PM -0400, Mike Snitzer wrote: > Yes. Already queued, likely won't send to Linus until end of next week > though.. unless you want it upstream sooner? We only really need it as a baseline for the multi-page bio work, so no real rush. I think I'll just include it with my pending cleanup series with a big 'DO NOT APPLY, will be in 4.18-rc2' marker.
diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 20a8d63..dfb4783 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1582,10 +1582,9 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md, * the usage of io->orig_bio in dm_remap_zone_report() * won't be affected by this reassignment. */ - struct bio *b = bio_clone_bioset(bio, GFP_NOIO, - &md->queue->bio_split); + struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count, + GFP_NOIO, &md->queue->bio_split); ci.io->orig_bio = b; - bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9); bio_chain(b, bio); ret = generic_make_request(bio); break;