[v7,0/6] badblocks improvement for multiple bad block ranges

Message

Coly Li Aug. 11, 2023, 5:05 p.m. UTC

This is the v7 version of the badblocks improvement series, which makes
badblocks APIs to handle multiple ranges in bad block table.

The change comparing to previous v6 version is the modifications
enlightened by the code review comments from Xiao Ni,
- Typo fixes in code comments and commit logs.
- Tiny but useful optimzation in prev_badblocks(), front_overwrite(),
  _badblocks_clear().

There is NO in-memory or on-disk format change in the whole series, all
existing API and data structures are consistent. This series just only
improve the code algorithm to handle more corner cases, the interfaces
are same and consistency to all existing callers (md raid and nvdimm
drivers).

The original motivation of the change is from the requirement from our
customer, that current badblocks routines don't handle multiple ranges.
For example if the bad block setting range covers multiple ranges from
bad block table, only the first two bad block ranges merged and rested
ranges are intact. The expected behavior should be all the covered
ranges to be handled.

All the patches are tested by modified user space code and the code
logic works as expected. The modified user space testing code is
provided in the last patch, which is not listed in the cover letter. The
testing code is an example how the improved code is tested.

The whole change is divided into 6 patches to make the code review more
clear and easier. If people prefer, I'd like to post a single large
patch finally after the code review accomplished.

Please review the code and response. Thank you all in advance.

Coly Li

Cc: Dan Williams <[e-mail]>
Cc: Geliang Tang <[e-mail]>
Cc: Hannes Reinecke <[e-mail]>
Cc: Jens Axboe <[e-mail]>
Cc: NeilBrown <[e-mail]>
Cc: Richard Fan <[e-mail]>
Cc: Vishal L Verma <[e-mail]>
Cc: Wols Lists <[e-mail]>
Cc: Xiao Ni <[e-mail]>
---

Coly Li (6):
  badblocks: add more helper structure and routines in badblocks.h
  badblocks: add helper routines for badblock ranges handling
  badblocks: improve badblocks_set() for multiple ranges handling
  badblocks: improve badblocks_clear() for multiple ranges handling
  badblocks: improve badblocks_check() for multiple ranges handling
  badblocks: switch to the improved badblock handling code

 block/badblocks.c         | 1618 ++++++++++++++++++++++++++++++-------
 include/linux/badblocks.h |   30 +
 2 files changed, 1354 insertions(+), 294 deletions(-)

Comments

Geliang Tang Sept. 26, 2023, 1:47 a.m. UTC | #1

On Sat, Aug 12, 2023 at 01:05:06AM +0800, Coly Li wrote:
> This is the v7 version of the badblocks improvement series, which makes
> badblocks APIs to handle multiple ranges in bad block table.
> 
> The change comparing to previous v6 version is the modifications
> enlightened by the code review comments from Xiao Ni,
> - Typo fixes in code comments and commit logs.
> - Tiny but useful optimzation in prev_badblocks(), front_overwrite(),
>   _badblocks_clear().
> 
> There is NO in-memory or on-disk format change in the whole series, all
> existing API and data structures are consistent. This series just only
> improve the code algorithm to handle more corner cases, the interfaces
> are same and consistency to all existing callers (md raid and nvdimm
> drivers).
> 
> The original motivation of the change is from the requirement from our
> customer, that current badblocks routines don't handle multiple ranges.
> For example if the bad block setting range covers multiple ranges from
> bad block table, only the first two bad block ranges merged and rested
> ranges are intact. The expected behavior should be all the covered
> ranges to be handled.
> 
> All the patches are tested by modified user space code and the code
> logic works as expected. The modified user space testing code is
> provided in the last patch, which is not listed in the cover letter. The
> testing code is an example how the improved code is tested.
> 
> The whole change is divided into 6 patches to make the code review more
> clear and easier. If people prefer, I'd like to post a single large
> patch finally after the code review accomplished.
> 
> Please review the code and response. Thank you all in advance.
> 
> Coly Li
> 
> Cc: Dan Williams <[e-mail]>
> Cc: Geliang Tang <[e-mail]>
> Cc: Hannes Reinecke <[e-mail]>
> Cc: Jens Axboe <[e-mail]>
> Cc: NeilBrown <[e-mail]>
> Cc: Richard Fan <[e-mail]>
> Cc: Vishal L Verma <[e-mail]>
> Cc: Wols Lists <[e-mail]>
> Cc: Xiao Ni <[e-mail]>

This series LGTM, thanks Coly.

Acked-by: Geliang Tang <[e-mail]>

> ---
> 
> Coly Li (6):
>   badblocks: add more helper structure and routines in badblocks.h
>   badblocks: add helper routines for badblock ranges handling
>   badblocks: improve badblocks_set() for multiple ranges handling
>   badblocks: improve badblocks_clear() for multiple ranges handling
>   badblocks: improve badblocks_check() for multiple ranges handling
>   badblocks: switch to the improved badblock handling code
> 
>  block/badblocks.c         | 1618 ++++++++++++++++++++++++++++++-------
>  include/linux/badblocks.h |   30 +
>  2 files changed, 1354 insertions(+), 294 deletions(-)
> 
> -- 
> 2.35.3
>

Jens Axboe Sept. 26, 2023, 6:44 a.m. UTC | #2

On Sat, 12 Aug 2023 01:05:06 +0800, Coly Li wrote:
> This is the v7 version of the badblocks improvement series, which makes
> badblocks APIs to handle multiple ranges in bad block table.
> 
> The change comparing to previous v6 version is the modifications
> enlightened by the code review comments from Xiao Ni,
> - Typo fixes in code comments and commit logs.
> - Tiny but useful optimzation in prev_badblocks(), front_overwrite(),
>   _badblocks_clear().
> 
> [...]

Applied, thanks!

[1/6] badblocks: add more helper structure and routines in badblocks.h
      commit: e850d9a52f4cd31521c80a7ea9718b69129af4d5
[2/6] badblocks: add helper routines for badblock ranges handling
      commit: c3c6a86e9efc5da5964260c322fe07feca6df782
[3/6] badblocks: improve badblocks_set() for multiple ranges handling
      commit: 1726c774678331b4af5e78db87e10ff5da448456
[4/6] badblocks: improve badblocks_clear() for multiple ranges handling
      commit: db448eb6862979aad2468ecf957a20ef98b82f29
[5/6] badblocks: improve badblocks_check() for multiple ranges handling
      commit: 3ea3354cb9f03e34ee3fab98f127ab8da4131eee
[6/6] badblocks: switch to the improved badblock handling code
      commit: aa511ff8218b3fb328181fbaac48aa5e9c5c6d93

Best regards,

Ira Weiny Dec. 22, 2023, 7:01 p.m. UTC | #3

Coly Li wrote:
> This is the v7 version of the badblocks improvement series, which makes
> badblocks APIs to handle multiple ranges in bad block table.

Just in case I missed anyone on this original thread I've found issues
with this series which I emailed Coly about here:

https://lore.kernel.org/all/[e-mail]/

Ira

> 
> The change comparing to previous v6 version is the modifications
> enlightened by the code review comments from Xiao Ni,
> - Typo fixes in code comments and commit logs.
> - Tiny but useful optimzation in prev_badblocks(), front_overwrite(),
>   _badblocks_clear().
> 
> There is NO in-memory or on-disk format change in the whole series, all
> existing API and data structures are consistent. This series just only
> improve the code algorithm to handle more corner cases, the interfaces
> are same and consistency to all existing callers (md raid and nvdimm
> drivers).
> 
> The original motivation of the change is from the requirement from our
> customer, that current badblocks routines don't handle multiple ranges.
> For example if the bad block setting range covers multiple ranges from
> bad block table, only the first two bad block ranges merged and rested
> ranges are intact. The expected behavior should be all the covered
> ranges to be handled.
> 
> All the patches are tested by modified user space code and the code
> logic works as expected. The modified user space testing code is
> provided in the last patch, which is not listed in the cover letter. The
> testing code is an example how the improved code is tested.
> 
> The whole change is divided into 6 patches to make the code review more
> clear and easier. If people prefer, I'd like to post a single large
> patch finally after the code review accomplished.
> 
> Please review the code and response. Thank you all in advance.
> 
> Coly Li
> 
> Cc: Dan Williams <[e-mail]>
> Cc: Geliang Tang <[e-mail]>
> Cc: Hannes Reinecke <[e-mail]>
> Cc: Jens Axboe <[e-mail]>
> Cc: NeilBrown <[e-mail]>
> Cc: Richard Fan <[e-mail]>
> Cc: Vishal L Verma <[e-mail]>
> Cc: Wols Lists <[e-mail]>
> Cc: Xiao Ni <[e-mail]>
> ---
> 
> Coly Li (6):
>   badblocks: add more helper structure and routines in badblocks.h
>   badblocks: add helper routines for badblock ranges handling
>   badblocks: improve badblocks_set() for multiple ranges handling
>   badblocks: improve badblocks_clear() for multiple ranges handling
>   badblocks: improve badblocks_check() for multiple ranges handling
>   badblocks: switch to the improved badblock handling code
> 
>  block/badblocks.c         | 1618 ++++++++++++++++++++++++++++++-------
>  include/linux/badblocks.h |   30 +
>  2 files changed, 1354 insertions(+), 294 deletions(-)
> 
> -- 
> 2.35.3
> 
>

Li Nan Dec. 23, 2023, 6:46 a.m. UTC | #4

Hi, Ira

在 2023/12/23 3:01, Ira Weiny 写道:

> 
> Just in case I missed anyone on this original thread I've found issues
> with this series which I emailed Coly about here:
> 
> https://lore.kernel.org/all/[e-mail]/
> 
> Ira
> 


I have also noticed this issue recently and try to fix it in:

https://lore.kernel.org/linux-block/[e-mail]/T/#u