Re: [PATCH v2 11/19] NFSv4: Delegreturn must set m/atime when they are delegated

[Jeff Layton <jlayton@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5683 bytes --]

On Sun, 2024-06-16 at 21:21 -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> If the atime or mtime attributes were delegated, then we need to
> propagate their new values back to the server when returning the
> delegation.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Lance Shelton <lance.shelton@hammerspace.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/delegation.c |  9 +++++----
>  fs/nfs/delegation.h |  4 +++-
>  fs/nfs/nfs4proc.c   | 27 ++++++++++++++++++++++++---
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index d9117630e062..d5edb3b3eeef 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -266,7 +266,9 @@ void nfs_inode_reclaim_delegation(struct inode *inode, const struct cred *cred,
>  	}
>  }
>  
> -static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *delegation, int issync)
> +static int nfs_do_return_delegation(struct inode *inode,
> +				    struct nfs_delegation *delegation,
> +				    int issync)
>  {
>  	const struct cred *cred;
>  	int res = 0;
> @@ -275,9 +277,8 @@ static int nfs_do_return_delegation(struct inode *inode, struct nfs_delegation *
>  		spin_lock(&delegation->lock);
>  		cred = get_cred(delegation->cred);
>  		spin_unlock(&delegation->lock);
> -		res = nfs4_proc_delegreturn(inode, cred,
> -				&delegation->stateid,
> -				issync);
> +		res = nfs4_proc_delegreturn(inode, cred, &delegation->stateid,
> +					    delegation, issync);
>  		put_cred(cred);
>  	}
>  	return res;
> diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
> index 001551e2ab60..71524d34ed20 100644
> --- a/fs/nfs/delegation.h
> +++ b/fs/nfs/delegation.h
> @@ -70,7 +70,9 @@ void nfs_test_expired_all_delegations(struct nfs_client *clp);
>  void nfs_reap_expired_delegations(struct nfs_client *clp);
>  
>  /* NFSv4 delegation-related procedures */
> -int nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred, const nfs4_stateid *stateid, int issync);
> +int nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
> +			  const nfs4_stateid *stateid,
> +			  struct nfs_delegation *delegation, int issync);
>  int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state *state, const nfs4_stateid *stateid);
>  int nfs4_lock_delegation_recall(struct file_lock *fl, struct nfs4_state *state, const nfs4_stateid *stateid);
>  bool nfs4_copy_delegation_stateid(struct inode *inode, fmode_t flags, nfs4_stateid *dst, const struct cred **cred);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 1209ce22158e..88edeaf5b5d5 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6739,7 +6739,10 @@ static const struct rpc_call_ops nfs4_delegreturn_ops = {
>  	.rpc_release = nfs4_delegreturn_release,
>  };
>  
> -static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred, const nfs4_stateid *stateid, int issync)
> +static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
> +				  const nfs4_stateid *stateid,
> +				  struct nfs_delegation *delegation,
> +				  int issync)
>  {
>  	struct nfs4_delegreturndata *data;
>  	struct nfs_server *server = NFS_SERVER(inode);
> @@ -6791,12 +6794,27 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
>  		}
>  	}
>  
> +	if (delegation &&
> +	    test_bit(NFS_DELEGATION_DELEGTIME, &delegation->flags)) {
> +		if (delegation->type & FMODE_READ) {
> +			data->sattr.atime = inode_get_atime(inode);

I'm seeing the client try to set bad atimes with newly-created files.
To reproduce, I just created a file with vim on an NFS mount and wrote
a few bytes to it and then closed it.

The client gets a WRITE_DELEG_ATTRS delegation, and after I close the
file it does a SETATTR before the DELEGRETURN for TIME_DELEG_ACCESS and
TIME_DELEG_MODIFY. The mtime looks fine, but the atime is ~1500 seconds
after the epoch.

pcap is attached.

Cheers,

> +			data->sattr.atime_set = true;
> +		}
> +		if (delegation->type & FMODE_WRITE) {
> +			data->sattr.mtime = inode_get_mtime(inode);
> +			data->sattr.mtime_set = true;
> +		}
> +		data->args.sattr_args = &data->sattr;
> +		data->res.sattr_res = true;
> +	}
> +
>  	if (!data->inode)
>  		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
>  				   1);
>  	else
>  		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
>  				   0);
> +
>  	task_setup_data.callback_data = data;
>  	msg.rpc_argp = &data->args;
>  	msg.rpc_resp = &data->res;
> @@ -6814,13 +6832,16 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
>  	return status;
>  }
>  
> -int nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred, const nfs4_stateid *stateid, int issync)
> +int nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
> +			  const nfs4_stateid *stateid,
> +			  struct nfs_delegation *delegation, int issync)
>  {
>  	struct nfs_server *server = NFS_SERVER(inode);
>  	struct nfs4_exception exception = { };
>  	int err;
>  	do {
> -		err = _nfs4_proc_delegreturn(inode, cred, stateid, issync);
> +		err = _nfs4_proc_delegreturn(inode, cred, stateid,
> +					     delegation, issync);
>  		trace_nfs4_delegreturn(inode, stateid, err);
>  		switch (err) {
>  			case -NFS4ERR_STALE_STATEID:

-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: bad_deleg_atime.pcapng.xz --]
[-- Type: application/x-xz, Size: 6168 bytes --]