[PATCH 0/2] NFS: layout changes while building mirrored writes

[PATCH 0/2] NFS: layout changes while building mirrored writes

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

These two fix trouble the pageio code has when a flexfiles layout is
recalled and comes back with a different mirror count while a write is
being built.  The first is data loss: an O_DIRECT write drops one
mirror's share of the bytes and still returns success.  The second is
a WARN_ON_ONCE in __nfs_pageio_add_request() that the same race trips
when a request has to be split, which looks like a pg_test bug but
isn't.

I reproduced this against a server that alternates each RW layout
between three mirrors and two and recalls it about a millisecond after
granting it, with mirrors at different wsizes (1M and 64K).  Without
these, 5 of 10 64MiB O_DIRECT rewrites left the 1M mirror holding the
previous data in 64K multiples.  With them I haven't seen a loss the
client was responsible for in a few hundred iterations, buffered or
O_DIRECT.  Buffered writes weren't affected by the first problem since
nfs_async_write_error() redirties on -EAGAIN.

Benjamin Coddington (2):
  NFS: Resend unsent mirror requests when an O_DIRECT write hits -EAGAIN
  NFS: Don't warn when a changed layout refuses a split subrequest

 fs/nfs/direct.c         | 28 ++++++++++++++++++++++++++--
 fs/nfs/pagelist.c       | 20 ++++++++++++++++----
 fs/nfs/read.c           |  3 ++-
 fs/nfs/write.c          |  5 +++--
 include/linux/nfs_xdr.h |  5 ++++-
 5 files changed, 51 insertions(+), 10 deletions(-)

-- 
2.53.0

[PATCH 1/2] NFS: Resend unsent mirror requests when an O_DIRECT write hits -EAGAIN

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

nfs_pageio_add_request() adds each page to mirrors 1..n before mirror
0.  When ff_layout_pg_init_write() finds that a recalled layout came
back with a different mirror count, it fails the descriptor with
-EAGAIN, and nfs_pageio_error_cleanup() runs the error_cleanup of every
mirror's queue.  For O_DIRECT that is nfs_write_sync_pgio_error(),
which releases the requests.  Mirrors flush at their own wsize, so the
released requests can be one mirror's share of bytes another mirror
has already written.  nfs_direct_write_schedule_iovec() treats -EAGAIN
as soft and reschedules only the current page and those after it, and
the bytes the other mirror wrote are counted: the write succeeds while
one mirror never receives that part of it.
nfs_direct_write_reschedule() loses requests the same way when the
layout changes during a resend.

Pass the descriptor to error_cleanup, and on -EAGAIN have
nfs_write_sync_pgio_error() mark the queued requests for rescheduling,
as nfs_direct_write_reschedule_io() does for a failed header, instead
of releasing them.  nfs_pageio_resend() passes no descriptor and keeps
its current behaviour.

Fixes: 8b0401373734 ("pNFS/flexfiles: Fix list corruption if the mirror count changes")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/nfs/direct.c         | 28 ++++++++++++++++++++++++++--
 fs/nfs/pagelist.c       |  6 +++---
 fs/nfs/read.c           |  3 ++-
 fs/nfs/write.c          |  5 +++--
 include/linux/nfs_xdr.h |  5 ++++-
 5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index ccafdc1ce64d..a5e5fd4b1812 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -286,7 +286,8 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr)
 	hdr->release(hdr);
 }
 
-static void nfs_read_sync_pgio_error(struct list_head *head, int error)
+static void nfs_read_sync_pgio_error(struct list_head *head, int error,
+				     struct nfs_pageio_descriptor *desc)
 {
 	struct nfs_page *req;
 
@@ -796,10 +797,33 @@ static void nfs_direct_write_completion(struct nfs_pgio_header *hdr)
 	hdr->release(hdr);
 }
 
-static void nfs_write_sync_pgio_error(struct list_head *head, int error)
+static void nfs_write_sync_pgio_error(struct list_head *head, int error,
+				      struct nfs_pageio_descriptor *desc)
 {
+	struct nfs_direct_req *dreq = desc ? desc->pg_dreq : NULL;
+	struct nfs_commit_info cinfo;
 	struct nfs_page *req;
 
+	/*
+	 * The layout changed under the descriptor: the queued requests were
+	 * never sent, and may be another mirror's share of a write that has
+	 * already completed elsewhere.  Resend them with the rest.
+	 */
+	if (error == -EAGAIN && dreq) {
+		nfs_init_cinfo_from_dreq(&cinfo, dreq);
+		spin_lock(&dreq->lock);
+		if (dreq->error == 0)
+			dreq->flags = NFS_ODIRECT_RESCHED_WRITES;
+		spin_unlock(&dreq->lock);
+		while (!list_empty(head)) {
+			req = nfs_list_entry(head->next);
+			nfs_list_remove_request(req);
+			nfs_unlock_request(req);
+			nfs_mark_request_commit(req, NULL, &cinfo, 0);
+		}
+		return;
+	}
+
 	while (!list_empty(head)) {
 		req = nfs_list_entry(head->next);
 		nfs_list_remove_request(req);
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 7dd478ffc2fa..06e301c8c291 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -1162,7 +1162,7 @@ nfs_pageio_cleanup_request(struct nfs_pageio_descriptor *desc,
 	LIST_HEAD(head);
 
 	nfs_list_move_request(req, &head);
-	desc->pg_completion_ops->error_cleanup(&head, desc->pg_error);
+	desc->pg_completion_ops->error_cleanup(&head, desc->pg_error, desc);
 }
 
 /**
@@ -1291,7 +1291,7 @@ static void nfs_pageio_error_cleanup(struct nfs_pageio_descriptor *desc)
 	for (midx = 0; midx < desc->pg_mirror_count; midx++) {
 		mirror = nfs_pgio_get_mirror(desc, midx);
 		desc->pg_completion_ops->error_cleanup(&mirror->pg_list,
-				desc->pg_error);
+				desc->pg_error, desc);
 	}
 }
 
@@ -1394,7 +1394,7 @@ int nfs_pageio_resend(struct nfs_pageio_descriptor *desc,
 	nfs_pageio_complete(desc);
 	if (!list_empty(&pages)) {
 		int err = desc->pg_error < 0 ? desc->pg_error : -EIO;
-		hdr->completion_ops->error_cleanup(&pages, err);
+		hdr->completion_ops->error_cleanup(&pages, err, NULL);
 		nfs_set_pgio_error(hdr, err, hdr->io_start);
 		return err;
 	}
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 2b70bd2b934b..1e6a88d5ea7c 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -220,7 +220,8 @@ static void nfs_initiate_read(struct nfs_pgio_header *hdr,
 }
 
 static void
-nfs_async_read_error(struct list_head *head, int error)
+nfs_async_read_error(struct list_head *head, int error,
+		     struct nfs_pageio_descriptor *desc)
 {
 	struct nfs_page	*req;
 
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 623e7ef1f73d..50f381b19fab 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1375,7 +1375,8 @@ static void nfs_redirty_request(struct nfs_page *req)
 	nfs_release_request(req);
 }
 
-static void nfs_async_write_error(struct list_head *head, int error)
+static void nfs_async_write_error(struct list_head *head, int error,
+				  struct nfs_pageio_descriptor *desc)
 {
 	struct nfs_page	*req;
 
@@ -1391,7 +1392,7 @@ static void nfs_async_write_error(struct list_head *head, int error)
 
 static void nfs_async_write_reschedule_io(struct nfs_pgio_header *hdr)
 {
-	nfs_async_write_error(&hdr->pages, 0);
+	nfs_async_write_error(&hdr->pages, 0, NULL);
 }
 
 static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops = {
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 7ed8fdb930d6..858d2bba8003 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1739,8 +1739,11 @@ struct nfs_commit_data {
 	unsigned long		flags;
 };
 
+struct nfs_pageio_descriptor;
+
 struct nfs_pgio_completion_ops {
-	void	(*error_cleanup)(struct list_head *head, int);
+	void	(*error_cleanup)(struct list_head *head, int error,
+				 struct nfs_pageio_descriptor *desc);
 	void	(*init_hdr)(struct nfs_pgio_header *hdr);
 	void	(*completion)(struct nfs_pgio_header *hdr);
 	void	(*reschedule_io)(struct nfs_pgio_header *hdr);
-- 
2.53.0

[PATCH 2/2] NFS: Don't warn when a changed layout refuses a split subrequest

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

When __nfs_pageio_add_request() splits a request, it sizes the
subrequest with pg_test and then adds it.  The current mirror's list
is still empty at that point, because sizing added nothing, so
nfs_pageio_do_add_request() runs pg_init a second time.  For pNFS,
pg_init re-checks the layout segment.  A layout recalled between the
two calls can make the second pg_init fail the descriptor, for
example with -EAGAIN from ff_layout_pg_init_write() when the mirror
count changed, or leave a smaller pg_bsize, and the subrequest is
then not accepted in full.

That trips WARN_ON_ONCE(subreq != req), which was meant to catch
pg_test changing its answer for an unchanged descriptor.  The cleanup
in that branch already handles a refused subrequest correctly.

Warn only when the descriptor has no error and still has the layout
segment and bsize the subrequest was sized against.

Fixes: 44a65a0c2783 ("NFS: Reverse the submission order of requests in __nfs_pageio_add_request()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/nfs/pagelist.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 06e301c8c291..cbcf19d5f61e 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -1181,8 +1181,10 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 			   struct nfs_page *req)
 {
 	struct nfs_pgio_mirror *mirror = nfs_pgio_current_mirror(desc);
+	struct pnfs_layout_segment *lseg = NULL;
 	struct nfs_page *subreq;
 	unsigned int size, subreq_size;
+	size_t bsize = 0;
 
 	nfs_page_group_lock(req);
 
@@ -1204,7 +1206,15 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 			subreq = req;
 			continue;
 		}
-		if (WARN_ON_ONCE(subreq != req)) {
+		if (subreq != req) {
+			/*
+			 * An empty mirror runs pg_init again for the subrequest,
+			 * and a layout that changed since it was sized can refuse
+			 * it.  Anything else means pg_test changed its answer.
+			 */
+			WARN_ON_ONCE(desc->pg_error >= 0 &&
+				     desc->pg_lseg == lseg &&
+				     mirror->pg_bsize == bsize);
 			nfs_page_group_unlock(req);
 			nfs_pageio_cleanup_request(desc, subreq);
 			subreq = req;
@@ -1227,6 +1237,8 @@ static int __nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
 		if (IS_ERR(subreq))
 			goto err_ptr;
 		subreq_size = size;
+		lseg = desc->pg_lseg;
+		bsize = mirror->pg_bsize;
 	}
 
 	nfs_page_group_unlock(req);
-- 
2.53.0