linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit
@ 2025-07-04 11:49 Sergey Bashirov
  2025-07-04 11:49 ` [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing Sergey Bashirov
  2025-07-04 11:49 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
  0 siblings, 2 replies; 10+ messages in thread
From: Sergey Bashirov @ 2025-07-04 11:49 UTC (permalink / raw)
  To: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing
  2025-07-04 11:49 [PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
@ 2025-07-04 11:49 ` Sergey Bashirov
  2025-07-10  7:29   ` Christoph Hellwig
  2025-07-04 11:49 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
  1 sibling, 1 reply; 10+ messages in thread
From: Sergey Bashirov @ 2025-07-04 11:49 UTC (permalink / raw)
  To: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko, 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-04 11:49 [PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
  2025-07-04 11:49 ` [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing Sergey Bashirov
@ 2025-07-04 11:49 ` Sergey Bashirov
  2025-07-04 16:29   ` Chuck Lever
  2025-07-10  7:27   ` Christoph Hellwig
  1 sibling, 2 replies; 10+ messages in thread
From: Sergey Bashirov @ 2025-07-04 11:49 UTC (permalink / raw)
  To: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko, 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-04 11:49 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
@ 2025-07-04 16:29   ` Chuck Lever
  2025-07-05  6:16     ` Sergey Bashirov
  2025-07-10  7:27   ` Christoph Hellwig
  1 sibling, 1 reply; 10+ messages in thread
From: Chuck Lever @ 2025-07-04 16:29 UTC (permalink / raw)
  To: Sergey Bashirov, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-04 16:29   ` Chuck Lever
@ 2025-07-05  6:16     ` Sergey Bashirov
  2025-07-06 15:01       ` Chuck Lever
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bashirov @ 2025-07-05  6:16 UTC (permalink / raw)
  To: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko, 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-05  6:16     ` Sergey Bashirov
@ 2025-07-06 15:01       ` Chuck Lever
  0 siblings, 0 replies; 10+ messages in thread
From: Chuck Lever @ 2025-07-06 15:01 UTC (permalink / raw)
  To: Sergey Bashirov, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey
  Cc: linux-nfs, linux-kernel, Konstantin Evtushenko

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-04 11:49 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
  2025-07-04 16:29   ` Chuck Lever
@ 2025-07-10  7:27   ` Christoph Hellwig
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2025-07-10  7:27 UTC (permalink / raw)
  To: Sergey Bashirov
  Cc: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-nfs, linux-kernel,
	Konstantin Evtushenko

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing
  2025-07-04 11:49 ` [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing Sergey Bashirov
@ 2025-07-10  7:29   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2025-07-10  7:29 UTC (permalink / raw)
  To: Sergey Bashirov
  Cc: Chuck Lever, Christoph Hellwig, Jeff Layton, NeilBrown,
	Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-nfs, linux-kernel,
	Konstantin Evtushenko

Looks good:

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-21 18:40 [PATCH 0/2] NFSD: Rebase dropped patches for block/scsi layout Sergey Bashirov
@ 2025-07-21 18:40 ` Sergey Bashirov
  2025-07-21 18:56   ` Jeff Layton
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bashirov @ 2025-07-21 18:40 UTC (permalink / raw)
  To: Chuck Lever
  Cc: Jeff Layton, Christoph Hellwig, NeilBrown, Olga Kornievskaia,
	Dai Ngo, Tom Talpey, linux-nfs, linux-kernel, Sergey Bashirov,
	stable, Konstantin Evtushenko

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 segment range. See RFC 8881, section 12.5.4.2.

Sometimes the current incorrect logic may cause clients to hang when
trying to sync an inode. If layoutcommit fails, the client marks the
inode as dirty again.

Fixes: 9cf514ccfacb ("nfsd: implement pNFS operations")
Cc: stable@vger.kernel.org
Co-developed-by: Konstantin Evtushenko <koevtushenko@yandex.com>
Signed-off-by: Konstantin Evtushenko <koevtushenko@yandex.com>
Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/blocklayout.c |  5 ++---
 fs/nfsd/nfs4proc.c    | 30 +++++++++++++++---------------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index 4c936132eb440..0822d8a119c6f 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -118,7 +118,6 @@ 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;
 	struct iattr iattr = { .ia_valid = 0 };
 	int error;
 
@@ -128,9 +127,9 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
 	iattr.ia_valid |= ATTR_ATIME | ATTR_CTIME | ATTR_MTIME;
 	iattr.ia_atime = iattr.ia_ctime = iattr.ia_mtime = lcp->lc_mtime;
 
-	if (new_size > i_size_read(inode)) {
+	if (lcp->lc_size_chg) {
 		iattr.ia_valid |= ATTR_SIZE;
-		iattr.ia_size = new_size;
+		iattr.ia_size = lcp->lc_newsize;
 	}
 
 	error = inode->i_sb->s_export_op->commit_blocks(inode, iomaps,
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 656b2e7d88407..7043fc475458d 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2475,7 +2475,6 @@ 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;
 	struct inode *inode;
 	struct nfs4_layout_stateid *ls;
 	__be32 nfserr;
@@ -2491,13 +2490,21 @@ 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;
+	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 = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
 						false, lcp->lc_layout_type,
@@ -2513,13 +2520,6 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
 	/* LAYOUTCOMMIT does not require any serialization */
 	mutex_unlock(&ls->ls_mutex);
 
-	if (new_size > i_size_read(inode)) {
-		lcp->lc_size_chg = true;
-		lcp->lc_newsize = new_size;
-	} else {
-		lcp->lc_size_chg = false;
-	}
-
 	nfserr = ops->proc_layoutcommit(inode, rqstp, lcp);
 	nfs4_put_stid(&ls->ls_stid);
 out:
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit
  2025-07-21 18:40 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
@ 2025-07-21 18:56   ` Jeff Layton
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff Layton @ 2025-07-21 18:56 UTC (permalink / raw)
  To: Sergey Bashirov, Chuck Lever
  Cc: Christoph Hellwig, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-nfs, linux-kernel, stable,
	Konstantin Evtushenko

On Mon, 2025-07-21 at 21:40 +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.

Gah, what a confusing label in the spec. I went down the rabbit hole of
trying to understand why "no_newoffset" being TRUE means that you do
get an offset. The "no_" in this case is just a prefix for "union
newoffset", and not a negation of the flag.


> 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 segment range. See RFC 8881, section 12.5.4.2.
> 
> Sometimes the current incorrect logic may cause clients to hang when
> trying to sync an inode. If layoutcommit fails, the client marks the
> inode as dirty again.
> 
> Fixes: 9cf514ccfacb ("nfsd: implement pNFS operations")
> Cc: stable@vger.kernel.org
> Co-developed-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@yandex.com>
> Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/blocklayout.c |  5 ++---
>  fs/nfsd/nfs4proc.c    | 30 +++++++++++++++---------------
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 4c936132eb440..0822d8a119c6f 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -118,7 +118,6 @@ 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;
>  	struct iattr iattr = { .ia_valid = 0 };
>  	int error;
>  
> @@ -128,9 +127,9 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  	iattr.ia_valid |= ATTR_ATIME | ATTR_CTIME | ATTR_MTIME;
>  	iattr.ia_atime = iattr.ia_ctime = iattr.ia_mtime = lcp->lc_mtime;
>  
> -	if (new_size > i_size_read(inode)) {
> +	if (lcp->lc_size_chg) {
>  		iattr.ia_valid |= ATTR_SIZE;
> -		iattr.ia_size = new_size;
> +		iattr.ia_size = lcp->lc_newsize;
>  	}
>  
>  	error = inode->i_sb->s_export_op->commit_blocks(inode, iomaps,
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 656b2e7d88407..7043fc475458d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2475,7 +2475,6 @@ 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;
>  	struct inode *inode;
>  	struct nfs4_layout_stateid *ls;
>  	__be32 nfserr;
> @@ -2491,13 +2490,21 @@ 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;
> +	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 = nfsd4_preprocess_layout_stateid(rqstp, cstate, &lcp->lc_sid,
>  						false, lcp->lc_layout_type,
> @@ -2513,13 +2520,6 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
>  	/* LAYOUTCOMMIT does not require any serialization */
>  	mutex_unlock(&ls->ls_mutex);
>  
> -	if (new_size > i_size_read(inode)) {
> -		lcp->lc_size_chg = true;
> -		lcp->lc_newsize = new_size;
> -	} else {
> -		lcp->lc_size_chg = false;
> -	}
> -
>  	nfserr = ops->proc_layoutcommit(inode, rqstp, lcp);
>  	nfs4_put_stid(&ls->ls_stid);
>  out:

Code looks correct to me though.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-07-21 18:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-04 11:49 [PATCH 0/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
2025-07-04 11:49 ` [PATCH 1/2] NFSD: Minor cleanup in layoutcommit processing Sergey Bashirov
2025-07-10  7:29   ` Christoph Hellwig
2025-07-04 11:49 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
2025-07-04 16:29   ` Chuck Lever
2025-07-05  6:16     ` Sergey Bashirov
2025-07-06 15:01       ` Chuck Lever
2025-07-10  7:27   ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2025-07-21 18:40 [PATCH 0/2] NFSD: Rebase dropped patches for block/scsi layout Sergey Bashirov
2025-07-21 18:40 ` [PATCH 2/2] NFSD: Fix last write offset handling in layoutcommit Sergey Bashirov
2025-07-21 18:56   ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).