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

[PATCH 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..

[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/pnfs.c                    |  8 +++++++-
 3 files changed, 20 insertions(+), 5 deletions(-)


base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
-- 
2.53.0

[PATCH 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

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

[Christoph Hellwig]

Looks good:

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

[PATCH 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
pnfs_generic_sync(), which only the block and SCSI layouts use, return
an error recorded during its LAYOUTCOMMIT.  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/extent_tree.c | 14 +++++++++++---
 fs/nfs/pnfs.c                    |  8 +++++++-
 2 files changed, 18 insertions(+), 4 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/pnfs.c b/fs/nfs/pnfs.c
index 4f9c0f639014..b21ccba9fbab 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -3444,7 +3444,13 @@ EXPORT_SYMBOL_GPL(pnfs_layoutcommit_inode);
 int
 pnfs_generic_sync(struct inode *inode, bool datasync)
 {
-	return pnfs_layoutcommit_inode(inode, true);
+	errseq_t since = filemap_sample_wb_err(inode->i_mapping);
+	int ret;
+
+	ret = pnfs_layoutcommit_inode(inode, true);
+	if (ret)
+		return ret;
+	return filemap_check_wb_err(inode->i_mapping, since);
 }
 EXPORT_SYMBOL_GPL(pnfs_generic_sync);
 
-- 
2.53.0

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

[Christoph Hellwig]

On Fri, Oct 16, 2026 at 07:36:39AM -0400, Benjamin Coddington wrote:
> 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
> pnfs_generic_sync(), which only the block and SCSI layouts use, return
> an error recorded during its LAYOUTCOMMIT.  The files and flexfiles
> layouts, whose LAYOUTCOMMIT does not decide what a read of the file
> returns, are unchanged.

Looks good:

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

Should pnfs_generic_sync move into the blocklayout code as that is
the only user and it thus isn't exactly overly generic?

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

[Benjamin Coddington]

On 16 Oct 2026, at 8:56, Christoph Hellwig wrote:

> On Fri, Oct 16, 2026 at 07:36:39AM -0400, Benjamin Coddington wrote:
>> 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
>> pnfs_generic_sync(), which only the block and SCSI layouts use, return
>> an error recorded during its LAYOUTCOMMIT.  The files and flexfiles
>> layouts, whose LAYOUTCOMMIT does not decide what a read of the file
>> returns, are unchanged.
>
> Looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Should pnfs_generic_sync move into the blocklayout code as that is
> the only user and it thus isn't exactly overly generic?

Yep, that can be done - I can do it on a v2 here.

Sashiko also found that I messed up the error handling - this doesn't
consume the error on the mapping, and I probably should switch this around
to something like:

errseq_t since = READ_ONCE(inode->i_mapping->wb_err);
...
return filemap_check_wb_err(inode->i_mapping, since);

That change will probably drop your R-b, unless you say otherwise.

Ben