[PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit

[PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit

[Sergey Bashirov]

These patches correct the behavior of the pNFS server when the client
sends a layoutcommit without a new file size and with zero number of
block/scsi extents.

Tested manually for the pNFS block layout setup.

Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
---
Sergey Bashirov (2):
  NFSD: Minor cleanup in layoutcommit processing
  NFSD: Fix last write offset handling in layoutcommit

 fs/nfsd/blocklayout.c |  2 +-
 fs/nfsd/nfs4proc.c    | 20 +++++++-------------
 2 files changed, 8 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing

[Sergey Bashirov]

Remove dprintk in nfsd4_layoutcommit. These are not needed
in day to day usage, and the information is also available
in Wireshark when capturing NFS traffic.

Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
---
 fs/nfsd/nfs4proc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f4edf222e00e..37bdb937a0ae 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2499,18 +2499,12 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
 	inode = d_inode(current_fh->fh_dentry);
 
 	nfserr = nfserr_inval;
-	if (new_size <= seg->offset) {
-		dprintk("pnfsd: last write before layout segment\n");
+	if (new_size <= seg->offset)
 		goto out;
-	}
-	if (new_size > seg->offset + seg->length) {
-		dprintk("pnfsd: last write beyond layout segment\n");
+	if (new_size > seg->offset + seg->length)
 		goto out;
-	}
-	if (!lcp->lc_newoffset && new_size > i_size_read(inode)) {
-		dprintk("pnfsd: layoutcommit beyond EOF\n");
+	if (!lcp->lc_newoffset && new_size > i_size_read(inode))
 		goto out;
-	}
 
 	nfserr = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
 						false, lcp->lc_layout_type,
-- 
2.43.0

Re: [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing

[Christoph Hellwig]

Looks good:

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

[PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

[Sergey Bashirov]

The data type of loca_last_write_offset is newoffset4 and is switched
on a boolean value, no_newoffset, that indicates if a previous write
occurred or not. If no_newoffset is FALSE, an offset is not given.
This means that client does not try to update the file size. Thus,
server should not try to calculate new file size and check if it fits
into the seg range.

Co-developed-by: Konstantin Evtushenko <koevtushenko@yandex.com>
Signed-off-by: Konstantin Evtushenko <koevtushenko@yandex.com>
Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
---
 fs/nfsd/blocklayout.c |  2 +-
 fs/nfsd/nfs4proc.c    | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index 19078a043e85..ee6544bdc045 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -118,7 +118,7 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
 		struct iomap *iomaps, int nr_iomaps)
 {
 	struct timespec64 mtime = inode_get_mtime(inode);
-	loff_t new_size = lcp->lc_last_wr + 1;
+	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
 	struct iattr iattr = { .ia_valid = 0 };
 	int error;
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 37bdb937a0ae..ff38be803d8b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2482,7 +2482,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
 	const struct nfsd4_layout_seg *seg = &lcp->lc_seg;
 	struct svc_fh *current_fh = &cstate->current_fh;
 	const struct nfsd4_layout_ops *ops;
-	loff_t new_size = lcp->lc_last_wr + 1;
+	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
 	struct inode *inode;
 	struct nfs4_layout_stateid *ls;
 	__be32 nfserr;
@@ -2498,13 +2498,13 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
 		goto out;
 	inode = d_inode(current_fh->fh_dentry);
 
-	nfserr = nfserr_inval;
-	if (new_size <= seg->offset)
-		goto out;
-	if (new_size > seg->offset + seg->length)
-		goto out;
-	if (!lcp->lc_newoffset && new_size > i_size_read(inode))
-		goto out;
+	if (new_size) {
+		nfserr = nfserr_inval;
+		if (new_size <= seg->offset)
+			goto out;
+		if (new_size > seg->offset + seg->length)
+			goto out;
+	}
 
 	nfserr = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
 						false, lcp->lc_layout_type,
-- 
2.43.0

Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

[Chuck Lever]

Hi Sergey, Konstantin -


On 7/4/25 7:49 AM, Sergey Bashirov wrote:
> The data type of loca_last_write_offset is newoffset4 and is switched
> on a boolean value, no_newoffset, that indicates if a previous write
> occurred or not. If no_newoffset is FALSE, an offset is not given.
> This means that client does not try to update the file size. Thus,
> server should not try to calculate new file size and check if it fits
> into the seg range.

The patch description should describe the impact of the current
incorrect logic -- does it result in file corruption, failed tests, etc?
That way support engineers at distributions can more easily find this
patch if a customer runs across bad behavior.

Also, let's reference RFC 8881 Section 12.5.4.2, where the properly
compliant behavior is specified.

Fixes: 9cf514ccfacb ("nfsd: implement pNFS operations")


> Co-developed-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
> ---
>  fs/nfsd/blocklayout.c |  2 +-
>  fs/nfsd/nfs4proc.c    | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 19078a043e85..ee6544bdc045 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -118,7 +118,7 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  		struct iomap *iomaps, int nr_iomaps)
>  {
>  	struct timespec64 mtime = inode_get_mtime(inode);
> -	loff_t new_size = lcp->lc_last_wr + 1;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
>  	struct iattr iattr = { .ia_valid = 0 };
>  	int error;

See below for an alternative.


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37bdb937a0ae..ff38be803d8b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2482,7 +2482,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	const struct nfsd4_layout_seg *seg = &lcp->lc_seg;
>  	struct svc_fh *current_fh = &cstate->current_fh;
>  	const struct nfsd4_layout_ops *ops;
> -	loff_t new_size = lcp->lc_last_wr + 1;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
>   	struct inode *inode;
>  	struct nfs4_layout_stateid *ls;
>  	__be32 nfserr;
> @@ -2498,13 +2498,13 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  		goto out;
>  	inode = d_inode(current_fh->fh_dentry);
>  

How about instead, drop the new_size initializer above, and do this:

	lcp->lc_size_chg = false;
	if (lcp->lc_newoffset) {
		loff_t new_size = lcp->lc_last_wr + 1;

		nfserr = nfserr_inval;
		if (new_size <= seg->offset)
			goto out;
		if (new_size > seg->offset + seg->length)
			goto out;
		if (new_size > i_size_read(inode)) {
			lcp->lc_size_chg = true;
			lcp->lc_newsize = new_size;
		}
	}


> -	nfserr = nfserr_inval;
> -	if (new_size <= seg->offset)
> -		goto out;
> -	if (new_size > seg->offset + seg->length)
> -		goto out;
> -	if (!lcp->lc_newoffset && new_size > i_size_read(inode))
> -		goto out;
> +	if (new_size) {
> +		nfserr = nfserr_inval;
> +		if (new_size <= seg->offset)
> +			goto out;
> +		if (new_size > seg->offset + seg->length)
> +			goto out;
> +	}
>  
>  	nfserr = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
>  						false, lcp->lc_layout_type,

And lastly:

-	if (new_size > i_size_read(inode)) {
-		lcp->lc_size_chg = true;
-		lcp->lc_newsize = new_size;
-	} else {
-		lcp->lc_size_chg = false;
-	}




Also, I notice that nfsd4_decode_layoutcommit() has:

        if (xdr_stream_decode_bool(argp->xdr, &lcp->lc_reclaim) < 0)
                return nfserr_bad_xdr;

but:

        if (xdr_stream_decode_u32(argp->xdr, &lcp->lc_newoffset) < 0)
                return nfserr_bad_xdr;

The no_newoffset field should be decoded with xdr_stream_decode_bool too
(though the end result is the same). For just this nit, please make a
separate patch. Thanks!

-- 
Chuck Lever

Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

[Sergey Bashirov]

Hi Chuck,

Thanks for your kind guidance, it really helps to understand the
process of code contribution better! I'm on vacation next week,
will rework and resubmit updated patches when I get back.

--
Sergey Bashirov

Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

[Chuck Lever]

On 7/5/25 2:16 AM, Sergey Bashirov wrote:
> Hi Chuck,
> 
> Thanks for your kind guidance, it really helps to understand the
> process of code contribution better! I'm on vacation next week,
> will rework and resubmit updated patches when I get back.

Dai, Jeff, and I appreciate your help with NFSD's pNFS implementation!

-- 
Chuck Lever

Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit

[Christoph Hellwig]

On Fri, Jul 04, 2025 at 02:49:05PM +0300, Sergey Bashirov wrote:
> The data type of loca_last_write_offset is newoffset4 and is switched
> on a boolean value, no_newoffset, that indicates if a previous write
> occurred or not. If no_newoffset is FALSE, an offset is not given.
> This means that client does not try to update the file size. Thus,
> server should not try to calculate new file size and check if it fits
> into the seg range.
> 
> Co-developed-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
> ---
>  fs/nfsd/blocklayout.c |  2 +-
>  fs/nfsd/nfs4proc.c    | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 19078a043e85..ee6544bdc045 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -118,7 +118,7 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  		struct iomap *iomaps, int nr_iomaps)
>  {
>  	struct timespec64 mtime = inode_get_mtime(inode);
> -	loff_t new_size = lcp->lc_last_wr + 1;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;
>  	struct iattr iattr = { .ia_valid = 0 };
>  	int error;

Please guard the entire new_size check below instead, i.e.

	if (lcp->lc_newoffset) {
		loff_t new_size = lcp->lc_last_wr + 1;

		if (new_size > i_size_read(inode)) {
			iattr.ia_valid |= ATTR_SIZE;
			iattr.ia_size = new_size;
		}
	}


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37bdb937a0ae..ff38be803d8b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2482,7 +2482,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	const struct nfsd4_layout_seg *seg = &lcp->lc_seg;
>  	struct svc_fh *current_fh = &cstate->current_fh;
>  	const struct nfsd4_layout_ops *ops;
> -	loff_t new_size = lcp->lc_last_wr + 1;
> +	loff_t new_size = (lcp->lc_newoffset) ? lcp->lc_last_wr + 1 : 0;

Same here.