[PATCH 00/12] badblocks: bugfix and cleanup for badblocks

[PATCH 00/12] badblocks: bugfix and cleanup for badblocks

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

During RAID feature implementation testing, we found several bugs
in badblocks.

This series contains bugfixes and cleanups for MD RAID badblocks
handling code.

Li Nan (8):
  badblocks: Fix error shitf ops
  badblocks: factor out a helper try_adjacent_combine
  badblocks: attempt to merge adjacent badblocks during
    ack_all_badblocks
  badblocks: return error directly when setting badblocks exceeds 512
  badblocks: return error if any badblock set fails
  badblocks: fix the using of MAX_BADBLOCKS
  badblocks: try can_merge_front before overlap_front
  badblocks: fix merge issue when new badblocks align with pre+1

Zheng Qixing (4):
  badblocks: fix missing bad blocks on retry in _badblocks_check()
  badblocks: return boolen from badblocks_set() and badblocks_clear()
  md: improve return types of badblocks handling functions
  badblocks: use sector_t instead of int to avoid truncation of
    badblocks length

 block/badblocks.c             | 317 +++++++++++++---------------------
 drivers/block/null_blk/main.c |  19 +-
 drivers/md/md.c               |  47 +++--
 drivers/md/md.h               |  14 +-
 drivers/md/raid1-10.c         |   2 +-
 drivers/md/raid1.c            |  10 +-
 drivers/md/raid10.c           |  14 +-
 drivers/nvdimm/badrange.c     |   2 +-
 drivers/nvdimm/nd.h           |   2 +-
 drivers/nvdimm/pfn_devs.c     |   7 +-
 drivers/nvdimm/pmem.c         |   2 +-
 include/linux/badblocks.h     |  10 +-
 12 files changed, 181 insertions(+), 265 deletions(-)

-- 
2.39.2

[PATCH 01/12] badblocks: Fix error shitf ops

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

'bb->shift' is used directly in badblocks. It is wrong, fix it.

Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index db4ec8b9b2a8..bcee057efc47 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -880,8 +880,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		/* round the start down, and the end up */
 		sector_t next = s + sectors;
 
-		rounddown(s, bb->shift);
-		roundup(next, bb->shift);
+		rounddown(s, 1 << bb->shift);
+		roundup(next, 1 << bb->shift);
 		sectors = next - s;
 	}
 
@@ -1157,8 +1157,8 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 		 * isn't than to think a block is not bad when it is.
 		 */
 		target = s + sectors;
-		roundup(s, bb->shift);
-		rounddown(target, bb->shift);
+		roundup(s, 1 << bb->shift);
+		rounddown(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
@@ -1288,8 +1288,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 
 		/* round the start down, and the end up */
 		target = s + sectors;
-		rounddown(s, bb->shift);
-		roundup(target, bb->shift);
+		rounddown(s, 1 << bb->shift);
+		roundup(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
-- 
2.39.2

[PATCH 02/12] badblocks: factor out a helper try_adjacent_combine

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

Factor out try_adjacent_combine(), and it will be used in the later patch.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index bcee057efc47..f069c93e986d 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -855,6 +855,31 @@ static void badblocks_update_acked(struct badblocks *bb)
 		bb->unacked_exist = 0;
 }
 
+/*
+ * Return 'true' if the range indicated by 'bad' is exactly backward
+ * overlapped with the bad range (from bad table) indexed by 'behind'.
+ */
+static bool try_adjacent_combine(struct badblocks *bb, int prev)
+{
+	u64 *p = bb->page;
+
+	if (prev >= 0 && (prev + 1) < bb->count &&
+	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
+	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
+	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
+		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
+				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
+				  BB_ACK(p[prev]));
+
+		if ((prev + 2) < bb->count)
+			memmove(p + prev + 1, p + prev + 2,
+				(bb->count -  (prev + 2)) * 8);
+		bb->count--;
+		return true;
+	}
+	return false;
+}
+
 /* Do exact work to set bad block range into the bad block table */
 static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			  int acknowledged)
@@ -1022,20 +1047,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	 * merged. (prev < 0) condition is not handled here,
 	 * because it's already complicated enough.
 	 */
-	if (prev >= 0 &&
-	    (prev + 1) < bb->count &&
-	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
-	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
-	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
-		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
-				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
-				  BB_ACK(p[prev]));
-
-		if ((prev + 2) < bb->count)
-			memmove(p + prev + 1, p + prev + 2,
-				(bb->count -  (prev + 2)) * 8);
-		bb->count--;
-	}
+	try_adjacent_combine(bb, prev);
 
 	if (space_desired && !badblocks_full(bb)) {
 		s = orig_start;
-- 
2.39.2

[PATCH 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

If ack and unack badblocks are adjacent, they will not be merged and will
remain as two separate badblocks. Even after the bad blocks are written
to disk and both become ack, they will still remain as two independent
bad blocks. This is not ideal as it wastes the limited space for
badblocks. Therefore, during ack_all_badblocks(), attempt to merge
badblocks if they are adjacent.

Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/badblocks.c b/block/badblocks.c
index f069c93e986d..ad8652fbe1c8 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1491,6 +1491,11 @@ void ack_all_badblocks(struct badblocks *bb)
 				p[i] = BB_MAKE(start, len, 1);
 			}
 		}
+
+		for (i = 0; i < bb->count ; i++)
+			while (try_adjacent_combine(bb, i))
+				;
+
 		bb->unacked_exist = 0;
 	}
 	write_sequnlock_irq(&bb->lock);
-- 
2.39.2

[PATCH 04/12] badblocks: return error directly when setting badblocks exceeds 512

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

In the current handling of badblocks settings, a lot of processing has
been done for scenarios where the number of badblocks exceeds 512.
This makes the code look quite complex and also introduces some issues,

Fixing those issues wouldn’t be too complicated, but it wouldn’t
simplify the code. In fact, a disk shouldn’t have too many badblocks,
and for disks with 512 badblocks, attempting to set more bad blocks
doesn’t make much sense. At that point, the more appropriate action
would be to replace the disk. Therefore, to resolve these issues and
simplify the code somewhat, return error directly when setting badblocks
exceeds 512.

Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 121 ++++++++--------------------------------------
 1 file changed, 19 insertions(+), 102 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index ad8652fbe1c8..1c8b8f65f6df 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -527,51 +527,6 @@ static int prev_badblocks(struct badblocks *bb, struct badblocks_context *bad,
 	return ret;
 }
 
-/*
- * Return 'true' if the range indicated by 'bad' can be backward merged
- * with the bad range (from the bad table) index by 'behind'.
- */
-static bool can_merge_behind(struct badblocks *bb,
-			     struct badblocks_context *bad, int behind)
-{
-	sector_t sectors = bad->len;
-	sector_t s = bad->start;
-	u64 *p = bb->page;
-
-	if ((s < BB_OFFSET(p[behind])) &&
-	    ((s + sectors) >= BB_OFFSET(p[behind])) &&
-	    ((BB_END(p[behind]) - s) <= BB_MAX_LEN) &&
-	    BB_ACK(p[behind]) == bad->ack)
-		return true;
-	return false;
-}
-
-/*
- * Do backward merge for range indicated by 'bad' and the bad range
- * (from the bad table) indexed by 'behind'. The return value is merged
- * sectors from bad->len.
- */
-static int behind_merge(struct badblocks *bb, struct badblocks_context *bad,
-			int behind)
-{
-	sector_t sectors = bad->len;
-	sector_t s = bad->start;
-	u64 *p = bb->page;
-	int merged = 0;
-
-	WARN_ON(s >= BB_OFFSET(p[behind]));
-	WARN_ON((s + sectors) < BB_OFFSET(p[behind]));
-
-	if (s < BB_OFFSET(p[behind])) {
-		merged = BB_OFFSET(p[behind]) - s;
-		p[behind] =  BB_MAKE(s, BB_LEN(p[behind]) + merged, bad->ack);
-
-		WARN_ON((BB_LEN(p[behind]) + merged) >= BB_MAX_LEN);
-	}
-
-	return merged;
-}
-
 /*
  * Return 'true' if the range indicated by 'bad' can be forward
  * merged with the bad range (from the bad table) indexed by 'prev'.
@@ -884,11 +839,9 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
 static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			  int acknowledged)
 {
-	int retried = 0, space_desired = 0;
-	int orig_len, len = 0, added = 0;
+	int len = 0, added = 0;
 	struct badblocks_context bad;
 	int prev = -1, hint = -1;
-	sector_t orig_start;
 	unsigned long flags;
 	int rv = 0;
 	u64 *p;
@@ -912,8 +865,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 
 	write_seqlock_irqsave(&bb->lock, flags);
 
-	orig_start = s;
-	orig_len = sectors;
 	bad.ack = acknowledged;
 	p = bb->page;
 
@@ -922,6 +873,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	bad.len = sectors;
 	len = 0;
 
+	if (badblocks_full(bb)) {
+		rv = 1;
+		goto out;
+	}
+
 	if (badblocks_empty(bb)) {
 		len = insert_at(bb, 0, &bad);
 		bb->count++;
@@ -933,32 +889,14 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 
 	/* start before all badblocks */
 	if (prev < 0) {
-		if (!badblocks_full(bb)) {
-			/* insert on the first */
-			if (bad.len > (BB_OFFSET(p[0]) - bad.start))
-				bad.len = BB_OFFSET(p[0]) - bad.start;
-			len = insert_at(bb, 0, &bad);
-			bb->count++;
-			added++;
-			hint = 0;
-			goto update_sectors;
-		}
-
-		/* No sapce, try to merge */
-		if (overlap_behind(bb, &bad, 0)) {
-			if (can_merge_behind(bb, &bad, 0)) {
-				len = behind_merge(bb, &bad, 0);
-				added++;
-			} else {
-				len = BB_OFFSET(p[0]) - s;
-				space_desired = 1;
-			}
-			hint = 0;
-			goto update_sectors;
-		}
-
-		/* no table space and give up */
-		goto out;
+		/* insert on the first */
+		if (bad.len > (BB_OFFSET(p[0]) - bad.start))
+			bad.len = BB_OFFSET(p[0]) - bad.start;
+		len = insert_at(bb, 0, &bad);
+		bb->count++;
+		added++;
+		hint = 0;
+		goto update_sectors;
 	}
 
 	/* in case p[prev-1] can be merged with p[prev] */
@@ -978,6 +916,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			int extra = 0;
 
 			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
+				if (extra > 0) {
+					rv = 1;
+					goto out;
+				}
+
 				len = min_t(sector_t,
 					    BB_END(p[prev]) - s, sectors);
 				hint = prev;
@@ -1004,24 +947,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		goto update_sectors;
 	}
 
-	/* if no space in table, still try to merge in the covered range */
-	if (badblocks_full(bb)) {
-		/* skip the cannot-merge range */
-		if (((prev + 1) < bb->count) &&
-		    overlap_behind(bb, &bad, prev + 1) &&
-		    ((s + sectors) >= BB_END(p[prev + 1]))) {
-			len = BB_END(p[prev + 1]) - s;
-			hint = prev + 1;
-			goto update_sectors;
-		}
-
-		/* no retry any more */
-		len = sectors;
-		space_desired = 1;
-		hint = -1;
-		goto update_sectors;
-	}
-
 	/* cannot merge and there is space in bad table */
 	if ((prev + 1) < bb->count &&
 	    overlap_behind(bb, &bad, prev + 1))
@@ -1049,14 +974,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	 */
 	try_adjacent_combine(bb, prev);
 
-	if (space_desired && !badblocks_full(bb)) {
-		s = orig_start;
-		sectors = orig_len;
-		space_desired = 0;
-		if (retried++ < 3)
-			goto re_insert;
-	}
-
 out:
 	if (added) {
 		set_changed(bb);
-- 
2.39.2

[PATCH 05/12] badblocks: return error if any badblock set fails

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

_badblocks_set() returns success if at least one badblock is set
successfully, even if others fail. This can lead to data inconsistencies
in raid, where a failed badblock set should trigger the disk to be kicked
out to prevent future reads from failed write areas.

_badblocks_set() should return error if any badblock set fails. Instead
of relying on 'rv', directly returning 'sectors' for clearer logic. If all
badblocks are successfully set, 'sectors' will be 0, otherwise it
indicates the number of badblocks that have not been set yet, thus
signaling failure.

By the way, it can also fix an issue: when a newly set unack badblock is
included in an existing ack badblock, the setting will return an error.
···
  echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
  echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
  -bash: echo: write error: No space left on device
```
After fix, it will return success.

Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 1c8b8f65f6df..a953d2e9417f 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -843,7 +843,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	struct badblocks_context bad;
 	int prev = -1, hint = -1;
 	unsigned long flags;
-	int rv = 0;
 	u64 *p;
 
 	if (bb->shift < 0)
@@ -873,10 +872,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	bad.len = sectors;
 	len = 0;
 
-	if (badblocks_full(bb)) {
-		rv = 1;
+	if (badblocks_full(bb))
 		goto out;
-	}
 
 	if (badblocks_empty(bb)) {
 		len = insert_at(bb, 0, &bad);
@@ -916,10 +913,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			int extra = 0;
 
 			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
-				if (extra > 0) {
-					rv = 1;
+				if (extra > 0)
 					goto out;
-				}
 
 				len = min_t(sector_t,
 					    BB_END(p[prev]) - s, sectors);
@@ -986,10 +981,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 
 	write_sequnlock_irqrestore(&bb->lock, flags);
 
-	if (!added)
-		rv = 1;
-
-	return rv;
+	return sectors;
 }
 
 /*
@@ -1353,7 +1345,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
  *
  * Return:
  *  0: success
- *  1: failed to set badblocks (out of space)
+ *  other: failed to set badblocks (out of space)
  */
 int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 			int acknowledged)
-- 
2.39.2

[PATCH 06/12] badblocks: fix the using of MAX_BADBLOCKS

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

The number of badblocks cannot exceed MAX_BADBLOCKS, but it should be
allowed to equal MAX_BADBLOCKS.

Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index a953d2e9417f..87267bae6836 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -700,7 +700,7 @@ static bool can_front_overwrite(struct badblocks *bb, int prev,
 			*extra = 2;
 	}
 
-	if ((bb->count + (*extra)) >= MAX_BADBLOCKS)
+	if ((bb->count + (*extra)) > MAX_BADBLOCKS)
 		return false;
 
 	return true;
@@ -1135,7 +1135,7 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 		if ((BB_OFFSET(p[prev]) < bad.start) &&
 		    (BB_END(p[prev]) > (bad.start + bad.len))) {
 			/* Splitting */
-			if ((bb->count + 1) < MAX_BADBLOCKS) {
+			if ((bb->count + 1) <= MAX_BADBLOCKS) {
 				len = front_splitting_clear(bb, prev, &bad);
 				bb->count += 1;
 				cleared++;
-- 
2.39.2

[PATCH 07/12] badblocks: try can_merge_front before overlap_front

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

Regardless of whether overlap_front() returns true or false,
can_merge_front() will be executed first. Therefore, move
can_merge_front() in front of can_merge_front() to simplify code.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 48 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 87267bae6836..bb46bab7e99f 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -905,39 +905,35 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		goto update_sectors;
 	}
 
+	if (can_merge_front(bb, prev, &bad)) {
+		len = front_merge(bb, prev, &bad);
+		added++;
+		hint = prev;
+		goto update_sectors;
+	}
+
 	if (overlap_front(bb, prev, &bad)) {
-		if (can_merge_front(bb, prev, &bad)) {
-			len = front_merge(bb, prev, &bad);
-			added++;
-		} else {
-			int extra = 0;
+		int extra = 0;
 
-			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
-				if (extra > 0)
-					goto out;
+		if (!can_front_overwrite(bb, prev, &bad, &extra)) {
+			if (extra > 0)
+				goto out;
 
-				len = min_t(sector_t,
-					    BB_END(p[prev]) - s, sectors);
-				hint = prev;
-				goto update_sectors;
-			}
+			len = min_t(sector_t,
+				    BB_END(p[prev]) - s, sectors);
+			hint = prev;
+			goto update_sectors;
+		}
 
-			len = front_overwrite(bb, prev, &bad, extra);
-			added++;
-			bb->count += extra;
+		len = front_overwrite(bb, prev, &bad, extra);
+		added++;
+		bb->count += extra;
 
-			if (can_combine_front(bb, prev, &bad)) {
-				front_combine(bb, prev);
-				bb->count--;
-			}
+		if (can_combine_front(bb, prev, &bad)) {
+			front_combine(bb, prev);
+			bb->count--;
 		}
-		hint = prev;
-		goto update_sectors;
-	}
 
-	if (can_merge_front(bb, prev, &bad)) {
-		len = front_merge(bb, prev, &bad);
-		added++;
 		hint = prev;
 		goto update_sectors;
 	}
-- 
2.39.2

[PATCH 08/12] badblocks: fix merge issue when new badblocks align with pre+1

[Zheng Qixing]

From: Li Nan <linan122@huawei.com>

There is a merge issue when adding badblocks as follow:
  echo 0 10 > bad_blocks
  echo 30 10 > bad_blocks
  echo 20 10 > bad_blocks
  cat bad_blocks
  0 10
  20 10    //should be merged with (30 10)
  30 10

In this case, if new badblocks does not intersect with prev, it is added
by insert_at(). If there is an intersection with prev+1, the merge will
be processed in the next re_insert loop.

However, when the end of the new badblocks is exactly equal to the offset
of prev+1, no further re_insert loop occurs, and the two badblocks are not
merge.

Fix it by inc prev, badblocks can be merged during the subsequent code.

Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index bb46bab7e99f..381f9db423d6 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -892,7 +892,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		len = insert_at(bb, 0, &bad);
 		bb->count++;
 		added++;
-		hint = 0;
+		hint = ++prev;
 		goto update_sectors;
 	}
 
@@ -947,7 +947,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	len = insert_at(bb, prev + 1, &bad);
 	bb->count++;
 	added++;
-	hint = prev + 1;
+	hint = ++prev;
 
 update_sectors:
 	s += len;
-- 
2.39.2

[PATCH 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check()

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

The bad blocks check would miss bad blocks when retrying under contention,
as checking parameters are not reset. These stale values from the previous
attempt could lead to incorrect scanning in the subsequent retry.

Move seqlock to outer function and reinitialize checking state for each
retry. This ensures a clean state for each check attempt, preventing any
missed bad blocks.

Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
---
 block/badblocks.c | 50 +++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 381f9db423d6..79d91be468c4 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1191,31 +1191,12 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 			    sector_t *first_bad, int *bad_sectors)
 {
-	int unacked_badblocks, acked_badblocks;
 	int prev = -1, hint = -1, set = 0;
 	struct badblocks_context bad;
-	unsigned int seq;
+	int unacked_badblocks = 0;
+	int acked_badblocks = 0;
+	u64 *p = bb->page;
 	int len, rv;
-	u64 *p;
-
-	WARN_ON(bb->shift < 0 || sectors == 0);
-
-	if (bb->shift > 0) {
-		sector_t target;
-
-		/* round the start down, and the end up */
-		target = s + sectors;
-		rounddown(s, 1 << bb->shift);
-		roundup(target, 1 << bb->shift);
-		sectors = target - s;
-	}
-
-retry:
-	seq = read_seqbegin(&bb->lock);
-
-	p = bb->page;
-	unacked_badblocks = 0;
-	acked_badblocks = 0;
 
 re_check:
 	bad.start = s;
@@ -1281,9 +1262,6 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	else
 		rv = 0;
 
-	if (read_seqretry(&bb->lock, seq))
-		goto retry;
-
 	return rv;
 }
 
@@ -1324,7 +1302,27 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 			sector_t *first_bad, int *bad_sectors)
 {
-	return _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
+	unsigned int seq;
+	int rv;
+
+	WARN_ON(bb->shift < 0 || sectors == 0);
+
+	if (bb->shift > 0) {
+		/* round the start down, and the end up */
+		sector_t target = s + sectors;
+
+		rounddown(s, 1 << bb->shift);
+		roundup(target, 1 << bb->shift);
+		sectors = target - s;
+	}
+
+retry:
+	seq = read_seqbegin(&bb->lock);
+	rv = _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
+	if (read_seqretry(&bb->lock, seq))
+		goto retry;
+
+	return rv;
 }
 EXPORT_SYMBOL_GPL(badblocks_check);
 
-- 
2.39.2

[PATCH 10/12] badblocks: return boolen from badblocks_set() and badblocks_clear()

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

Change the return type of badblocks_set() and badblocks_clear()
from int to bool, indicating success or failure. Specifically:

- _badblocks_set() and _badblocks_clear() functions now return
true for success and false for failure.
- All calls to these functions have been updated to handle the
new boolean return type.
- This change improves code clarity and ensures a more consistent
handling of success and failure states.

Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
---
 block/badblocks.c             | 37 +++++++++++++++++------------------
 drivers/block/null_blk/main.c | 17 ++++++++--------
 drivers/md/md.c               | 35 +++++++++++++++++----------------
 drivers/nvdimm/badrange.c     |  2 +-
 include/linux/badblocks.h     |  6 +++---
 5 files changed, 49 insertions(+), 48 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 79d91be468c4..8f057563488a 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -836,8 +836,8 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
 }
 
 /* Do exact work to set bad block range into the bad block table */
-static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
-			  int acknowledged)
+static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+			   int acknowledged)
 {
 	int len = 0, added = 0;
 	struct badblocks_context bad;
@@ -847,11 +847,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 
 	if (bb->shift < 0)
 		/* badblocks are disabled */
-		return 1;
+		return false;
 
 	if (sectors == 0)
 		/* Invalid sectors number */
-		return 1;
+		return false;
 
 	if (bb->shift) {
 		/* round the start down, and the end up */
@@ -977,7 +977,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 
 	write_sequnlock_irqrestore(&bb->lock, flags);
 
-	return sectors;
+	return sectors == 0;
 }
 
 /*
@@ -1048,21 +1048,20 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
 }
 
 /* Do the exact work to clear bad block range from the bad block table */
-static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
+static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 {
 	struct badblocks_context bad;
 	int prev = -1, hint = -1;
 	int len = 0, cleared = 0;
-	int rv = 0;
 	u64 *p;
 
 	if (bb->shift < 0)
 		/* badblocks are disabled */
-		return 1;
+		return false;
 
 	if (sectors == 0)
 		/* Invalid sectors number */
-		return 1;
+		return false;
 
 	if (bb->shift) {
 		sector_t target;
@@ -1182,9 +1181,9 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 	write_sequnlock_irq(&bb->lock);
 
 	if (!cleared)
-		rv = 1;
+		return false;
 
-	return rv;
+	return true;
 }
 
 /* Do the exact work to check bad blocks range from the bad block table */
@@ -1338,11 +1337,11 @@ EXPORT_SYMBOL_GPL(badblocks_check);
  * decide how best to handle it.
  *
  * Return:
- *  0: success
- *  other: failed to set badblocks (out of space)
+ *  true: success
+ *  false: failed to set badblocks (out of space)
  */
-int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
-			int acknowledged)
+bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+		   int acknowledged)
 {
 	return _badblocks_set(bb, s, sectors, acknowledged);
 }
@@ -1359,10 +1358,10 @@ EXPORT_SYMBOL_GPL(badblocks_set);
  * drop the remove request.
  *
  * Return:
- *  0: success
- *  1: failed to clear badblocks
+ *  true: success
+ *  false: failed to clear badblocks
  */
-int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
+bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 {
 	return _badblocks_clear(bb, s, sectors);
 }
@@ -1484,7 +1483,7 @@ ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
 		return -EINVAL;
 	}
 
-	if (badblocks_set(bb, sector, length, !unack))
+	if (!badblocks_set(bb, sector, length, !unack))
 		return -ENOSPC;
 	else
 		return len;
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index d94ef37480bd..623db72ad66b 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -559,14 +559,15 @@ static ssize_t nullb_device_badblocks_store(struct config_item *item,
 		goto out;
 	/* enable badblocks */
 	cmpxchg(&t_dev->badblocks.shift, -1, 0);
-	if (buf[0] == '+')
-		ret = badblocks_set(&t_dev->badblocks, start,
-			end - start + 1, 1);
-	else
-		ret = badblocks_clear(&t_dev->badblocks, start,
-			end - start + 1);
-	if (ret == 0)
-		ret = count;
+	if (buf[0] == '+') {
+		if (badblocks_set(&t_dev->badblocks, start,
+				  end - start + 1, 1))
+			ret = count;
+	} else {
+		if (badblocks_clear(&t_dev->badblocks, start,
+				    end - start + 1))
+			ret = count;
+	}
 out:
 	kfree(orig);
 	return ret;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 30b3dbbce2d2..49d826e475cb 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1748,7 +1748,7 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
 			count <<= sb->bblog_shift;
 			if (bb + 1 == 0)
 				break;
-			if (badblocks_set(&rdev->badblocks, sector, count, 1))
+			if (!badblocks_set(&rdev->badblocks, sector, count, 1))
 				return -EINVAL;
 		}
 	} else if (sb->bblog_offset != 0)
@@ -9846,7 +9846,6 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 		       int is_new)
 {
 	struct mddev *mddev = rdev->mddev;
-	int rv;
 
 	/*
 	 * Recording new badblocks for faulty rdev will force unnecessary
@@ -9862,33 +9861,35 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 		s += rdev->new_data_offset;
 	else
 		s += rdev->data_offset;
-	rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
-	if (rv == 0) {
-		/* Make sure they get written out promptly */
-		if (test_bit(ExternalBbl, &rdev->flags))
-			sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
-		sysfs_notify_dirent_safe(rdev->sysfs_state);
-		set_mask_bits(&mddev->sb_flags, 0,
-			      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
-		md_wakeup_thread(rdev->mddev->thread);
-		return 1;
-	} else
+
+	if (!badblocks_set(&rdev->badblocks, s, sectors, 0))
 		return 0;
+
+	/* Make sure they get written out promptly */
+	if (test_bit(ExternalBbl, &rdev->flags))
+		sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
+	sysfs_notify_dirent_safe(rdev->sysfs_state);
+	set_mask_bits(&mddev->sb_flags, 0,
+		      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
+	md_wakeup_thread(rdev->mddev->thread);
+	return 1;
 }
 EXPORT_SYMBOL_GPL(rdev_set_badblocks);
 
 int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 			 int is_new)
 {
-	int rv;
 	if (is_new)
 		s += rdev->new_data_offset;
 	else
 		s += rdev->data_offset;
-	rv = badblocks_clear(&rdev->badblocks, s, sectors);
-	if ((rv == 0) && test_bit(ExternalBbl, &rdev->flags))
+
+	if (!badblocks_clear(&rdev->badblocks, s, sectors))
+		return 0;
+
+	if (test_bit(ExternalBbl, &rdev->flags))
 		sysfs_notify_dirent_safe(rdev->sysfs_badblocks);
-	return rv;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
 
diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index a002ea6fdd84..ee478ccde7c6 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -167,7 +167,7 @@ static void set_badblock(struct badblocks *bb, sector_t s, int num)
 	dev_dbg(bb->dev, "Found a bad range (0x%llx, 0x%llx)\n",
 			(u64) s * 512, (u64) num * 512);
 	/* this isn't an error as the hardware will still throw an exception */
-	if (badblocks_set(bb, s, num, 1))
+	if (!badblocks_set(bb, s, num, 1))
 		dev_info_once(bb->dev, "%s: failed for sector %llx\n",
 				__func__, (u64) s);
 }
diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
index 670f2dae692f..8764bed9ff16 100644
--- a/include/linux/badblocks.h
+++ b/include/linux/badblocks.h
@@ -50,9 +50,9 @@ struct badblocks_context {
 
 int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 		   sector_t *first_bad, int *bad_sectors);
-int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
-			int acknowledged);
-int badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
+bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+		   int acknowledged);
+bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
 void ack_all_badblocks(struct badblocks *bb);
 ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
 ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
-- 
2.39.2

[PATCH 11/12] md: improve return types of badblocks handling functions

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

rdev_set_badblocks() only indicates success/failure, so convert its return
type from int to boolean for better semantic clarity.

rdev_clear_badblocks() return value is never used by any caller, convert it
to void. This removes unnecessary value returns.

Also update narrow_write_error() in both raid1 and raid10 to use boolean
return type to match rdev_set_badblocks().

Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
---
 drivers/md/md.c     | 20 +++++++++-----------
 drivers/md/md.h     |  8 ++++----
 drivers/md/raid1.c  |  6 +++---
 drivers/md/raid10.c |  6 +++---
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 49d826e475cb..76c437376542 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9841,9 +9841,9 @@ EXPORT_SYMBOL(md_finish_reshape);
 
 /* Bad block management */
 
-/* Returns 1 on success, 0 on failure */
-int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
-		       int is_new)
+/* Returns true on success, false on failure */
+bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+			int is_new)
 {
 	struct mddev *mddev = rdev->mddev;
 
@@ -9855,7 +9855,7 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 	 * avoid it.
 	 */
 	if (test_bit(Faulty, &rdev->flags))
-		return 1;
+		return true;
 
 	if (is_new)
 		s += rdev->new_data_offset;
@@ -9863,7 +9863,7 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 		s += rdev->data_offset;
 
 	if (!badblocks_set(&rdev->badblocks, s, sectors, 0))
-		return 0;
+		return false;
 
 	/* Make sure they get written out promptly */
 	if (test_bit(ExternalBbl, &rdev->flags))
@@ -9872,24 +9872,22 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
 	set_mask_bits(&mddev->sb_flags, 0,
 		      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
 	md_wakeup_thread(rdev->mddev->thread);
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL_GPL(rdev_set_badblocks);
 
-int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
-			 int is_new)
+void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+			  int is_new)
 {
 	if (is_new)
 		s += rdev->new_data_offset;
 	else
 		s += rdev->data_offset;
 
-	if (!badblocks_clear(&rdev->badblocks, s, sectors))
-		return 0;
+	badblocks_clear(&rdev->badblocks, s, sectors);
 
 	if (test_bit(ExternalBbl, &rdev->flags))
 		sysfs_notify_dirent_safe(rdev->sysfs_badblocks);
-	return 1;
 }
 EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
 
diff --git a/drivers/md/md.h b/drivers/md/md.h
index def808064ad8..923a0ef51efe 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -289,10 +289,10 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
 	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
 }
 
-extern int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
-			      int is_new);
-extern int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
-				int is_new);
+extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+			       int is_new);
+extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+				 int is_new);
 struct md_cluster_info;
 
 /**
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 9d57a88dbd26..8beb8cccc6af 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2486,7 +2486,7 @@ static void fix_read_error(struct r1conf *conf, struct r1bio *r1_bio)
 	}
 }
 
-static int narrow_write_error(struct r1bio *r1_bio, int i)
+static bool narrow_write_error(struct r1bio *r1_bio, int i)
 {
 	struct mddev *mddev = r1_bio->mddev;
 	struct r1conf *conf = mddev->private;
@@ -2507,10 +2507,10 @@ static int narrow_write_error(struct r1bio *r1_bio, int i)
 	sector_t sector;
 	int sectors;
 	int sect_to_write = r1_bio->sectors;
-	int ok = 1;
+	bool ok = true;
 
 	if (rdev->badblocks.shift < 0)
-		return 0;
+		return false;
 
 	block_sectors = roundup(1 << rdev->badblocks.shift,
 				bdev_logical_block_size(rdev->bdev) >> 9);
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index efe93b979167..7ed933181712 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2786,7 +2786,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
 	}
 }
 
-static int narrow_write_error(struct r10bio *r10_bio, int i)
+static bool narrow_write_error(struct r10bio *r10_bio, int i)
 {
 	struct bio *bio = r10_bio->master_bio;
 	struct mddev *mddev = r10_bio->mddev;
@@ -2807,10 +2807,10 @@ static int narrow_write_error(struct r10bio *r10_bio, int i)
 	sector_t sector;
 	int sectors;
 	int sect_to_write = r10_bio->sectors;
-	int ok = 1;
+	bool ok = true;
 
 	if (rdev->badblocks.shift < 0)
-		return 0;
+		return false;
 
 	block_sectors = roundup(1 << rdev->badblocks.shift,
 				bdev_logical_block_size(rdev->bdev) >> 9);
-- 
2.39.2

[PATCH 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

There is a truncation of badblocks length issue when set badblocks as
follow:

echo "2055 4294967299" > bad_blocks
cat bad_blocks
2055 3

Change 'sectors' argument type from 'int' to 'sector_t'.

This change avoids truncation of badblocks length for large sectors by
replacing 'int' with 'sector_t' (u64), enabling proper handling of larger
disk sizes and ensuring compatibility with 64-bit sector addressing.

Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
---
 block/badblocks.c             | 20 ++++++++------------
 drivers/block/null_blk/main.c |  2 +-
 drivers/md/md.h               |  6 +++---
 drivers/md/raid1-10.c         |  2 +-
 drivers/md/raid1.c            |  4 ++--
 drivers/md/raid10.c           |  8 ++++----
 drivers/nvdimm/nd.h           |  2 +-
 drivers/nvdimm/pfn_devs.c     |  7 ++++---
 drivers/nvdimm/pmem.c         |  2 +-
 include/linux/badblocks.h     |  8 ++++----
 10 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 8f057563488a..14e3be47d22d 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -836,7 +836,7 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
 }
 
 /* Do exact work to set bad block range into the bad block table */
-static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 			   int acknowledged)
 {
 	int len = 0, added = 0;
@@ -956,8 +956,6 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 	if (sectors > 0)
 		goto re_insert;
 
-	WARN_ON(sectors < 0);
-
 	/*
 	 * Check whether the following already set range can be
 	 * merged. (prev < 0) condition is not handled here,
@@ -1048,7 +1046,7 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
 }
 
 /* Do the exact work to clear bad block range from the bad block table */
-static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
+static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 {
 	struct badblocks_context bad;
 	int prev = -1, hint = -1;
@@ -1171,8 +1169,6 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 	if (sectors > 0)
 		goto re_clear;
 
-	WARN_ON(sectors < 0);
-
 	if (cleared) {
 		badblocks_update_acked(bb);
 		set_changed(bb);
@@ -1187,8 +1183,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 }
 
 /* Do the exact work to check bad blocks range from the bad block table */
-static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
-			    sector_t *first_bad, int *bad_sectors)
+static int _badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
+			    sector_t *first_bad, sector_t *bad_sectors)
 {
 	int prev = -1, hint = -1, set = 0;
 	struct badblocks_context bad;
@@ -1298,8 +1294,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
  * -1: there are bad blocks which have not yet been acknowledged in metadata.
  * plus the start/length of the first bad section we overlap.
  */
-int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
-			sector_t *first_bad, int *bad_sectors)
+int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
+			sector_t *first_bad, sector_t *bad_sectors)
 {
 	unsigned int seq;
 	int rv;
@@ -1340,7 +1336,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
  *  true: success
  *  false: failed to set badblocks (out of space)
  */
-bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 		   int acknowledged)
 {
 	return _badblocks_set(bb, s, sectors, acknowledged);
@@ -1361,7 +1357,7 @@ EXPORT_SYMBOL_GPL(badblocks_set);
  *  true: success
  *  false: failed to clear badblocks
  */
-bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
+bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 {
 	return _badblocks_clear(bb, s, sectors);
 }
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 623db72ad66b..6e7d80b6e92b 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1302,7 +1302,7 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd,
 {
 	struct badblocks *bb = &cmd->nq->dev->badblocks;
 	sector_t first_bad;
-	int bad_sectors;
+	sector_t bad_sectors;
 
 	if (badblocks_check(bb, sector, nr_sectors, &first_bad, &bad_sectors))
 		return BLK_STS_IOERR;
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 923a0ef51efe..6edc0f71b7d4 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -266,8 +266,8 @@ enum flag_bits {
 	Nonrot,			/* non-rotational device (SSD) */
 };
 
-static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
-			      sector_t *first_bad, int *bad_sectors)
+static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors,
+			      sector_t *first_bad, sector_t *bad_sectors)
 {
 	if (unlikely(rdev->badblocks.count)) {
 		int rv = badblocks_check(&rdev->badblocks, rdev->data_offset + s,
@@ -284,7 +284,7 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
 				    int sectors)
 {
 	sector_t first_bad;
-	int bad_sectors;
+	sector_t bad_sectors;
 
 	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
 }
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 4378d3250bd7..62b980b12f93 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -247,7 +247,7 @@ static inline int raid1_check_read_range(struct md_rdev *rdev,
 					 sector_t this_sector, int *len)
 {
 	sector_t first_bad;
-	int bad_sectors;
+	sector_t bad_sectors;
 
 	/* no bad block overlap */
 	if (!is_badblock(rdev, this_sector, *len, &first_bad, &bad_sectors))
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 8beb8cccc6af..0b2839105857 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1537,7 +1537,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
 		atomic_inc(&rdev->nr_pending);
 		if (test_bit(WriteErrorSeen, &rdev->flags)) {
 			sector_t first_bad;
-			int bad_sectors;
+			sector_t bad_sectors;
 			int is_bad;
 
 			is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
@@ -2886,7 +2886,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 		} else {
 			/* may need to read from here */
 			sector_t first_bad = MaxSector;
-			int bad_sectors;
+			sector_t bad_sectors;
 
 			if (is_badblock(rdev, sector_nr, good_sectors,
 					&first_bad, &bad_sectors)) {
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 7ed933181712..a8664e29aada 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -747,7 +747,7 @@ static struct md_rdev *read_balance(struct r10conf *conf,
 
 	for (slot = 0; slot < conf->copies ; slot++) {
 		sector_t first_bad;
-		int bad_sectors;
+		sector_t bad_sectors;
 		sector_t dev_sector;
 		unsigned int pending;
 		bool nonrot;
@@ -1438,7 +1438,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
 		if (rdev && test_bit(WriteErrorSeen, &rdev->flags)) {
 			sector_t first_bad;
 			sector_t dev_sector = r10_bio->devs[i].addr;
-			int bad_sectors;
+			sector_t bad_sectors;
 			int is_bad;
 
 			is_bad = is_badblock(rdev, dev_sector, max_sectors,
@@ -3413,7 +3413,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				sector_t from_addr, to_addr;
 				struct md_rdev *rdev = conf->mirrors[d].rdev;
 				sector_t sector, first_bad;
-				int bad_sectors;
+				sector_t bad_sectors;
 				if (!rdev ||
 				    !test_bit(In_sync, &rdev->flags))
 					continue;
@@ -3609,7 +3609,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 		for (i = 0; i < conf->copies; i++) {
 			int d = r10_bio->devs[i].devnum;
 			sector_t first_bad, sector;
-			int bad_sectors;
+			sector_t bad_sectors;
 			struct md_rdev *rdev;
 
 			if (r10_bio->devs[i].repl_bio)
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 5ca06e9a2d29..cc5c8f3f81e8 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -673,7 +673,7 @@ static inline bool is_bad_pmem(struct badblocks *bb, sector_t sector,
 {
 	if (bb->count) {
 		sector_t first_bad;
-		int num_bad;
+		sector_t num_bad;
 
 		return !!badblocks_check(bb, sector, len / 512, &first_bad,
 				&num_bad);
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index cfdfe0eaa512..8f3e816e805d 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -367,9 +367,10 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
 	struct nd_namespace_common *ndns = nd_pfn->ndns;
 	void *zero_page = page_address(ZERO_PAGE(0));
 	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
-	int num_bad, meta_num, rc, bb_present;
+	int meta_num, rc, bb_present;
 	sector_t first_bad, meta_start;
 	struct nd_namespace_io *nsio;
+	sector_t num_bad;
 
 	if (nd_pfn->mode != PFN_MODE_PMEM)
 		return 0;
@@ -394,7 +395,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
 		bb_present = badblocks_check(&nd_region->bb, meta_start,
 				meta_num, &first_bad, &num_bad);
 		if (bb_present) {
-			dev_dbg(&nd_pfn->dev, "meta: %x badblocks at %llx\n",
+			dev_dbg(&nd_pfn->dev, "meta: %llx badblocks at %llx\n",
 					num_bad, first_bad);
 			nsoff = ALIGN_DOWN((nd_region->ndr_start
 					+ (first_bad << 9)) - nsio->res.start,
@@ -413,7 +414,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
 			}
 			if (rc) {
 				dev_err(&nd_pfn->dev,
-					"error clearing %x badblocks at %llx\n",
+					"error clearing %llx badblocks at %llx\n",
 					num_bad, first_bad);
 				return rc;
 			}
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index d81faa9d89c9..43156e1576c9 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -249,7 +249,7 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
 	unsigned int num = PFN_PHYS(nr_pages) >> SECTOR_SHIFT;
 	struct badblocks *bb = &pmem->bb;
 	sector_t first_bad;
-	int num_bad;
+	sector_t num_bad;
 
 	if (kaddr)
 		*kaddr = pmem->virt_addr + offset;
diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
index 8764bed9ff16..996493917f36 100644
--- a/include/linux/badblocks.h
+++ b/include/linux/badblocks.h
@@ -48,11 +48,11 @@ struct badblocks_context {
 	int		ack;
 };
 
-int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
-		   sector_t *first_bad, int *bad_sectors);
-bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
+int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
+		    sector_t *first_bad, sector_t *bad_sectors);
+bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 		   int acknowledged);
-bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
+bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors);
 void ack_all_badblocks(struct badblocks *bb);
 ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
 ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
-- 
2.39.2

Re: [PATCH 00/12] badblocks: bugfix and cleanup for badblocks

[Yu Kuai]

Hi,

在 2025/02/21 16:10, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> During RAID feature implementation testing, we found several bugs
> in badblocks.
> 
> This series contains bugfixes and cleanups for MD RAID badblocks
> handling code.

In addition, patch 1-8 is found while testing badblocks APIs, by mdraid
sysfs APIs, and it's applied and running in downstream kernels for a
long time.

Patch 9-12 is found recently by RAID new feature that is still in
development.

Thanks,
Kuai

> 
> Li Nan (8):
>    badblocks: Fix error shitf ops
>    badblocks: factor out a helper try_adjacent_combine
>    badblocks: attempt to merge adjacent badblocks during
>      ack_all_badblocks
>    badblocks: return error directly when setting badblocks exceeds 512
>    badblocks: return error if any badblock set fails
>    badblocks: fix the using of MAX_BADBLOCKS
>    badblocks: try can_merge_front before overlap_front
>    badblocks: fix merge issue when new badblocks align with pre+1
> 
> Zheng Qixing (4):
>    badblocks: fix missing bad blocks on retry in _badblocks_check()
>    badblocks: return boolen from badblocks_set() and badblocks_clear()
>    md: improve return types of badblocks handling functions
>    badblocks: use sector_t instead of int to avoid truncation of
>      badblocks length
> 
>   block/badblocks.c             | 317 +++++++++++++---------------------
>   drivers/block/null_blk/main.c |  19 +-
>   drivers/md/md.c               |  47 +++--
>   drivers/md/md.h               |  14 +-
>   drivers/md/raid1-10.c         |   2 +-
>   drivers/md/raid1.c            |  10 +-
>   drivers/md/raid10.c           |  14 +-
>   drivers/nvdimm/badrange.c     |   2 +-
>   drivers/nvdimm/nd.h           |   2 +-
>   drivers/nvdimm/pfn_devs.c     |   7 +-
>   drivers/nvdimm/pmem.c         |   2 +-
>   include/linux/badblocks.h     |  10 +-
>   12 files changed, 181 insertions(+), 265 deletions(-)
> 

Re: [PATCH 01/12] badblocks: Fix error shitf ops

[Yu Kuai]

在 2025/02/21 16:10, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> 'bb->shift' is used directly in badblocks. It is wrong, fix it.
> 
> Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index db4ec8b9b2a8..bcee057efc47 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -880,8 +880,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   		/* round the start down, and the end up */
>   		sector_t next = s + sectors;
>   
> -		rounddown(s, bb->shift);
> -		roundup(next, bb->shift);
> +		rounddown(s, 1 << bb->shift);
> +		roundup(next, 1 << bb->shift);
>   		sectors = next - s;
>   	}
>   
> @@ -1157,8 +1157,8 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   		 * isn't than to think a block is not bad when it is.
>   		 */
>   		target = s + sectors;
> -		roundup(s, bb->shift);
> -		rounddown(target, bb->shift);
> +		roundup(s, 1 << bb->shift);
> +		rounddown(target, 1 << bb->shift);
>   		sectors = target - s;
>   	}
>   
> @@ -1288,8 +1288,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   
>   		/* round the start down, and the end up */
>   		target = s + sectors;
> -		rounddown(s, bb->shift);
> -		roundup(target, bb->shift);
> +		rounddown(s, 1 << bb->shift);
> +		roundup(target, 1 << bb->shift);
>   		sectors = target - s;
>   	}
>   
> 

Re: [PATCH 02/12] badblocks: factor out a helper try_adjacent_combine

[Yu Kuai]

在 2025/02/21 16:10, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> Factor out try_adjacent_combine(), and it will be used in the later patch.
> 
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 40 ++++++++++++++++++++++++++--------------
>   1 file changed, 26 insertions(+), 14 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index bcee057efc47..f069c93e986d 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -855,6 +855,31 @@ static void badblocks_update_acked(struct badblocks *bb)
>   		bb->unacked_exist = 0;
>   }
>   
> +/*
> + * Return 'true' if the range indicated by 'bad' is exactly backward
> + * overlapped with the bad range (from bad table) indexed by 'behind'.
> + */
> +static bool try_adjacent_combine(struct badblocks *bb, int prev)
> +{
> +	u64 *p = bb->page;
> +
> +	if (prev >= 0 && (prev + 1) < bb->count &&
> +	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
> +	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
> +	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
> +		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
> +				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
> +				  BB_ACK(p[prev]));
> +
> +		if ((prev + 2) < bb->count)
> +			memmove(p + prev + 1, p + prev + 2,
> +				(bb->count -  (prev + 2)) * 8);
> +		bb->count--;
> +		return true;
> +	}
> +	return false;
> +}
> +
>   /* Do exact work to set bad block range into the bad block table */
>   static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   			  int acknowledged)
> @@ -1022,20 +1047,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   	 * merged. (prev < 0) condition is not handled here,
>   	 * because it's already complicated enough.
>   	 */
> -	if (prev >= 0 &&
> -	    (prev + 1) < bb->count &&
> -	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
> -	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
> -	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
> -		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
> -				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
> -				  BB_ACK(p[prev]));
> -
> -		if ((prev + 2) < bb->count)
> -			memmove(p + prev + 1, p + prev + 2,
> -				(bb->count -  (prev + 2)) * 8);
> -		bb->count--;
> -	}
> +	try_adjacent_combine(bb, prev);
>   
>   	if (space_desired && !badblocks_full(bb)) {
>   		s = orig_start;
> 

Re: [PATCH 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> If ack and unack badblocks are adjacent, they will not be merged and will
> remain as two separate badblocks. Even after the bad blocks are written
> to disk and both become ack, they will still remain as two independent
> bad blocks. This is not ideal as it wastes the limited space for
> badblocks. Therefore, during ack_all_badblocks(), attempt to merge
> badblocks if they are adjacent.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index f069c93e986d..ad8652fbe1c8 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -1491,6 +1491,11 @@ void ack_all_badblocks(struct badblocks *bb)
>   				p[i] = BB_MAKE(start, len, 1);
>   			}
>   		}
> +
> +		for (i = 0; i < bb->count ; i++)
> +			while (try_adjacent_combine(bb, i))
> +				;
> +
>   		bb->unacked_exist = 0;
>   	}
>   	write_sequnlock_irq(&bb->lock);
> 

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
> 
> _badblocks_set() returns success if at least one badblock is set
> successfully, even if others fail. This can lead to data inconsistencies
> in raid, where a failed badblock set should trigger the disk to be kicked
> out to prevent future reads from failed write areas.
> 
> _badblocks_set() should return error if any badblock set fails. Instead
> of relying on 'rv', directly returning 'sectors' for clearer logic. If all
> badblocks are successfully set, 'sectors' will be 0, otherwise it
> indicates the number of badblocks that have not been set yet, thus
> signaling failure.
> 
> By the way, it can also fix an issue: when a newly set unack badblock is
> included in an existing ack badblock, the setting will return an error.
> ···
>   echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
>   echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
>   -bash: echo: write error: No space left on device
> ```
> After fix, it will return success.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  block/badblocks.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>

NACK.   Such modification will break current API.

Current API doesn’t handle partial success/fail condition, if any bad block range is set it should return true.

It is not about correct or wrong, just current fragile design. A new API is necessary to handle such thing. This is why I leave the return value as int other than bool.

Thanks.

Coly Li


 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 1c8b8f65f6df..a953d2e9417f 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -843,7 +843,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	struct badblocks_context bad;
>  	int prev = -1, hint = -1;
>  	unsigned long flags;
> -	int rv = 0;
>  	u64 *p;
>  
>  	if (bb->shift < 0)
> @@ -873,10 +872,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	bad.len = sectors;
>  	len = 0;
>  
> -	if (badblocks_full(bb)) {
> -		rv = 1;
> +	if (badblocks_full(bb))
>  		goto out;
> -	}
>  
>  	if (badblocks_empty(bb)) {
>  		len = insert_at(bb, 0, &bad);
> @@ -916,10 +913,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  			int extra = 0;
>  
>  			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
> -				if (extra > 0) {
> -					rv = 1;
> +				if (extra > 0)
>  					goto out;
> -				}
>  
>  				len = min_t(sector_t,
>  					    BB_END(p[prev]) - s, sectors);
> @@ -986,10 +981,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  
>  	write_sequnlock_irqrestore(&bb->lock, flags);
>  
> -	if (!added)
> -		rv = 1;
> -
> -	return rv;
> +	return sectors;
>  }
>  
>  /*
> @@ -1353,7 +1345,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>   *
>   * Return:
>   *  0: success
> - *  1: failed to set badblocks (out of space)
> + *  other: failed to set badblocks (out of space)
>   */
>  int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  			int acknowledged)
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 04/12] badblocks: return error directly when setting badblocks exceeds 512

[Yu Kuai]

Hi,

+CC Linan

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> In the current handling of badblocks settings, a lot of processing has
> been done for scenarios where the number of badblocks exceeds 512.
> This makes the code look quite complex and also introduces some issues,

It's better to add explanations about these issues here.
> 
> Fixing those issues wouldn’t be too complicated, but it wouldn’t
> simplify the code. In fact, a disk shouldn’t have too many badblocks,
> and for disks with 512 badblocks, attempting to set more bad blocks
> doesn’t make much sense. At that point, the more appropriate action
> would be to replace the disk. Therefore, to resolve these issues and
> simplify the code somewhat, return error directly when setting badblocks
> exceeds 512.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 121 ++++++++--------------------------------------
>   1 file changed, 19 insertions(+), 102 deletions(-)
> 

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index ad8652fbe1c8..1c8b8f65f6df 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -527,51 +527,6 @@ static int prev_badblocks(struct badblocks *bb, struct badblocks_context *bad,
>   	return ret;
>   }
>   
> -/*
> - * Return 'true' if the range indicated by 'bad' can be backward merged
> - * with the bad range (from the bad table) index by 'behind'.
> - */
> -static bool can_merge_behind(struct badblocks *bb,
> -			     struct badblocks_context *bad, int behind)
> -{
> -	sector_t sectors = bad->len;
> -	sector_t s = bad->start;
> -	u64 *p = bb->page;
> -
> -	if ((s < BB_OFFSET(p[behind])) &&
> -	    ((s + sectors) >= BB_OFFSET(p[behind])) &&
> -	    ((BB_END(p[behind]) - s) <= BB_MAX_LEN) &&
> -	    BB_ACK(p[behind]) == bad->ack)
> -		return true;
> -	return false;
> -}
> -
> -/*
> - * Do backward merge for range indicated by 'bad' and the bad range
> - * (from the bad table) indexed by 'behind'. The return value is merged
> - * sectors from bad->len.
> - */
> -static int behind_merge(struct badblocks *bb, struct badblocks_context *bad,
> -			int behind)
> -{
> -	sector_t sectors = bad->len;
> -	sector_t s = bad->start;
> -	u64 *p = bb->page;
> -	int merged = 0;
> -
> -	WARN_ON(s >= BB_OFFSET(p[behind]));
> -	WARN_ON((s + sectors) < BB_OFFSET(p[behind]));
> -
> -	if (s < BB_OFFSET(p[behind])) {
> -		merged = BB_OFFSET(p[behind]) - s;
> -		p[behind] =  BB_MAKE(s, BB_LEN(p[behind]) + merged, bad->ack);
> -
> -		WARN_ON((BB_LEN(p[behind]) + merged) >= BB_MAX_LEN);
> -	}
> -
> -	return merged;
> -}
> -
>   /*
>    * Return 'true' if the range indicated by 'bad' can be forward
>    * merged with the bad range (from the bad table) indexed by 'prev'.
> @@ -884,11 +839,9 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
>   static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   			  int acknowledged)
>   {
> -	int retried = 0, space_desired = 0;
> -	int orig_len, len = 0, added = 0;
> +	int len = 0, added = 0;
>   	struct badblocks_context bad;
>   	int prev = -1, hint = -1;
> -	sector_t orig_start;
>   	unsigned long flags;
>   	int rv = 0;
>   	u64 *p;
> @@ -912,8 +865,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   
>   	write_seqlock_irqsave(&bb->lock, flags);
>   
> -	orig_start = s;
> -	orig_len = sectors;
>   	bad.ack = acknowledged;
>   	p = bb->page;
>   
> @@ -922,6 +873,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   	bad.len = sectors;
>   	len = 0;
>   
> +	if (badblocks_full(bb)) {
> +		rv = 1;
> +		goto out;
> +	}
> +
>   	if (badblocks_empty(bb)) {
>   		len = insert_at(bb, 0, &bad);
>   		bb->count++;
> @@ -933,32 +889,14 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   
>   	/* start before all badblocks */
>   	if (prev < 0) {
> -		if (!badblocks_full(bb)) {
> -			/* insert on the first */
> -			if (bad.len > (BB_OFFSET(p[0]) - bad.start))
> -				bad.len = BB_OFFSET(p[0]) - bad.start;
> -			len = insert_at(bb, 0, &bad);
> -			bb->count++;
> -			added++;
> -			hint = 0;
> -			goto update_sectors;
> -		}
> -
> -		/* No sapce, try to merge */
> -		if (overlap_behind(bb, &bad, 0)) {
> -			if (can_merge_behind(bb, &bad, 0)) {
> -				len = behind_merge(bb, &bad, 0);
> -				added++;
> -			} else {
> -				len = BB_OFFSET(p[0]) - s;
> -				space_desired = 1;
> -			}
> -			hint = 0;
> -			goto update_sectors;
> -		}
> -
> -		/* no table space and give up */
> -		goto out;
> +		/* insert on the first */
> +		if (bad.len > (BB_OFFSET(p[0]) - bad.start))
> +			bad.len = BB_OFFSET(p[0]) - bad.start;
> +		len = insert_at(bb, 0, &bad);
> +		bb->count++;
> +		added++;
> +		hint = 0;
> +		goto update_sectors;
>   	}
>   
>   	/* in case p[prev-1] can be merged with p[prev] */
> @@ -978,6 +916,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   			int extra = 0;
>   
>   			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
> +				if (extra > 0) {
> +					rv = 1;
> +					goto out;
> +				}
> +
>   				len = min_t(sector_t,
>   					    BB_END(p[prev]) - s, sectors);
>   				hint = prev;
> @@ -1004,24 +947,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   		goto update_sectors;
>   	}
>   
> -	/* if no space in table, still try to merge in the covered range */
> -	if (badblocks_full(bb)) {
> -		/* skip the cannot-merge range */
> -		if (((prev + 1) < bb->count) &&
> -		    overlap_behind(bb, &bad, prev + 1) &&
> -		    ((s + sectors) >= BB_END(p[prev + 1]))) {
> -			len = BB_END(p[prev + 1]) - s;
> -			hint = prev + 1;
> -			goto update_sectors;
> -		}
> -
> -		/* no retry any more */
> -		len = sectors;
> -		space_desired = 1;
> -		hint = -1;
> -		goto update_sectors;
> -	}
> -
>   	/* cannot merge and there is space in bad table */
>   	if ((prev + 1) < bb->count &&
>   	    overlap_behind(bb, &bad, prev + 1))
> @@ -1049,14 +974,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   	 */
>   	try_adjacent_combine(bb, prev);
>   
> -	if (space_desired && !badblocks_full(bb)) {
> -		s = orig_start;
> -		sectors = orig_len;
> -		space_desired = 0;
> -		if (retried++ < 3)
> -			goto re_insert;
> -	}
> -
>   out:
>   	if (added) {
>   		set_changed(bb);
> 

Re: [PATCH 01/12] badblocks: Fix error shitf ops

[Coly Li]

On Fri, Feb 21, 2025 at 04:10:58PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
>

Looks good to me.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.
 
> 'bb->shift' is used directly in badblocks. It is wrong, fix it.
> 
> Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  block/badblocks.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index db4ec8b9b2a8..bcee057efc47 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -880,8 +880,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  		/* round the start down, and the end up */
>  		sector_t next = s + sectors;
>  
> -		rounddown(s, bb->shift);
> -		roundup(next, bb->shift);
> +		rounddown(s, 1 << bb->shift);
> +		roundup(next, 1 << bb->shift);
>  		sectors = next - s;
>  	}
>  
> @@ -1157,8 +1157,8 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  		 * isn't than to think a block is not bad when it is.
>  		 */
>  		target = s + sectors;
> -		roundup(s, bb->shift);
> -		rounddown(target, bb->shift);
> +		roundup(s, 1 << bb->shift);
> +		rounddown(target, 1 << bb->shift);
>  		sectors = target - s;
>  	}
>  
> @@ -1288,8 +1288,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  
>  		/* round the start down, and the end up */
>  		target = s + sectors;
> -		rounddown(s, bb->shift);
> -		roundup(target, bb->shift);
> +		rounddown(s, 1 << bb->shift);
> +		roundup(target, 1 << bb->shift);
>  		sectors = target - s;
>  	}
>  
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 02/12] badblocks: factor out a helper try_adjacent_combine

[Coly Li]

On Fri, Feb 21, 2025 at 04:10:59PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
> 
> Factor out try_adjacent_combine(), and it will be used in the later patch.
>

Which patch is try_adjacent_combine() used in? I don't see that at a quick glance.

Thanks.

Coly Li

 
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  block/badblocks.c | 40 ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index bcee057efc47..f069c93e986d 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -855,6 +855,31 @@ static void badblocks_update_acked(struct badblocks *bb)
>  		bb->unacked_exist = 0;
>  }
>  
> +/*
> + * Return 'true' if the range indicated by 'bad' is exactly backward
> + * overlapped with the bad range (from bad table) indexed by 'behind'.
> + */
> +static bool try_adjacent_combine(struct badblocks *bb, int prev)
> +{
> +	u64 *p = bb->page;
> +
> +	if (prev >= 0 && (prev + 1) < bb->count &&
> +	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
> +	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
> +	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
> +		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
> +				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
> +				  BB_ACK(p[prev]));
> +
> +		if ((prev + 2) < bb->count)
> +			memmove(p + prev + 1, p + prev + 2,
> +				(bb->count -  (prev + 2)) * 8);
> +		bb->count--;
> +		return true;
> +	}
> +	return false;
> +}
> +
>  /* Do exact work to set bad block range into the bad block table */
>  static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  			  int acknowledged)
> @@ -1022,20 +1047,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	 * merged. (prev < 0) condition is not handled here,
>  	 * because it's already complicated enough.
>  	 */
> -	if (prev >= 0 &&
> -	    (prev + 1) < bb->count &&
> -	    BB_END(p[prev]) == BB_OFFSET(p[prev + 1]) &&
> -	    (BB_LEN(p[prev]) + BB_LEN(p[prev + 1])) <= BB_MAX_LEN &&
> -	    BB_ACK(p[prev]) == BB_ACK(p[prev + 1])) {
> -		p[prev] = BB_MAKE(BB_OFFSET(p[prev]),
> -				  BB_LEN(p[prev]) + BB_LEN(p[prev + 1]),
> -				  BB_ACK(p[prev]));
> -
> -		if ((prev + 2) < bb->count)
> -			memmove(p + prev + 1, p + prev + 2,
> -				(bb->count -  (prev + 2)) * 8);
> -		bb->count--;
> -	}
> +	try_adjacent_combine(bb, prev);
>  
>  	if (space_desired && !badblocks_full(bb)) {
>  		s = orig_start;
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 02/12] badblocks: factor out a helper try_adjacent_combine

[Coly Li]

> 2025年2月21日 18:04，Coly Li <i@coly.li> 写道：
> 
> On Fri, Feb 21, 2025 at 04:10:59PM +0800, Zheng Qixing wrote:
>> From: Li Nan <linan122@huawei.com>
>> 
>> Factor out try_adjacent_combine(), and it will be used in the later patch.
>> 
> 
> Which patch is try_adjacent_combine() used in? I don't see that at a quick glance.


OK, I see it is in ack_all_badblocks().  Ignore the above question.

Coly Li

Re: [PATCH 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:00PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
> 
> If ack and unack badblocks are adjacent, they will not be merged and will
> remain as two separate badblocks. Even after the bad blocks are written
> to disk and both become ack, they will still remain as two independent
> bad blocks. This is not ideal as it wastes the limited space for
> badblocks. Therefore, during ack_all_badblocks(), attempt to merge
> badblocks if they are adjacent.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>

Looks good to me.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

> ---
>  block/badblocks.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index f069c93e986d..ad8652fbe1c8 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -1491,6 +1491,11 @@ void ack_all_badblocks(struct badblocks *bb)
>  				p[i] = BB_MAKE(start, len, 1);
>  			}
>  		}
> +
> +		for (i = 0; i < bb->count ; i++)
> +			while (try_adjacent_combine(bb, i))
> +				;
> +
>  		bb->unacked_exist = 0;
>  	}
>  	write_sequnlock_irq(&bb->lock);
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 06/12] badblocks: fix the using of MAX_BADBLOCKS

[Zhu Yanjun]

On 21.02.25 09:11, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
>
> The number of badblocks cannot exceed MAX_BADBLOCKS, but it should be
> allowed to equal MAX_BADBLOCKS.
>
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index a953d2e9417f..87267bae6836 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -700,7 +700,7 @@ static bool can_front_overwrite(struct badblocks *bb, int prev,
>   			*extra = 2;
>   	}
>   
> -	if ((bb->count + (*extra)) >= MAX_BADBLOCKS)
> +	if ((bb->count + (*extra)) > MAX_BADBLOCKS)
>   		return false;


In this commit,

commit c3c6a86e9efc5da5964260c322fe07feca6df782
Author: Coly Li <colyli@suse.de>
Date:   Sat Aug 12 01:05:08 2023 +0800

     badblocks: add helper routines for badblock ranges handling

     This patch adds several helper routines to improve badblock ranges
     handling. These helper routines will be used later in the improved
     version of badblocks_set()/badblocks_clear()/badblocks_check().

     - Helpers prev_by_hint() and prev_badblocks() are used to find the bad
       range from bad table which the searching range starts at or after.

The above is changed to MAX_BADBLOCKS. Thus, perhaps, the Fixes tag 
should include the above commit?

Except that, I am fine with this commit.

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun


>   
>   	return true;
> @@ -1135,7 +1135,7 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   		if ((BB_OFFSET(p[prev]) < bad.start) &&
>   		    (BB_END(p[prev]) > (bad.start + bad.len))) {
>   			/* Splitting */
> -			if ((bb->count + 1) < MAX_BADBLOCKS) {
> +			if ((bb->count + 1) <= MAX_BADBLOCKS) {
>   				len = front_splitting_clear(bb, prev, &bad);
>   				bb->count += 1;
>   				cleared++;

-- 
Best Regards,
Yanjun.Zhu

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Yu Kuai]

Hi,

在 2025/02/21 17:52, Coly Li 写道:
> On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
>> From: Li Nan <linan122@huawei.com>
>>
>> _badblocks_set() returns success if at least one badblock is set
>> successfully, even if others fail. This can lead to data inconsistencies
>> in raid, where a failed badblock set should trigger the disk to be kicked
>> out to prevent future reads from failed write areas.
>>
>> _badblocks_set() should return error if any badblock set fails. Instead
>> of relying on 'rv', directly returning 'sectors' for clearer logic. If all
>> badblocks are successfully set, 'sectors' will be 0, otherwise it
>> indicates the number of badblocks that have not been set yet, thus
>> signaling failure.
>>
>> By the way, it can also fix an issue: when a newly set unack badblock is
>> included in an existing ack badblock, the setting will return an error.
>> ···
>>    echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
>>    echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
>>    -bash: echo: write error: No space left on device
>> ```
>> After fix, it will return success.
>>
>> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
>> Signed-off-by: Li Nan <linan122@huawei.com>
>> ---
>>   block/badblocks.c | 16 ++++------------
>>   1 file changed, 4 insertions(+), 12 deletions(-)
>>
> 
> NACK.   Such modification will break current API.

Take a look at current APIs:
- for raid, error should be returned, otherwise data may be corrupted.
- for nvdimm, there is only error message if fail, and it make sense as
well if any badblocks set failed:
         if (badblocks_set(bb, s, num, 1))
                 dev_info_once(bb->dev, "%s: failed for sector %llx\n",
                                 __func__, (u64) s);
- for null_blk, I think it's fine as well.

Hence I think it's fine to return error if any badblocks set failed.
There is no need to invent a new API and switch all callers to a new
API.

Thanks,
Kuai

> 
> Current API doesn’t handle partial success/fail condition, if any bad block range is set it should return true.
> 
> It is not about correct or wrong, just current fragile design. A new API is necessary to handle such thing. This is why I leave the return value as int other than bool.
> 
> Thanks.
> 
> Coly Li
> 
> 
>   
>> diff --git a/block/badblocks.c b/block/badblocks.c
>> index 1c8b8f65f6df..a953d2e9417f 100644
>> --- a/block/badblocks.c
>> +++ b/block/badblocks.c
>> @@ -843,7 +843,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   	struct badblocks_context bad;
>>   	int prev = -1, hint = -1;
>>   	unsigned long flags;
>> -	int rv = 0;
>>   	u64 *p;
>>   
>>   	if (bb->shift < 0)
>> @@ -873,10 +872,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   	bad.len = sectors;
>>   	len = 0;
>>   
>> -	if (badblocks_full(bb)) {
>> -		rv = 1;
>> +	if (badblocks_full(bb))
>>   		goto out;
>> -	}
>>   
>>   	if (badblocks_empty(bb)) {
>>   		len = insert_at(bb, 0, &bad);
>> @@ -916,10 +913,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   			int extra = 0;
>>   
>>   			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
>> -				if (extra > 0) {
>> -					rv = 1;
>> +				if (extra > 0)
>>   					goto out;
>> -				}
>>   
>>   				len = min_t(sector_t,
>>   					    BB_END(p[prev]) - s, sectors);
>> @@ -986,10 +981,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   
>>   	write_sequnlock_irqrestore(&bb->lock, flags);
>>   
>> -	if (!added)
>> -		rv = 1;
>> -
>> -	return rv;
>> +	return sectors;
>>   }
>>   
>>   /*
>> @@ -1353,7 +1345,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>>    *
>>    * Return:
>>    *  0: success
>> - *  1: failed to set badblocks (out of space)
>> + *  other: failed to set badblocks (out of space)
>>    */
>>   int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   			int acknowledged)
>> -- 
>> 2.39.2
>>
> 

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Coly Li]

> 2025年2月21日 18:09，Yu Kuai <yukuai1@huaweicloud.com> 写道：
> 
> Hi,
> 
> 在 2025/02/21 17:52, Coly Li 写道:
>> On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
>>> From: Li Nan <linan122@huawei.com>
>>> 
>>> _badblocks_set() returns success if at least one badblock is set
>>> successfully, even if others fail. This can lead to data inconsistencies
>>> in raid, where a failed badblock set should trigger the disk to be kicked
>>> out to prevent future reads from failed write areas.
>>> 
>>> _badblocks_set() should return error if any badblock set fails. Instead
>>> of relying on 'rv', directly returning 'sectors' for clearer logic. If all
>>> badblocks are successfully set, 'sectors' will be 0, otherwise it
>>> indicates the number of badblocks that have not been set yet, thus
>>> signaling failure.
>>> 
>>> By the way, it can also fix an issue: when a newly set unack badblock is
>>> included in an existing ack badblock, the setting will return an error.
>>> ···
>>>   echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
>>>   echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
>>>   -bash: echo: write error: No space left on device
>>> ```
>>> After fix, it will return success.
>>> 
>>> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
>>> Signed-off-by: Li Nan <linan122@huawei.com>
>>> ---
>>>  block/badblocks.c | 16 ++++------------
>>>  1 file changed, 4 insertions(+), 12 deletions(-)
>>> 
>> NACK.   Such modification will break current API.
> 
> Take a look at current APIs:
> - for raid, error should be returned, otherwise data may be corrupted.
> - for nvdimm, there is only error message if fail, and it make sense as
> well if any badblocks set failed:
>        if (badblocks_set(bb, s, num, 1))
>                dev_info_once(bb->dev, "%s: failed for sector %llx\n",
>                                __func__, (u64) s);
> - for null_blk, I think it's fine as well.
> 
> Hence I think it's fine to return error if any badblocks set failed.
> There is no need to invent a new API and switch all callers to a new
> API.

So we don’t need to add a negative return value for partial success/failure?

Coly Li

Re: [PATCH 06/12] badblocks: fix the using of MAX_BADBLOCKS

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:03PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
> 
> The number of badblocks cannot exceed MAX_BADBLOCKS, but it should be
> allowed to equal MAX_BADBLOCKS.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>

Looks good to me.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

> ---
>  block/badblocks.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index a953d2e9417f..87267bae6836 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -700,7 +700,7 @@ static bool can_front_overwrite(struct badblocks *bb, int prev,
>  			*extra = 2;
>  	}
>  
> -	if ((bb->count + (*extra)) >= MAX_BADBLOCKS)
> +	if ((bb->count + (*extra)) > MAX_BADBLOCKS)
>  		return false;
>  
>  	return true;
> @@ -1135,7 +1135,7 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  		if ((BB_OFFSET(p[prev]) < bad.start) &&
>  		    (BB_END(p[prev]) > (bad.start + bad.len))) {
>  			/* Splitting */
> -			if ((bb->count + 1) < MAX_BADBLOCKS) {
> +			if ((bb->count + 1) <= MAX_BADBLOCKS) {
>  				len = front_splitting_clear(bb, prev, &bad);
>  				bb->count += 1;
>  				cleared++;
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check()

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:06PM +0800, Zheng Qixing wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> The bad blocks check would miss bad blocks when retrying under contention,
> as checking parameters are not reset. These stale values from the previous
> attempt could lead to incorrect scanning in the subsequent retry.
> 
> Move seqlock to outer function and reinitialize checking state for each
> retry. This ensures a clean state for each check attempt, preventing any
> missed bad blocks.
> 
> Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>

Looks good to me.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

> ---
>  block/badblocks.c | 50 +++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 381f9db423d6..79d91be468c4 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -1191,31 +1191,12 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  			    sector_t *first_bad, int *bad_sectors)
>  {
> -	int unacked_badblocks, acked_badblocks;
>  	int prev = -1, hint = -1, set = 0;
>  	struct badblocks_context bad;
> -	unsigned int seq;
> +	int unacked_badblocks = 0;
> +	int acked_badblocks = 0;
> +	u64 *p = bb->page;
>  	int len, rv;
> -	u64 *p;
> -
> -	WARN_ON(bb->shift < 0 || sectors == 0);
> -
> -	if (bb->shift > 0) {
> -		sector_t target;
> -
> -		/* round the start down, and the end up */
> -		target = s + sectors;
> -		rounddown(s, 1 << bb->shift);
> -		roundup(target, 1 << bb->shift);
> -		sectors = target - s;
> -	}
> -
> -retry:
> -	seq = read_seqbegin(&bb->lock);
> -
> -	p = bb->page;
> -	unacked_badblocks = 0;
> -	acked_badblocks = 0;
>  
>  re_check:
>  	bad.start = s;
> @@ -1281,9 +1262,6 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  	else
>  		rv = 0;
>  
> -	if (read_seqretry(&bb->lock, seq))
> -		goto retry;
> -
>  	return rv;
>  }
>  
> @@ -1324,7 +1302,27 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  			sector_t *first_bad, int *bad_sectors)
>  {
> -	return _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
> +	unsigned int seq;
> +	int rv;
> +
> +	WARN_ON(bb->shift < 0 || sectors == 0);
> +
> +	if (bb->shift > 0) {
> +		/* round the start down, and the end up */
> +		sector_t target = s + sectors;
> +
> +		rounddown(s, 1 << bb->shift);
> +		roundup(target, 1 << bb->shift);
> +		sectors = target - s;
> +	}
> +
> +retry:
> +	seq = read_seqbegin(&bb->lock);
> +	rv = _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
> +	if (read_seqretry(&bb->lock, seq))
> +		goto retry;
> +
> +	return rv;
>  }
>  EXPORT_SYMBOL_GPL(badblocks_check);
>  
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:09PM +0800, Zheng Qixing wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> There is a truncation of badblocks length issue when set badblocks as
> follow:
> 
> echo "2055 4294967299" > bad_blocks
> cat bad_blocks
> 2055 3
> 
> Change 'sectors' argument type from 'int' to 'sector_t'.
> 
> This change avoids truncation of badblocks length for large sectors by
> replacing 'int' with 'sector_t' (u64), enabling proper handling of larger
> disk sizes and ensuring compatibility with 64-bit sector addressing.
> 
> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>

Looks good to me.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

> ---
>  block/badblocks.c             | 20 ++++++++------------
>  drivers/block/null_blk/main.c |  2 +-
>  drivers/md/md.h               |  6 +++---
>  drivers/md/raid1-10.c         |  2 +-
>  drivers/md/raid1.c            |  4 ++--
>  drivers/md/raid10.c           |  8 ++++----
>  drivers/nvdimm/nd.h           |  2 +-
>  drivers/nvdimm/pfn_devs.c     |  7 ++++---
>  drivers/nvdimm/pmem.c         |  2 +-
>  include/linux/badblocks.h     |  8 ++++----
>  10 files changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 8f057563488a..14e3be47d22d 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -836,7 +836,7 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
>  }
>  
>  /* Do exact work to set bad block range into the bad block table */
> -static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  			   int acknowledged)
>  {
>  	int len = 0, added = 0;
> @@ -956,8 +956,6 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	if (sectors > 0)
>  		goto re_insert;
>  
> -	WARN_ON(sectors < 0);
> -
>  	/*
>  	 * Check whether the following already set range can be
>  	 * merged. (prev < 0) condition is not handled here,
> @@ -1048,7 +1046,7 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
>  }
>  
>  /* Do the exact work to clear bad block range from the bad block table */
> -static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  {
>  	struct badblocks_context bad;
>  	int prev = -1, hint = -1;
> @@ -1171,8 +1169,6 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  	if (sectors > 0)
>  		goto re_clear;
>  
> -	WARN_ON(sectors < 0);
> -
>  	if (cleared) {
>  		badblocks_update_acked(bb);
>  		set_changed(bb);
> @@ -1187,8 +1183,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  }
>  
>  /* Do the exact work to check bad blocks range from the bad block table */
> -static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -			    sector_t *first_bad, int *bad_sectors)
> +static int _badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +			    sector_t *first_bad, sector_t *bad_sectors)
>  {
>  	int prev = -1, hint = -1, set = 0;
>  	struct badblocks_context bad;
> @@ -1298,8 +1294,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   * -1: there are bad blocks which have not yet been acknowledged in metadata.
>   * plus the start/length of the first bad section we overlap.
>   */
> -int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -			sector_t *first_bad, int *bad_sectors)
> +int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +			sector_t *first_bad, sector_t *bad_sectors)
>  {
>  	unsigned int seq;
>  	int rv;
> @@ -1340,7 +1336,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>   *  true: success
>   *  false: failed to set badblocks (out of space)
>   */
> -bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  		   int acknowledged)
>  {
>  	return _badblocks_set(bb, s, sectors, acknowledged);
> @@ -1361,7 +1357,7 @@ EXPORT_SYMBOL_GPL(badblocks_set);
>   *  true: success
>   *  false: failed to clear badblocks
>   */
> -bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  {
>  	return _badblocks_clear(bb, s, sectors);
>  }
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 623db72ad66b..6e7d80b6e92b 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1302,7 +1302,7 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd,
>  {
>  	struct badblocks *bb = &cmd->nq->dev->badblocks;
>  	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>  
>  	if (badblocks_check(bb, sector, nr_sectors, &first_bad, &bad_sectors))
>  		return BLK_STS_IOERR;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 923a0ef51efe..6edc0f71b7d4 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -266,8 +266,8 @@ enum flag_bits {
>  	Nonrot,			/* non-rotational device (SSD) */
>  };
>  
> -static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
> -			      sector_t *first_bad, int *bad_sectors)
> +static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors,
> +			      sector_t *first_bad, sector_t *bad_sectors)
>  {
>  	if (unlikely(rdev->badblocks.count)) {
>  		int rv = badblocks_check(&rdev->badblocks, rdev->data_offset + s,
> @@ -284,7 +284,7 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
>  				    int sectors)
>  {
>  	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>  
>  	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
>  }
> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
> index 4378d3250bd7..62b980b12f93 100644
> --- a/drivers/md/raid1-10.c
> +++ b/drivers/md/raid1-10.c
> @@ -247,7 +247,7 @@ static inline int raid1_check_read_range(struct md_rdev *rdev,
>  					 sector_t this_sector, int *len)
>  {
>  	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>  
>  	/* no bad block overlap */
>  	if (!is_badblock(rdev, this_sector, *len, &first_bad, &bad_sectors))
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 8beb8cccc6af..0b2839105857 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1537,7 +1537,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		atomic_inc(&rdev->nr_pending);
>  		if (test_bit(WriteErrorSeen, &rdev->flags)) {
>  			sector_t first_bad;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>  			int is_bad;
>  
>  			is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
> @@ -2886,7 +2886,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
>  		} else {
>  			/* may need to read from here */
>  			sector_t first_bad = MaxSector;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>  
>  			if (is_badblock(rdev, sector_nr, good_sectors,
>  					&first_bad, &bad_sectors)) {
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 7ed933181712..a8664e29aada 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -747,7 +747,7 @@ static struct md_rdev *read_balance(struct r10conf *conf,
>  
>  	for (slot = 0; slot < conf->copies ; slot++) {
>  		sector_t first_bad;
> -		int bad_sectors;
> +		sector_t bad_sectors;
>  		sector_t dev_sector;
>  		unsigned int pending;
>  		bool nonrot;
> @@ -1438,7 +1438,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
>  		if (rdev && test_bit(WriteErrorSeen, &rdev->flags)) {
>  			sector_t first_bad;
>  			sector_t dev_sector = r10_bio->devs[i].addr;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>  			int is_bad;
>  
>  			is_bad = is_badblock(rdev, dev_sector, max_sectors,
> @@ -3413,7 +3413,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>  				sector_t from_addr, to_addr;
>  				struct md_rdev *rdev = conf->mirrors[d].rdev;
>  				sector_t sector, first_bad;
> -				int bad_sectors;
> +				sector_t bad_sectors;
>  				if (!rdev ||
>  				    !test_bit(In_sync, &rdev->flags))
>  					continue;
> @@ -3609,7 +3609,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>  		for (i = 0; i < conf->copies; i++) {
>  			int d = r10_bio->devs[i].devnum;
>  			sector_t first_bad, sector;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>  			struct md_rdev *rdev;
>  
>  			if (r10_bio->devs[i].repl_bio)
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 5ca06e9a2d29..cc5c8f3f81e8 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -673,7 +673,7 @@ static inline bool is_bad_pmem(struct badblocks *bb, sector_t sector,
>  {
>  	if (bb->count) {
>  		sector_t first_bad;
> -		int num_bad;
> +		sector_t num_bad;
>  
>  		return !!badblocks_check(bb, sector, len / 512, &first_bad,
>  				&num_bad);
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index cfdfe0eaa512..8f3e816e805d 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -367,9 +367,10 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>  	struct nd_namespace_common *ndns = nd_pfn->ndns;
>  	void *zero_page = page_address(ZERO_PAGE(0));
>  	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> -	int num_bad, meta_num, rc, bb_present;
> +	int meta_num, rc, bb_present;
>  	sector_t first_bad, meta_start;
>  	struct nd_namespace_io *nsio;
> +	sector_t num_bad;
>  
>  	if (nd_pfn->mode != PFN_MODE_PMEM)
>  		return 0;
> @@ -394,7 +395,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>  		bb_present = badblocks_check(&nd_region->bb, meta_start,
>  				meta_num, &first_bad, &num_bad);
>  		if (bb_present) {
> -			dev_dbg(&nd_pfn->dev, "meta: %x badblocks at %llx\n",
> +			dev_dbg(&nd_pfn->dev, "meta: %llx badblocks at %llx\n",
>  					num_bad, first_bad);
>  			nsoff = ALIGN_DOWN((nd_region->ndr_start
>  					+ (first_bad << 9)) - nsio->res.start,
> @@ -413,7 +414,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>  			}
>  			if (rc) {
>  				dev_err(&nd_pfn->dev,
> -					"error clearing %x badblocks at %llx\n",
> +					"error clearing %llx badblocks at %llx\n",
>  					num_bad, first_bad);
>  				return rc;
>  			}
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index d81faa9d89c9..43156e1576c9 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -249,7 +249,7 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
>  	unsigned int num = PFN_PHYS(nr_pages) >> SECTOR_SHIFT;
>  	struct badblocks *bb = &pmem->bb;
>  	sector_t first_bad;
> -	int num_bad;
> +	sector_t num_bad;
>  
>  	if (kaddr)
>  		*kaddr = pmem->virt_addr + offset;
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 8764bed9ff16..996493917f36 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -48,11 +48,11 @@ struct badblocks_context {
>  	int		ack;
>  };
>  
> -int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -		   sector_t *first_bad, int *bad_sectors);
> -bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +		    sector_t *first_bad, sector_t *bad_sectors);
> +bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  		   int acknowledged);
> -bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
> +bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors);
>  void ack_all_badblocks(struct badblocks *bb);
>  ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
>  ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 00/12] badblocks: bugfix and cleanup for badblocks

[Coly Li]

On Fri, Feb 21, 2025 at 04:10:57PM +0800, Zheng Qixing wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> During RAID feature implementation testing, we found several bugs
> in badblocks.
> 
> This series contains bugfixes and cleanups for MD RAID badblocks
> handling code.
> 
> Li Nan (8):
>   badblocks: Fix error shitf ops
>   badblocks: factor out a helper try_adjacent_combine
>   badblocks: attempt to merge adjacent badblocks during
>     ack_all_badblocks
>   badblocks: return error directly when setting badblocks exceeds 512
>   badblocks: return error if any badblock set fails
>   badblocks: fix the using of MAX_BADBLOCKS
>   badblocks: try can_merge_front before overlap_front
>   badblocks: fix merge issue when new badblocks align with pre+1
> 
> Zheng Qixing (4):
>   badblocks: fix missing bad blocks on retry in _badblocks_check()
>   badblocks: return boolen from badblocks_set() and badblocks_clear()
>   md: improve return types of badblocks handling functions
>   badblocks: use sector_t instead of int to avoid truncation of
>     badblocks length
>

Thank you all for the testing and fix up!

Coly Li

 
>  block/badblocks.c             | 317 +++++++++++++---------------------
>  drivers/block/null_blk/main.c |  19 +-
>  drivers/md/md.c               |  47 +++--
>  drivers/md/md.h               |  14 +-
>  drivers/md/raid1-10.c         |   2 +-
>  drivers/md/raid1.c            |  10 +-
>  drivers/md/raid10.c           |  14 +-
>  drivers/nvdimm/badrange.c     |   2 +-
>  drivers/nvdimm/nd.h           |   2 +-
>  drivers/nvdimm/pfn_devs.c     |   7 +-
>  drivers/nvdimm/pmem.c         |   2 +-
>  include/linux/badblocks.h     |  10 +-
>  12 files changed, 181 insertions(+), 265 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Yu Kuai]

Hi,

在 2025/02/21 18:12, Coly Li 写道:
> So we don’t need to add a negative return value for partial success/failure?
> 
> Coly Li.

Yes, I think so. No one really use this value, and patch 10 clean this
up by changing return type to bool.

Thanks,
Kuai

Re: [PATCH 07/12] badblocks: try can_merge_front before overlap_front

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> Regardless of whether overlap_front() returns true or false,
> can_merge_front() will be executed first. Therefore, move
> can_merge_front() in front of can_merge_front() to simplify code.
> 
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 48 ++++++++++++++++++++++-------------------------
>   1 file changed, 22 insertions(+), 26 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index 87267bae6836..bb46bab7e99f 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -905,39 +905,35 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   		goto update_sectors;
>   	}
>   
> +	if (can_merge_front(bb, prev, &bad)) {
> +		len = front_merge(bb, prev, &bad);
> +		added++;
> +		hint = prev;
> +		goto update_sectors;
> +	}
> +
>   	if (overlap_front(bb, prev, &bad)) {
> -		if (can_merge_front(bb, prev, &bad)) {
> -			len = front_merge(bb, prev, &bad);
> -			added++;
> -		} else {
> -			int extra = 0;
> +		int extra = 0;
>   
> -			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
> -				if (extra > 0)
> -					goto out;
> +		if (!can_front_overwrite(bb, prev, &bad, &extra)) {
> +			if (extra > 0)
> +				goto out;
>   
> -				len = min_t(sector_t,
> -					    BB_END(p[prev]) - s, sectors);
> -				hint = prev;
> -				goto update_sectors;
> -			}
> +			len = min_t(sector_t,
> +				    BB_END(p[prev]) - s, sectors);
> +			hint = prev;
> +			goto update_sectors;
> +		}
>   
> -			len = front_overwrite(bb, prev, &bad, extra);
> -			added++;
> -			bb->count += extra;
> +		len = front_overwrite(bb, prev, &bad, extra);
> +		added++;
> +		bb->count += extra;
>   
> -			if (can_combine_front(bb, prev, &bad)) {
> -				front_combine(bb, prev);
> -				bb->count--;
> -			}
> +		if (can_combine_front(bb, prev, &bad)) {
> +			front_combine(bb, prev);
> +			bb->count--;
>   		}
> -		hint = prev;
> -		goto update_sectors;
> -	}
>   
> -	if (can_merge_front(bb, prev, &bad)) {
> -		len = front_merge(bb, prev, &bad);
> -		added++;
>   		hint = prev;
>   		goto update_sectors;
>   	}
> 

Re: [PATCH 06/12] badblocks: fix the using of MAX_BADBLOCKS

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> The number of badblocks cannot exceed MAX_BADBLOCKS, but it should be
> allowed to equal MAX_BADBLOCKS.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index a953d2e9417f..87267bae6836 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -700,7 +700,7 @@ static bool can_front_overwrite(struct badblocks *bb, int prev,
>   			*extra = 2;
>   	}
>   
> -	if ((bb->count + (*extra)) >= MAX_BADBLOCKS)
> +	if ((bb->count + (*extra)) > MAX_BADBLOCKS)
>   		return false;
>   
>   	return true;
> @@ -1135,7 +1135,7 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   		if ((BB_OFFSET(p[prev]) < bad.start) &&
>   		    (BB_END(p[prev]) > (bad.start + bad.len))) {
>   			/* Splitting */
> -			if ((bb->count + 1) < MAX_BADBLOCKS) {
> +			if ((bb->count + 1) <= MAX_BADBLOCKS) {
>   				len = front_splitting_clear(bb, prev, &bad);
>   				bb->count += 1;
>   				cleared++;
> 

Re: [PATCH 08/12] badblocks: fix merge issue when new badblocks align with pre+1

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Li Nan <linan122@huawei.com>
> 
> There is a merge issue when adding badblocks as follow:
>    echo 0 10 > bad_blocks
>    echo 30 10 > bad_blocks
>    echo 20 10 > bad_blocks
>    cat bad_blocks
>    0 10
>    20 10    //should be merged with (30 10)
>    30 10
> 
> In this case, if new badblocks does not intersect with prev, it is added
> by insert_at(). If there is an intersection with prev+1, the merge will
> be processed in the next re_insert loop.
> 
> However, when the end of the new badblocks is exactly equal to the offset
> of prev+1, no further re_insert loop occurs, and the two badblocks are not
> merge.
> 
> Fix it by inc prev, badblocks can be merged during the subsequent code.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   block/badblocks.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index bb46bab7e99f..381f9db423d6 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -892,7 +892,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   		len = insert_at(bb, 0, &bad);
>   		bb->count++;
>   		added++;
> -		hint = 0;
> +		hint = ++prev;
>   		goto update_sectors;
>   	}
>   
> @@ -947,7 +947,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   	len = insert_at(bb, prev + 1, &bad);
>   	bb->count++;
>   	added++;
> -	hint = prev + 1;
> +	hint = ++prev;
>   
>   update_sectors:
>   	s += len;
> 

Re: [PATCH 10/12] badblocks: return boolen from badblocks_set() and badblocks_clear()

[Yu Kuai]

Hi,

Just two simple coding styes below.

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> Change the return type of badblocks_set() and badblocks_clear()
> from int to bool, indicating success or failure. Specifically:
> 
> - _badblocks_set() and _badblocks_clear() functions now return
> true for success and false for failure.
> - All calls to these functions have been updated to handle the
> new boolean return type.
> - This change improves code clarity and ensures a more consistent
> handling of success and failure states.
> 
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   block/badblocks.c             | 37 +++++++++++++++++------------------
>   drivers/block/null_blk/main.c | 17 ++++++++--------
>   drivers/md/md.c               | 35 +++++++++++++++++----------------
>   drivers/nvdimm/badrange.c     |  2 +-
>   include/linux/badblocks.h     |  6 +++---
>   5 files changed, 49 insertions(+), 48 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 79d91be468c4..8f057563488a 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -836,8 +836,8 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
>   }
>   
>   /* Do exact work to set bad block range into the bad block table */
> -static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			  int acknowledged)
> +static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +			   int acknowledged)
>   {
>   	int len = 0, added = 0;
>   	struct badblocks_context bad;
> @@ -847,11 +847,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   
>   	if (bb->shift < 0)
>   		/* badblocks are disabled */
> -		return 1;
> +		return false;
>   
>   	if (sectors == 0)
>   		/* Invalid sectors number */
> -		return 1;
> +		return false;
>   
>   	if (bb->shift) {
>   		/* round the start down, and the end up */
> @@ -977,7 +977,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   
>   	write_sequnlock_irqrestore(&bb->lock, flags);
>   
> -	return sectors;
> +	return sectors == 0;
>   }
>   
>   /*
> @@ -1048,21 +1048,20 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
>   }
>   
>   /* Do the exact work to clear bad block range from the bad block table */
> -static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   {
>   	struct badblocks_context bad;
>   	int prev = -1, hint = -1;
>   	int len = 0, cleared = 0;
> -	int rv = 0;
>   	u64 *p;
>   
>   	if (bb->shift < 0)
>   		/* badblocks are disabled */
> -		return 1;
> +		return false;
>   
>   	if (sectors == 0)
>   		/* Invalid sectors number */
> -		return 1;
> +		return false;
>   
>   	if (bb->shift) {
>   		sector_t target;
> @@ -1182,9 +1181,9 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   	write_sequnlock_irq(&bb->lock);
>   
>   	if (!cleared)
> -		rv = 1;
> +		return false;
>   
> -	return rv;
> +	return true;
>   }
>   
>   /* Do the exact work to check bad blocks range from the bad block table */
> @@ -1338,11 +1337,11 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>    * decide how best to handle it.
>    *
>    * Return:
> - *  0: success
> - *  other: failed to set badblocks (out of space)
> + *  true: success
> + *  false: failed to set badblocks (out of space)
>    */
> -int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			int acknowledged)
> +bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +		   int acknowledged)
>   {
>   	return _badblocks_set(bb, s, sectors, acknowledged);
>   }
> @@ -1359,10 +1358,10 @@ EXPORT_SYMBOL_GPL(badblocks_set);
>    * drop the remove request.
>    *
>    * Return:
> - *  0: success
> - *  1: failed to clear badblocks
> + *  true: success
> + *  false: failed to clear badblocks
>    */
> -int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   {
>   	return _badblocks_clear(bb, s, sectors);
>   }
> @@ -1484,7 +1483,7 @@ ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
>   		return -EINVAL;
>   	}
>   
> -	if (badblocks_set(bb, sector, length, !unack))
> +	if (!badblocks_set(bb, sector, length, !unack))
>   		return -ENOSPC;
>   	else

Since we're here, can you also remove the else branch as well?

>   		return len;
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index d94ef37480bd..623db72ad66b 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -559,14 +559,15 @@ static ssize_t nullb_device_badblocks_store(struct config_item *item,
>   		goto out;
>   	/* enable badblocks */
>   	cmpxchg(&t_dev->badblocks.shift, -1, 0);
> -	if (buf[0] == '+')
> -		ret = badblocks_set(&t_dev->badblocks, start,
> -			end - start + 1, 1);
> -	else
> -		ret = badblocks_clear(&t_dev->badblocks, start,
> -			end - start + 1);
> -	if (ret == 0)
> -		ret = count;
> +	if (buf[0] == '+') {
> +		if (badblocks_set(&t_dev->badblocks, start,
> +				  end - start + 1, 1))
> +			ret = count;
> +	} else {
> +		if (badblocks_clear(&t_dev->badblocks, start,
> +				    end - start + 1))

else if ()

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Thanks
> +			ret = count;
> +	}
>   out:
>   	kfree(orig);
>   	return ret;
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 30b3dbbce2d2..49d826e475cb 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1748,7 +1748,7 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
>   			count <<= sb->bblog_shift;
>   			if (bb + 1 == 0)
>   				break;
> -			if (badblocks_set(&rdev->badblocks, sector, count, 1))
> +			if (!badblocks_set(&rdev->badblocks, sector, count, 1))
>   				return -EINVAL;
>   		}
>   	} else if (sb->bblog_offset != 0)
> @@ -9846,7 +9846,6 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   		       int is_new)
>   {
>   	struct mddev *mddev = rdev->mddev;
> -	int rv;
>   
>   	/*
>   	 * Recording new badblocks for faulty rdev will force unnecessary
> @@ -9862,33 +9861,35 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   		s += rdev->new_data_offset;
>   	else
>   		s += rdev->data_offset;
> -	rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
> -	if (rv == 0) {
> -		/* Make sure they get written out promptly */
> -		if (test_bit(ExternalBbl, &rdev->flags))
> -			sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
> -		sysfs_notify_dirent_safe(rdev->sysfs_state);
> -		set_mask_bits(&mddev->sb_flags, 0,
> -			      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
> -		md_wakeup_thread(rdev->mddev->thread);
> -		return 1;
> -	} else
> +
> +	if (!badblocks_set(&rdev->badblocks, s, sectors, 0))
>   		return 0;
> +
> +	/* Make sure they get written out promptly */
> +	if (test_bit(ExternalBbl, &rdev->flags))
> +		sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
> +	sysfs_notify_dirent_safe(rdev->sysfs_state);
> +	set_mask_bits(&mddev->sb_flags, 0,
> +		      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
> +	md_wakeup_thread(rdev->mddev->thread);
> +	return 1;
>   }
>   EXPORT_SYMBOL_GPL(rdev_set_badblocks);
>   
>   int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   			 int is_new)
>   {
> -	int rv;
>   	if (is_new)
>   		s += rdev->new_data_offset;
>   	else
>   		s += rdev->data_offset;
> -	rv = badblocks_clear(&rdev->badblocks, s, sectors);
> -	if ((rv == 0) && test_bit(ExternalBbl, &rdev->flags))
> +
> +	if (!badblocks_clear(&rdev->badblocks, s, sectors))
> +		return 0;
> +
> +	if (test_bit(ExternalBbl, &rdev->flags))
>   		sysfs_notify_dirent_safe(rdev->sysfs_badblocks);
> -	return rv;
> +	return 1;
>   }
>   EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
>   
> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
> index a002ea6fdd84..ee478ccde7c6 100644
> --- a/drivers/nvdimm/badrange.c
> +++ b/drivers/nvdimm/badrange.c
> @@ -167,7 +167,7 @@ static void set_badblock(struct badblocks *bb, sector_t s, int num)
>   	dev_dbg(bb->dev, "Found a bad range (0x%llx, 0x%llx)\n",
>   			(u64) s * 512, (u64) num * 512);
>   	/* this isn't an error as the hardware will still throw an exception */
> -	if (badblocks_set(bb, s, num, 1))
> +	if (!badblocks_set(bb, s, num, 1))
>   		dev_info_once(bb->dev, "%s: failed for sector %llx\n",
>   				__func__, (u64) s);
>   }
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 670f2dae692f..8764bed9ff16 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -50,9 +50,9 @@ struct badblocks_context {
>   
>   int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   		   sector_t *first_bad, int *bad_sectors);
> -int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			int acknowledged);
> -int badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
> +bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +		   int acknowledged);
> +bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
>   void ack_all_badblocks(struct badblocks *bb);
>   ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
>   ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> 

Re: [PATCH 11/12] md: improve return types of badblocks handling functions

[Yu Kuai]

Hi, one nit below

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> rdev_set_badblocks() only indicates success/failure, so convert its return
> type from int to boolean for better semantic clarity.
> 
> rdev_clear_badblocks() return value is never used by any caller, convert it
> to void. This removes unnecessary value returns.
> 
> Also update narrow_write_error() in both raid1 and raid10 to use boolean
> return type to match rdev_set_badblocks().
> 
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   drivers/md/md.c     | 20 +++++++++-----------
>   drivers/md/md.h     |  8 ++++----
>   drivers/md/raid1.c  |  6 +++---
>   drivers/md/raid10.c |  6 +++---
>   4 files changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 49d826e475cb..76c437376542 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9841,9 +9841,9 @@ EXPORT_SYMBOL(md_finish_reshape);
>   
>   /* Bad block management */
>   
> -/* Returns 1 on success, 0 on failure */
> -int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> -		       int is_new)
> +/* Returns true on success, false on failure */
> +bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> +			int is_new)
>   {
>   	struct mddev *mddev = rdev->mddev;
>   
> @@ -9855,7 +9855,7 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   	 * avoid it.
>   	 */
>   	if (test_bit(Faulty, &rdev->flags))
> -		return 1;
> +		return true;
>   
>   	if (is_new)
>   		s += rdev->new_data_offset;
> @@ -9863,7 +9863,7 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   		s += rdev->data_offset;
>   
>   	if (!badblocks_set(&rdev->badblocks, s, sectors, 0))
> -		return 0;
> +		return false;
>   
>   	/* Make sure they get written out promptly */
>   	if (test_bit(ExternalBbl, &rdev->flags))
> @@ -9872,24 +9872,22 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>   	set_mask_bits(&mddev->sb_flags, 0,
>   		      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
>   	md_wakeup_thread(rdev->mddev->thread);
> -	return 1;
> +	return true;
>   }
>   EXPORT_SYMBOL_GPL(rdev_set_badblocks);
>   
> -int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> -			 int is_new)
> +void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> +			  int is_new)
>   {
>   	if (is_new)
>   		s += rdev->new_data_offset;
>   	else
>   		s += rdev->data_offset;
>   
> -	if (!badblocks_clear(&rdev->badblocks, s, sectors))
> -		return 0;
> +	badblocks_clear(&rdev->badblocks, s, sectors);

Plese don't make functional change, and this dones not make sense
to me.

Thanks,
Kuai

>   
>   	if (test_bit(ExternalBbl, &rdev->flags))
>   		sysfs_notify_dirent_safe(rdev->sysfs_badblocks);
> -	return 1;
>   }
>   EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
>   
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index def808064ad8..923a0ef51efe 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -289,10 +289,10 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
>   	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
>   }
>   
> -extern int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> -			      int is_new);
> -extern int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> -				int is_new);
> +extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> +			       int is_new);
> +extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> +				 int is_new);
>   struct md_cluster_info;
>   
>   /**
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 9d57a88dbd26..8beb8cccc6af 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2486,7 +2486,7 @@ static void fix_read_error(struct r1conf *conf, struct r1bio *r1_bio)
>   	}
>   }
>   
> -static int narrow_write_error(struct r1bio *r1_bio, int i)
> +static bool narrow_write_error(struct r1bio *r1_bio, int i)
>   {
>   	struct mddev *mddev = r1_bio->mddev;
>   	struct r1conf *conf = mddev->private;
> @@ -2507,10 +2507,10 @@ static int narrow_write_error(struct r1bio *r1_bio, int i)
>   	sector_t sector;
>   	int sectors;
>   	int sect_to_write = r1_bio->sectors;
> -	int ok = 1;
> +	bool ok = true;
>   
>   	if (rdev->badblocks.shift < 0)
> -		return 0;
> +		return false;
>   
>   	block_sectors = roundup(1 << rdev->badblocks.shift,
>   				bdev_logical_block_size(rdev->bdev) >> 9);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index efe93b979167..7ed933181712 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2786,7 +2786,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
>   	}
>   }
>   
> -static int narrow_write_error(struct r10bio *r10_bio, int i)
> +static bool narrow_write_error(struct r10bio *r10_bio, int i)
>   {
>   	struct bio *bio = r10_bio->master_bio;
>   	struct mddev *mddev = r10_bio->mddev;
> @@ -2807,10 +2807,10 @@ static int narrow_write_error(struct r10bio *r10_bio, int i)
>   	sector_t sector;
>   	int sectors;
>   	int sect_to_write = r10_bio->sectors;
> -	int ok = 1;
> +	bool ok = true;
>   
>   	if (rdev->badblocks.shift < 0)
> -		return 0;
> +		return false;
>   
>   	block_sectors = roundup(1 << rdev->badblocks.shift,
>   				bdev_logical_block_size(rdev->bdev) >> 9);
> 

Re: [PATCH 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check()

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> The bad blocks check would miss bad blocks when retrying under contention,
> as checking parameters are not reset. These stale values from the previous
> attempt could lead to incorrect scanning in the subsequent retry.
> 
> Move seqlock to outer function and reinitialize checking state for each
> retry. This ensures a clean state for each check attempt, preventing any
> missed bad blocks.
> 
> Fixes: 3ea3354cb9f0 ("badblocks: improve badblocks_check() for multiple ranges handling")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   block/badblocks.c | 50 +++++++++++++++++++++++------------------------
>   1 file changed, 24 insertions(+), 26 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/block/badblocks.c b/block/badblocks.c
> index 381f9db423d6..79d91be468c4 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -1191,31 +1191,12 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   			    sector_t *first_bad, int *bad_sectors)
>   {
> -	int unacked_badblocks, acked_badblocks;
>   	int prev = -1, hint = -1, set = 0;
>   	struct badblocks_context bad;
> -	unsigned int seq;
> +	int unacked_badblocks = 0;
> +	int acked_badblocks = 0;
> +	u64 *p = bb->page;
>   	int len, rv;
> -	u64 *p;
> -
> -	WARN_ON(bb->shift < 0 || sectors == 0);
> -
> -	if (bb->shift > 0) {
> -		sector_t target;
> -
> -		/* round the start down, and the end up */
> -		target = s + sectors;
> -		rounddown(s, 1 << bb->shift);
> -		roundup(target, 1 << bb->shift);
> -		sectors = target - s;
> -	}
> -
> -retry:
> -	seq = read_seqbegin(&bb->lock);
> -
> -	p = bb->page;
> -	unacked_badblocks = 0;
> -	acked_badblocks = 0;
>   
>   re_check:
>   	bad.start = s;
> @@ -1281,9 +1262,6 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   	else
>   		rv = 0;
>   
> -	if (read_seqretry(&bb->lock, seq))
> -		goto retry;
> -
>   	return rv;
>   }
>   
> @@ -1324,7 +1302,27 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>   			sector_t *first_bad, int *bad_sectors)
>   {
> -	return _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
> +	unsigned int seq;
> +	int rv;
> +
> +	WARN_ON(bb->shift < 0 || sectors == 0);
> +
> +	if (bb->shift > 0) {
> +		/* round the start down, and the end up */
> +		sector_t target = s + sectors;
> +
> +		rounddown(s, 1 << bb->shift);
> +		roundup(target, 1 << bb->shift);
> +		sectors = target - s;
> +	}
> +
> +retry:
> +	seq = read_seqbegin(&bb->lock);
> +	rv = _badblocks_check(bb, s, sectors, first_bad, bad_sectors);
> +	if (read_seqretry(&bb->lock, seq))
> +		goto retry;
> +
> +	return rv;
>   }
>   EXPORT_SYMBOL_GPL(badblocks_check);
>   
> 

Re: [PATCH 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length

[Yu Kuai]

在 2025/02/21 16:11, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> There is a truncation of badblocks length issue when set badblocks as
> follow:
> 
> echo "2055 4294967299" > bad_blocks
> cat bad_blocks
> 2055 3
> 
> Change 'sectors' argument type from 'int' to 'sector_t'.
> 
> This change avoids truncation of badblocks length for large sectors by
> replacing 'int' with 'sector_t' (u64), enabling proper handling of larger
> disk sizes and ensuring compatibility with 64-bit sector addressing.
> 
> Fixes: 9e0e252a048b ("badblocks: Add core badblock management code")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   block/badblocks.c             | 20 ++++++++------------
>   drivers/block/null_blk/main.c |  2 +-
>   drivers/md/md.h               |  6 +++---
>   drivers/md/raid1-10.c         |  2 +-
>   drivers/md/raid1.c            |  4 ++--
>   drivers/md/raid10.c           |  8 ++++----
>   drivers/nvdimm/nd.h           |  2 +-
>   drivers/nvdimm/pfn_devs.c     |  7 ++++---
>   drivers/nvdimm/pmem.c         |  2 +-
>   include/linux/badblocks.h     |  8 ++++----
>   10 files changed, 29 insertions(+), 32 deletions(-)
> 
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 8f057563488a..14e3be47d22d 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -836,7 +836,7 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
>   }
>   
>   /* Do exact work to set bad block range into the bad block table */
> -static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>   			   int acknowledged)
>   {
>   	int len = 0, added = 0;
> @@ -956,8 +956,6 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>   	if (sectors > 0)
>   		goto re_insert;
>   
> -	WARN_ON(sectors < 0);
> -
>   	/*
>   	 * Check whether the following already set range can be
>   	 * merged. (prev < 0) condition is not handled here,
> @@ -1048,7 +1046,7 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
>   }
>   
>   /* Do the exact work to clear bad block range from the bad block table */
> -static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>   {
>   	struct badblocks_context bad;
>   	int prev = -1, hint = -1;
> @@ -1171,8 +1169,6 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   	if (sectors > 0)
>   		goto re_clear;
>   
> -	WARN_ON(sectors < 0);
> -
>   	if (cleared) {
>   		badblocks_update_acked(bb);
>   		set_changed(bb);
> @@ -1187,8 +1183,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>   }
>   
>   /* Do the exact work to check bad blocks range from the bad block table */
> -static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -			    sector_t *first_bad, int *bad_sectors)
> +static int _badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +			    sector_t *first_bad, sector_t *bad_sectors)
>   {
>   	int prev = -1, hint = -1, set = 0;
>   	struct badblocks_context bad;
> @@ -1298,8 +1294,8 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>    * -1: there are bad blocks which have not yet been acknowledged in metadata.
>    * plus the start/length of the first bad section we overlap.
>    */
> -int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -			sector_t *first_bad, int *bad_sectors)
> +int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +			sector_t *first_bad, sector_t *bad_sectors)
>   {
>   	unsigned int seq;
>   	int rv;
> @@ -1340,7 +1336,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>    *  true: success
>    *  false: failed to set badblocks (out of space)
>    */
> -bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>   		   int acknowledged)
>   {
>   	return _badblocks_set(bb, s, sectors, acknowledged);
> @@ -1361,7 +1357,7 @@ EXPORT_SYMBOL_GPL(badblocks_set);
>    *  true: success
>    *  false: failed to clear badblocks
>    */
> -bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>   {
>   	return _badblocks_clear(bb, s, sectors);
>   }
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 623db72ad66b..6e7d80b6e92b 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1302,7 +1302,7 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd,
>   {
>   	struct badblocks *bb = &cmd->nq->dev->badblocks;
>   	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>   
>   	if (badblocks_check(bb, sector, nr_sectors, &first_bad, &bad_sectors))
>   		return BLK_STS_IOERR;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 923a0ef51efe..6edc0f71b7d4 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -266,8 +266,8 @@ enum flag_bits {
>   	Nonrot,			/* non-rotational device (SSD) */
>   };
>   
> -static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
> -			      sector_t *first_bad, int *bad_sectors)
> +static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors,
> +			      sector_t *first_bad, sector_t *bad_sectors)
>   {
>   	if (unlikely(rdev->badblocks.count)) {
>   		int rv = badblocks_check(&rdev->badblocks, rdev->data_offset + s,
> @@ -284,7 +284,7 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
>   				    int sectors)
>   {
>   	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>   
>   	return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
>   }
> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
> index 4378d3250bd7..62b980b12f93 100644
> --- a/drivers/md/raid1-10.c
> +++ b/drivers/md/raid1-10.c
> @@ -247,7 +247,7 @@ static inline int raid1_check_read_range(struct md_rdev *rdev,
>   					 sector_t this_sector, int *len)
>   {
>   	sector_t first_bad;
> -	int bad_sectors;
> +	sector_t bad_sectors;
>   
>   	/* no bad block overlap */
>   	if (!is_badblock(rdev, this_sector, *len, &first_bad, &bad_sectors))
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 8beb8cccc6af..0b2839105857 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1537,7 +1537,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>   		atomic_inc(&rdev->nr_pending);
>   		if (test_bit(WriteErrorSeen, &rdev->flags)) {
>   			sector_t first_bad;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>   			int is_bad;
>   
>   			is_bad = is_badblock(rdev, r1_bio->sector, max_sectors,
> @@ -2886,7 +2886,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
>   		} else {
>   			/* may need to read from here */
>   			sector_t first_bad = MaxSector;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>   
>   			if (is_badblock(rdev, sector_nr, good_sectors,
>   					&first_bad, &bad_sectors)) {
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 7ed933181712..a8664e29aada 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -747,7 +747,7 @@ static struct md_rdev *read_balance(struct r10conf *conf,
>   
>   	for (slot = 0; slot < conf->copies ; slot++) {
>   		sector_t first_bad;
> -		int bad_sectors;
> +		sector_t bad_sectors;
>   		sector_t dev_sector;
>   		unsigned int pending;
>   		bool nonrot;
> @@ -1438,7 +1438,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
>   		if (rdev && test_bit(WriteErrorSeen, &rdev->flags)) {
>   			sector_t first_bad;
>   			sector_t dev_sector = r10_bio->devs[i].addr;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>   			int is_bad;
>   
>   			is_bad = is_badblock(rdev, dev_sector, max_sectors,
> @@ -3413,7 +3413,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				sector_t from_addr, to_addr;
>   				struct md_rdev *rdev = conf->mirrors[d].rdev;
>   				sector_t sector, first_bad;
> -				int bad_sectors;
> +				sector_t bad_sectors;
>   				if (!rdev ||
>   				    !test_bit(In_sync, &rdev->flags))
>   					continue;
> @@ -3609,7 +3609,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   		for (i = 0; i < conf->copies; i++) {
>   			int d = r10_bio->devs[i].devnum;
>   			sector_t first_bad, sector;
> -			int bad_sectors;
> +			sector_t bad_sectors;
>   			struct md_rdev *rdev;
>   
>   			if (r10_bio->devs[i].repl_bio)
> diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
> index 5ca06e9a2d29..cc5c8f3f81e8 100644
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -673,7 +673,7 @@ static inline bool is_bad_pmem(struct badblocks *bb, sector_t sector,
>   {
>   	if (bb->count) {
>   		sector_t first_bad;
> -		int num_bad;
> +		sector_t num_bad;
>   
>   		return !!badblocks_check(bb, sector, len / 512, &first_bad,
>   				&num_bad);
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index cfdfe0eaa512..8f3e816e805d 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -367,9 +367,10 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>   	struct nd_namespace_common *ndns = nd_pfn->ndns;
>   	void *zero_page = page_address(ZERO_PAGE(0));
>   	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> -	int num_bad, meta_num, rc, bb_present;
> +	int meta_num, rc, bb_present;
>   	sector_t first_bad, meta_start;
>   	struct nd_namespace_io *nsio;
> +	sector_t num_bad;
>   
>   	if (nd_pfn->mode != PFN_MODE_PMEM)
>   		return 0;
> @@ -394,7 +395,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>   		bb_present = badblocks_check(&nd_region->bb, meta_start,
>   				meta_num, &first_bad, &num_bad);
>   		if (bb_present) {
> -			dev_dbg(&nd_pfn->dev, "meta: %x badblocks at %llx\n",
> +			dev_dbg(&nd_pfn->dev, "meta: %llx badblocks at %llx\n",
>   					num_bad, first_bad);
>   			nsoff = ALIGN_DOWN((nd_region->ndr_start
>   					+ (first_bad << 9)) - nsio->res.start,
> @@ -413,7 +414,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>   			}
>   			if (rc) {
>   				dev_err(&nd_pfn->dev,
> -					"error clearing %x badblocks at %llx\n",
> +					"error clearing %llx badblocks at %llx\n",
>   					num_bad, first_bad);
>   				return rc;
>   			}
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index d81faa9d89c9..43156e1576c9 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -249,7 +249,7 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
>   	unsigned int num = PFN_PHYS(nr_pages) >> SECTOR_SHIFT;
>   	struct badblocks *bb = &pmem->bb;
>   	sector_t first_bad;
> -	int num_bad;
> +	sector_t num_bad;
>   
>   	if (kaddr)
>   		*kaddr = pmem->virt_addr + offset;
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 8764bed9ff16..996493917f36 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -48,11 +48,11 @@ struct badblocks_context {
>   	int		ack;
>   };
>   
> -int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> -		   sector_t *first_bad, int *bad_sectors);
> -bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
> +		    sector_t *first_bad, sector_t *bad_sectors);
> +bool badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>   		   int acknowledged);
> -bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
> +bool badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors);
>   void ack_all_badblocks(struct badblocks *bb);
>   ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
>   ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> 

Re: [PATCH 10/12] badblocks: return boolen from badblocks_set() and badblocks_clear()

[Paul Menzel]

Dear Zheng,


Thank you for your patch. *boolean* in the summary/title is missing an *a*.

Am 22.02.25 um 02:26 schrieb Yu Kuai:

> Just two simple coding styes below.

I’d put these into a separate commit.

> 在 2025/02/21 16:11, Zheng Qixing 写道:
>> From: Zheng Qixing <zhengqixing@huawei.com>
>>
>> Change the return type of badblocks_set() and badblocks_clear()
>> from int to bool, indicating success or failure. Specifically:
>>
>> - _badblocks_set() and _badblocks_clear() functions now return
>> true for success and false for failure.
>> - All calls to these functions have been updated to handle the
>> new boolean return type.

I’d use present tense: are updated

>> - This change improves code clarity and ensures a more consistent
>> handling of success and failure states.
>>
>> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
>> ---
>>   block/badblocks.c             | 37 +++++++++++++++++------------------
>>   drivers/block/null_blk/main.c | 17 ++++++++--------
>>   drivers/md/md.c               | 35 +++++++++++++++++----------------
>>   drivers/nvdimm/badrange.c     |  2 +-
>>   include/linux/badblocks.h     |  6 +++---
>>   5 files changed, 49 insertions(+), 48 deletions(-)

[…]


Kind regards,

Paul

Re: [PATCH 10/12] badblocks: return boolen from badblocks_set() and badblocks_clear()

[Paul Menzel]

[Remove non-working colyli@suse.de]

Am 22.02.25 um 05:32 schrieb Paul Menzel:
> Dear Zheng,
> 
> 
> Thank you for your patch. *boolean* in the summary/title is missing an *a*.
> 
> Am 22.02.25 um 02:26 schrieb Yu Kuai:
> 
>> Just two simple coding styes below.
> 
> I’d put these into a separate commit.
> 
>> 在 2025/02/21 16:11, Zheng Qixing 写道:
>>> From: Zheng Qixing <zhengqixing@huawei.com>
>>>
>>> Change the return type of badblocks_set() and badblocks_clear()
>>> from int to bool, indicating success or failure. Specifically:
>>>
>>> - _badblocks_set() and _badblocks_clear() functions now return
>>> true for success and false for failure.
>>> - All calls to these functions have been updated to handle the
>>> new boolean return type.
> 
> I’d use present tense: are updated
> 
>>> - This change improves code clarity and ensures a more consistent
>>> handling of success and failure states.
>>>
>>> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
>>> ---
>>>   block/badblocks.c             | 37 +++++++++++++++++------------------
>>>   drivers/block/null_blk/main.c | 17 ++++++++--------
>>>   drivers/md/md.c               | 35 +++++++++++++++++----------------
>>>   drivers/nvdimm/badrange.c     |  2 +-
>>>   include/linux/badblocks.h     |  6 +++---
>>>   5 files changed, 49 insertions(+), 48 deletions(-)
> 
> […]
> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Coly Li]

On Sat, Feb 22, 2025 at 09:12:53AM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/02/21 18:12, Coly Li 写道:
> > So we don’t need to add a negative return value for partial success/failure?
> > 
> > Coly Li.
> 
> Yes, I think so. No one really use this value, and patch 10 clean this
> up by changing return type to bool.

OK, then it is fine to me.

It will be good to add a code comment that parital setting will be treated as failure.

Thanks.


-- 
Coly Li

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@huawei.com>
> 
> _badblocks_set() returns success if at least one badblock is set
> successfully, even if others fail. This can lead to data inconsistencies
> in raid, where a failed badblock set should trigger the disk to be kicked
> out to prevent future reads from failed write areas.
> 
> _badblocks_set() should return error if any badblock set fails. Instead
> of relying on 'rv', directly returning 'sectors' for clearer logic. If all
> badblocks are successfully set, 'sectors' will be 0, otherwise it
> indicates the number of badblocks that have not been set yet, thus
> signaling failure.
> 
> By the way, it can also fix an issue: when a newly set unack badblock is
> included in an existing ack badblock, the setting will return an error.
> ···
>   echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
>   echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
>   -bash: echo: write error: No space left on device
> ```
> After fix, it will return success.
> 
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Signed-off-by: Li Nan <linan122@huawei.com>

Based on Kuai's reply that we dont handle partial set and treat it as failure,
I am fine with this patch.

It will be good to add comment to explain that the parital set condition will be
treated as failure.

Acked-by: Coly Li <colyli@kernel.org>

Thanks.


> ---
>  block/badblocks.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 1c8b8f65f6df..a953d2e9417f 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -843,7 +843,6 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	struct badblocks_context bad;
>  	int prev = -1, hint = -1;
>  	unsigned long flags;
> -	int rv = 0;
>  	u64 *p;
>  
>  	if (bb->shift < 0)
> @@ -873,10 +872,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  	bad.len = sectors;
>  	len = 0;
>  
> -	if (badblocks_full(bb)) {
> -		rv = 1;
> +	if (badblocks_full(bb))
>  		goto out;
> -	}
>  
>  	if (badblocks_empty(bb)) {
>  		len = insert_at(bb, 0, &bad);
> @@ -916,10 +913,8 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  			int extra = 0;
>  
>  			if (!can_front_overwrite(bb, prev, &bad, &extra)) {
> -				if (extra > 0) {
> -					rv = 1;
> +				if (extra > 0)
>  					goto out;
> -				}
>  
>  				len = min_t(sector_t,
>  					    BB_END(p[prev]) - s, sectors);
> @@ -986,10 +981,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  
>  	write_sequnlock_irqrestore(&bb->lock, flags);
>  
> -	if (!added)
> -		rv = 1;
> -
> -	return rv;
> +	return sectors;
>  }
>  
>  /*
> @@ -1353,7 +1345,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>   *
>   * Return:
>   *  0: success
> - *  1: failed to set badblocks (out of space)
> + *  other: failed to set badblocks (out of space)
>   */
>  int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  			int acknowledged)
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 10/12] badblocks: return boolen from badblocks_set() and badblocks_clear()

[Coly Li]

On Fri, Feb 21, 2025 at 04:11:07PM +0800, Zheng Qixing wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> Change the return type of badblocks_set() and badblocks_clear()
> from int to bool, indicating success or failure. Specifically:
> 
> - _badblocks_set() and _badblocks_clear() functions now return
> true for success and false for failure.
> - All calls to these functions have been updated to handle the
> new boolean return type.
> - This change improves code clarity and ensures a more consistent
> handling of success and failure states.
> 
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>

For block/badblocks.c and include/linux/badblocks.h it is fine to me,

Acked-by: Coly Li <colyli@kernel.org>

Thanks.

> ---
>  block/badblocks.c             | 37 +++++++++++++++++------------------
>  drivers/block/null_blk/main.c | 17 ++++++++--------
>  drivers/md/md.c               | 35 +++++++++++++++++----------------
>  drivers/nvdimm/badrange.c     |  2 +-
>  include/linux/badblocks.h     |  6 +++---
>  5 files changed, 49 insertions(+), 48 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 79d91be468c4..8f057563488a 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -836,8 +836,8 @@ static bool try_adjacent_combine(struct badblocks *bb, int prev)
>  }
>  
>  /* Do exact work to set bad block range into the bad block table */
> -static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			  int acknowledged)
> +static bool _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +			   int acknowledged)
>  {
>  	int len = 0, added = 0;
>  	struct badblocks_context bad;
> @@ -847,11 +847,11 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  
>  	if (bb->shift < 0)
>  		/* badblocks are disabled */
> -		return 1;
> +		return false;
>  
>  	if (sectors == 0)
>  		/* Invalid sectors number */
> -		return 1;
> +		return false;
>  
>  	if (bb->shift) {
>  		/* round the start down, and the end up */
> @@ -977,7 +977,7 @@ static int _badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  
>  	write_sequnlock_irqrestore(&bb->lock, flags);
>  
> -	return sectors;
> +	return sectors == 0;
>  }
>  
>  /*
> @@ -1048,21 +1048,20 @@ static int front_splitting_clear(struct badblocks *bb, int prev,
>  }
>  
>  /* Do the exact work to clear bad block range from the bad block table */
> -static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +static bool _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  {
>  	struct badblocks_context bad;
>  	int prev = -1, hint = -1;
>  	int len = 0, cleared = 0;
> -	int rv = 0;
>  	u64 *p;
>  
>  	if (bb->shift < 0)
>  		/* badblocks are disabled */
> -		return 1;
> +		return false;
>  
>  	if (sectors == 0)
>  		/* Invalid sectors number */
> -		return 1;
> +		return false;
>  
>  	if (bb->shift) {
>  		sector_t target;
> @@ -1182,9 +1181,9 @@ static int _badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  	write_sequnlock_irq(&bb->lock);
>  
>  	if (!cleared)
> -		rv = 1;
> +		return false;
>  
> -	return rv;
> +	return true;
>  }
>  
>  /* Do the exact work to check bad blocks range from the bad block table */
> @@ -1338,11 +1337,11 @@ EXPORT_SYMBOL_GPL(badblocks_check);
>   * decide how best to handle it.
>   *
>   * Return:
> - *  0: success
> - *  other: failed to set badblocks (out of space)
> + *  true: success
> + *  false: failed to set badblocks (out of space)
>   */
> -int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			int acknowledged)
> +bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +		   int acknowledged)
>  {
>  	return _badblocks_set(bb, s, sectors, acknowledged);
>  }
> @@ -1359,10 +1358,10 @@ EXPORT_SYMBOL_GPL(badblocks_set);
>   * drop the remove request.
>   *
>   * Return:
> - *  0: success
> - *  1: failed to clear badblocks
> + *  true: success
> + *  false: failed to clear badblocks
>   */
> -int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  {
>  	return _badblocks_clear(bb, s, sectors);
>  }
> @@ -1484,7 +1483,7 @@ ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
>  		return -EINVAL;
>  	}
>  
> -	if (badblocks_set(bb, sector, length, !unack))
> +	if (!badblocks_set(bb, sector, length, !unack))
>  		return -ENOSPC;
>  	else
>  		return len;
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index d94ef37480bd..623db72ad66b 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -559,14 +559,15 @@ static ssize_t nullb_device_badblocks_store(struct config_item *item,
>  		goto out;
>  	/* enable badblocks */
>  	cmpxchg(&t_dev->badblocks.shift, -1, 0);
> -	if (buf[0] == '+')
> -		ret = badblocks_set(&t_dev->badblocks, start,
> -			end - start + 1, 1);
> -	else
> -		ret = badblocks_clear(&t_dev->badblocks, start,
> -			end - start + 1);
> -	if (ret == 0)
> -		ret = count;
> +	if (buf[0] == '+') {
> +		if (badblocks_set(&t_dev->badblocks, start,
> +				  end - start + 1, 1))
> +			ret = count;
> +	} else {
> +		if (badblocks_clear(&t_dev->badblocks, start,
> +				    end - start + 1))
> +			ret = count;
> +	}
>  out:
>  	kfree(orig);
>  	return ret;
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 30b3dbbce2d2..49d826e475cb 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1748,7 +1748,7 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
>  			count <<= sb->bblog_shift;
>  			if (bb + 1 == 0)
>  				break;
> -			if (badblocks_set(&rdev->badblocks, sector, count, 1))
> +			if (!badblocks_set(&rdev->badblocks, sector, count, 1))
>  				return -EINVAL;
>  		}
>  	} else if (sb->bblog_offset != 0)
> @@ -9846,7 +9846,6 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>  		       int is_new)
>  {
>  	struct mddev *mddev = rdev->mddev;
> -	int rv;
>  
>  	/*
>  	 * Recording new badblocks for faulty rdev will force unnecessary
> @@ -9862,33 +9861,35 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>  		s += rdev->new_data_offset;
>  	else
>  		s += rdev->data_offset;
> -	rv = badblocks_set(&rdev->badblocks, s, sectors, 0);
> -	if (rv == 0) {
> -		/* Make sure they get written out promptly */
> -		if (test_bit(ExternalBbl, &rdev->flags))
> -			sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
> -		sysfs_notify_dirent_safe(rdev->sysfs_state);
> -		set_mask_bits(&mddev->sb_flags, 0,
> -			      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
> -		md_wakeup_thread(rdev->mddev->thread);
> -		return 1;
> -	} else
> +
> +	if (!badblocks_set(&rdev->badblocks, s, sectors, 0))
>  		return 0;
> +
> +	/* Make sure they get written out promptly */
> +	if (test_bit(ExternalBbl, &rdev->flags))
> +		sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
> +	sysfs_notify_dirent_safe(rdev->sysfs_state);
> +	set_mask_bits(&mddev->sb_flags, 0,
> +		      BIT(MD_SB_CHANGE_CLEAN) | BIT(MD_SB_CHANGE_PENDING));
> +	md_wakeup_thread(rdev->mddev->thread);
> +	return 1;
>  }
>  EXPORT_SYMBOL_GPL(rdev_set_badblocks);
>  
>  int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
>  			 int is_new)
>  {
> -	int rv;
>  	if (is_new)
>  		s += rdev->new_data_offset;
>  	else
>  		s += rdev->data_offset;
> -	rv = badblocks_clear(&rdev->badblocks, s, sectors);
> -	if ((rv == 0) && test_bit(ExternalBbl, &rdev->flags))
> +
> +	if (!badblocks_clear(&rdev->badblocks, s, sectors))
> +		return 0;
> +
> +	if (test_bit(ExternalBbl, &rdev->flags))
>  		sysfs_notify_dirent_safe(rdev->sysfs_badblocks);
> -	return rv;
> +	return 1;
>  }
>  EXPORT_SYMBOL_GPL(rdev_clear_badblocks);
>  
> diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
> index a002ea6fdd84..ee478ccde7c6 100644
> --- a/drivers/nvdimm/badrange.c
> +++ b/drivers/nvdimm/badrange.c
> @@ -167,7 +167,7 @@ static void set_badblock(struct badblocks *bb, sector_t s, int num)
>  	dev_dbg(bb->dev, "Found a bad range (0x%llx, 0x%llx)\n",
>  			(u64) s * 512, (u64) num * 512);
>  	/* this isn't an error as the hardware will still throw an exception */
> -	if (badblocks_set(bb, s, num, 1))
> +	if (!badblocks_set(bb, s, num, 1))
>  		dev_info_once(bb->dev, "%s: failed for sector %llx\n",
>  				__func__, (u64) s);
>  }
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 670f2dae692f..8764bed9ff16 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -50,9 +50,9 @@ struct badblocks_context {
>  
>  int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
>  		   sector_t *first_bad, int *bad_sectors);
> -int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> -			int acknowledged);
> -int badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
> +bool badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> +		   int acknowledged);
> +bool badblocks_clear(struct badblocks *bb, sector_t s, int sectors);
>  void ack_all_badblocks(struct badblocks *bb);
>  ssize_t badblocks_show(struct badblocks *bb, char *page, int unack);
>  ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> -- 
> 2.39.2
> 

-- 
Coly Li

Re: [PATCH 05/12] badblocks: return error if any badblock set fails

[Li Nan]

在 2025/2/22 14:16, Coly Li 写道:
> On Sat, Feb 22, 2025 at 09:12:53AM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/02/21 18:12, Coly Li 写道:
>>> So we don’t need to add a negative return value for partial success/failure?
>>>
>>> Coly Li.
>>
>> Yes, I think so. No one really use this value, and patch 10 clean this
>> up by changing return type to bool.
> 
> OK, then it is fine to me.
> 
> It will be good to add a code comment that parital setting will be treated as failure.
> 
> Thanks.
> 
> 

Thank you for your review. I will add comment in the next version.

-- 
Thanks,
Nan

Re: [PATCH 04/12] badblocks: return error directly when setting badblocks exceeds 512

[Li Nan]

在 2025/2/21 17:55, Yu Kuai 写道:
> Hi,
> 
> +CC Linan
> 
> 在 2025/02/21 16:11, Zheng Qixing 写道:
>> From: Li Nan <linan122@huawei.com>
>>
>> In the current handling of badblocks settings, a lot of processing has
>> been done for scenarios where the number of badblocks exceeds 512.
>> This makes the code look quite complex and also introduces some issues,
> 
> It's better to add explanations about these issues here.
>>

Thank you for your review. I will add more details in the next version.

-- 
Thanks,
Nan

Re: [PATCH 06/12] badblocks: fix the using of MAX_BADBLOCKS

[Li Nan]

在 2025/2/21 18:09, Zhu Yanjun 写道:
> 
> On 21.02.25 09:11, Zheng Qixing wrote:
>> From: Li Nan <linan122@huawei.com>
>>
>> The number of badblocks cannot exceed MAX_BADBLOCKS, but it should be
>> allowed to equal MAX_BADBLOCKS.
>>
>> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling 
>> code")
>> Signed-off-by: Li Nan <linan122@huawei.com>
>> ---
>>   block/badblocks.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/badblocks.c b/block/badblocks.c
>> index a953d2e9417f..87267bae6836 100644
>> --- a/block/badblocks.c
>> +++ b/block/badblocks.c
>> @@ -700,7 +700,7 @@ static bool can_front_overwrite(struct badblocks *bb, 
>> int prev,
>>               *extra = 2;
>>       }
>> -    if ((bb->count + (*extra)) >= MAX_BADBLOCKS)
>> +    if ((bb->count + (*extra)) > MAX_BADBLOCKS)
>>           return false;
> 
> 
> In this commit,
> 
> commit c3c6a86e9efc5da5964260c322fe07feca6df782
> Author: Coly Li <colyli@suse.de>
> Date:   Sat Aug 12 01:05:08 2023 +0800
> 
>      badblocks: add helper routines for badblock ranges handling
> 
>      This patch adds several helper routines to improve badblock ranges
>      handling. These helper routines will be used later in the improved
>      version of badblocks_set()/badblocks_clear()/badblocks_check().
> 
>      - Helpers prev_by_hint() and prev_badblocks() are used to find the bad
>        range from bad table which the searching range starts at or after.
> 
> The above is changed to MAX_BADBLOCKS. Thus, perhaps, the Fixes tag should 
> include the above commit?
> 
> Except that, I am fine with this commit.
> 
> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
> 
> Zhu Yanjun
> 

Thank! I will bring this fix tag in v2.

-- 
Thanks,
Nan