Received-SPF: pass (google.com: domain of linux-kernel+bounces-10506-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 23 Dec 2023 09:13:15 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Coly Li <colyli@suse.de>, Ira Weiny <ira.weiny@intel.com>,
	<linan666@huaweicloud.com>
CC: 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@vger.kernel.org>,
	<nvdimm@lists.linux.dev>, <linux-kernel@vger.kernel.org>
Subject: Re: Bug in commit aa511ff8218b ("badblocks: switch to the improved
 badblock handling
Message-ID: <6587152b64d9f_c579e29437@iweiny-mobl.notmuch>
References: <6585d5fda5183_9f731294b9@iweiny-mobl.notmuch>
 <6585dc32bebce_ab80829462@iweiny-mobl.notmuch>
 <658628b098aeb_b31b42945b@iweiny-mobl.notmuch>
 <nhza4xsnbmcmka7463jxgmdvb27pqvbvcuzs7xp4vzpqlo262d@dp7laevqtaka>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <nhza4xsnbmcmka7463jxgmdvb27pqvbvcuzs7xp4vzpqlo262d@dp7laevqtaka>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?u66zOj1n85vwnGMy0dko2blN9YWhG+S3tU5MjOs2+gle76+uotI5fKvuE8RT?=
 =?us-ascii?Q?JeRGTuK9wlYTT6LGvtfWS34lEFqWv7X2WlTrCvGdWgGAQFOIrs5wCb5Wg8Z+?=
 =?us-ascii?Q?Riwdp0oTUodCyKKEI+2hnmHZXaljEJ5VDJI9FCAFyGDvbZ4pztIEn96zxLfP?=
 =?us-ascii?Q?xM4pIOqBLb+kNypdFf/GDIZVfc5y+6SgAIJM1z2OF1iMbwdD5I6p7wv972aP?=
 =?us-ascii?Q?MDD/ZNWvwbR/qMY17arjxaGRjKRwrAw5tUqNwu3OnpzFwThS4ucZ5EdLc5aS?=
 =?us-ascii?Q?16346PQ2NP82s0Pp7z1MsJ0lcvu0zPkvF6zcI4ObR5DgmtwwjIr5mt6IoQkt?=
 =?us-ascii?Q?rwkejXunXfMBuv6mbJSpWLdnkWxj5iB9FkF37zUyYItYgLxJF2djQ0mBTBvn?=
 =?us-ascii?Q?k7UzTGg/i0GRkiuMtzGeRxjFZbxoH1E+yMM+3CL6xNcCmT7IFv2jCpL+Nvk4?=
 =?us-ascii?Q?PXcwYumY3MET1ABICBh9Dl9aRfd4nZV+yB5bRDM8OFjQED9EVmhO7609X/rm?=
 =?us-ascii?Q?oiCRrYuO3GL8glqjQC/eF4aB2sgbkUqGHz8REqqjFhXcHd8/01EJ3sanFFCa?=
 =?us-ascii?Q?biGeQyivoIhYjFw6PSIzZMhJrHFtrxgFHjOMFNrB6vmOoR1RnC+/gW6aQkLC?=
 =?us-ascii?Q?cdhJmQdVlWevQsrtSmMgOhK78KfW5/lD5MQizIt2GIxBsDtTshcVtUjPfvmq?=
 =?us-ascii?Q?Zm9lOiPRiDbccyu53C7/Ea22FsC7xYQtPuvOyRJqlmFyfUT2ZLBabVV4nB4m?=
 =?us-ascii?Q?aPeAgAdmDJD+6e2MqiBfHK6laMoq9ByArcsk579AnVvOfWWUB/k9yVK8Z9SR?=
 =?us-ascii?Q?Nr/jqcArn1Jp+h2kcCzC6MeBORCB1VZYT9RgdneluaC0NM3l1OHyRuxMZpId?=
 =?us-ascii?Q?Kej8i+vdMEdV35htZ6ozYPb2oNeR3quF+QFKip37GeFUMJ3s2T5EFwg2ULUJ?=
 =?us-ascii?Q?D2/cQXIG5RXaeVMKgsoDFnANyGf2f/MXpFd4LU34T7XDx1bwyT65Sk48D2ri?=
 =?us-ascii?Q?npFt4+e4F9i16gKEozXTfBY/99A+b54fmHAMhI4L76KHrfopQottGauW3sIB?=
 =?us-ascii?Q?8keKjjtdS2pZU7aO0dhqFSr+4F0w9PBs8P8cUAj75E8HjbE0o2iGEPRgCjHy?=
 =?us-ascii?Q?yEI529qyDZQpfZdMdkZ/4x4ViXZM2a2qt8CKmF+TNarxlKuMsq3ftcdmDjDB?=
 =?us-ascii?Q?GE/PDQFP+GP/BuaKw69mwcaGoSI7DBdmWmiCV1vZqsU/LcFyranUT8xKz/j5?=
 =?us-ascii?Q?bCji+1gs3s/X1lpxzKJtXWPjz2I0n59ulYBetaFOL9ZC8Q68cabiXpEzcFC4?=
 =?us-ascii?Q?0VCZ9nhttGNfuk21Mu2SV3EjZns/R1m1mgp7z4G2Gwum8US2rTB1fdDDuZED?=
 =?us-ascii?Q?OlEpdtGKeCG5/eOCsmOP1KVdYb6yyN5NGROJSPSLAV+nZrCqU8UBMKBDua7X?=
 =?us-ascii?Q?a0KKSUWNwh1iW9p9YEDH58tav4Zf46l3OCsQZPa7kdU6almLnrrJXKi0KYy/?=
 =?us-ascii?Q?ox08yCe3iVS7rzTsZmlOwea4z/YJYU4bPTNPjhZLRMpqHaheCl/78N2T5zMO?=
 =?us-ascii?Q?ydUgUnR0xaB8FW6HDhyVKd+KwMT8v0ChyxZDdzty?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b8d10511-7954-4f30-4198-08dc03da754a
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Dec 2023 17:13:18.7794
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AGU4MnM+bflgwnoqJiG9hkgZQ0TvedFne7XgjZfJZH38H5+iLcb3NB8ndYEuQxnqp/Xj+PIIsPl1Mec1Zq/8Iw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6253
X-OriginatorOrg: intel.com

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.

Thanks!
Ira

[1] https://lore.kernel.org/linux-block/20231223063728.3229446-1-linan666@huaweicloud.com/