On Wed, 30 Jul 2025, Jeff Layton wrote:
> Instead of allowing the ctime to roll backward with a WRITE_ATTRS
> delegation, set FMODE_NOCMTIME on the file and have it skip mtime and
> ctime updates.
>
> It is possible that the client will never send a SETATTR to set the
> times before returning the delegation. Add two new bools to struct
> nfs4_delegation:
>
> dl_written: tracks whether the file has been written since the
> delegation was granted. This is set in the WRITE and LAYOUTCOMMIT
> handlers.
>
> dl_setattr: tracks whether the client has sent at least one valid
> mtime that can also update the ctime in a SETATTR.
Do we really need both of these?
Could we set dl_written on a write and clear it on a setattr that sets
mtime/ctime?
Then on close, if it is still set we force an mtime update.
I would be inclined to put this bit in ->f_mode setting it precisely when
FMODE_NOCMTIME is uses to skip the update. (There are 4 spare fmode bits).
But none of that need delay the current patchset which looks good to me
(though I haven't dug quite enough to give a Reviewed-by).
Thanks,
NeilBRown
>
> When unlocking the lease for the delegation, clear FMODE_NOCMTIME. If
> the file has been written, but no setattr for the delegated mtime and
> ctime has been done, update the timestamps to current_time().
>
> Suggested-by: NeilBrown <n...@brown.name>
> Signed-off-by: Jeff Layton <jlay...@kernel.org>
> ---
> fs/nfsd/nfs4proc.c | 26 ++++++++++++++++++++++++--
> fs/nfsd/nfs4state.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/state.h | 4 +++-
> 3 files changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index
> aacd912a5fbe29ba5ccac206d13243308f36b7fa..bfebe6e25638a76d3607bb79a239bdc92e42e7b5
> 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1151,7 +1151,9 @@ vet_deleg_attrs(struct nfsd4_setattr *setattr, struct
> nfs4_delegation *dp)
> if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
> if (nfsd4_vet_deleg_time(&iattr->ia_mtime, &dp->dl_mtime,
> &now)) {
> iattr->ia_ctime = iattr->ia_mtime;
> - if (!nfsd4_vet_deleg_time(&iattr->ia_ctime,
> &dp->dl_ctime, &now))
> + if (nfsd4_vet_deleg_time(&iattr->ia_ctime,
> &dp->dl_ctime, &now))
> + dp->dl_setattr = true;
> + else
> iattr->ia_valid &= ~(ATTR_CTIME |
> ATTR_CTIME_SET);
> } else {
> iattr->ia_valid &= ~(ATTR_CTIME | ATTR_CTIME_SET |
> @@ -1238,12 +1240,26 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
> return status;
> }
>
> +static void nfsd4_file_mark_deleg_written(struct nfs4_file *fi)
> +{
> + spin_lock(&fi->fi_lock);
> + if (!list_empty(&fi->fi_delegations)) {
> + struct nfs4_delegation *dp =
> list_first_entry(&fi->fi_delegations,
> + struct
> nfs4_delegation, dl_perfile);
> +
> + if (dp->dl_type == OPEN_DELEGATE_WRITE_ATTRS_DELEG)
> + dp->dl_written = true;
> + }
> + spin_unlock(&fi->fi_lock);
> +}
> +
> static __be32
> nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> union nfsd4_op_u *u)
> {
> struct nfsd4_write *write = &u->write;
> stateid_t *stateid = &write->wr_stateid;
> + struct nfs4_stid *stid = NULL;
> struct nfsd_file *nf = NULL;
> __be32 status = nfs_ok;
> unsigned long cnt;
> @@ -1256,10 +1272,15 @@ nfsd4_write(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
> trace_nfsd_write_start(rqstp, &cstate->current_fh,
> write->wr_offset, cnt);
> status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> - stateid, WR_STATE, &nf, NULL);
> + stateid, WR_STATE, &nf, &stid);
> if (status)
> return status;
>
> + if (stid) {
> + nfsd4_file_mark_deleg_written(stid->sc_file);
> + nfs4_put_stid(stid);
> + }
> +
> write->wr_how_written = write->wr_stable_how;
> status = nfsd_vfs_write(rqstp, &cstate->current_fh, nf,
> write->wr_offset, &write->wr_payload,
> @@ -2550,6 +2571,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
> mutex_unlock(&ls->ls_mutex);
>
> nfserr = ops->proc_layoutcommit(inode, rqstp, lcp);
> + nfsd4_file_mark_deleg_written(ls->ls_stid.sc_file);
> nfs4_put_stid(&ls->ls_stid);
> out:
> return nfserr;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index
> 205ee8cc6fa2b9f74d08f7938b323d03bdf8286c..81fa7cc6c77b3cdc5ff22bc60ab0654f95dc258d
> 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1222,6 +1222,42 @@ static void put_deleg_file(struct nfs4_file *fp)
> nfs4_file_put_access(fp, NFS4_SHARE_ACCESS_READ);
> }
>
> +static void nfsd4_finalize_deleg_timestamps(struct nfs4_delegation *dp,
> struct file *f)
> +{
> + struct iattr ia = { .ia_valid = ATTR_ATIME | ATTR_CTIME | ATTR_MTIME };
> + struct inode *inode = file_inode(f);
> + int ret;
> +
> + /* don't do anything if FMODE_NOCMTIME isn't set */
> + if ((READ_ONCE(f->f_mode) & FMODE_NOCMTIME) == 0)
> + return;
> +
> + spin_lock(&f->f_lock);
> + f->f_mode &= ~FMODE_NOCMTIME;
> + spin_unlock(&f->f_lock);
> +
> + /* was it never written? */
> + if (!dp->dl_written)
> + return;
> +
> + /* did it get a setattr for the timestamps at some point? */
> + if (dp->dl_setattr)
> + return;
> +
> + /* Stamp everything to "now" */
> + inode_lock(inode);
> + ret = notify_change(&nop_mnt_idmap, f->f_path.dentry, &ia, NULL);
> + inode_unlock(inode);
> + if (ret) {
> + struct inode *inode = file_inode(f);
> +
> + pr_notice_ratelimited("Unable to update timestamps on inode
> %02x:%02x:%lu: %d\n",
> + MAJOR(inode->i_sb->s_dev),
> + MINOR(inode->i_sb->s_dev),
> + inode->i_ino, ret);
> + }
> +}
> +
> static void nfs4_unlock_deleg_lease(struct nfs4_delegation *dp)
> {
> struct nfs4_file *fp = dp->dl_stid.sc_file;
> @@ -1229,6 +1265,7 @@ static void nfs4_unlock_deleg_lease(struct
> nfs4_delegation *dp)
>
> WARN_ON_ONCE(!fp->fi_delegees);
>
> + nfsd4_finalize_deleg_timestamps(dp, nf->nf_file);
> kernel_setlease(nf->nf_file, F_UNLCK, NULL, (void **)&dp);
> put_deleg_file(fp);
> }
> @@ -6265,6 +6302,8 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct
> nfsd4_open *open,
> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid,
> sizeof(dp->dl_stid.sc_stateid));
>
> if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> + struct file *f = dp->dl_stid.sc_file->fi_deleg_file->nf_file;
> +
> if (!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp) ||
> !nfs4_delegation_stat(dp, currentfh, &stat)) {
> nfs4_put_stid(&dp->dl_stid);
> @@ -6278,6 +6317,9 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct
> nfsd4_open *open,
> dp->dl_atime = stat.atime;
> dp->dl_ctime = stat.ctime;
> dp->dl_mtime = stat.mtime;
> + spin_lock(&f->f_lock);
> + f->f_mode |= FMODE_NOCMTIME;
> + spin_unlock(&f->f_lock);
> trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> } else {
> open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp,
> currentfh, &stat) ?
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index
> bf9436cdb93c5dd5502ecf83433ea311e3678711..b6ac0f37e9cdfcfddde5861c8c0c51bad60101b7
> 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -217,10 +217,12 @@ struct nfs4_delegation {
> struct nfs4_clnt_odstate *dl_clnt_odstate;
> time64_t dl_time;
> u32 dl_type;
> -/* For recall: */
> + /* For recall: */
> int dl_retries;
> struct nfsd4_callback dl_recall;
> bool dl_recalled;
> + bool dl_written;
> + bool dl_setattr;
>
> /* for CB_GETATTR */
> struct nfs4_cb_fattr dl_cb_fattr;
>
> --
> 2.50.1
>
>
