why does __split_and_process_bio use bio_clone_bioset?
diff mbox

Message ID 20180614200808.GA46373@redhat.com
State New
Headers show

Commit Message

Mike Snitzer June 14, 2018, 8:08 p.m. UTC
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

Comments

NeilBrown June 14, 2018, 11:34 p.m. UTC | #1
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
Christoph Hellwig June 15, 2018, 7:38 a.m. UTC | #2
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?
Mike Snitzer June 15, 2018, 6:26 p.m. UTC | #3
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
Christoph Hellwig June 16, 2018, 3:20 p.m. UTC | #4
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.

Patch
diff mbox

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;