[PATCH 0/1] WARNING dump caused by PG_UPTODATE in nfs_free_request()

[PATCH 0/1] WARNING dump caused by PG_UPTODATE in nfs_free_request()

[xiaoning.wang]

From: Clark Wang <xiaoning.wang@nxp.com>

Hi,

I noticed this issue occasionally occurring in the recent daily test based
on 6.18.

[ 1829.562634] WARNING: CPU: 2 PID: 4406 at /usr/src/kernel/fs/nfs/pagelist.c:587 nfs_free_request+0x1e0/0x240
[ 1829.572381] Modules linked in: wave5 snd_soc_fsl_asoc_card mali_kbase snd_soc_imx_card [...]
[ 1829.601384] CPU: 2 UID: 0 PID: 4406 Comm: kworker/u16:13 Tainted: G           O        6.18.20-2.0.0-g3f60d773760c #1 PREEMPT 
[ 1829.612750] Tainted: [O]=OOT_MODULE
[ 1829.616225] Hardware name: NXP i.MX952 EVK board (DT)
[ 1829.621262] Workqueue: nfsiod rpc_async_release
[ 1829.625785] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 1829.632734] pc : nfs_free_request+0x1e0/0x240
[ 1829.637077] lr : nfs_page_group_destroy+0x78/0x108
[ 1829.641853] sp : ffff8000812cbc70
[ 1829.645154] x29: ffff8000812cbc70 x28: 0000000000000000 x27: 0000000000000000
[ 1829.652278] x26: 0000000000000000 x25: 0000000000000000 x24: ffff0000801e9205
[ 1829.659402] x23: ffff000087486900 x22: ffff000081c11e10 x21: ffff000086898e00
[ 1829.666526] x20: ffff000086898e00 x19: ffff000086898e00 x18: ffff0001b75db780
[ 1829.673650] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000042
[ 1829.680774] x14: 000000012a874ebe x13: 0000000046000000 x12: 0000000000000000
[ 1829.687898] x11: ffff00009bcbd800 x10: ffff0000ab133998 x9 : 0000000000000000
[ 1829.695022] x8 : ffff00009bcbd9e8 x7 : 0000000000000000 x6 : 0000000000000001
[ 1829.702146] x5 : 0000000000000000 x4 : 0000000000000080 x3s : 0000000000000005
[ 1829.709270] x2 : 0000000000000244 x1 : 0000000000000204 x0 : 0000000000000204
[ 1829.716395] Call trace:
[ 1829.718831]  nfs_free_request+0x1e0/0x240 (P)
[ 1829.723180]  nfs_page_group_destroy+0x78/0x108
[ 1829.727617]  nfs_page_group_destroy+0xe8/0x108
[ 1829.732046]  nfs_release_request+0x68/0x98
[ 1829.736137]  nfs_readpage_release.isra.0+0x5c/0x70
[ 1829.740921]  nfs_read_completion+0xbc/0x13c
[ 1829.745089]  nfs_pgio_release+0x18/0x24
[ 1829.748911]  rpc_free_task+0x34/0x68
[ 1829.752482]  rpc_async_release+0x2c/0x48
[ 1829.756399]  process_one_work+0x150/0x290
[ 1829.760403]  worker_thread+0x180/0x2f4
[ 1829.764139]  kthread+0x12c/0x204
[ 1829.767363]  ret_from_fork+0x10/0x20
[ 1829.770934] ---[ end trace 0000000000000000 ]---

This WARN dump can be reliably reproduced on my side by using rsize=1024
during NFS mount and disconnecting the connection while reading data from
NFS.

I found when a folio is split into multiple subrequests, the PG_UPTODATE flag
is set each time a subrequest data read succeeds. As earlier subrequests
succeed, this bit accumulates progressively; however, if a later subrequest
fails, all previously accumulated bits must be invalidated.
The problem arises because the error path of nfs_read_completion() does not
properly clear these stale bits, leading to a WARN when the page group is
destroyed.

The logic causing this issue appears to have existed since
"67d0338edd71 ('nfs: page group syncing in read path')", which is a very old
patch. No one seemed to have reported this issue before. I'm not sure if
this was intentionally designed that way? (Sorry, I don't know much about
NFS; Just study some code to try fixing this issue.)

Hope to discuss this with everyone. Thanks!

The fix I proposed clears the previously set flag when an error occurs,
ensuring that subsequent processing within the same folio will not set the
flag again.

Clark Wang (1):
  nfs: keep PG_UPTODATE clear after read errors in page groups

 fs/nfs/read.c            | 25 ++++++++++++++++++++++++-
 include/linux/nfs_page.h |  1 +
 2 files changed, 25 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/1] nfs: keep PG_UPTODATE clear after read errors in page groups

[xiaoning.wang]

From: Clark Wang <xiaoning.wang@nxp.com>

When a read request is split into multiple subrequests, earlier
completions may advance PG_UPTODATE state for the page group once
their bytes fall within hdr->good_bytes. If a later subrequest in
the same group then completes with NFS_IOHDR_ERROR, the read path
needs to clear any accumulated PG_UPTODATE state and keep later
completions from rebuilding it.

Otherwise, a subsequent successful subrequest can re-enter
nfs_page_group_set_uptodate(), restore the page-group sync state,
and leave stale PG_UPTODATE behind for nfs_page_group_destroy()
to trip over in nfs_free_request().

Add a sticky page-group read-failed flag. Once any subrequest in
the group is known to be bad, mark the group failed, clear any
accumulated PG_UPTODATE state, and refuse further PG_UPTODATE
synchronization for the rest of the completion walk.

Fixes: 67d0338edd71 ("nfs: page group syncing in read path")
Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 fs/nfs/read.c            | 25 ++++++++++++++++++++++++-
 include/linux/nfs_page.h |  1 +
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index e1fe78d7b8d0..2b70bd2b934b 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -132,10 +132,32 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
 
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
 {
-	if (nfs_page_group_sync_on_bit(req, PG_UPTODATE))
+	bool uptodate = false;
+
+	nfs_page_group_lock(req);
+	if (!test_bit(PG_READ_FAILED, &req->wb_head->wb_flags) &&
+	    nfs_page_group_sync_on_bit_locked(req, PG_UPTODATE))
+		uptodate = true;
+	nfs_page_group_unlock(req);
+
+	if (uptodate)
 		folio_mark_uptodate(nfs_page_to_folio(req));
 }
 
+static void nfs_page_group_mark_read_failed(struct nfs_page *req)
+{
+	struct nfs_page *tmp;
+
+	nfs_page_group_lock(req);
+	set_bit(PG_READ_FAILED, &req->wb_head->wb_flags);
+	tmp = req;
+	do {
+		clear_bit(PG_UPTODATE, &tmp->wb_flags);
+		tmp = tmp->wb_this_page;
+	} while (tmp != req);
+	nfs_page_group_unlock(req);
+}
+
 static void nfs_read_completion(struct nfs_pgio_header *hdr)
 {
 	unsigned long bytes = 0;
@@ -172,6 +194,7 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)
 			if (bytes <= hdr->good_bytes)
 				nfs_page_group_set_uptodate(req);
 			else {
+				nfs_page_group_mark_read_failed(req);
 				error = hdr->error;
 				xchg(&nfs_req_openctx(req)->error, error);
 			}
diff --git a/include/linux/nfs_page.h b/include/linux/nfs_page.h
index afe1d8f09d89..4b9a35dbc062 100644
--- a/include/linux/nfs_page.h
+++ b/include/linux/nfs_page.h
@@ -33,6 +33,7 @@ enum {
 	PG_TEARDOWN,		/* page group sync for destroy */
 	PG_UNLOCKPAGE,		/* page group sync bit in read path */
 	PG_UPTODATE,		/* page group sync bit in read path */
+	PG_READ_FAILED,		/* page group saw a read error */
 	PG_WB_END,		/* page group sync bit in write path */
 	PG_REMOVE,		/* page group sync bit in write path */
 	PG_CONTENDED1,		/* Is someone waiting for a lock? */
-- 
2.34.1