[dm-devel] why does __split_and_process_bio use bio_clone_bioset?
NeilBrown
neilb at suse.com
Thu Jun 14 23:34:29 UTC 2018
On Thu, Jun 14 2018, Mike Snitzer wrote:
> On Thu, Jun 14 2018 at 2:12P -0400,
> Mike Snitzer <snitzer at redhat.com> wrote:
>
>> On Thu, Jun 14 2018 at 4:19am -0400,
>> Christoph Hellwig <hch at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20180615/78fba5c2/attachment.sig>
More information about the dm-devel
mailing list