[PATCH v2 0/6] md/raid10: several simple obvious bugfix

[PATCH v2 0/6] md/raid10: several simple obvious bugfix

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

We're running many tests for raid10 currently, and we found a lot of
problems already. This patchset is the first part for some simple and
obvious problems. Most of the patches were sent separately already, but
I think a patchset is better for review.

Yu Kuai (6):
  md/raid10: don't call bio_start_io_acct twice for bio which
    experienced read error
  md: fix soft lockup in status_resync
  md/raid10: don't BUG_ON() in raise_barrier()
  md/radi10: fix leak of 'r10bio->remaining' for recovery
  md/raid10: fix memleak for 'conf->bio_split'
  md/raid10: fix memleak of md thread

 drivers/md/md.c     | 18 +++++------
 drivers/md/raid10.c | 78 +++++++++++++++++++++++----------------------
 2 files changed, 49 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] md/raid10: don't call bio_start_io_acct twice for bio which experienced read error

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

handle_read_error() will resumit r10_bio by raid10_read_request(), which
will call bio_start_io_acct() again, while bio_end_io_acct() will only
be called once.

Fix the problem by don't account io again from handle_read_error().

Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/md/raid10.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 6c66357f92f5..4f8edb6ea3e2 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1173,7 +1173,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
 }
 
 static void raid10_read_request(struct mddev *mddev, struct bio *bio,
-				struct r10bio *r10_bio)
+				struct r10bio *r10_bio, bool handle_error)
 {
 	struct r10conf *conf = mddev->private;
 	struct bio *read_bio;
@@ -1244,7 +1244,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
 	}
 	slot = r10_bio->read_slot;
 
-	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+	if (!handle_error && blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
 		r10_bio->start_time = bio_start_io_acct(bio);
 	read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
 
@@ -1578,7 +1578,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
 			conf->geo.raid_disks);
 
 	if (bio_data_dir(bio) == READ)
-		raid10_read_request(mddev, bio, r10_bio);
+		raid10_read_request(mddev, bio, r10_bio, false);
 	else
 		raid10_write_request(mddev, bio, r10_bio);
 }
@@ -2980,7 +2980,7 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
 	rdev_dec_pending(rdev, mddev);
 	allow_barrier(conf);
 	r10_bio->state = 0;
-	raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
+	raid10_read_request(mddev, r10_bio->master_bio, r10_bio, true);
 }
 
 static void handle_write_completed(struct r10conf *conf, struct r10bio *r10_bio)
-- 
2.31.1

[PATCH v2 2/6] md: fix soft lockup in status_resync

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

status_resync() will calculate 'curr_resync - recovery_active' to show
user a progress bar like following:

[============>........]  resync = 61.4%

'curr_resync' and 'recovery_active' is updated in md_do_sync(), and
status_resync() can read them concurrently, hence it's possible that
'curr_resync - recovery_active' can overflow to a huge number. In this
case status_resync() will be stuck in the loop to print a large amount
of '=', which will end up soft lockup.

Fix the problem by setting 'resync' to MD_RESYNC_ACTIVE in this case,
this way resync in progress will be reported to user.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 546b1b81eb28..98970bbe32bf 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8009,16 +8009,16 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
 	} else if (resync > max_sectors) {
 		resync = max_sectors;
 	} else {
-		resync -= atomic_read(&mddev->recovery_active);
-		if (resync < MD_RESYNC_ACTIVE) {
-			/*
-			 * Resync has started, but the subtraction has
-			 * yielded one of the special values. Force it
-			 * to active to ensure the status reports an
-			 * active resync.
-			 */
+		res = atomic_read(&mddev->recovery_active);
+		/*
+		 * Resync has started, but the subtraction has overflowed or
+		 * yielded one of the special values. Force it to active to
+		 * ensure the status reports an active resync.
+		 */
+		if (resync < res || resync - res < MD_RESYNC_ACTIVE)
 			resync = MD_RESYNC_ACTIVE;
-		}
+		else
+			resync -= res;
 	}
 
 	if (resync == MD_RESYNC_NONE) {
-- 
2.31.1

[PATCH v2 3/6] md/raid10: don't BUG_ON() in raise_barrier()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

If raise_barrier() is called the first time in raid10_sync_request(), which
means the first non-normal io is handled, raise_barrier() should wait for
all dispatched normal io to be done. This ensures that normal io won't
starve.

However, BUG_ON() if this is broken is too aggressive. This patch replace
BUG_ON() with WARN and fall back to not force.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 4f8edb6ea3e2..a8b5fecef136 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -952,7 +952,9 @@ static void flush_pending_writes(struct r10conf *conf)
 static void raise_barrier(struct r10conf *conf, int force)
 {
 	write_seqlock_irq(&conf->resync_lock);
-	BUG_ON(force && !conf->barrier);
+
+	if (WARN_ON_ONCE(force && !conf->barrier))
+		force = false;
 
 	/* Wait until no block IO is waiting (unless 'force') */
 	wait_event_barrier(conf, force || !conf->nr_waiting);
-- 
2.31.1

[PATCH v2 4/6] md/radi10: fix leak of 'r10bio->remaining' for recovery

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

raid10_sync_request() will add 'r10bio->remaining' for both rdev and
replacement rdev. However, if the read io fails, recovery_request_write()
returns without issuing the write io, in this case, end_sync_request()
is only called once and 'remaining' is leaked, cause an io hang.

Fix the problem by decreasing 'remaining' according to if 'bio' and
'repl_bio' is valid.

Fixes: 24afd80d99f8 ("md/raid10: handle recovery of replacement devices.")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index a8b5fecef136..f7002a1aa9cf 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2611,11 +2611,22 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 {
 	struct r10conf *conf = mddev->private;
 	int d;
-	struct bio *wbio, *wbio2;
+	struct bio *wbio = r10_bio->devs[1].bio;
+	struct bio *wbio2 = r10_bio->devs[1].repl_bio;
+
+	/* Need to test wbio2->bi_end_io before we call
+	 * submit_bio_noacct as if the former is NULL,
+	 * the latter is free to free wbio2.
+	 */
+	if (wbio2 && !wbio2->bi_end_io)
+		wbio2 = NULL;
 
 	if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) {
 		fix_recovery_read_error(r10_bio);
-		end_sync_request(r10_bio);
+		if (wbio->bi_end_io)
+			end_sync_request(r10_bio);
+		if (wbio2)
+			end_sync_request(r10_bio);
 		return;
 	}
 
@@ -2624,14 +2635,6 @@ static void recovery_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	 * and submit the write request
 	 */
 	d = r10_bio->devs[1].devnum;
-	wbio = r10_bio->devs[1].bio;
-	wbio2 = r10_bio->devs[1].repl_bio;
-	/* Need to test wbio2->bi_end_io before we call
-	 * submit_bio_noacct as if the former is NULL,
-	 * the latter is free to free wbio2.
-	 */
-	if (wbio2 && !wbio2->bi_end_io)
-		wbio2 = NULL;
 	if (wbio->bi_end_io) {
 		atomic_inc(&conf->mirrors[d].rdev->nr_pending);
 		md_sync_acct(conf->mirrors[d].rdev->bdev, bio_sectors(wbio));
-- 
2.31.1

[PATCH v2 5/6] md/raid10: fix memleak for 'conf->bio_split'

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

In the error path of raid10_run(), 'conf' need be freed, however,
'conf->bio_split' is missed and memory will be leaked.

Since there are 3 places to free 'conf', factor out a helper to fix the
problem.

Fixes: fc9977dd069e ("md/raid10: simplify the splitting of requests.")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index f7002a1aa9cf..bdfa02e8fe7e 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4009,6 +4009,20 @@ static int setup_geo(struct geom *geo, struct mddev *mddev, enum geo_type new)
 	return nc*fc;
 }
 
+static void raid10_free_conf(struct r10conf *conf)
+{
+	if (!conf)
+		return;
+
+	mempool_exit(&conf->r10bio_pool);
+	kfree(conf->mirrors);
+	kfree(conf->mirrors_old);
+	kfree(conf->mirrors_new);
+	safe_put_page(conf->tmppage);
+	bioset_exit(&conf->bio_split);
+	kfree(conf);
+}
+
 static struct r10conf *setup_conf(struct mddev *mddev)
 {
 	struct r10conf *conf = NULL;
@@ -4091,13 +4105,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	return conf;
 
  out:
-	if (conf) {
-		mempool_exit(&conf->r10bio_pool);
-		kfree(conf->mirrors);
-		safe_put_page(conf->tmppage);
-		bioset_exit(&conf->bio_split);
-		kfree(conf);
-	}
+	raid10_free_conf(conf);
 	return ERR_PTR(err);
 }
 
@@ -4288,10 +4296,7 @@ static int raid10_run(struct mddev *mddev)
 
 out_free_conf:
 	md_unregister_thread(&mddev->thread);
-	mempool_exit(&conf->r10bio_pool);
-	safe_put_page(conf->tmppage);
-	kfree(conf->mirrors);
-	kfree(conf);
+	raid10_free_conf(conf);
 	mddev->private = NULL;
 out:
 	return -EIO;
@@ -4299,15 +4304,7 @@ static int raid10_run(struct mddev *mddev)
 
 static void raid10_free(struct mddev *mddev, void *priv)
 {
-	struct r10conf *conf = priv;
-
-	mempool_exit(&conf->r10bio_pool);
-	safe_put_page(conf->tmppage);
-	kfree(conf->mirrors);
-	kfree(conf->mirrors_old);
-	kfree(conf->mirrors_new);
-	bioset_exit(&conf->bio_split);
-	kfree(conf);
+	raid10_free_conf(priv);
 }
 
 static void raid10_quiesce(struct mddev *mddev, int quiesce)
-- 
2.31.1

[PATCH v2 6/6] md/raid10: fix memleak of md thread

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

In raid10_run(), if setup_conf() succeed and raid10_run() failed before
setting 'mddev->thread', then in the error path 'conf->thread' is not
freed.

Fix the problem by setting 'mddev->thread' right after setup_conf().

Fixes: 43a521238aca ("md-cluster: choose correct label when clustered layout is not supported")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index bdfa02e8fe7e..2f1522bba80d 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4142,6 +4142,9 @@ static int raid10_run(struct mddev *mddev)
 	if (!conf)
 		goto out;
 
+	mddev->thread = conf->thread;
+	conf->thread = NULL;
+
 	if (mddev_is_clustered(conf->mddev)) {
 		int fc, fo;
 
@@ -4154,9 +4157,6 @@ static int raid10_run(struct mddev *mddev)
 		}
 	}
 
-	mddev->thread = conf->thread;
-	conf->thread = NULL;
-
 	if (mddev->queue) {
 		blk_queue_max_write_zeroes_sectors(mddev->queue, 0);
 		blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
-- 
2.31.1

Re: [PATCH v2 1/6] md/raid10: don't call bio_start_io_acct twice for bio which experienced read error

[Song Liu]

On Thu, Mar 9, 2023 at 11:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> handle_read_error() will resumit r10_bio by raid10_read_request(), which
> will call bio_start_io_acct() again, while bio_end_io_acct() will only
> be called once.
>
> Fix the problem by don't account io again from handle_read_error().
>
> Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

I would rather keep same argument lists for raid10_read_request
and raid10_write_request. How about we do something like this
instead?

diff --git i/drivers/md/raid10.c w/drivers/md/raid10.c
index 6b39e6c7ada3..13f33a8a8fe8 100644
--- i/drivers/md/raid10.c
+++ w/drivers/md/raid10.c
@@ -1248,7 +1248,8 @@ static void raid10_read_request(struct mddev
*mddev, struct bio *bio,
        }
        slot = r10_bio->read_slot;

-       if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+       if (!r10_bio->start_time &&
+           blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
                r10_bio->start_time = bio_start_io_acct(bio);
        read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);

@@ -1578,6 +1579,7 @@ static void __make_request(struct mddev *mddev,
struct bio *bio, int sectors)
        r10_bio->sector = bio->bi_iter.bi_sector;
        r10_bio->state = 0;
        r10_bio->read_slot = -1;
+       r10_bio->start_time = 0;
        memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
                        conf->geo.raid_disks);

Thanks,
Song

> ---
>  drivers/md/raid10.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 6c66357f92f5..4f8edb6ea3e2 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1173,7 +1173,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
>  }
>
>  static void raid10_read_request(struct mddev *mddev, struct bio *bio,
> -                               struct r10bio *r10_bio)
> +                               struct r10bio *r10_bio, bool handle_error)
>  {
>         struct r10conf *conf = mddev->private;
>         struct bio *read_bio;
> @@ -1244,7 +1244,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>         }
>         slot = r10_bio->read_slot;
>
> -       if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
> +       if (!handle_error && blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
>                 r10_bio->start_time = bio_start_io_acct(bio);
>         read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
>
> @@ -1578,7 +1578,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>                         conf->geo.raid_disks);
>
>         if (bio_data_dir(bio) == READ)
> -               raid10_read_request(mddev, bio, r10_bio);
> +               raid10_read_request(mddev, bio, r10_bio, false);
>         else
>                 raid10_write_request(mddev, bio, r10_bio);
>  }
> @@ -2980,7 +2980,7 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>         rdev_dec_pending(rdev, mddev);
>         allow_barrier(conf);
>         r10_bio->state = 0;
> -       raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
> +       raid10_read_request(mddev, r10_bio->master_bio, r10_bio, true);
>  }
>
>  static void handle_write_completed(struct r10conf *conf, struct r10bio *r10_bio)
> --
> 2.31.1
>

Re: [PATCH v2 2/6] md: fix soft lockup in status_resync

[Song Liu]

On Thu, Mar 9, 2023 at 11:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> status_resync() will calculate 'curr_resync - recovery_active' to show
> user a progress bar like following:
>
> [============>........]  resync = 61.4%
>
> 'curr_resync' and 'recovery_active' is updated in md_do_sync(), and
> status_resync() can read them concurrently, hence it's possible that
> 'curr_resync - recovery_active' can overflow to a huge number. In this
> case status_resync() will be stuck in the loop to print a large amount
> of '=', which will end up soft lockup.
>
> Fix the problem by setting 'resync' to MD_RESYNC_ACTIVE in this case,
> this way resync in progress will be reported to user.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good. Applied to md-next.

Thanks,
Song

> ---
>  drivers/md/md.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 546b1b81eb28..98970bbe32bf 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8009,16 +8009,16 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
>         } else if (resync > max_sectors) {
>                 resync = max_sectors;
>         } else {
> -               resync -= atomic_read(&mddev->recovery_active);
> -               if (resync < MD_RESYNC_ACTIVE) {
> -                       /*
> -                        * Resync has started, but the subtraction has
> -                        * yielded one of the special values. Force it
> -                        * to active to ensure the status reports an
> -                        * active resync.
> -                        */
> +               res = atomic_read(&mddev->recovery_active);
> +               /*
> +                * Resync has started, but the subtraction has overflowed or
> +                * yielded one of the special values. Force it to active to
> +                * ensure the status reports an active resync.
> +                */
> +               if (resync < res || resync - res < MD_RESYNC_ACTIVE)
>                         resync = MD_RESYNC_ACTIVE;
> -               }
> +               else
> +                       resync -= res;
>         }
>
>         if (resync == MD_RESYNC_NONE) {
> --
> 2.31.1
>

Re: [PATCH v2 0/6] md/raid10: several simple obvious bugfix

[Song Liu]

On Thu, Mar 9, 2023 at 11:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> We're running many tests for raid10 currently, and we found a lot of
> problems already. This patchset is the first part for some simple and
> obvious problems. Most of the patches were sent separately already, but
> I think a patchset is better for review.
>
> Yu Kuai (6):
>   md/raid10: don't call bio_start_io_acct twice for bio which
>     experienced read error
>   md: fix soft lockup in status_resync
>   md/raid10: don't BUG_ON() in raise_barrier()
>   md/radi10: fix leak of 'r10bio->remaining' for recovery
>   md/raid10: fix memleak for 'conf->bio_split'
>   md/raid10: fix memleak of md thread

Applied 2/6 to 6/6 to md-next. Thanks!

Song

>
>  drivers/md/md.c     | 18 +++++------
>  drivers/md/raid10.c | 78 +++++++++++++++++++++++----------------------
>  2 files changed, 49 insertions(+), 47 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH v2 1/6] md/raid10: don't call bio_start_io_acct twice for bio which experienced read error

[Yu Kuai]

Hi,

在 2023/03/14 6:08, Song Liu 写道:
> On Thu, Mar 9, 2023 at 11:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> handle_read_error() will resumit r10_bio by raid10_read_request(), which
>> will call bio_start_io_acct() again, while bio_end_io_acct() will only
>> be called once.
>>
>> Fix the problem by don't account io again from handle_read_error().
>>
>> Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting")
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> 
> I would rather keep same argument lists for raid10_read_request
> and raid10_write_request. How about we do something like this
> instead?
> 
> diff --git i/drivers/md/raid10.c w/drivers/md/raid10.c
> index 6b39e6c7ada3..13f33a8a8fe8 100644
> --- i/drivers/md/raid10.c
> +++ w/drivers/md/raid10.c
> @@ -1248,7 +1248,8 @@ static void raid10_read_request(struct mddev
> *mddev, struct bio *bio,
>          }
>          slot = r10_bio->read_slot;
> 
> -       if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
> +       if (!r10_bio->start_time &&
> +           blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
>                  r10_bio->start_time = bio_start_io_acct(bio);
>          read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
> 
> @@ -1578,6 +1579,7 @@ static void __make_request(struct mddev *mddev,
> struct bio *bio, int sectors)
>          r10_bio->sector = bio->bi_iter.bi_sector;
>          r10_bio->state = 0;
>          r10_bio->read_slot = -1;
> +       r10_bio->start_time = 0;
>          memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>                          conf->geo.raid_disks);
> 
> Thanks,
> Song

Of course, this looks better. I'll send a new verison for this patch.

Thanks,
Kuai

[PATCH v3] md/raid10: don't call bio_start_io_acct twice for bio which experienced read error

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

handle_read_error() will resumit r10_bio by raid10_read_request(), which
will call bio_start_io_acct() again, while bio_end_io_acct() will only
be called once.

Fix the problem by don't account io again from handle_read_error().

Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting")
Suggested-by: Song Liu <song@kernel.org>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
Changes in v3:
 - use r10_bo->start_time instead of adding a new paramater.
Changes in v3:
 - Change the patch title

 drivers/md/raid10.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 6c66357f92f5..3483fdf796ec 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1244,7 +1244,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
 	}
 	slot = r10_bio->read_slot;
 
-	if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
+	if (!r10_bio->start_time &&
+	    blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
 		r10_bio->start_time = bio_start_io_acct(bio);
 	read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
 
@@ -1574,6 +1575,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
 	r10_bio->sector = bio->bi_iter.bi_sector;
 	r10_bio->state = 0;
 	r10_bio->read_slot = -1;
+	r10_bio->start_time = 0;
 	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
 			conf->geo.raid_disks);
 
-- 
2.31.1

Re: [PATCH v2 0/6] md/raid10: several simple obvious bugfix

[Yu Kuai]

Hi, Song!

在 2023/03/14 6:37, Song Liu 写道:
> On Thu, Mar 9, 2023 at 11:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> We're running many tests for raid10 currently, and we found a lot of
>> problems already. This patchset is the first part for some simple and
>> obvious problems. Most of the patches were sent separately already, but
>> I think a patchset is better for review.
>>

We had many testcase for raid10 locally，just consulte that is mdadm the
only place to push some new tests? We're maintaining tests through
blktests, is't ok if we push those tests to blktests?

Thanks,
Kuai

>> Yu Kuai (6):
>>    md/raid10: don't call bio_start_io_acct twice for bio which
>>      experienced read error
>>    md: fix soft lockup in status_resync
>>    md/raid10: don't BUG_ON() in raise_barrier()
>>    md/radi10: fix leak of 'r10bio->remaining' for recovery
>>    md/raid10: fix memleak for 'conf->bio_split'
>>    md/raid10: fix memleak of md thread
> 
> Applied 2/6 to 6/6 to md-next. Thanks!
> 
> Song
> 
>>
>>   drivers/md/md.c     | 18 +++++------
>>   drivers/md/raid10.c | 78 +++++++++++++++++++++++----------------------
>>   2 files changed, 49 insertions(+), 47 deletions(-)
>>
>> --
>> 2.31.1
>>
> .
> 

Re: [PATCH v3] md/raid10: don't call bio_start_io_acct twice for bio which experienced read error

[Song Liu]

On Mon, Mar 13, 2023 at 6:23 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> handle_read_error() will resumit r10_bio by raid10_read_request(), which
> will call bio_start_io_acct() again, while bio_end_io_acct() will only
> be called once.
>
> Fix the problem by don't account io again from handle_read_error().
>
> Fixes: 528bc2cf2fcc ("md/raid10: enable io accounting")
> Suggested-by: Song Liu <song@kernel.org>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Applied to md-next.

Thanks!
Song

> ---
> Changes in v3:
>  - use r10_bo->start_time instead of adding a new paramater.
> Changes in v3:
>  - Change the patch title
>
>  drivers/md/raid10.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 6c66357f92f5..3483fdf796ec 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1244,7 +1244,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>         }
>         slot = r10_bio->read_slot;
>
> -       if (blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
> +       if (!r10_bio->start_time &&
> +           blk_queue_io_stat(bio->bi_bdev->bd_disk->queue))
>                 r10_bio->start_time = bio_start_io_acct(bio);
>         read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
>
> @@ -1574,6 +1575,7 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>         r10_bio->sector = bio->bi_iter.bi_sector;
>         r10_bio->state = 0;
>         r10_bio->read_slot = -1;
> +       r10_bio->start_time = 0;
>         memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>                         conf->geo.raid_disks);
>
> --
> 2.31.1
>