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

[Ming Lei <ming.lei@redhat.com>]

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