On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
> From: Li Nan <linan122@xxxxxxxxxx>
>
> _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@xxxxxxxxxx>
> ---
> 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
