Received-SPF: pass (google.com: domain of linux-kernel+bounces-10150-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 22 Dec 2023 16:24:16 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Coly Li <colyli@suse.de>
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: <658628b098aeb_b31b42945b@iweiny-mobl.notmuch>
References: <6585d5fda5183_9f731294b9@iweiny-mobl.notmuch>
 <6585dc32bebce_ab80829462@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6585dc32bebce_ab80829462@iweiny-mobl.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6j7/cO2qYyEEBTnmT4BJ+Y3/Gi2eWflzs9WJr+YAyVwE3Oo8xwULQGxX6ood?=
 =?us-ascii?Q?dQUnZxRn5huvbWT5nhdAqjUwS1S7w5fqZ+gTRTNy9h26d9nTgj1OprJ3ys5l?=
 =?us-ascii?Q?c9mOomGAEDyOvcRAF8lK2FMhmCFIayHddRLhRWwAmSYKRjunzJdSYc5WOrKS?=
 =?us-ascii?Q?7cBRvl8T97zWKf7cztgA0k3RAUcFfdL0y1T3dSQJ2HLWJG3HXVZdWnVzdFlp?=
 =?us-ascii?Q?Am4vbyrgQ/MeivwHCQdVsnvRLmwmRkwUZz62EJgg/kyJ4SPyLKZ//SJE8zpU?=
 =?us-ascii?Q?6ivuYxz2+PRK9PdaklAmTIBOLjrWUjrcPTKok7FcTjA2H26PkMC0gYC4MbiF?=
 =?us-ascii?Q?NTgb7ohfZI9WuTITVp+7bDJQJ9NC8CBOsjrI4cFTOJy3vkOiUwVtAbuw/n1P?=
 =?us-ascii?Q?tygxz1Btif90xDlue2i5iLMN+IM/jUIGrbWkoYeDpuH0w+eA6u1SYU9mHato?=
 =?us-ascii?Q?KON3cZIgxmMmPvBjpnx4cX3rkcKk0Xg5wsq/p1dG4rKxWGbc3rkrsSlmFBmp?=
 =?us-ascii?Q?gewFfIVO6I+WwQRQFyyxibZMtHst1haEDf33RTfROH0lDIIByoB9McUHqbZ6?=
 =?us-ascii?Q?zS8Bg+d9EGdAEdpzpg5CcrUj+YgpNQk5Sg+pFieajfCT4RDOBsU94XFrHMiF?=
 =?us-ascii?Q?Vz0wzB3pEqa8H5QF9jN1ODDIKwAnjO/oKp6dPfBnxe5C/jfyeg3tLGYysb0D?=
 =?us-ascii?Q?HON5P6pSdYTmIcUd9i2dKjD6K9egFxI9VyR6HmAXiq3Czw23wPsG9u8agch1?=
 =?us-ascii?Q?57DdOSojOK8xxjz7e3a8pMkHyIiSe6D9p0i+h7ZH/HkB2SopmBpZsnCuv+JQ?=
 =?us-ascii?Q?F7sXtpbrPrNWtUCvSJ+OFt+kQPJ6jrry6O4JVm6C212awDERJPkiFZoeM1hW?=
 =?us-ascii?Q?Ym0d8lGCvP1migB7G6FDCAj+VDGIoxvXVtqdAPAanZSlVdIghLxY240Slr7i?=
 =?us-ascii?Q?h+mvBvND6TL4urU3gZflq1zxH4GvZs+rIJRC8NEvk99XeqFi5QUaWR9njvPe?=
 =?us-ascii?Q?tVfwSBLbpMxJftrTBmZQu/KOpCuieYAujYt4Wqu6d/5Cxyq2aZRGjVZWQijt?=
 =?us-ascii?Q?OSKzCTjbw8irOAYoJL9klG0XYDqwBYSgqQRVVKqhfL0/itdewnZ0LWkMsGDc?=
 =?us-ascii?Q?lSvZgC1XOw2vYGsE4kTeKBe1g6BZ76iadXFUInSc0BL0oUZ5M9/wY9tgLEDr?=
 =?us-ascii?Q?fN3Wb6rkZEKVxC7M/+UynuhoyUNu9Z+4CDxrSeWIxDfsdw+R71LT3GJeOJyE?=
 =?us-ascii?Q?JStbUa/oC/067+/yC35vdHsgVU3IArF3i/D6lQX9rYgD7HyEthjAVrkSfvbS?=
 =?us-ascii?Q?vy8QWuofiZ/Qp1Lfarh7xVQqdhbbxBS6/i3LRq9d2vV7njS8rdPbCXXSchvd?=
 =?us-ascii?Q?ProOEohJiWFN3EBLMm/7hELFMcjSr6OXgBQeKkIhi8LdWZ8ynxtw031aCBlw?=
 =?us-ascii?Q?rU6Zj/Ku4xgyZkMbpkoHtxKrGFf/hVXMhq5H+iBnxq+tXW9FUAFLmsE9uVEC?=
 =?us-ascii?Q?HbfNYJZaxDvSYZjeZxv0KmBumSkDxDmT5muWUjfeKD2gj1y39e4rdUnKB7Lj?=
 =?us-ascii?Q?SsqvnXnJ34VuncTXBYX406LbcqLe8QXLsqxBmj11?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2a6f887b-435b-4e69-34ec-08dc034d814a
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Dec 2023 00:24:19.7768
 (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: Y1umtiVxbfu8QWcf0bXNqByALUF629UAxK3zFwz6eY2Diq6z9fWNPOLlLDz3lZVeLwozUIJU+zV7aC0eHD7NIA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6761
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> Ira Weiny wrote:
> > Coly,
> > 
> > Yesterday I noticed that a few of our nvdimm tests were failing.  I bisected
> > the problem to the following commit.
> > 
> > aa511ff8218b ("badblocks: switch to the improved badblock handling code") 
> > 
> > Reverting this patch fixed our tests.
> > 

[snip]

I added some prints[1] to try and see what is happening.  Perhaps this will
help you.

...
[   99.919237] IKW set_badblock 00000000aa44c55d 8000 1
[   99.921448] IKW set_badblock 00000000aa44c55d 8001 1
[   99.924051] IKW set_badblock 00000000aa44c55d 8002 1
[   99.926135] IKW set_badblock 00000000aa44c55d 8003 1
[   99.928516] IKW set_badblock 00000000aa44c55d 8004 1
[   99.930491] IKW set_badblock 00000000aa44c55d 8005 1
[   99.932894] IKW set_badblock 00000000aa44c55d 8006 1
[   99.936638] IKW set_badblock 00000000aa44c55d 8007 1
[  100.999297] IKW _badblocks_check() 00000000aa44c55d s 8000 num 1
[  101.000027]    IKW table count 1 shift 0
[  101.000644]    IKW 0: off 8000 end 8008
[  101.001271]    IKW prev 0, cnt 1
[  101.002481]    IKW start 8000, len 1
[  101.003464]    IKW front overlap 0
[  101.004256] IKW rv 1

...            ^^^^^^^^^
<This is a valid failure as part of the test>
...

[  101.148783] IKW set_badblock 00000000721b4f3d 8000 1
[  101.150629] IKW set_badblock 00000000721b4f3d 8001 1
[  101.152315] IKW set_badblock 00000000721b4f3d 8002 1
[  101.154544] IKW set_badblock 00000000721b4f3d 8003 1
[  101.156238] IKW set_badblock 00000000721b4f3d 8004 1
[  101.158310] IKW set_badblock 00000000721b4f3d 8005 1
[  101.160196] IKW set_badblock 00000000721b4f3d 8006 1
[  101.162158] IKW set_badblock 00000000721b4f3d 8007 1
[  101.163543] IKW _badblocks_check() 00000000721b4f3d s 0 num 8
[  101.164427]    IKW table count 1 shift 0
[  101.165310]    IKW 0: off 8000 end 8008
[  101.166398]    IKW prev -1, cnt 1
[  101.167178]    IKW start 0, len 8
[  101.168107] IKW rv 0
[  101.168858] IKW _badblocks_check() 00000000721b4f3d s 8 num 8
[  101.169814]    IKW table count 1 shift 0
[  101.170547]    IKW 0: off 8000 end 8008
[  101.171238]    IKW prev -1, cnt 1
[  101.171985]    IKW start 8, len 8
[  101.173007]    IKW front overlap -1  <== this is prev which is used to index bb->pages
[  101.174157]    IKW prev -1, cnt 1
[  101.175268]    IKW start 9, len 7
[  101.176557] IKW rv -1

...            ^^^^^^^^^
This is where the failure occurs.
...

I think overlap_front() is not working correctly in this case.  And from
my reading of the code I don't know how it would.  But overlap_front() is
used elsewhere and I'm not confident in making the change.

Hope this helps,
Ira


[1]
diff --git a/block/badblocks.c b/block/badblocks.c
index fc92d4e18aa3..21e22ee576e5 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -1280,6 +1280,16 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	unsigned int seq;
 	int len, rv;
 	u64 *p;
+	int i;
+
+	printk(KERN_CRIT "IKW %s() %p s %llx num %d\n", __func__,
+		bb, s, sectors);
+
+	printk(KERN_CRIT "   IKW table count %d shift %d\n", bb->count, bb->shift);
+	for (i = 0; i < bb->count; i++) {
+		printk(KERN_CRIT "   IKW %d: off %llx end %llx\n", i,
+			BB_OFFSET(bb->page[i]), BB_END(bb->page[i]));
+	}
 
 	WARN_ON(bb->shift < 0 || sectors == 0);
 
@@ -1311,6 +1321,9 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 
 	prev = prev_badblocks(bb, &bad, hint);
 
+	printk(KERN_CRIT "   IKW prev %d, cnt %d\n", prev, bb->count);
+	printk(KERN_CRIT "   IKW start %llx, len %llx\n", bad.start, bad.len);
+
 	/* start after all badblocks */
 	if ((prev + 1) >= bb->count && !overlap_front(bb, prev, &bad)) {
 		len = sectors;
@@ -1318,6 +1331,7 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	}
 
 	if (overlap_front(bb, prev, &bad)) {
+		printk(KERN_CRIT "   IKW front overlap %d\n", prev);
 		if (BB_ACK(p[prev]))
 			acked_badblocks++;
 		else
@@ -1365,6 +1379,7 @@ static int _badblocks_check(struct badblocks *bb, sector_t s, int sectors,
 	if (read_seqretry(&bb->lock, seq))
 		goto retry;
 
+	printk(KERN_CRIT "IKW rv %d\n", rv);
 	return rv;
 }
 
diff --git a/drivers/nvdimm/badrange.c b/drivers/nvdimm/badrange.c
index a002ea6fdd84..93ffd189bc75 100644
--- a/drivers/nvdimm/badrange.c
+++ b/drivers/nvdimm/badrange.c
@@ -167,6 +167,7 @@ static void set_badblock(struct badblocks *bb, sector_t s, int num)
 	dev_dbg(bb->dev, "Found a bad range (0x%llx, 0x%llx)\n",
 			(u64) s * 512, (u64) num * 512);
 	/* this isn't an error as the hardware will still throw an exception */
+	printk(KERN_CRIT "IKW %s %p %llx %x\n", __func__, bb, s, num);
 	if (badblocks_set(bb, s, num, 1))
 		dev_info_once(bb->dev, "%s: failed for sector %llx\n",
 				__func__, (u64) s);