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@xxxxxxxxxxxxxxx
> Co-developed-by: Konstantin Evtushenko <koevtushenko@xxxxxxxxxx>
> Signed-off-by: Konstantin Evtushenko <koevtushenko@xxxxxxxxxx>
> Signed-off-by: Sergey Bashirov <sergeybashirov@xxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> 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@xxxxxxxxxx>
