| Message ID | 20250221081109.734170-10-zhengqixing@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | badblocks: bugfix and cleanup for badblocks | expand |
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 >
在 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); > >
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);