[PATCH] block: fix bio chaining in blk_next_bio()

[PATCH] block: fix bio chaining in blk_next_bio()

[Tom Yan]

While it seems to have worked for so long, it doesn't seem right
that we set the new bio as the parent. bio_chain() seems to be used
in the other way everywhere else anyway.

Signed-off-by: Tom Yan <tom.ty89@gmail.com>
---
 block/blk-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index e90614fd8d6a..918deaf5c8a4 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
 	struct bio *new = bio_alloc(gfp, nr_pages);
 
 	if (bio) {
-		bio_chain(bio, new);
+		bio_chain(new, bio);
 		submit_bio(bio);
 	}
 
-- 
2.29.2

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Hannes Reinecke]

On 12/6/20 6:18 AM, Tom Yan wrote:
> While it seems to have worked for so long, it doesn't seem right
> that we set the new bio as the parent. bio_chain() seems to be used
> in the other way everywhere else anyway.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
>   block/blk-lib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index e90614fd8d6a..918deaf5c8a4 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
>   	struct bio *new = bio_alloc(gfp, nr_pages);
>   
>   	if (bio) {
> -		bio_chain(bio, new);
> +		bio_chain(new, bio);
>   		submit_bio(bio);
>   	}
>   
> 
I don't think this is correct.
This code is submitting the original bio, and we _want_ to keep the 
newly allocated one even though the original might have been completed 
already. If we were setting the 'parent' to the original bio upper 
layers might infer that the entire request has been completed (as the 
original bio is now the 'parent' bio), which is patently not true.

So, rather not.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Tom Yan]

I still don't think this sounds right.

See the definition of bio_chain():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bio.c?h=v5.10-rc6#n344

And in turn bio_inc_remaining():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/bio.h?h=v5.10-rc6#n667

With the current blk_next_bio(), the first bio will never even be
handled by bio_chain()/bio_inc_remaining()/bio_set_flag(bio,
BIO_CHAIN). When the first submit_bio() is called, it won't have any
idea that the next/new bio exists. What you said actually sounds a bit
like the current situation.

On Sun, 6 Dec 2020 at 19:20, Hannes Reinecke <hare@suse.de> wrote:
>
> On 12/6/20 6:18 AM, Tom Yan wrote:
> > While it seems to have worked for so long, it doesn't seem right
> > that we set the new bio as the parent. bio_chain() seems to be used
> > in the other way everywhere else anyway.
> >
> > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > ---
> >   block/blk-lib.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > index e90614fd8d6a..918deaf5c8a4 100644
> > --- a/block/blk-lib.c
> > +++ b/block/blk-lib.c
> > @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
> >       struct bio *new = bio_alloc(gfp, nr_pages);
> >
> >       if (bio) {
> > -             bio_chain(bio, new);
> > +             bio_chain(new, bio);
> >               submit_bio(bio);
> >       }
> >
> >
> I don't think this is correct.
> This code is submitting the original bio, and we _want_ to keep the
> newly allocated one even though the original might have been completed
> already. If we were setting the 'parent' to the original bio upper
> layers might infer that the entire request has been completed (as the
> original bio is now the 'parent' bio), which is patently not true.
>
> So, rather not.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Tom Yan]

Also see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bio.c?h=v5.10-rc6#n1384
btw. I really think the "new as parent" is a careless typo.
(Christoph?)

On Sun, 6 Dec 2020 at 21:17, Tom Yan <tom.ty89@gmail.com> wrote:
>
> I still don't think this sounds right.
>
> See the definition of bio_chain():
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bio.c?h=v5.10-rc6#n344
>
> And in turn bio_inc_remaining():
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/bio.h?h=v5.10-rc6#n667
>
> With the current blk_next_bio(), the first bio will never even be
> handled by bio_chain()/bio_inc_remaining()/bio_set_flag(bio,
> BIO_CHAIN). When the first submit_bio() is called, it won't have any
> idea that the next/new bio exists. What you said actually sounds a bit
> like the current situation.
>
> On Sun, 6 Dec 2020 at 19:20, Hannes Reinecke <hare@suse.de> wrote:
> >
> > On 12/6/20 6:18 AM, Tom Yan wrote:
> > > While it seems to have worked for so long, it doesn't seem right
> > > that we set the new bio as the parent. bio_chain() seems to be used
> > > in the other way everywhere else anyway.
> > >
> > > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > > ---
> > >   block/blk-lib.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > > index e90614fd8d6a..918deaf5c8a4 100644
> > > --- a/block/blk-lib.c
> > > +++ b/block/blk-lib.c
> > > @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
> > >       struct bio *new = bio_alloc(gfp, nr_pages);
> > >
> > >       if (bio) {
> > > -             bio_chain(bio, new);
> > > +             bio_chain(new, bio);
> > >               submit_bio(bio);
> > >       }
> > >
> > >
> > I don't think this is correct.
> > This code is submitting the original bio, and we _want_ to keep the
> > newly allocated one even though the original might have been completed
> > already. If we were setting the 'parent' to the original bio upper
> > layers might infer that the entire request has been completed (as the
> > original bio is now the 'parent' bio), which is patently not true.
> >
> > So, rather not.
> >
> > Cheers,
> >
> > Hannes
> > --
> > Dr. Hannes Reinecke                Kernel Storage Architect
> > hare@suse.de                              +49 911 74053 688
> > SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> > HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Ming Lei]

On Sun, Dec 06, 2020 at 01:18:02PM +0800, Tom Yan wrote:
> While it seems to have worked for so long, it doesn't seem right
> that we set the new bio as the parent. bio_chain() seems to be used
> in the other way everywhere else anyway.
> 
> Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> ---
>  block/blk-lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index e90614fd8d6a..918deaf5c8a4 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
>  	struct bio *new = bio_alloc(gfp, nr_pages);
>  
>  	if (bio) {
> -		bio_chain(bio, new);
> +		bio_chain(new, bio);
>  		submit_bio(bio);
>  	}

This patch isn't needed. We simply wait for completion of the last issued bio, which
.bi_end_io(submit_bio_wait_endio) is only called after all previous bios are done.

And the last bio is the ancestor of the whole chained bios, which are
submitted in the following way(order):

	bio 0 ---> bio 1 ---> ... -> bio N(the last bio)


thanks,
Ming

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Tom Yan]

Are you saying that it would work either way?

On Mon, 7 Dec 2020 at 11:12, Ming Lei <ming.lei@redhat.com> wrote:
>
> On Sun, Dec 06, 2020 at 01:18:02PM +0800, Tom Yan wrote:
> > While it seems to have worked for so long, it doesn't seem right
> > that we set the new bio as the parent. bio_chain() seems to be used
> > in the other way everywhere else anyway.
> >
> > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > ---
> >  block/blk-lib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > index e90614fd8d6a..918deaf5c8a4 100644
> > --- a/block/blk-lib.c
> > +++ b/block/blk-lib.c
> > @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
> >       struct bio *new = bio_alloc(gfp, nr_pages);
> >
> >       if (bio) {
> > -             bio_chain(bio, new);
> > +             bio_chain(new, bio);
> >               submit_bio(bio);
> >       }
>
> This patch isn't needed. We simply wait for completion of the last issued bio, which
> .bi_end_io(submit_bio_wait_endio) is only called after all previous bios are done.
>
> And the last bio is the ancestor of the whole chained bios, which are
> submitted in the following way(order):
>
>         bio 0 ---> bio 1 ---> ... -> bio N(the last bio)
>
>
> thanks,
> Ming
>

Re: [PATCH] block: fix bio chaining in blk_next_bio()

[Ming Lei]

On Tue, Dec 08, 2020 at 08:46:31PM +0800, Tom Yan wrote:
> Are you saying that it would work either way?

Please don't do top reply which is hard to follow context.

> 
> On Mon, 7 Dec 2020 at 11:12, Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Sun, Dec 06, 2020 at 01:18:02PM +0800, Tom Yan wrote:
> > > While it seems to have worked for so long, it doesn't seem right
> > > that we set the new bio as the parent. bio_chain() seems to be used
> > > in the other way everywhere else anyway.
> > >
> > > Signed-off-by: Tom Yan <tom.ty89@gmail.com>
> > > ---
> > >  block/blk-lib.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > > index e90614fd8d6a..918deaf5c8a4 100644
> > > --- a/block/blk-lib.c
> > > +++ b/block/blk-lib.c
> > > @@ -15,7 +15,7 @@ struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp)
> > >       struct bio *new = bio_alloc(gfp, nr_pages);
> > >
> > >       if (bio) {
> > > -             bio_chain(bio, new);
> > > +             bio_chain(new, bio);
> > >               submit_bio(bio);
> > >       }
> >
> > This patch isn't needed. We simply wait for completion of the last issued bio, which
> > .bi_end_io(submit_bio_wait_endio) is only called after all previous bios are done.
> >
> > And the last bio is the ancestor of the whole chained bios, which are
> > submitted in the following way(order):
> >
> >         bio 0 ---> bio 1 ---> ... -> bio N(the last bio)
...
> 
> Are you saying that it would work either way?

No.

The current way of blk_next_bio() works just as expected, and your patch
is actually wrong. Let's see one simple example, suppose one discard request
is splitted into two bios(bio 0, and bio 1), after your patch is applied:

1) bio 0 becomes bio 1's parent, and bio 0 is submitted first

2) bio 1 is submitted finally via submit_bio_wait(), and bio 1's .bi_end_io/.bi_private
is updated to submit_bio_wait_endio()/&done, so when bio 1 is completed, there
isn't any way to know if bio 0 is completed, because the chain is cut by your patch.

-- 
Ming