* [PATCH] block: fix bio chaining in blk_next_bio() @ 2020-12-06 5:18 Tom Yan 2020-12-06 11:20 ` Hannes Reinecke 2020-12-07 3:12 ` Ming Lei 0 siblings, 2 replies; 7+ messages in thread From: Tom Yan @ 2020-12-06 5:18 UTC (permalink / raw) To: linux-block, hch, ming.l, sagig, axboe; +Cc: tom.leiming, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-06 5:18 [PATCH] block: fix bio chaining in blk_next_bio() Tom Yan @ 2020-12-06 11:20 ` Hannes Reinecke 2020-12-06 13:17 ` Tom Yan 2020-12-07 3:12 ` Ming Lei 1 sibling, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2020-12-06 11:20 UTC (permalink / raw) To: Tom Yan, linux-block, hch, ming.l, sagig, axboe; +Cc: tom.leiming 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-06 11:20 ` Hannes Reinecke @ 2020-12-06 13:17 ` Tom Yan 2020-12-06 14:17 ` Tom Yan 0 siblings, 1 reply; 7+ messages in thread From: Tom Yan @ 2020-12-06 13:17 UTC (permalink / raw) To: Hannes Reinecke; +Cc: linux-block, Christoph Hellwig, axboe, tom.leiming 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-06 13:17 ` Tom Yan @ 2020-12-06 14:17 ` Tom Yan 0 siblings, 0 replies; 7+ messages in thread From: Tom Yan @ 2020-12-06 14:17 UTC (permalink / raw) To: Hannes Reinecke; +Cc: linux-block, Christoph Hellwig, axboe, tom.leiming 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-06 5:18 [PATCH] block: fix bio chaining in blk_next_bio() Tom Yan 2020-12-06 11:20 ` Hannes Reinecke @ 2020-12-07 3:12 ` Ming Lei 2020-12-08 12:46 ` Tom Yan 1 sibling, 1 reply; 7+ messages in thread From: Ming Lei @ 2020-12-07 3:12 UTC (permalink / raw) To: Tom Yan; +Cc: linux-block, hch, ming.l, sagig, axboe, tom.leiming 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-07 3:12 ` Ming Lei @ 2020-12-08 12:46 ` Tom Yan 2020-12-09 1:20 ` Ming Lei 0 siblings, 1 reply; 7+ messages in thread From: Tom Yan @ 2020-12-08 12:46 UTC (permalink / raw) To: Ming Lei Cc: linux-block, Christoph Hellwig, ming.l, sagig, axboe, tom.leiming 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: fix bio chaining in blk_next_bio() 2020-12-08 12:46 ` Tom Yan @ 2020-12-09 1:20 ` Ming Lei 0 siblings, 0 replies; 7+ messages in thread From: Ming Lei @ 2020-12-09 1:20 UTC (permalink / raw) To: Tom Yan; +Cc: linux-block, Christoph Hellwig, ming.l, sagig, axboe, tom.leiming 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-09 1:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-06 5:18 [PATCH] block: fix bio chaining in blk_next_bio() Tom Yan 2020-12-06 11:20 ` Hannes Reinecke 2020-12-06 13:17 ` Tom Yan 2020-12-06 14:17 ` Tom Yan 2020-12-07 3:12 ` Ming Lei 2020-12-08 12:46 ` Tom Yan 2020-12-09 1:20 ` Ming Lei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.