Received-SPF: pass (google.com: domain of linux-kernel+bounces-10611-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.300.61.1.2\))
Subject: Re: Bug in commit aa511ff8218b ("badblocks: switch to the improved
 badblock handling
From: Coly Li <colyli@suse.de>
In-Reply-To: <6587152b64d9f_c579e29437@iweiny-mobl.notmuch>
Date: Sun, 24 Dec 2023 08:18:18 +0800
Cc: Li Nan <linan666@huaweicloud.com>,
 Dan Williams <dan.j.williams@intel.com>,
 Jens Axboe <axboe@kernel.dk>,
 Xiao Ni <xni@redhat.com>,
 Geliang Tang <geliang.tang@suse.com>,
 Hannes Reinecke <hare@suse.de>,
 NeilBrown <neilb@suse.de>,
 Vishal L Verma <vishal.l.verma@intel.com>,
 Linux Block Devices <linux-block@vger.kernel.org>,
 nvdimm@lists.linux.dev,
 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <5422004A-84A8-4E9E-B1E2-98503BCC80BC@suse.de>
References: <6585d5fda5183_9f731294b9@iweiny-mobl.notmuch>
 <6585dc32bebce_ab80829462@iweiny-mobl.notmuch>
 <658628b098aeb_b31b42945b@iweiny-mobl.notmuch>
 <nhza4xsnbmcmka7463jxgmdvb27pqvbvcuzs7xp4vzpqlo262d@dp7laevqtaka>
 <6587152b64d9f_c579e29437@iweiny-mobl.notmuch>
To: Ira Weiny <ira.weiny@intel.com>


> 2023=E5=B9=B412=E6=9C=8824=E6=97=A5 01:13=EF=BC=8CIra Weiny =
<ira.weiny@intel.com> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> Coly Li wrote:
>=20
> [snip]
>=20
>>=20
>> Hi Ira,
>>=20
>> The above information is accurate and very helpful, thank you!
>>=20
>> =46rom __badblocks_check(), the problematic code block is,
>> 1303 re_check:
>> 1304         bad.start =3D s;
>> 1305         bad.len =3D sectors;
>> 1306
>> 1307         if (badblocks_empty(bb)) {
>> 1308                 len =3D sectors;
>> 1309                 goto update_sectors;
>> 1310         }
>> 1311
>> 1312         prev =3D prev_badblocks(bb, &bad, hint);
>> 1313
>> 1314         /* start after all badblocks */
>> 1315         if ((prev + 1) >=3D bb->count && !overlap_front(bb, =
prev, &bad)) {
>> 1316                 len =3D 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]) >=3D (s + sectors))
>> 1327                         len =3D sectors;
>> 1328                 else
>> 1329                         len =3D BB_END(p[prev]) - s;
>> 1330
>> 1331                 if (set =3D=3D 0) {
>> 1332                         *first_bad =3D BB_OFFSET(p[prev]);
>> 1333                         *bad_sectors =3D BB_LEN(p[prev]);
>> 1334                         set =3D 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 =3D BB_OFFSET(p[prev + 1]) - bad.start;
>> 1342                 hint =3D prev + 1;
>> 1343                 goto update_sectors;
>> 1344         }
>> 1345
>> 1346         /* not cover any badblocks range in the table */
>> 1347         len =3D sectors;
>> 1348
>> 1349 update_sectors:
>>=20
>> 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.
>>=20
>> So the value prev should be checked and make sure '>=3D 0' before =
comparing
>> the checking range with a badblock record returned by =
prev_badblocks(). Other
>> wise it dones't make sense.
>>=20
>> For badblocks_set() and badblocks_clear(), 'prev < 0' is explicitly =
checked,
>> value '-1' doesn't go though into following code.
>>=20
>> Could you please apply and try the attached patch? Hope it may help a =
bit.
>>=20
>> 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.
>>=20
>> Thanks for the report and debug!
>>=20
>> Coly Li
>>=20
>> [debug patch snipped]
>=20
> This debug patch does fix our tests.  Thanks!
>=20
> But Nan has submitted a series to fix this as well.[1]
>=20
> 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=