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 <colyli@xxxxxxx> wrote: >> >> 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. > >> - 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 > >> 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]
