* why does __split_and_process_bio use bio_clone_bioset?
@ 2018-06-14 8:19 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2018-06-14 8:19 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-block, dm-devel, Ming Lei
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?
^ permalink raw reply [flat|nested] 13+ messages in thread* why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-14 8:19 ` Christoph Hellwig 0 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2018-06-14 8:19 UTC (permalink / raw) To: NeilBrown; +Cc: Ming Lei, linux-block, dm-devel 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-14 8:19 ` Christoph Hellwig @ 2018-06-14 18:12 ` Mike Snitzer -1 siblings, 0 replies; 13+ messages in thread From: Mike Snitzer @ 2018-06-14 18:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-block, dm-devel, NeilBrown, Ming Lei 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? 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() - could be quirk of how the code is constructing shallow chains (all chained to the same parent) but even that doesn't seem to explain it. I'll just test using bio_clone_fast() and see what happens ;) 2) The final dm.c:end_io_acct() in terms of the smaller and smaller clone bio looks prone to insufficient IO accounting. 3) I'm really not liking the mix of bio_chain refcount and DM's own io->io_count in the DM endio path. I'll work through all of this some more and let you know what I find (hopefully by end of tomorrow). Mike ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-14 18:12 ` Mike Snitzer 0 siblings, 0 replies; 13+ messages in thread From: Mike Snitzer @ 2018-06-14 18:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: NeilBrown, linux-block, dm-devel, Ming Lei 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? 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() - could be quirk of how the code is constructing shallow chains (all chained to the same parent) but even that doesn't seem to explain it. I'll just test using bio_clone_fast() and see what happens ;) 2) The final dm.c:end_io_acct() in terms of the smaller and smaller clone bio looks prone to insufficient IO accounting. 3) I'm really not liking the mix of bio_chain refcount and DM's own io->io_count in the DM endio path. I'll work through all of this some more and let you know what I find (hopefully by end of tomorrow). Mike ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-14 18:12 ` Mike Snitzer (?) @ 2018-06-14 20:08 ` Mike Snitzer 2018-06-14 23:34 ` [dm-devel] " NeilBrown 2018-06-15 7:38 ` [dm-devel] " Christoph Hellwig -1 siblings, 2 replies; 13+ messages in thread From: Mike Snitzer @ 2018-06-14 20:08 UTC (permalink / raw) To: Christoph Hellwig, NeilBrown; +Cc: linux-block, dm-devel, Ming Lei 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? > > 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. 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; ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-14 20:08 ` Mike Snitzer @ 2018-06-14 23:34 ` NeilBrown 2018-06-15 7:38 ` [dm-devel] " Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: NeilBrown @ 2018-06-14 23:34 UTC (permalink / raw) To: Mike Snitzer, Christoph Hellwig; +Cc: linux-block, dm-devel, Ming Lei [-- Attachment #1.1: Type: text/plain, Size: 2906 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-14 23:34 ` NeilBrown 0 siblings, 0 replies; 13+ messages in thread From: NeilBrown @ 2018-06-14 23:34 UTC (permalink / raw) To: Mike Snitzer, Christoph Hellwig; +Cc: linux-block, dm-devel, Ming Lei [-- Attachment #1: Type: text/plain, Size: 2906 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-14 20:08 ` Mike Snitzer @ 2018-06-15 7:38 ` Christoph Hellwig 2018-06-15 7:38 ` [dm-devel] " Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2018-06-15 7:38 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, linux-block, dm-devel, NeilBrown, Ming Lei 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [dm-devel] why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-15 7:38 ` Christoph Hellwig 0 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2018-06-15 7:38 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, NeilBrown, linux-block, dm-devel, Ming Lei 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-15 7:38 ` [dm-devel] " Christoph Hellwig @ 2018-06-15 18:26 ` Mike Snitzer -1 siblings, 0 replies; 13+ messages in thread From: Mike Snitzer @ 2018-06-15 18:26 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-block, dm-devel, NeilBrown, Ming Lei 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-15 18:26 ` Mike Snitzer 0 siblings, 0 replies; 13+ messages in thread From: Mike Snitzer @ 2018-06-15 18:26 UTC (permalink / raw) To: Christoph Hellwig; +Cc: NeilBrown, linux-block, dm-devel, Ming Lei 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? 2018-06-15 18:26 ` Mike Snitzer @ 2018-06-16 15:20 ` Christoph Hellwig -1 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2018-06-16 15:20 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, linux-block, dm-devel, NeilBrown, Ming Lei 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: why does __split_and_process_bio use bio_clone_bioset? @ 2018-06-16 15:20 ` Christoph Hellwig 0 siblings, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2018-06-16 15:20 UTC (permalink / raw) To: Mike Snitzer Cc: Christoph Hellwig, NeilBrown, linux-block, dm-devel, Ming Lei 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-06-16 15:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-14 8:19 why does __split_and_process_bio use bio_clone_bioset? Christoph Hellwig 2018-06-14 8:19 ` Christoph Hellwig 2018-06-14 18:12 ` Mike Snitzer 2018-06-14 18:12 ` Mike Snitzer 2018-06-14 20:08 ` Mike Snitzer 2018-06-14 23:34 ` NeilBrown 2018-06-14 23:34 ` [dm-devel] " NeilBrown 2018-06-15 7:38 ` Christoph Hellwig 2018-06-15 7:38 ` [dm-devel] " Christoph Hellwig 2018-06-15 18:26 ` Mike Snitzer 2018-06-15 18:26 ` Mike Snitzer 2018-06-16 15:20 ` Christoph Hellwig 2018-06-16 15:20 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.