* [PATCH V2 01/12] badblocks: Fix error shitf ops
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
@ 2025-02-27 7:54 ` Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 02/12] badblocks: factor out a helper try_adjacent_combine Zheng Qixing
` (12 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:54 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 02/12] badblocks: factor out a helper try_adjacent_combine
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 01/12] badblocks: Fix error shitf ops Zheng Qixing
@ 2025-02-27 7:54 ` Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks Zheng Qixing
` (11 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:54 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 01/12] badblocks: Fix error shitf ops Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 02/12] badblocks: factor out a helper try_adjacent_combine Zheng Qixing
@ 2025-02-27 7:54 ` Zheng Qixing
2025-02-27 7:54 ` [PATCH V2 04/12] badblocks: return error directly when setting badblocks exceeds 512 Zheng Qixing
` (10 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:54 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 04/12] badblocks: return error directly when setting badblocks exceeds 512
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (2 preceding siblings ...)
2025-02-27 7:54 ` [PATCH V2 03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks Zheng Qixing
@ 2025-02-27 7:54 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 05/12] badblocks: return error if any badblock set fails Zheng Qixing
` (9 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:54 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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,
For example, if there is 512 badblocks already:
for((i=0; i<510; i++)); do ((sector=i*2)); echo "$sector 1" > bad_blocks; done
echo 2100 10 > bad_blocks
echo 2200 10 > bad_blocks
Set new one, exceed 512:
echo 2000 500 > bad_blocks
Expected:
2000 500
Actual:
2100 400
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>
Reviewed-by: Yu Kuai <yukuai3@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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 05/12] badblocks: return error if any badblock set fails
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (3 preceding siblings ...)
2025-02-27 7:54 ` [PATCH V2 04/12] badblocks: return error directly when setting badblocks exceeds 512 Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 06/12] badblocks: fix the using of MAX_BADBLOCKS Zheng Qixing
` (8 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
block/badblocks.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 1c8b8f65f6df..88f27d4f3856 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,8 @@ EXPORT_SYMBOL_GPL(badblocks_check);
*
* Return:
* 0: success
- * 1: failed to set badblocks (out of space)
+ * other: failed to set badblocks (out of space). Parital setting will be
+ * treated as failure.
*/
int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
int acknowledged)
--
2.39.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 06/12] badblocks: fix the using of MAX_BADBLOCKS
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (4 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 05/12] badblocks: return error if any badblock set fails Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 07/12] badblocks: try can_merge_front before overlap_front Zheng Qixing
` (7 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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")
Fixes: c3c6a86e9efc ("badblocks: add helper routines for badblock ranges handling")
Signed-off-by: Li Nan <linan122@huawei.com>
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
block/badblocks.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 88f27d4f3856..43430bd3efa7 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 07/12] badblocks: try can_merge_front before overlap_front
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (5 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 06/12] badblocks: fix the using of MAX_BADBLOCKS Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 08/12] badblocks: fix merge issue when new badblocks align with pre+1 Zheng Qixing
` (6 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
---
block/badblocks.c | 48 ++++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 43430bd3efa7..57e9edf9b848 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 08/12] badblocks: fix merge issue when new badblocks align with pre+1
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (6 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 07/12] badblocks: try can_merge_front before overlap_front Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check() Zheng Qixing
` (5 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
---
block/badblocks.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 57e9edf9b848..92bd43f7fff1 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check()
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (7 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 08/12] badblocks: fix merge issue when new badblocks align with pre+1 Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 7:55 ` [PATCH V2 10/12] badblocks: return boolean from badblocks_set() and badblocks_clear() Zheng Qixing
` (4 subsequent siblings)
13 siblings, 0 replies; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
block/badblocks.c | 50 +++++++++++++++++++++++------------------------
1 file changed, 24 insertions(+), 26 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index 92bd43f7fff1..b66d5f12a766 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH V2 10/12] badblocks: return boolean from badblocks_set() and badblocks_clear()
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (8 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 09/12] badblocks: fix missing bad blocks on retry in _badblocks_check() Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 15:08 ` Ira Weiny
2025-02-27 7:55 ` [PATCH V2 11/12] md: improve return types of badblocks handling functions Zheng Qixing
` (3 subsequent siblings)
13 siblings, 1 reply; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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 are 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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
block/badblocks.c | 41 +++++++++++++++++------------------
drivers/block/null_blk/main.c | 14 ++++++------
drivers/md/md.c | 35 +++++++++++++++---------------
drivers/nvdimm/badrange.c | 2 +-
include/linux/badblocks.h | 6 ++---
5 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/block/badblocks.c b/block/badblocks.c
index b66d5f12a766..e326a16fd056 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,12 +1337,12 @@ EXPORT_SYMBOL_GPL(badblocks_check);
* decide how best to handle it.
*
* Return:
- * 0: success
- * other: failed to set badblocks (out of space). Parital setting will be
+ * true: success
+ * false: failed to set badblocks (out of space). Parital setting will be
* treated as failure.
*/
-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);
}
@@ -1360,10 +1359,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);
}
@@ -1485,10 +1484,10 @@ 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;
+
+ return len;
}
EXPORT_SYMBOL_GPL(badblocks_store);
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index d94ef37480bd..1f39617d780b 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -559,14 +559,14 @@ 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)
+ 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH V2 10/12] badblocks: return boolean from badblocks_set() and badblocks_clear()
2025-02-27 7:55 ` [PATCH V2 10/12] badblocks: return boolean from badblocks_set() and badblocks_clear() Zheng Qixing
@ 2025-02-27 15:08 ` Ira Weiny
0 siblings, 0 replies; 19+ messages in thread
From: Ira Weiny @ 2025-02-27 15:08 UTC (permalink / raw)
To: Zheng Qixing, axboe, song, yukuai3, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, dlemoal, kch, yanjun.zhu,
hare, zhengqixing, colyli, geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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 are 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>
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> Acked-by: Coly Li <colyli@kernel.org>
I'm not really sure this patch adds much. But for the nvdimm part.
Acked-by: Ira Weiny <ira.weiny@intel.com>
[snip]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 11/12] md: improve return types of badblocks handling functions
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (9 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 10/12] badblocks: return boolean from badblocks_set() and badblocks_clear() Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 11:48 ` Yu Kuai
2025-02-27 7:55 ` [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length Zheng Qixing
` (2 subsequent siblings)
13 siblings, 1 reply; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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 | 19 +++++++++----------
drivers/md/md.h | 8 ++++----
drivers/md/raid1.c | 6 +++---
drivers/md/raid10.c | 6 +++---
4 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 49d826e475cb..9b9b2b4131d0 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,12 +9872,12 @@ 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;
@@ -9885,11 +9885,10 @@ int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
s += rdev->data_offset;
if (!badblocks_clear(&rdev->badblocks, s, sectors))
- return 0;
+ return;
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 10ea3af40991..8e9f303c5603 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 15b9ae5bf84d..45faa34f0be8 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH V2 11/12] md: improve return types of badblocks handling functions
2025-02-27 7:55 ` [PATCH V2 11/12] md: improve return types of badblocks handling functions Zheng Qixing
@ 2025-02-27 11:48 ` Yu Kuai
0 siblings, 0 replies; 19+ messages in thread
From: Yu Kuai @ 2025-02-27 11:48 UTC (permalink / raw)
To: Zheng Qixing, axboe, song, dan.j.williams, vishal.l.verma,
dave.jiang, ira.weiny, dlemoal, kch, yanjun.zhu, hare,
zhengqixing, colyli, geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun, yukuai (C)
在 2025/02/27 15:55, 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 | 19 +++++++++----------
> drivers/md/md.h | 8 ++++----
> drivers/md/raid1.c | 6 +++---
> drivers/md/raid10.c | 6 +++---
> 4 files changed, 19 insertions(+), 20 deletions(-)
>
LGTM
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 49d826e475cb..9b9b2b4131d0 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,12 +9872,12 @@ 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;
> @@ -9885,11 +9885,10 @@ int rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
> s += rdev->data_offset;
>
> if (!badblocks_clear(&rdev->badblocks, s, sectors))
> - return 0;
> + return;
>
> 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 10ea3af40991..8e9f303c5603 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 15b9ae5bf84d..45faa34f0be8 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);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (10 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 11/12] md: improve return types of badblocks handling functions Zheng Qixing
@ 2025-02-27 7:55 ` Zheng Qixing
2025-02-27 15:27 ` Ira Weiny
2025-03-05 1:41 ` [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Yu Kuai
2025-03-06 15:05 ` Jens Axboe
13 siblings, 1 reply; 19+ messages in thread
From: Zheng Qixing @ 2025-02-27 7:55 UTC (permalink / raw)
To: axboe, song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Coly Li <colyli@kernel.org>
---
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 e326a16fd056..673ef068423a 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;
@@ -1341,7 +1337,7 @@ EXPORT_SYMBOL_GPL(badblocks_check);
* false: failed to set badblocks (out of space). Parital setting will be
* treated as failure.
*/
-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);
@@ -1362,7 +1358,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 1f39617d780b..318c3ab448fb 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1301,7 +1301,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 8e9f303c5603..652c101e522b 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 45faa34f0be8..f5e272a54844 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length
2025-02-27 7:55 ` [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length Zheng Qixing
@ 2025-02-27 15:27 ` Ira Weiny
2025-03-05 1:29 ` Yu Kuai
0 siblings, 1 reply; 19+ messages in thread
From: Ira Weiny @ 2025-02-27 15:27 UTC (permalink / raw)
To: Zheng Qixing, axboe, song, yukuai3, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, dlemoal, kch, yanjun.zhu,
hare, zhengqixing, colyli, geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
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.
__add_badblock_range() in drivers/nvdimm/badrange.c limits the number of
badblocks which can be set in each call to badblocks_set().
After this change can that algorithm be eliminated? I'm not familiar with
the badblocks code to know for certain.
Regardless I think the types used in badrange.c could be updated with this
change.
Also pmem_clear_bb() should have it's type changed to match
badblocks_clear()
Ira
[snip]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length
2025-02-27 15:27 ` Ira Weiny
@ 2025-03-05 1:29 ` Yu Kuai
0 siblings, 0 replies; 19+ messages in thread
From: Yu Kuai @ 2025-03-05 1:29 UTC (permalink / raw)
To: Ira Weiny, Zheng Qixing, axboe, song, dan.j.williams,
vishal.l.verma, dave.jiang, dlemoal, kch, yanjun.zhu, hare,
zhengqixing, colyli, geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun, yukuai (C)
Hi,
在 2025/02/27 23:27, Ira Weiny 写道:
> __add_badblock_range() in drivers/nvdimm/badrange.c limits the number of
> badblocks which can be set in each call to badblocks_set().
>
> After this change can that algorithm be eliminated? I'm not familiar with
> the badblocks code to know for certain.
This is another story, badblocks records are at most 512, and each
record is at most 512 sectors, so pass in INT_MAX will fail 100%.
Thanks,
Kuai
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (11 preceding siblings ...)
2025-02-27 7:55 ` [PATCH V2 12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length Zheng Qixing
@ 2025-03-05 1:41 ` Yu Kuai
2025-03-06 15:05 ` Jens Axboe
13 siblings, 0 replies; 19+ messages in thread
From: Yu Kuai @ 2025-03-05 1:41 UTC (permalink / raw)
To: Zheng Qixing, axboe, song, dan.j.williams, vishal.l.verma,
dave.jiang, ira.weiny, dlemoal, kch, yanjun.zhu, hare,
zhengqixing, colyli, geliang, xni
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun, yukuai (C)
Hi, Jens!
在 2025/02/27 15:54, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
>
> Hi,
>
> during RAID feature implementation testing, we found several bugs
> in badblocks.
>
> This series contains bugfixes and cleanups for MD RAID badblocks
> handling code.
>
> V2:
> - patch 4: add a description of the issue
> - patch 5: add comment of parital setting
> - patch 6: add fix tag
> - patch 10: two code style modifications
> - patch 11: keep original functionality of rdev_clear_badblocks(),
> functionality was incorrectly modified in V1.
> - patch 1-10 and patch 12 are reviewed by Yu Kuai
> <yukuai3@huawei.com>
> - patch 1, 3, 5, 6, 8, 9, 10, 12 are acked by Coly Li
> <colyli@kernel.org>
>
> 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 boolean 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
>
This set contains fixes that are found by testing mdraid, please
consider this set for the next merge window, or I can apply it to
md-6.15.
Thanks,
Kuai
> block/badblocks.c | 322 +++++++++++++---------------------
> drivers/block/null_blk/main.c | 16 +-
> drivers/md/md.c | 48 ++---
> 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, 183 insertions(+), 266 deletions(-)
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks
2025-02-27 7:54 [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Zheng Qixing
` (12 preceding siblings ...)
2025-03-05 1:41 ` [PATCH V2 00/12] badblocks: bugfix and cleanup for badblocks Yu Kuai
@ 2025-03-06 15:05 ` Jens Axboe
13 siblings, 0 replies; 19+ messages in thread
From: Jens Axboe @ 2025-03-06 15:05 UTC (permalink / raw)
To: song, yukuai3, dan.j.williams, vishal.l.verma, dave.jiang,
ira.weiny, dlemoal, kch, yanjun.zhu, hare, zhengqixing, colyli,
geliang, xni, Zheng Qixing
Cc: linux-block, linux-kernel, linux-raid, nvdimm, yi.zhang,
yangerkun
On Thu, 27 Feb 2025 15:54:55 +0800, Zheng Qixing wrote:
> during RAID feature implementation testing, we found several bugs
> in badblocks.
>
> This series contains bugfixes and cleanups for MD RAID badblocks
> handling code.
>
> V2:
> - patch 4: add a description of the issue
> - patch 5: add comment of parital setting
> - patch 6: add fix tag
> - patch 10: two code style modifications
> - patch 11: keep original functionality of rdev_clear_badblocks(),
> functionality was incorrectly modified in V1.
> - patch 1-10 and patch 12 are reviewed by Yu Kuai
> <yukuai3@huawei.com>
> - patch 1, 3, 5, 6, 8, 9, 10, 12 are acked by Coly Li
> <colyli@kernel.org>
>
> [...]
Applied, thanks!
[01/12] badblocks: Fix error shitf ops
commit: 7d83c5d73c1a3c7b71ba70d0ad2ae66e7a0e7ace
[02/12] badblocks: factor out a helper try_adjacent_combine
commit: 270b68fee9688428e0a98d4a2c3e6d4c434a84ba
[03/12] badblocks: attempt to merge adjacent badblocks during ack_all_badblocks
commit: 32e9ad4d11f69949ff331e35a417871ee0d31d99
[04/12] badblocks: return error directly when setting badblocks exceeds 512
commit: 28243dcd1f49cc8be398a1396d16a45527882ce5
[05/12] badblocks: return error if any badblock set fails
commit: 7f500f0a59b1d7345a05ec4ae703babf34b7e470
[06/12] badblocks: fix the using of MAX_BADBLOCKS
commit: 37446680dfbfbba7cbedd680047182f70a0b857b
[07/12] badblocks: try can_merge_front before overlap_front
commit: 3a23d05f9c1abf8238fe48167ab5574062d1606e
[08/12] badblocks: fix merge issue when new badblocks align with pre+1
commit: 9ec65dec634a752ab0a1203510ee190356e4cf1a
[09/12] badblocks: fix missing bad blocks on retry in _badblocks_check()
commit: 5236f041fa6c81c71eabad44897e54a0d6d5bbf6
[10/12] badblocks: return boolean from badblocks_set() and badblocks_clear()
commit: c8775aefba959cdfbaa25408a84d3dd15bbeb991
[11/12] md: improve return types of badblocks handling functions
commit: 7e5102dd99f3ad1f981671ad5b4f24ac48c568ad
[12/12] badblocks: use sector_t instead of int to avoid truncation of badblocks length
commit: d301f164c3fbff611bd71f57dfa553b9219f0f5e
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 19+ messages in thread