Re: Bug in commit aa511ff8218b ("badblocks: switch to the improved badblock handling

[Coly Li]

> 2023年12月24日 01:13，Ira Weiny <ira.weiny@xxxxxxxxx> 写道：
> 
> Coly Li wrote:
> 
> [snip]
> 
>> 
>> Hi Ira,
>> 
>> The above information is accurate and very helpful, thank you!
>> 
>> From __badblocks_check(), the problematic code block is,
>> 1303 re_check:
>> 1304         bad.start = s;
>> 1305         bad.len = sectors;
>> 1306
>> 1307         if (badblocks_empty(bb)) {
>> 1308                 len = sectors;
>> 1309                 goto update_sectors;
>> 1310         }
>> 1311
>> 1312         prev = prev_badblocks(bb, &bad, hint);
>> 1313
>> 1314         /* start after all badblocks */
>> 1315         if ((prev + 1) >= bb->count && !overlap_front(bb, prev, &bad)) {
>> 1316                 len = sectors;
>> 1317                 goto update_sectors;
>> 1318         }
>> 1319
>> 1320         if (overlap_front(bb, prev, &bad)) {
>> 1321                 if (BB_ACK(p[prev]))
>> 1322                         acked_badblocks++;
>> 1323                 else
>> 1324                         unacked_badblocks++;
>> 1325
>> 1326                 if (BB_END(p[prev]) >= (s + sectors))
>> 1327                         len = sectors;
>> 1328                 else
>> 1329                         len = BB_END(p[prev]) - s;
>> 1330
>> 1331                 if (set == 0) {
>> 1332                         *first_bad = BB_OFFSET(p[prev]);
>> 1333                         *bad_sectors = BB_LEN(p[prev]);
>> 1334                         set = 1;
>> 1335                 }
>> 1336                 goto update_sectors;
>> 1337         }
>> 1338
>> 1339         /* Not front overlap, but behind overlap */
>> 1340         if ((prev + 1) < bb->count && overlap_behind(bb, &bad, prev + 1)) {
>> 1341                 len = BB_OFFSET(p[prev + 1]) - bad.start;
>> 1342                 hint = prev + 1;
>> 1343                 goto update_sectors;
>> 1344         }
>> 1345
>> 1346         /* not cover any badblocks range in the table */
>> 1347         len = sectors;
>> 1348
>> 1349 update_sectors:
>> 
>> If the checking range is before all badblocks records in the badblocks table,
>> value -1 is returned from prev_badblock(). Code blocks between line 1314 and
>> line 1337 doesn't hanle the implicit '-1' value properly. Then counter
>> unacked_badblocks is increased at line 1324 mistakenly.
>> 
>> So the value prev should be checked and make sure '>= 0' before comparing
>> the checking range with a badblock record returned by prev_badblocks(). Other
>> wise it dones't make sense.
>> 
>> For badblocks_set() and badblocks_clear(), 'prev < 0' is explicitly checked,
>> value '-1' doesn't go though into following code.
>> 
>> Could you please apply and try the attached patch? Hope it may help a bit.
>> 
>> And now it is weekend time, you may be out of office and not able to access
>> the testing hardware. I will do more testing from myside and update more info
>> if necessary.
>> 
>> Thanks for the report and debug!
>> 
>> Coly Li
>> 
>> [debug patch snipped]
> 
> This debug patch does fix our tests.  Thanks!
> 
> But Nan has submitted a series to fix this as well.[1]
> 
> I'm going to test his series as well.

Hi Ira,

Thanks for the very quick response, and the positive result. Now I compose a official patch and submit to Jens.

Coly Li