On Thu, Jun 14 2018, Mike Snitzer wrote: > On Thu, Jun 14 2018 at 2:12P -0400, > Mike Snitzer <[email protected]> wrote: > >> On Thu, Jun 14 2018 at 4:19am -0400, >> Christoph Hellwig <[email protected]> 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 <[email protected]> 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 > [email protected] > https://www.redhat.com/mailman/listinfo/dm-devel
signature.asc
Description: PGP signature
-- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
