On 12/18/20 11:25 AM, Dan Williams wrote:

[ add Neil, original gooodguy who wrote badblocks ]


On Thu, Dec 3, 2020 at 9:16 AM Coly Li [off-list ref] wrote:

quoted

Recently I received a bug report that current badblocks code does not
properly handle multiple ranges. For example,
        badblocks_set(bb, 32, 1, true);
        badblocks_set(bb, 34, 1, true);
        badblocks_set(bb, 36, 1, true);
        badblocks_set(bb, 32, 12, true);
Then indeed badblocks_show() reports,
        32 3
        36 1
But the expected bad blocks table should be,
        32 12
Obviously only the first 2 ranges are merged and badblocks_set() returns
and ignores the rest setting range.

This behavior is improper, if the caller of badblocks_set() wants to set
a range of blocks into bad blocks table, all of the blocks in the range
should be handled even the previous part encountering failure.

The desired way to set bad blocks range by badblocks_set() is,
- Set as many as blocks in the setting range into bad blocks table.
- Merge the bad blocks ranges and occupy as less as slots in the bad
  blocks table.
- Fast.

Indeed the above proposal is complicated, especially with the following
restrictions,
- The setting bad blocks range can be ackknowledged or not acknowledged.

Hi Dan,

s/ackknowledged/acknowledged/

I'd run checkpatch --codespell for future versions...

Thanks for the hint. I will do it next time.

quoted

- The bad blocks table size is limited.
- Memory allocation should be avoided.

This patch is an initial effort to improve badblocks_set() for setting
bad blocks range when it covers multiple already set bad ranges in the
bad blocks table, and to do it as fast as possible.

The basic idea of the patch is to categorize all possible bad blocks
range setting combinationsinto to much less simplified and more less
special conditions. Inside badblocks_set() there is an implicit loop
composed by jumping between labels 're_insert' and 'update_sectors'. No
matter how large the setting bad blocks range is, in every loop just a
minimized range from the head is handled by a pre-defined behavior from
one of the categorized conditions. The logic is simple and code flow is
manageable.

This patch is unfinished yet, it only improves badblocks_set() and not
touch badblocks_clear() and badblocks_show() yet. I post it earlier
because this patch will be large (more then 1000 lines of change), I
want more people to give me comments earlier before I go too far away.

I wonder if this isn't indication that the base data structure should
be replaced... but I have not had a chance to devote deeper thought to
this.

No existing data structure changed. Even the in-memory badblocks table I
don't change it at all. I just fix the report issue by handle more
corner cases, on-disk and in-memory stuffs are untouched and consistent.


Coly Li

quoted

The code logic is tested as user space programmer, this patch passes
compiling but not tested in kernel mode yet. Right now it is only for
RFC purpose. I will post tested patch in further versions.

Thank you in advance for any review or comments on this patch.

[snipped]