| Message ID | 20201206051802.1890-1-tom.ty89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix bio chaining in blk_next_bio() | expand |
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
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
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
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
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 >
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.
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); }
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(-)