From: Yu Kuai <[email protected]>
With the following two conditions, bio will be lost:
1) blk plug is not enabled, for example, __blkdev_direct_IO_simple() and
__blkdev_direct_IO_async();
2) bio plug is enabled, for example write IO for raid1/raid10 while
bitmap is enabled;
Root cause is that blk_finish_plug() will add the bio to
curent->bio_list, while such bio will not be handled:
__submit_bio_noacct
current->bio_list = bio_list_on_stack;
blk_start_plug
do {
dm_submit_bio
md_handle_request
raid10_write_request
-> generate new bio for underlying disks
raid1_add_bio_to_plug -> bio is added to plug
} while ((bio = bio_list_pop(&bio_list_on_stack[0])))
-> previous bio are all handled
blk_finish_plug
raid10_unplug
raid1_submit_write
submit_bio_noacct
if (current->bio_list)
bio_list_add(¤t->bio_list[0], bio)
-> add new bio
current->bio_list = NULL
-> new bio is lost
Fix the problem by moving plug into the while loop, so that
current->bio_list will still be handled after blk_finish_plug().
By the way, enable plug for raid1/raid10 in this case will also prevent
delay IO handling into daemon thread, which should also improve IO
performance.
Fixes: 060406c61c7c ("block: add plug while submitting IO")
Reported-by: Changhui Zhong <[email protected]>
Closes: https://lore.kernel.org/all/CAGVVp+Xsmzy2G9YuEatfMT6qv1M--YdOCQ0g7z7OVmcTbBxQAg@mail.gmail.com/
Signed-off-by: Yu Kuai <[email protected]>
---
block/blk-core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 01186333c88e..dd29d5465af6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -613,9 +613,14 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
static void __submit_bio(struct bio *bio)
{
+ /* If plug is not used, add new plug here to cache nsecs time. */
+ struct blk_plug plug;
+
if (unlikely(!blk_crypto_bio_prep(&bio)))
return;
+ blk_start_plug(&plug);
+
if (!bio->bi_bdev->bd_has_submit_bio) {
blk_mq_submit_bio(bio);
} else if (likely(bio_queue_enter(bio) == 0)) {
@@ -624,6 +629,8 @@ static void __submit_bio(struct bio *bio)
disk->fops->submit_bio(bio);
blk_queue_exit(disk->queue);
}
+
+ blk_finish_plug(&plug);
}
/*
@@ -648,13 +655,11 @@ static void __submit_bio(struct bio *bio)
static void __submit_bio_noacct(struct bio *bio)
{
struct bio_list bio_list_on_stack[2];
- struct blk_plug plug;
BUG_ON(bio->bi_next);
bio_list_init(&bio_list_on_stack[0]);
current->bio_list = bio_list_on_stack;
- blk_start_plug(&plug);
do {
struct request_queue *q = bdev_get_queue(bio->bi_bdev);
@@ -688,23 +693,19 @@ static void __submit_bio_noacct(struct bio *bio)
bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
} while ((bio = bio_list_pop(&bio_list_on_stack[0])));
- blk_finish_plug(&plug);
current->bio_list = NULL;
}
static void __submit_bio_noacct_mq(struct bio *bio)
{
struct bio_list bio_list[2] = { };
- struct blk_plug plug;
current->bio_list = bio_list;
- blk_start_plug(&plug);
do {
__submit_bio(bio);
} while ((bio = bio_list_pop(&bio_list[0])));
- blk_finish_plug(&plug);
current->bio_list = NULL;
}
--
2.39.2
On Tue, May 21, 2024 at 8:10 PM Yu Kuai <[email protected]> wrote:
>
> From: Yu Kuai <[email protected]>
>
> With the following two conditions, bio will be lost:
>
> 1) blk plug is not enabled, for example, __blkdev_direct_IO_simple() and
> __blkdev_direct_IO_async();
> 2) bio plug is enabled, for example write IO for raid1/raid10 while
> bitmap is enabled;
>
> Root cause is that blk_finish_plug() will add the bio to
> curent->bio_list, while such bio will not be handled:
>
> __submit_bio_noacct
> current->bio_list = bio_list_on_stack;
> blk_start_plug
>
> do {
> dm_submit_bio
> md_handle_request
> raid10_write_request
> -> generate new bio for underlying disks
> raid1_add_bio_to_plug -> bio is added to plug
> } while ((bio = bio_list_pop(&bio_list_on_stack[0])))
> -> previous bio are all handled
>
> blk_finish_plug
> raid10_unplug
> raid1_submit_write
> submit_bio_noacct
> if (current->bio_list)
> bio_list_add(¤t->bio_list[0], bio)
> -> add new bio
>
> current->bio_list = NULL
> -> new bio is lost
>
> Fix the problem by moving plug into the while loop, so that
> current->bio_list will still be handled after blk_finish_plug().
>
> By the way, enable plug for raid1/raid10 in this case will also prevent
> delay IO handling into daemon thread, which should also improve IO
> performance.
>
> Fixes: 060406c61c7c ("block: add plug while submitting IO")
> Reported-by: Changhui Zhong <[email protected]>
> Closes: https://lore.kernel.org/all/CAGVVp+Xsmzy2G9YuEatfMT6qv1M--YdOCQ0g7z7OVmcTbBxQAg@mail.gmail.com/
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> block/blk-core.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 01186333c88e..dd29d5465af6 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -613,9 +613,14 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
>
> static void __submit_bio(struct bio *bio)
> {
> + /* If plug is not used, add new plug here to cache nsecs time. */
> + struct blk_plug plug;
> +
> if (unlikely(!blk_crypto_bio_prep(&bio)))
> return;
>
> + blk_start_plug(&plug);
> +
> if (!bio->bi_bdev->bd_has_submit_bio) {
> blk_mq_submit_bio(bio);
> } else if (likely(bio_queue_enter(bio) == 0)) {
> @@ -624,6 +629,8 @@ static void __submit_bio(struct bio *bio)
> disk->fops->submit_bio(bio);
> blk_queue_exit(disk->queue);
> }
> +
> + blk_finish_plug(&plug);
> }
>
> /*
> @@ -648,13 +655,11 @@ static void __submit_bio(struct bio *bio)
> static void __submit_bio_noacct(struct bio *bio)
> {
> struct bio_list bio_list_on_stack[2];
> - struct blk_plug plug;
>
> BUG_ON(bio->bi_next);
>
> bio_list_init(&bio_list_on_stack[0]);
> current->bio_list = bio_list_on_stack;
> - blk_start_plug(&plug);
>
> do {
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> @@ -688,23 +693,19 @@ static void __submit_bio_noacct(struct bio *bio)
> bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
> } while ((bio = bio_list_pop(&bio_list_on_stack[0])));
>
> - blk_finish_plug(&plug);
> current->bio_list = NULL;
> }
>
> static void __submit_bio_noacct_mq(struct bio *bio)
> {
> struct bio_list bio_list[2] = { };
> - struct blk_plug plug;
>
> current->bio_list = bio_list;
> - blk_start_plug(&plug);
>
> do {
> __submit_bio(bio);
> } while ((bio = bio_list_pop(&bio_list[0])));
>
> - blk_finish_plug(&plug);
> current->bio_list = NULL;
> }
>
> --
> 2.39.2
>
Hi， Kuai
the raid1 and raid10 issue has been fixed by this patch，
please feel free to add:
Tested-by: Changhui Zhong <[email protected]>
Thanks，
On Wed, 22 May 2024 04:03:08 +0800, Yu Kuai wrote:
> With the following two conditions, bio will be lost:
>
> 1) blk plug is not enabled, for example, __blkdev_direct_IO_simple() and
> __blkdev_direct_IO_async();
> 2) bio plug is enabled, for example write IO for raid1/raid10 while
> bitmap is enabled;
>
> [...]
Applied, thanks!
[1/1] block: fix bio lost for plug enabeld bio based device
commit: 9a42891c35d50a8472b42c61256867b4dfcc1941
Best regards,
--
Jens Axboe