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

[PATCH v2 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 returns it from the block/SCSI ->sync.  The files and
flexfiles layouts are unchanged.

NB: these are compile-tested only; I don't have a setup handy that can
revoke the layout..

Changes since v1:
- Patch 2: replace pnfs_generic_sync() with bl_sync() in the block
  layout driver (Christoph).
- Patch 2: check the raw wb_err value instead of a sample, so ->sync
  from inode revalidation no longer returns an older unseen writeback
  error from every stat() (sashiko).

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 | 20 +++++++++++++++++---
 fs/nfs/blocklayout/extent_tree.c | 14 +++++++++++---
 fs/nfs/pnfs.c                    |  7 -------
 fs/nfs/pnfs.h                    |  1 -
 4 files changed, 28 insertions(+), 14 deletions(-)


base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
-- 
2.53.0

[PATCH v2 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")
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 v2 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, and have the block and
SCSI ->sync return an error recorded during its LAYOUTCOMMIT.  ->sync
also runs from inode revalidation, so compare against the raw errseq
value rather than a sample, which would return an older unseen error
from every stat().  The block and SCSI layouts are the only users of
pnfs_generic_sync(), so replace it with bl_sync() in the block layout
driver.  The files and flexfiles layouts, whose LAYOUTCOMMIT does not
decide what a read of the file returns, are unchanged.

Fixes: b2be7811dd94 ("pnfsblock: cleanup_layoutcommit")
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 fs/nfs/blocklayout/blocklayout.c | 17 +++++++++++++++--
 fs/nfs/blocklayout/extent_tree.c | 14 +++++++++++---
 fs/nfs/pnfs.c                    |  7 -------
 fs/nfs/pnfs.h                    |  1 -
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
index aafcb8e30057..b41fc7a6f65c 100644
--- a/fs/nfs/blocklayout/blocklayout.c
+++ b/fs/nfs/blocklayout/blocklayout.c
@@ -798,6 +798,19 @@ bl_cleanup_layoutcommit(struct nfs4_layoutcommit_data *lcdata)
 				lcdata->res.status ?: lcdata->task.tk_status);
 }
 
+static int
+bl_sync(struct inode *inode, bool datasync)
+{
+	/* An error nobody has seen yet is not this LAYOUTCOMMIT's to return */
+	errseq_t since = READ_ONCE(inode->i_mapping->wb_err);
+	int ret;
+
+	ret = pnfs_layoutcommit_inode(inode, true);
+	if (ret)
+		return ret;
+	return filemap_check_wb_err(inode->i_mapping, since);
+}
+
 static int
 bl_set_layoutdriver(struct nfs_server *server, const struct nfs_fh *fh)
 {
@@ -976,7 +989,7 @@ static struct pnfs_layoutdriver_type blocklayout_type = {
 	.free_deviceid_node		= bl_free_deviceid_node,
 	.pg_read_ops			= &bl_pg_read_ops,
 	.pg_write_ops			= &bl_pg_write_ops,
-	.sync				= pnfs_generic_sync,
+	.sync				= bl_sync,
 };
 
 static struct pnfs_layoutdriver_type scsilayout_type = {
@@ -1000,7 +1013,7 @@ static struct pnfs_layoutdriver_type scsilayout_type = {
 	.free_deviceid_node		= bl_free_deviceid_node,
 	.pg_read_ops			= &bl_pg_read_ops,
 	.pg_write_ops			= &bl_pg_write_ops,
-	.sync				= pnfs_generic_sync,
+	.sync				= bl_sync,
 };
 
 
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/pnfs.c b/fs/nfs/pnfs.c
index 4f9c0f639014..99e33a69c087 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -3441,13 +3441,6 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 }
 EXPORT_SYMBOL_GPL(pnfs_layoutcommit_inode);
 
-int
-pnfs_generic_sync(struct inode *inode, bool datasync)
-{
-	return pnfs_layoutcommit_inode(inode, true);
-}
-EXPORT_SYMBOL_GPL(pnfs_generic_sync);
-
 struct nfs4_threshold *pnfs_mdsthreshold_alloc(void)
 {
 	struct nfs4_threshold *thp;
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 70d20f779678..85b2ba557b11 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -316,7 +316,6 @@ bool pnfs_wait_on_layoutreturn(struct inode *ino, struct rpc_task *task);
 void pnfs_set_layoutcommit(struct inode *, struct pnfs_layout_segment *, loff_t);
 void pnfs_cleanup_layoutcommit(struct nfs4_layoutcommit_data *data);
 int pnfs_layoutcommit_inode(struct inode *inode, bool sync);
-int pnfs_generic_sync(struct inode *inode, bool datasync);
 int pnfs_nfs_generic_sync(struct inode *inode, bool datasync);
 int _pnfs_return_layout(struct inode *);
 int pnfs_commit_and_return_layout(struct inode *);
-- 
2.53.0

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>