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@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.

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