Re: [PATCH v2 4/4] NFSD: handle GETATTR conflict with write delegation

[dai.ngo@oracle.com]

On 5/15/23 4:51 AM, Jeff Layton wrote:
> On Sun, 2023-05-14 at 17:20 -0700, Dai Ngo wrote:
>> If the GETATTR request on a file that has write delegation in effect
>> and the request attributes include the change info and size attribute
>> then the request is handled as below:
>>
>> Server sends CB_GETATTR to client to get the latest change info and file
>> size. If these values are the same as the server's cached values then
>> the GETATTR proceeds as normal.
>>
>> If either the change info or file size is different from the server's
>> cached values, or the file was already marked as modified, then:
>>
>>     . update time_modify and time_metadata into file's metadata
>>       with current time
>>
>>     . encode GETATTR as normal except the file size is encoded with
>>       the value returned from CB_GETATTR
>>
>>     . mark the file as modified
>>
>> If the CB_GETATTR fails for any reasons, the delegation is recalled
>> and NFS4ERR_DELAY is returned for the GETATTR.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 58 ++++++++++++++++++++++++++++++++++++
>>   fs/nfsd/nfs4xdr.c   | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   fs/nfsd/state.h     |  7 +++++
>>   3 files changed, 148 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 09a9e16407f9..fb305b28a090 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -127,6 +127,7 @@ static void free_session(struct nfsd4_session *);
>>   
>>   static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
>>   static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
>> +static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops;
>>   
>>   static struct workqueue_struct *laundry_wq;
>>   
>> @@ -1175,6 +1176,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>   	dp->dl_recalled = false;
>>   	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
>>   		      &nfsd4_cb_recall_ops, NFSPROC4_CLNT_CB_RECALL);
>> +	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
>> +			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
>> +	dp->dl_cb_fattr.ncf_file_modified = false;
>> +	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
>>   	get_nfs4_file(fp);
>>   	dp->dl_stid.sc_file = fp;
>>   	return dp;
>> @@ -2882,11 +2887,49 @@ nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
>>   	spin_unlock(&nn->client_lock);
>>   }
>>   
>> +static int
>> +nfsd4_cb_getattr_done(struct nfsd4_callback *cb, struct rpc_task *task)
>> +{
>> +	struct nfs4_cb_fattr *ncf =
>> +		container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> +
>> +	ncf->ncf_cb_status = task->tk_status;
>> +	switch (task->tk_status) {
>> +	case -NFS4ERR_DELAY:
>> +		rpc_delay(task, 2 * HZ);
>> +		return 0;
>> +	default:
>> +		return 1;
>> +	}
>> +}
>> +
>> +static void
>> +nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
>> +{
>> +	struct nfs4_cb_fattr *ncf =
>> +		container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> +
>> +	clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
>> +	wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
>> +}
>> +
>>   static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops = {
>>   	.done		= nfsd4_cb_recall_any_done,
>>   	.release	= nfsd4_cb_recall_any_release,
>>   };
>>   
>> +static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops = {
>> +	.done		= nfsd4_cb_getattr_done,
>> +	.release	= nfsd4_cb_getattr_release,
>> +};
>> +
>> +void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf)
>> +{
>> +	if (test_and_set_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags))
>> +		return;
>> +	nfsd4_run_cb(&ncf->ncf_getattr);
>> +}
>> +
>>   static struct nfs4_client *create_client(struct xdr_netobj name,
>>   		struct svc_rqst *rqstp, nfs4_verifier *verf)
>>   {
>> @@ -5591,6 +5634,8 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   	int cb_up;
>>   	int status = 0;
>>   	u32 wdeleg = false;
>> +	struct kstat stat;
>> +	struct path path;
>>   
>>   	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
>>   	open->op_recall = 0;
>> @@ -5626,6 +5671,19 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>   	wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
>>   	open->op_delegate_type = wdeleg ?
>>   			NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
>> +	if (wdeleg) {
>> +		path.mnt = currentfh->fh_export->ex_path.mnt;
>> +		path.dentry = currentfh->fh_dentry;
>> +		if (vfs_getattr(&path, &stat, STATX_BASIC_STATS,
> I think you want (STATX_SIZE|STATX_CTIME|STATX_CHANGE_COOKIE) here
> instead of BASIC_STATS. You might not get the change cookie otherwise,
> even when it's supported.

Fix in v3.

>
>> +						AT_STATX_SYNC_AS_STAT)) {
>> +			nfs4_put_stid(&dp->dl_stid);
>> +			destroy_delegation(dp);
>> +			goto out_no_deleg;
>> +		}
>> +		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
>> +		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat,
>> +							d_inode(currentfh->fh_dentry));
>> +	}
>>   	nfs4_put_stid(&dp->dl_stid);
>>   	return;
>>   out_no_deleg:
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 76db2fe29624..5d7e11db8ccf 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -2920,6 +2920,77 @@ nfsd4_encode_bitmap(struct xdr_stream *xdr, u32 bmval0, u32 bmval1, u32 bmval2)
>>   	return nfserr_resource;
>>   }
>>   
>> +static struct file_lock *
>> +nfs4_wrdeleg_filelock(struct svc_rqst *rqstp, struct inode *inode)
>> +{
>> +	struct file_lock_context *ctx;
>> +	struct file_lock *fl;
>> +
>> +	ctx = locks_inode_context(inode);
>> +	if (!ctx)
>> +		return NULL;
>> +	spin_lock(&ctx->flc_lock);
>> +	list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
>> +		if (fl->fl_type == F_WRLCK) {
>> +			spin_unlock(&ctx->flc_lock);
>> +			return fl;
>> +		}
>> +	}
> When there is a write lease, then there cannot be any read leases, so
> you don't need to walk the entire list here. Just check the first
> element and see whether it's a write lease.

Right, fix in v3.

Thanks,
-Dai

>
>> +	spin_unlock(&ctx->flc_lock);
>> +	return NULL;
>> +}
>> +
>> +static __be32
>> +nfs4_handle_wrdeleg_conflict(struct svc_rqst *rqstp, struct inode *inode,
>> +			bool *modified, u64 *size)
>> +{
>> +	__be32 status;
>> +	struct file_lock *fl;
>> +	struct nfs4_delegation *dp;
>> +	struct nfs4_cb_fattr *ncf;
>> +	struct iattr attrs;
>> +
>> +	*modified = false;
>> +	fl = nfs4_wrdeleg_filelock(rqstp, inode);
>> +	if (!fl)
>> +		return 0;
>> +	dp = fl->fl_owner;
>> +	ncf = &dp->dl_cb_fattr;
>> +	if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker))
>> +		return 0;
>> +
>> +	refcount_inc(&dp->dl_stid.sc_count);
>> +	nfs4_cb_getattr(&dp->dl_cb_fattr);
>> +	wait_on_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY, TASK_INTERRUPTIBLE);
>> +	if (ncf->ncf_cb_status) {
>> +		status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>> +		nfs4_put_stid(&dp->dl_stid);
>> +		return status;
>> +	}
>> +	ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
>> +	if (!ncf->ncf_file_modified &&
>> +			(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
>> +			ncf->ncf_cur_fsize != ncf->ncf_cb_fsize)) {
>> +		ncf->ncf_file_modified = true;
>> +	}
>> +
>> +	if (ncf->ncf_file_modified) {
>> +		/*
>> +		 * The server would not update the file's metadata
>> +		 * with the client's modified size.
>> +		 * nfsd4 change attribute is constructed from ctime.
>> +		 */
>> +		attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
>> +		attrs.ia_valid = ATTR_MTIME | ATTR_CTIME;
>> +		setattr_copy(&nop_mnt_idmap, inode, &attrs);
>> +		mark_inode_dirty(inode);
>> +		*size = ncf->ncf_cur_fsize;
>> +		*modified = true;
>> +	}
>> +	nfs4_put_stid(&dp->dl_stid);
>> +	return 0;
>> +}
>> +
>>   /*
>>    * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
>>    * ourselves.
>> @@ -2957,6 +3028,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>   		.dentry	= dentry,
>>   	};
>>   	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> +	bool file_modified;
>> +	u64 size = 0;
>>   
>>   	BUG_ON(bmval1 & NFSD_WRITEONLY_ATTRS_WORD1);
>>   	BUG_ON(!nfsd_attrs_supported(minorversion, bmval));
>> @@ -2966,6 +3039,12 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>   		if (status)
>>   			goto out;
>>   	}
>> +	if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
>> +		status = nfs4_handle_wrdeleg_conflict(rqstp, d_inode(dentry),
>> +						&file_modified, &size);
>> +		if (status)
>> +			goto out;
>> +	}
>>   
>>   	err = vfs_getattr(&path, &stat,
>>   			  STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
>> @@ -3089,7 +3168,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>>   		p = xdr_reserve_space(xdr, 8);
>>   		if (!p)
>>   			goto out_resource;
>> -		p = xdr_encode_hyper(p, stat.size);
>> +		if (file_modified)
>> +			p = xdr_encode_hyper(p, size);
>> +		else
>> +			p = xdr_encode_hyper(p, stat.size);
>>   	}
>>   	if (bmval0 & FATTR4_WORD0_LINK_SUPPORT) {
>>   		p = xdr_reserve_space(xdr, 4);
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 9fb69ed8ae80..b20b65fe89b4 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -121,6 +121,10 @@ struct nfs4_cb_fattr {
>>   	struct nfsd4_callback ncf_getattr;
>>   	u32 ncf_cb_status;
>>   	u32 ncf_cb_bmap[1];
>> +	unsigned long ncf_cb_flags;
>> +	bool ncf_file_modified;
>> +	u64 ncf_initial_cinfo;
>> +	u64 ncf_cur_fsize;
>>   
>>   	/* from CB_GETATTR reply */
>>   	u64 ncf_cb_change;
>> @@ -744,6 +748,9 @@ extern void nfsd4_client_record_remove(struct nfs4_client *clp);
>>   extern int nfsd4_client_record_check(struct nfs4_client *clp);
>>   extern void nfsd4_record_grace_done(struct nfsd_net *nn);
>>   
>> +/* CB_GETTTAR */
>> +extern void nfs4_cb_getattr(struct nfs4_cb_fattr *ncf);
>> +
>>   static inline bool try_to_expire_client(struct nfs4_client *clp)
>>   {
>>   	cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);