[PATCH] block: only update parent bi_status when bio fail

[PATCH] block: only update parent bi_status when bio fail

[Yufen Yu]

For multiple split bios, if one of the bio is fail, the whole
should return error to application. But we found there is a race
between bio_integrity_verify_fn and bio complete, which return
io success to application after one of the bio fail. The race as
following:

split bio(READ)          kworker

nvme_complete_rq
blk_update_request //split error=0
  bio_endio
    bio_integrity_endio
      queue_work(kintegrityd_wq, &bip->bip_work);

                         bio_integrity_verify_fn
                         bio_endio //split bio
                          __bio_chain_endio
                             if (!parent->bi_status)

                               <interrupt entry>
                               nvme_irq
                                 blk_update_request //parent error=7
                                 req_bio_endio
                                    bio->bi_status = 7 //parent bio
                               <interrupt exit>

                               parent->bi_status = 0
                        parent->bi_end_io() // return bi_status=0

The bio has been split as two: split and parent. When split
bio completed, it depends on kworker to do endio, while
bio_integrity_verify_fn have been interrupted by parent bio
complete irq handler. Then, parent bio->bi_status which have
been set in irq handler will overwrite by kworker.

In fact, even without the above race, we also need to conside
the concurrency beteen mulitple split bio complete and update
the same parent bi_status. Normally, multiple split bios will
be issued to the same hctx and complete from the same irq
vector. But if we have updated queue map between multiple split
bios, these bios may complete on different hw queue and different
irq vector. Then the concurrency update parent bi_status may
cause the final status error.

Suggested-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 block/bio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 26b7f721cda8..f49713ff8ad3 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -277,7 +277,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
 {
 	struct bio *parent = bio->bi_private;
 
-	if (!parent->bi_status)
+	if (bio->bi_status && !parent->bi_status)
 		parent->bi_status = bio->bi_status;
 	bio_put(bio);
 	return parent;
-- 
2.25.4

Re: [PATCH] block: only update parent bi_status when bio fail

[Ming Lei]

On Wed, Mar 31, 2021 at 07:53:59AM -0400, Yufen Yu wrote:
> For multiple split bios, if one of the bio is fail, the whole
> should return error to application. But we found there is a race
> between bio_integrity_verify_fn and bio complete, which return
> io success to application after one of the bio fail. The race as
> following:
> 
> split bio(READ)          kworker
> 
> nvme_complete_rq
> blk_update_request //split error=0
>   bio_endio
>     bio_integrity_endio
>       queue_work(kintegrityd_wq, &bip->bip_work);
> 
>                          bio_integrity_verify_fn
>                          bio_endio //split bio
>                           __bio_chain_endio
>                              if (!parent->bi_status)
> 
>                                <interrupt entry>
>                                nvme_irq
>                                  blk_update_request //parent error=7
>                                  req_bio_endio
>                                     bio->bi_status = 7 //parent bio
>                                <interrupt exit>
> 
>                                parent->bi_status = 0
>                         parent->bi_end_io() // return bi_status=0
> 
> The bio has been split as two: split and parent. When split
> bio completed, it depends on kworker to do endio, while
> bio_integrity_verify_fn have been interrupted by parent bio
> complete irq handler. Then, parent bio->bi_status which have
> been set in irq handler will overwrite by kworker.
> 
> In fact, even without the above race, we also need to conside
> the concurrency beteen mulitple split bio complete and update
> the same parent bi_status. Normally, multiple split bios will
> be issued to the same hctx and complete from the same irq
> vector. But if we have updated queue map between multiple split
> bios, these bios may complete on different hw queue and different
> irq vector. Then the concurrency update parent bi_status may
> cause the final status error.
> 
> Suggested-by: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
>  block/bio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 26b7f721cda8..f49713ff8ad3 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -277,7 +277,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
>  {
>  	struct bio *parent = bio->bi_private;
>  
> -	if (!parent->bi_status)
> +	if (bio->bi_status && !parent->bi_status)
>  		parent->bi_status = bio->bi_status;
>  	bio_put(bio);
>  	return parent;
> -- 
> 2.25.4
> 

Looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH] block: only update parent bi_status when bio fail

[Jens Axboe]

On 3/31/21 5:53 AM, Yufen Yu wrote:
> For multiple split bios, if one of the bio is fail, the whole
> should return error to application. But we found there is a race
> between bio_integrity_verify_fn and bio complete, which return
> io success to application after one of the bio fail. The race as
> following:
> 
> split bio(READ)          kworker
> 
> nvme_complete_rq
> blk_update_request //split error=0
>   bio_endio
>     bio_integrity_endio
>       queue_work(kintegrityd_wq, &bip->bip_work);
> 
>                          bio_integrity_verify_fn
>                          bio_endio //split bio
>                           __bio_chain_endio
>                              if (!parent->bi_status)
> 
>                                <interrupt entry>
>                                nvme_irq
>                                  blk_update_request //parent error=7
>                                  req_bio_endio
>                                     bio->bi_status = 7 //parent bio
>                                <interrupt exit>
> 
>                                parent->bi_status = 0
>                         parent->bi_end_io() // return bi_status=0
> 
> The bio has been split as two: split and parent. When split
> bio completed, it depends on kworker to do endio, while
> bio_integrity_verify_fn have been interrupted by parent bio
> complete irq handler. Then, parent bio->bi_status which have
> been set in irq handler will overwrite by kworker.
> 
> In fact, even without the above race, we also need to conside
> the concurrency beteen mulitple split bio complete and update
> the same parent bi_status. Normally, multiple split bios will
> be issued to the same hctx and complete from the same irq
> vector. But if we have updated queue map between multiple split
> bios, these bios may complete on different hw queue and different
> irq vector. Then the concurrency update parent bi_status may
> cause the final status error.

Applied, thanks.

-- 
Jens Axboe