[PATCH v3 0/2] pnfs/blocklayout: report LAYOUTCOMMIT failures

[PATCH v3 0/2] pnfs/blocklayout: report LAYOUTCOMMIT failures

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

Data a block or SCSI layout client writes into unwritten extents becomes
file data only when a LAYOUTCOMMIT succeeds.  A server that has revoked
the layout answers NFS4ERR_BADLAYOUT, nfs4_layoutcommit_done() ignores
it, and fsync() and close() return 0 for data that reads back as
zeroes.  nfsd reaches this through its recall failure path today, and
the layout recall series [1] makes it more likely.

Patch 1 keeps the extents uncommitted when the LAYOUTCOMMIT fails before
its result is decoded.  Patch 2 records a writeback error when extents go
uncommitted, and has fsync() check for it after the LAYOUTCOMMIT.

Daejun tested the patch 2 approach on v2 in VMs, against an nfsd that
answers a LAYOUTCOMMIT with NFS4ERR_BADLAYOUT; I've only compile-tested
v3.

Changes since v2:
- Patch 1: add Reviewed-by tags.
- Patch 2: leave ->sync alone and check the file's writeback error in
  nfs_file_fsync() after pnfs_sync_inode(), so each fd reports the error
  once and stat() never returns it (Daejun).  This drops the bl_sync()
  move and the raw wb_err read, and Christoph's v2 Reviewed-by.

v2: https://lore.kernel.org/linux-nfs/cover.1792157092.git.bcodding@hammerspace.com/
v1: https://lore.kernel.org/linux-nfs/cover.1792097970.git.bcodding@hammerspace.com/
[1] https://lore.kernel.org/linux-nfs/20261015-nfsd-layout-recall-v3-0-ba3269f8ca22@samsung.com/

Benjamin Coddington (2):
  pnfs/blocklayout: don't mark extents committed after a failed
    LAYOUTCOMMIT
  pnfs/blocklayout: report data lost to a failed LAYOUTCOMMIT

 fs/nfs/blocklayout/blocklayout.c |  3 ++-
 fs/nfs/blocklayout/extent_tree.c | 14 +++++++++++---
 fs/nfs/file.c                    |  3 +++
 3 files changed, 16 insertions(+), 4 deletions(-)


base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
-- 
2.53.0

[PATCH v3 1/2] pnfs/blocklayout: don't mark extents committed after a failed LAYOUTCOMMIT

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

bl_cleanup_layoutcommit() decides whether the server converted the
extents from res.status, which only decode_layoutcommit() sets.  When the
compound fails before the LAYOUTCOMMIT op is decoded, as with a soft
timeout or an error on PUTFH, res.status is still zero and
ext_tree_mark_committed() turns the extents into
PNFS_BLOCK_READWRITE_DATA, although the server may not have converted
them.

Fall back to the task status when res.status is zero.

Fixes: b2be7811dd94 ("pnfsblock: cleanup_layoutcommit")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Daejun Park <daejun7.park@samsung.com>
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/nfs/blocklayout/blocklayout.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
index d54a141a89b3..aafcb8e30057 100644
--- a/fs/nfs/blocklayout/blocklayout.c
+++ b/fs/nfs/blocklayout/blocklayout.c
@@ -794,7 +794,8 @@ bl_prepare_layoutcommit(struct nfs4_layoutcommit_args *arg)
 static void
 bl_cleanup_layoutcommit(struct nfs4_layoutcommit_data *lcdata)
 {
-	ext_tree_mark_committed(&lcdata->args, lcdata->res.status);
+	ext_tree_mark_committed(&lcdata->args,
+				lcdata->res.status ?: lcdata->task.tk_status);
 }
 
 static int
-- 
2.53.0

[PATCH v3 2/2] pnfs/blocklayout: report data lost to a failed LAYOUTCOMMIT

[Benjamin Coddington]

From: Benjamin Coddington <bcodding@hammerspace.com>

Data written through a block or SCSI layout into PNFS_BLOCK_INVALID_DATA
extents stays unwritten on the server until a LAYOUTCOMMIT converts
those extents.  When the LAYOUTCOMMIT fails, ext_tree_mark_committed()
tags the extents written again so that a later LAYOUTCOMMIT can cover
them, and reports nothing.  nfs4_layoutcommit_done() also zeroes the
task status for NFS4ERR_BADLAYOUT, NFS4ERR_BADIOMODE,
NFS4ERR_DELEG_REVOKED and NFS4ERR_GRACE, so a synchronous LAYOUTCOMMIT
returns 0 for those.  A server that has revoked the layout answers
NFS4ERR_BADLAYOUT; once the client drops that layout its extent tree
goes with it, and fsync() and close() have returned 0 for data that
reads back as zeroes.

When ext_tree_mark_committed() puts extents back, record -EIO in the
mapping's writeback error and the superblock's.  close() already checks
the writeback error after nfs_wb_all(), but nfs_file_fsync() checks it
in nfs_file_fsync_commit(), before pnfs_sync_inode() sends the
LAYOUTCOMMIT, so check the file again after pnfs_sync_inode().

Fixes: b2be7811dd94 ("pnfsblock: cleanup_layoutcommit")
Suggested-by: Daejun Park <daejun7.park@samsung.com>
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/nfs/blocklayout/extent_tree.c | 14 +++++++++++---
 fs/nfs/file.c                    |  3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c
index 7393118c42a2..e408dba57401 100644
--- a/fs/nfs/blocklayout/extent_tree.c
+++ b/fs/nfs/blocklayout/extent_tree.c
@@ -696,6 +696,7 @@ ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status)
 	struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout);
 	struct rb_root *root = &bl->bl_ext_rw;
 	struct pnfs_block_extent *be;
+	bool uncommitted = false;
 
 	dprintk("%s status %d\n", __func__, status);
 
@@ -709,11 +710,13 @@ ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status)
 
 		if (status) {
 			/*
-			 * Mark as written and try again.
-			 *
-			 * XXX: some real error handling here wouldn't hurt..
+			 * The server may not have converted these blocks, so
+			 * what we wrote to them may not be part of the file.
+			 * Keep them for a later LAYOUTCOMMIT, but tell fsync
+			 * and close.
 			 */
 			be->be_tag = EXTENT_WRITTEN;
+			uncommitted = true;
 		} else {
 			be->be_state = PNFS_BLOCK_READWRITE_DATA;
 			be->be_tag = 0;
@@ -723,4 +726,9 @@ ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status)
 		be = ext_try_to_merge_right(root, be);
 	}
 	spin_unlock(&bl->bl_ext_lock);
+
+	if (uncommitted) {
+		filemap_set_wb_err(arg->inode->i_mapping, -EIO);
+		errseq_set(&arg->inode->i_sb->s_wb_err, -EIO);
+	}
 }
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index e1bdd10b35f1..59174ffed242 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -277,6 +277,9 @@ nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 		if (ret != 0)
 			break;
 		ret = pnfs_sync_inode(inode, !!datasync);
+		if (ret != 0)
+			break;
+		ret = file_check_and_advance_wb_err(file);
 		if (ret != 0)
 			break;
 		nredirtied = atomic_long_read(&nfsi->redirtied_pages);
-- 
2.53.0