why does __split_and_process_bio use bio_clone_bioset?

why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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?

why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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?

Re: why does __split_and_process_bio use bio_clone_bioset?

[Mike Snitzer]

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

Re: why does __split_and_process_bio use bio_clone_bioset?

[Mike Snitzer]

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

Re: why does __split_and_process_bio use bio_clone_bioset?

[Mike Snitzer]

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;

Re: [dm-devel] why does __split_and_process_bio use bio_clone_bioset?

[NeilBrown]

[-- 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 --]

Re: why does __split_and_process_bio use bio_clone_bioset?

[NeilBrown]

[-- 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 --]

Re: [dm-devel] why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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?

Re: why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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?

Re: why does __split_and_process_bio use bio_clone_bioset?

[Mike Snitzer]

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

Re: why does __split_and_process_bio use bio_clone_bioset?

[Mike Snitzer]

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

Re: why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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.

Re: why does __split_and_process_bio use bio_clone_bioset?

[Christoph Hellwig]

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.