[PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

The lifetime of badblocks:

- IO error, and decide to record badblocks, and record sb_flags;
- write IO found rdev has badblocks and not yet acknowledged, then this
IO is blocked;
- daemon found sb_flags is set, update superblock and flush badblocks;
- write IO continue;

Main idea is that badblocks will be set in memory fist, before badblocks
are acknowledged, new write request must be blocked to prevent reading
old data after power failure, and this behaviour is not necessary if rdev
is faulty in the first place.

Yu Kuai (7):
  md: add a new helper rdev_blocked()
  md: don't wait faulty rdev in md_wait_for_blocked_rdev()
  md: don't record new badblocks for faulty rdev
  md/raid1: factor out helper to handle blocked rdev from
    raid1_write_request()
  md/raid1: don't wait for Faulty rdev in wait_blocked_rdev()
  md/raid10: don't wait for Faulty rdev in wait_blocked_rdev()
  md/raid5: don't set Faulty rdev for blocked_rdev

 drivers/md/md.c     |  8 +++--
 drivers/md/md.h     | 24 +++++++++++++++
 drivers/md/raid1.c  | 75 +++++++++++++++++++++++----------------------
 drivers/md/raid10.c | 40 +++++++++++-------------
 drivers/md/raid5.c  | 13 ++++----
 5 files changed, 92 insertions(+), 68 deletions(-)

-- 
2.39.2

[PATCH md-6.12 1/7] md: add a new helper rdev_blocked()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

The helper will be used in later patches for raid1/raid10/raid5, the
difference is that Faulty rdev with unacknowledged bad block will not
be considered blocked.

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

diff --git a/drivers/md/md.h b/drivers/md/md.h
index 5d2e6bd58e4d..4ba93af36126 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -1002,6 +1002,30 @@ static inline void mddev_trace_remap(struct mddev *mddev, struct bio *bio,
 		trace_block_bio_remap(bio, disk_devt(mddev->gendisk), sector);
 }
 
+static inline bool rdev_blocked(struct md_rdev *rdev)
+{
+	/*
+	 * Blocked will be set by error handler and cleared by daemon after
+	 * updating superblock, meanwhile write IO should be blocked to prevent
+	 * reading old data after power failure.
+	 */
+	if (test_bit(Blocked, &rdev->flags))
+		return true;
+
+	/*
+	 * Faulty device should not be accessed anymore, there is no need to
+	 * wait for bad block to be acknowledged.
+	 */
+	if (test_bit(Faulty, &rdev->flags))
+		return false;
+
+	/* rdev is blocked by badblocks. */
+	if (test_bit(BlockedBadBlocks, &rdev->flags))
+		return true;
+
+	return false;
+}
+
 #define mddev_add_trace_msg(mddev, fmt, args...)			\
 do {									\
 	if (!mddev_is_dm(mddev))					\
-- 
2.39.2

[PATCH md-6.12 2/7] md: don't wait faulty rdev in md_wait_for_blocked_rdev()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

md_wait_for_blocked_rdev() is called for write IO while rdev is
blocked, howerver, rdev can be faulty after choosing this rdev to write,
and faulty rdev should never be accessed anymore, hence there is no point
to wait for faulty rdev to be unblocked.

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

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 414146111425..675d89597c7b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9728,9 +9728,7 @@ EXPORT_SYMBOL(md_reap_sync_thread);
 void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev)
 {
 	sysfs_notify_dirent_safe(rdev->sysfs_state);
-	wait_event_timeout(rdev->blocked_wait,
-			   !test_bit(Blocked, &rdev->flags) &&
-			   !test_bit(BlockedBadBlocks, &rdev->flags),
+	wait_event_timeout(rdev->blocked_wait, rdev_blocked(rdev),
 			   msecs_to_jiffies(5000));
 	rdev_dec_pending(rdev, mddev);
 }
-- 
2.39.2

[PATCH md-6.12 3/7] md: don't record new badblocks for faulty rdev

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Faulty will be checked before issuing IO to the rdev, however, rdev can
be faulty at any time, hence it's possible that rdev_set_badblocks()
will be called for faulty rdev. In this case, mddev->sb_flags will be
set and some other path can be blocked by updating super block.

Since faulty rdev will not be accesed anymore, there is no need to
record new babblocks for faulty rdev and forcing updating super block.

Noted this is not a bugfix, just prevent updating superblock in some
corner cases, and will help to slice a bug related to external
metadata[1].

[1] https://lore.kernel.org/all/f34452df-810b-48b2-a9b4-7f925699a9e7@linux.intel.com/

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

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 675d89597c7b..a3f7f407fe42 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9757,6 +9757,10 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 {
 	struct mddev *mddev = rdev->mddev;
 	int rv;
+
+	if (test_bit(Faulty, &rdev->flags))
+		return 1;
+
 	if (is_new)
 		s += rdev->new_data_offset;
 	else
-- 
2.39.2

[PATCH md-6.12 4/7] md/raid1: factor out helper to handle blocked rdev from raid1_write_request()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently raid1 is preparing IO for underlying disks while checking if
any disk is blocked, if so allocated resources must be released, then
waiting for rdev to be unblocked and try to prepare IO again.

Make code cleaner by checking blocked rdev first, it doesn't matter if
rdev is blocked while issuing this IO.

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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f55c8e67d059..aa30c3240c85 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1406,6 +1406,49 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
 	submit_bio_noacct(read_bio);
 }
 
+static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
+{
+	struct r1conf *conf = mddev->private;
+	int disks = conf->raid_disks * 2;
+	int i;
+
+retry:
+	for (i = 0; i < disks; i++) {
+		struct md_rdev *rdev = conf->mirrors[i].rdev;
+
+		if (!rdev)
+			continue;
+
+		if (test_bit(Blocked, &rdev->flags)) {
+			if (bio->bi_opf & REQ_NOWAIT)
+				return false;
+
+			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d blocked",
+					    rdev->raid_disk);
+			atomic_inc(&rdev->nr_pending);
+			md_wait_for_blocked_rdev(rdev, rdev->mddev);
+			goto retry;
+		}
+
+		/* don't write here until the bad block is acknowledged */
+		if (test_bit(WriteErrorSeen, &rdev->flags) &&
+		    rdev_has_badblock(rdev, bio->bi_iter.bi_sector,
+				      bio_sectors(bio)) < 0) {
+			if (bio->bi_opf & REQ_NOWAIT)
+				return false;
+
+			set_bit(BlockedBadBlocks, &rdev->flags);
+			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d blocked",
+					    rdev->raid_disk);
+			atomic_inc(&rdev->nr_pending);
+			md_wait_for_blocked_rdev(rdev, rdev->mddev);
+			goto retry;
+		}
+	}
+
+	return true;
+}
+
 static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 				int max_write_sectors)
 {
@@ -1413,7 +1456,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 	struct r1bio *r1_bio;
 	int i, disks;
 	unsigned long flags;
-	struct md_rdev *blocked_rdev;
 	int first_clone;
 	int max_sectors;
 	bool write_behind = false;
@@ -1451,7 +1493,11 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		return;
 	}
 
- retry_write:
+	if (!wait_blocked_rdev(mddev, bio)) {
+		bio_wouldblock_error(bio);
+		return;
+	}
+
 	r1_bio = alloc_r1bio(mddev, bio);
 	r1_bio->sectors = max_write_sectors;
 
@@ -1467,7 +1513,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 	 */
 
 	disks = conf->raid_disks * 2;
-	blocked_rdev = NULL;
 	max_sectors = r1_bio->sectors;
 	for (i = 0;  i < disks; i++) {
 		struct md_rdev *rdev = conf->mirrors[i].rdev;
@@ -1480,11 +1525,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags))
 			write_behind = true;
 
-		if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
-			atomic_inc(&rdev->nr_pending);
-			blocked_rdev = rdev;
-			break;
-		}
 		r1_bio->bios[i] = NULL;
 		if (!rdev || test_bit(Faulty, &rdev->flags)) {
 			if (i < conf->raid_disks)
@@ -1500,13 +1540,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 
 			is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
 					     &first_bad, &bad_sectors);
-			if (is_bad < 0) {
-				/* mustn't write here until the bad block is
-				 * acknowledged*/
-				set_bit(BlockedBadBlocks, &rdev->flags);
-				blocked_rdev = rdev;
-				break;
-			}
 			if (is_bad && first_bad <= r1_bio->sector) {
 				/* Cannot write here at all */
 				bad_sectors -= (r1_bio->sector - first_bad);
@@ -1537,27 +1570,6 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		r1_bio->bios[i] = bio;
 	}
 
-	if (unlikely(blocked_rdev)) {
-		/* Wait for this device to become unblocked */
-		int j;
-
-		for (j = 0; j < i; j++)
-			if (r1_bio->bios[j])
-				rdev_dec_pending(conf->mirrors[j].rdev, mddev);
-		mempool_free(r1_bio, &conf->r1bio_pool);
-		allow_barrier(conf, bio->bi_iter.bi_sector);
-
-		if (bio->bi_opf & REQ_NOWAIT) {
-			bio_wouldblock_error(bio);
-			return;
-		}
-		mddev_add_trace_msg(mddev, "raid1 wait rdev %d blocked",
-				blocked_rdev->raid_disk);
-		md_wait_for_blocked_rdev(blocked_rdev, mddev);
-		wait_barrier(conf, bio->bi_iter.bi_sector, false);
-		goto retry_write;
-	}
-
 	/*
 	 * When using a bitmap, we may call alloc_behind_master_bio below.
 	 * alloc_behind_master_bio allocates a copy of the data payload a page
-- 
2.39.2

[PATCH md-6.12 5/7] md/raid1: don't wait for Faulty rdev in wait_blocked_rdev()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Faulty rdev should never be accessed anymore, hence there is no point to
wait for bad block to be acknowledged in this case while handling write
request.

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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index aa30c3240c85..f08d7f607d8b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1419,25 +1419,16 @@ static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
 		if (!rdev)
 			continue;
 
-		if (test_bit(Blocked, &rdev->flags)) {
-			if (bio->bi_opf & REQ_NOWAIT)
-				return false;
-
-			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d blocked",
-					    rdev->raid_disk);
-			atomic_inc(&rdev->nr_pending);
-			md_wait_for_blocked_rdev(rdev, rdev->mddev);
-			goto retry;
-		}
-
 		/* don't write here until the bad block is acknowledged */
 		if (test_bit(WriteErrorSeen, &rdev->flags) &&
 		    rdev_has_badblock(rdev, bio->bi_iter.bi_sector,
-				      bio_sectors(bio)) < 0) {
+				      bio_sectors(bio)) < 0)
+			set_bit(BlockedBadBlocks, &rdev->flags);
+
+		if (rdev_blocked(rdev)) {
 			if (bio->bi_opf & REQ_NOWAIT)
 				return false;
 
-			set_bit(BlockedBadBlocks, &rdev->flags);
 			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d blocked",
 					    rdev->raid_disk);
 			atomic_inc(&rdev->nr_pending);
-- 
2.39.2

[PATCH md-6.12 6/7] md/raid10: don't wait for Faulty rdev in wait_blocked_rdev()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Faulty rdev should never be accessed anymore, hence there is no point to
wait for bad block to be acknowledged in this case while handling write
request.

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

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index f3bf1116794a..ff73db2f6c41 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1285,9 +1285,9 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
 
 static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
 {
-	int i;
 	struct r10conf *conf = mddev->private;
 	struct md_rdev *blocked_rdev;
+	int i;
 
 retry_wait:
 	blocked_rdev = NULL;
@@ -1295,40 +1295,36 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
 		struct md_rdev *rdev, *rrdev;
 
 		rdev = conf->mirrors[i].rdev;
-		rrdev = conf->mirrors[i].replacement;
-		if (rdev && unlikely(test_bit(Blocked, &rdev->flags))) {
-			atomic_inc(&rdev->nr_pending);
-			blocked_rdev = rdev;
-			break;
-		}
-		if (rrdev && unlikely(test_bit(Blocked, &rrdev->flags))) {
-			atomic_inc(&rrdev->nr_pending);
-			blocked_rdev = rrdev;
-			break;
-		}
-
-		if (rdev && test_bit(WriteErrorSeen, &rdev->flags)) {
+		if (rdev) {
 			sector_t dev_sector = r10_bio->devs[i].addr;
 
 			/*
 			 * Discard request doesn't care the write result
 			 * so it doesn't need to wait blocked disk here.
 			 */
-			if (!r10_bio->sectors)
-				continue;
-
-			if (rdev_has_badblock(rdev, dev_sector,
-					      r10_bio->sectors) < 0) {
+			if (test_bit(WriteErrorSeen, &rdev->flags) &&
+			    r10_bio->sectors &&
+			    rdev_has_badblock(rdev, dev_sector,
+					      r10_bio->sectors) < 0)
 				/*
-				 * Mustn't write here until the bad block
-				 * is acknowledged
+				 * Mustn't write here until the bad
+				 * block is acknowledged
 				 */
-				atomic_inc(&rdev->nr_pending);
 				set_bit(BlockedBadBlocks, &rdev->flags);
+
+			if (rdev_blocked(rdev)) {
 				blocked_rdev = rdev;
+				atomic_inc(&rdev->nr_pending);
 				break;
 			}
 		}
+
+		rrdev = conf->mirrors[i].replacement;
+		if (rrdev && rdev_blocked(rrdev)) {
+			atomic_inc(&rrdev->nr_pending);
+			blocked_rdev = rrdev;
+			break;
+		}
 	}
 
 	if (unlikely(blocked_rdev)) {
-- 
2.39.2

[PATCH md-6.12 7/7] md/raid5: don't set Faulty rdev for blocked_rdev

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Faulty rdev should never be accessed anymore, hence there is no point to
wait for bad block to be acknowledged in this case while handling write
request.

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

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c84a7e0263cd..fb56c3f9ea87 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -4724,14 +4724,13 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
 		if (rdev) {
 			is_bad = rdev_has_badblock(rdev, sh->sector,
 						   RAID5_STRIPE_SECTORS(conf));
-			if (s->blocked_rdev == NULL
-			    && (test_bit(Blocked, &rdev->flags)
-				|| is_bad < 0)) {
+			if (s->blocked_rdev == NULL) {
 				if (is_bad < 0)
-					set_bit(BlockedBadBlocks,
-						&rdev->flags);
-				s->blocked_rdev = rdev;
-				atomic_inc(&rdev->nr_pending);
+					set_bit(BlockedBadBlocks, &rdev->flags);
+				if (rdev_blocked(rdev)) {
+					s->blocked_rdev = rdev;
+					atomic_inc(&rdev->nr_pending);
+				}
 			}
 		}
 		clear_bit(R5_Insync, &dev->flags);
-- 
2.39.2

Re: [PATCH md-6.12 3/7] md: don't record new badblocks for faulty rdev

[Mariusz Tkaczyk]

On Fri, 30 Aug 2024 15:27:17 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> Faulty will be checked before issuing IO to the rdev, however, rdev can
> be faulty at any time, hence it's possible that rdev_set_badblocks()
> will be called for faulty rdev. In this case, mddev->sb_flags will be
> set and some other path can be blocked by updating super block.
> 
> Since faulty rdev will not be accesed anymore, there is no need to
> record new babblocks for faulty rdev and forcing updating super block.
> 
> Noted this is not a bugfix, just prevent updating superblock in some
> corner cases, and will help to slice a bug related to external
> metadata[1].
> 
> [1]
> https://lore.kernel.org/all/f34452df-810b-48b2-a9b4-7f925699a9e7@linux.intel.com/
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 675d89597c7b..a3f7f407fe42 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9757,6 +9757,10 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t
> s, int sectors, {
>  	struct mddev *mddev = rdev->mddev;
>  	int rv;
> +
> +	if (test_bit(Faulty, &rdev->flags))
> +		return 1;
> +

Blame is volatile, this is why we need a comment here :)
Otherwise, someone may remove that.

Thanks,
Mariusz

Re: [PATCH md-6.12 4/7] md/raid1: factor out helper to handle blocked rdev from raid1_write_request()

[Mariusz Tkaczyk]

On Fri, 30 Aug 2024 15:27:18 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently raid1 is preparing IO for underlying disks while checking if
> any disk is blocked, if so allocated resources must be released, then
> waiting for rdev to be unblocked and try to prepare IO again.
> 
> Make code cleaner by checking blocked rdev first, it doesn't matter if
> rdev is blocked while issuing this IO.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/raid1.c | 84 ++++++++++++++++++++++++++--------------------
>  1 file changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f55c8e67d059..aa30c3240c85 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1406,6 +1406,49 @@ static void raid1_read_request(struct mddev *mddev,
> struct bio *bio, submit_bio_noacct(read_bio);
>  }
>  
> +static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
> +{
> +	struct r1conf *conf = mddev->private;
> +	int disks = conf->raid_disks * 2;
> +	int i;
> +
> +retry:
> +	for (i = 0; i < disks; i++) {
> +		struct md_rdev *rdev = conf->mirrors[i].rdev;
> +
> +		if (!rdev)
> +			continue;
> +
> +		if (test_bit(Blocked, &rdev->flags)) {
Don't we need unlikely here?


> +			if (bio->bi_opf & REQ_NOWAIT)
> +				return false;
> +
> +			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d
> blocked",
> +					    rdev->raid_disk);
> +			atomic_inc(&rdev->nr_pending);


retry moves us before for (ugh, ugly) and "theoretically" we can back here
with the same disk and increase nr_pending twice or more because rdve can become
block again from different thread.

This is what I suspect but it could be wrong.

Mariusz

Re: [PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Mariusz Tkaczyk]

On Fri, 30 Aug 2024 15:27:14 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> The lifetime of badblocks:
> 
> - IO error, and decide to record badblocks, and record sb_flags;
> - write IO found rdev has badblocks and not yet acknowledged, then this
> IO is blocked;
> - daemon found sb_flags is set, update superblock and flush badblocks;
> - write IO continue;
> 
> Main idea is that badblocks will be set in memory fist, before badblocks
> are acknowledged, new write request must be blocked to prevent reading
> old data after power failure, and this behaviour is not necessary if rdev
> is faulty in the first place.
> 
> Yu Kuai (7):
>   md: add a new helper rdev_blocked()
>   md: don't wait faulty rdev in md_wait_for_blocked_rdev()
>   md: don't record new badblocks for faulty rdev
>   md/raid1: factor out helper to handle blocked rdev from
>     raid1_write_request()
>   md/raid1: don't wait for Faulty rdev in wait_blocked_rdev()
>   md/raid10: don't wait for Faulty rdev in wait_blocked_rdev()
>   md/raid5: don't set Faulty rdev for blocked_rdev
> 
>  drivers/md/md.c     |  8 +++--
>  drivers/md/md.h     | 24 +++++++++++++++
>  drivers/md/raid1.c  | 75 +++++++++++++++++++++++----------------------
>  drivers/md/raid10.c | 40 +++++++++++-------------
>  drivers/md/raid5.c  | 13 ++++----
>  5 files changed, 92 insertions(+), 68 deletions(-)
> 

Hi Song,
We need to test this with external metadata so please wait for our green light
before you will take this.
I checked the code and it looks safe but I need to double confirm it to avoid
hung tasks.

Thanks,
Mariusz

Re: [PATCH md-6.12 4/7] md/raid1: factor out helper to handle blocked rdev from raid1_write_request()

[Yu Kuai]

Hi,

在 2024/08/30 19:06, Mariusz Tkaczyk 写道:
> On Fri, 30 Aug 2024 15:27:18 +0800
> Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Currently raid1 is preparing IO for underlying disks while checking if
>> any disk is blocked, if so allocated resources must be released, then
>> waiting for rdev to be unblocked and try to prepare IO again.
>>
>> Make code cleaner by checking blocked rdev first, it doesn't matter if
>> rdev is blocked while issuing this IO.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid1.c | 84 ++++++++++++++++++++++++++--------------------
>>   1 file changed, 48 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index f55c8e67d059..aa30c3240c85 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1406,6 +1406,49 @@ static void raid1_read_request(struct mddev *mddev,
>> struct bio *bio, submit_bio_noacct(read_bio);
>>   }
>>   
>> +static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
>> +{
>> +	struct r1conf *conf = mddev->private;
>> +	int disks = conf->raid_disks * 2;
>> +	int i;
>> +
>> +retry:
>> +	for (i = 0; i < disks; i++) {
>> +		struct md_rdev *rdev = conf->mirrors[i].rdev;
>> +
>> +		if (!rdev)
>> +			continue;
>> +
>> +		if (test_bit(Blocked, &rdev->flags)) {
> Don't we need unlikely here?
> 
> 
>> +			if (bio->bi_opf & REQ_NOWAIT)
>> +				return false;
>> +
>> +			mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d
>> blocked",
>> +					    rdev->raid_disk);
>> +			atomic_inc(&rdev->nr_pending);
> 
> 
> retry moves us before for (ugh, ugly) and "theoretically" we can back here
> with the same disk and increase nr_pending twice or more because rdve can become
> block again from different thread.

Rety is always after md_wait_for_blocked_rdev(), which decrease the
nr_pending.

Thanks,
Kuai

> 
> This is what I suspect but it could be wrong.
> 
> Mariusz
> 
> 
> 
> .
> 

Re: [PATCH md-6.12 3/7] md: don't record new badblocks for faulty rdev

[Yu Kuai]

Hi,

在 2024/08/30 18:28, Mariusz Tkaczyk 写道:
> On Fri, 30 Aug 2024 15:27:17 +0800
> Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Faulty will be checked before issuing IO to the rdev, however, rdev can
>> be faulty at any time, hence it's possible that rdev_set_badblocks()
>> will be called for faulty rdev. In this case, mddev->sb_flags will be
>> set and some other path can be blocked by updating super block.
>>
>> Since faulty rdev will not be accesed anymore, there is no need to
>> record new babblocks for faulty rdev and forcing updating super block.
>>
>> Noted this is not a bugfix, just prevent updating superblock in some
>> corner cases, and will help to slice a bug related to external
>> metadata[1].
>>
>> [1]
>> https://lore.kernel.org/all/f34452df-810b-48b2-a9b4-7f925699a9e7@linux.intel.com/
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/md.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 675d89597c7b..a3f7f407fe42 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9757,6 +9757,10 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t
>> s, int sectors, {
>>   	struct mddev *mddev = rdev->mddev;
>>   	int rv;
>> +
>> +	if (test_bit(Faulty, &rdev->flags))
>> +		return 1;
>> +
> 
> Blame is volatile, this is why we need a comment here :)
> Otherwise, someone may remove that.

Perhaps something like following?

/*
  * record new babblocks for faulty rdev will force unnecessary
  * super block updating.
  */

Thanks,
Kuai

> 
> Thanks,
> Mariusz
> 
> 
> .
> 

Re: [PATCH md-6.12 3/7] md: don't record new badblocks for faulty rdev

[Mariusz Tkaczyk]

On Sat, 31 Aug 2024 09:14:39 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> Hi,
> 
> 在 2024/08/30 18:28, Mariusz Tkaczyk 写道:
> > On Fri, 30 Aug 2024 15:27:17 +0800
> > Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >   
> >> From: Yu Kuai <yukuai3@huawei.com>
> >>
> >> Faulty will be checked before issuing IO to the rdev, however, rdev can
> >> be faulty at any time, hence it's possible that rdev_set_badblocks()
> >> will be called for faulty rdev. In this case, mddev->sb_flags will be
> >> set and some other path can be blocked by updating super block.
> >>
> >> Since faulty rdev will not be accesed anymore, there is no need to
> >> record new babblocks for faulty rdev and forcing updating super block.
> >>
> >> Noted this is not a bugfix, just prevent updating superblock in some
> >> corner cases, and will help to slice a bug related to external
> >> metadata[1].
> >>
> >> [1]
> >> https://lore.kernel.org/all/f34452df-810b-48b2-a9b4-7f925699a9e7@linux.intel.com/
> >>
> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >> ---
> >>   drivers/md/md.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 675d89597c7b..a3f7f407fe42 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -9757,6 +9757,10 @@ int rdev_set_badblocks(struct md_rdev *rdev,
> >> sector_t s, int sectors, {
> >>   	struct mddev *mddev = rdev->mddev;
> >>   	int rv;
> >> +
> >> +	if (test_bit(Faulty, &rdev->flags))
> >> +		return 1;
> >> +  
> > 
> > Blame is volatile, this is why we need a comment here :)
> > Otherwise, someone may remove that.  
> 
> Perhaps something like following?
> 
> /*
>   * record new babblocks for faulty rdev will force unnecessary
>   * super block updating.
>   */
> 

Almost, we need to refer to external context because this is important to
mention where to expect issues:

/*
 * Recording new badblocks for faulty rdev will force unnecessary
 * super block updating. This is fragile for external management because
 * userspace daemon may trying to remove this device and deadlock may
 * occur. This will be probably solved in the mdadm, but it is safer to avoid
 * it.
 */

In my testing, I observed that it improves failing bios and device removal
path (recording badblock is simply expensive if there are many badblocks) so
the devices are removed faster but I don't have data here, this is what I saw.

Obviously, it is optimization.

Mariusz

Re: [PATCH md-6.12 3/7] md: don't record new badblocks for faulty rdev

[Yu Kuai]

Hi,

在 2024/09/02 16:55, Mariusz Tkaczyk 写道:
> On Sat, 31 Aug 2024 09:14:39 +0800
> Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> Hi,
>>
>> 在 2024/08/30 18:28, Mariusz Tkaczyk 写道:
>>> On Fri, 30 Aug 2024 15:27:17 +0800
>>> Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>    
>>>> From: Yu Kuai <yukuai3@huawei.com>
>>>>
>>>> Faulty will be checked before issuing IO to the rdev, however, rdev can
>>>> be faulty at any time, hence it's possible that rdev_set_badblocks()
>>>> will be called for faulty rdev. In this case, mddev->sb_flags will be
>>>> set and some other path can be blocked by updating super block.
>>>>
>>>> Since faulty rdev will not be accesed anymore, there is no need to
>>>> record new babblocks for faulty rdev and forcing updating super block.
>>>>
>>>> Noted this is not a bugfix, just prevent updating superblock in some
>>>> corner cases, and will help to slice a bug related to external
>>>> metadata[1].
>>>>
>>>> [1]
>>>> https://lore.kernel.org/all/f34452df-810b-48b2-a9b4-7f925699a9e7@linux.intel.com/
>>>>
>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>> ---
>>>>    drivers/md/md.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>>> index 675d89597c7b..a3f7f407fe42 100644
>>>> --- a/drivers/md/md.c
>>>> +++ b/drivers/md/md.c
>>>> @@ -9757,6 +9757,10 @@ int rdev_set_badblocks(struct md_rdev *rdev,
>>>> sector_t s, int sectors, {
>>>>    	struct mddev *mddev = rdev->mddev;
>>>>    	int rv;
>>>> +
>>>> +	if (test_bit(Faulty, &rdev->flags))
>>>> +		return 1;
>>>> +
>>>
>>> Blame is volatile, this is why we need a comment here :)
>>> Otherwise, someone may remove that.
>>
>> Perhaps something like following?
>>
>> /*
>>    * record new babblocks for faulty rdev will force unnecessary
>>    * super block updating.
>>    */
>>
> 
> Almost, we need to refer to external context because this is important to
> mention where to expect issues:
> 
> /*
>   * Recording new badblocks for faulty rdev will force unnecessary
>   * super block updating. This is fragile for external management because
>   * userspace daemon may trying to remove this device and deadlock may
>   * occur. This will be probably solved in the mdadm, but it is safer to avoid
>   * it.
>   */
> 
> In my testing, I observed that it improves failing bios and device removal
> path (recording badblock is simply expensive if there are many badblocks) so
> the devices are removed faster but I don't have data here, this is what I saw.

I'll mention this in the commit message, and add the above comment in
v2.

Thanks,
Kuai

> 
> Obviously, it is optimization.
> 
> Mariusz
> 
> .
> 

Re: [PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Mariusz Tkaczyk]

On Fri, 30 Aug 2024 15:27:14 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> From: Yu Kuai <yukuai3@huawei.com>
> 
> The lifetime of badblocks:
> 
> - IO error, and decide to record badblocks, and record sb_flags;
> - write IO found rdev has badblocks and not yet acknowledged, then this
> IO is blocked;
> - daemon found sb_flags is set, update superblock and flush badblocks;
> - write IO continue;
> 
> Main idea is that badblocks will be set in memory fist, before badblocks
> are acknowledged, new write request must be blocked to prevent reading
> old data after power failure, and this behaviour is not necessary if rdev
> is faulty in the first place.
> 
> Yu Kuai (7):
>   md: add a new helper rdev_blocked()
>   md: don't wait faulty rdev in md_wait_for_blocked_rdev()
>   md: don't record new badblocks for faulty rdev
>   md/raid1: factor out helper to handle blocked rdev from
>     raid1_write_request()
>   md/raid1: don't wait for Faulty rdev in wait_blocked_rdev()
>   md/raid10: don't wait for Faulty rdev in wait_blocked_rdev()
>   md/raid5: don't set Faulty rdev for blocked_rdev
> 
>  drivers/md/md.c     |  8 +++--
>  drivers/md/md.h     | 24 +++++++++++++++
>  drivers/md/raid1.c  | 75 +++++++++++++++++++++++----------------------
>  drivers/md/raid10.c | 40 +++++++++++-------------
>  drivers/md/raid5.c  | 13 ++++----
>  5 files changed, 92 insertions(+), 68 deletions(-)
> 


Hi,
We tested this patchset.

mdmon rework:
https://github.com/md-raid-utilities/mdadm/pull/66 

Kernel build torvalds/linux.git master:
commit e32cde8d2bd7d251a8f9b434143977ddf13dcec6

I applied this patchset on top of that.

My tests proved that:
- If only mdmon PR is applied - hangs are reproducible.
- If only this patchset is applied - hangs are reproducible.
- If both kernel patchset and mdmon rework are applied- hangs are not
  reproducible (at least until now).

It was tricky topic (I needed to deal with weird issues related to shared
descriptors in mdmon).

What the most important- there is no regression detected.

Thanks,
Mariusz

Re: [PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Paul Menzel]

Dear Kuai,


Thank you for this patch series. Just a note about the typo in che*ck*ing.


Kind regards,

Paul

Re: [PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Yu Kuai]

Hi,

在 2024/10/09 15:14, Mariusz Tkaczyk 写道:
> On Fri, 30 Aug 2024 15:27:14 +0800
> Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> The lifetime of badblocks:
>>
>> - IO error, and decide to record badblocks, and record sb_flags;
>> - write IO found rdev has badblocks and not yet acknowledged, then this
>> IO is blocked;
>> - daemon found sb_flags is set, update superblock and flush badblocks;
>> - write IO continue;
>>
>> Main idea is that badblocks will be set in memory fist, before badblocks
>> are acknowledged, new write request must be blocked to prevent reading
>> old data after power failure, and this behaviour is not necessary if rdev
>> is faulty in the first place.
>>
>> Yu Kuai (7):
>>    md: add a new helper rdev_blocked()
>>    md: don't wait faulty rdev in md_wait_for_blocked_rdev()
>>    md: don't record new badblocks for faulty rdev
>>    md/raid1: factor out helper to handle blocked rdev from
>>      raid1_write_request()
>>    md/raid1: don't wait for Faulty rdev in wait_blocked_rdev()
>>    md/raid10: don't wait for Faulty rdev in wait_blocked_rdev()
>>    md/raid5: don't set Faulty rdev for blocked_rdev
>>
>>   drivers/md/md.c     |  8 +++--
>>   drivers/md/md.h     | 24 +++++++++++++++
>>   drivers/md/raid1.c  | 75 +++++++++++++++++++++++----------------------
>>   drivers/md/raid10.c | 40 +++++++++++-------------
>>   drivers/md/raid5.c  | 13 ++++----
>>   5 files changed, 92 insertions(+), 68 deletions(-)
>>
> 
> 
> Hi,
> We tested this patchset.
> 
> mdmon rework:
> https://github.com/md-raid-utilities/mdadm/pull/66
> 
> Kernel build torvalds/linux.git master:
> commit e32cde8d2bd7d251a8f9b434143977ddf13dcec6
> 
> I applied this patchset on top of that.
> 
> My tests proved that:
> - If only mdmon PR is applied - hangs are reproducible.
> - If only this patchset is applied - hangs are reproducible.
> - If both kernel patchset and mdmon rework are applied- hangs are not
>    reproducible (at least until now).
> 
> It was tricky topic (I needed to deal with weird issues related to shared
> descriptors in mdmon).
> 
> What the most important- there is no regression detected.

Good to here that, I'll send a V2 then. Usually this set will land in
v6.13, because this doesn't look like a fix in kernel. :)

Thanks,
Kuai

> 
> Thanks,
> Mariusz
> 
> .
> 

Re: [PATCH md-6.12 0/7] md: enhance faulty chekcing for blocked handling

[Yu Kuai]

Hi,

在 2024/10/09 16:52, Paul Menzel 写道:
> Dear Kuai,
> 
> 
> Thank you for this patch series. Just a note about the typo in che*ck*ing.

Thanks for the notice. :)

Kuai

> 
> 
> Kind regards,
> 
> Paul
> .
>