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

[dai.ngo@oracle.com]

On 2/14/24 10:14 AM, Chuck Lever wrote:
> On Wed, Feb 14, 2024 at 10:10:50AM -0800, dai.ngo@oracle.com wrote:
>> On 2/14/24 6:50 AM, Chuck Lever wrote:
>>> On Tue, Feb 13, 2024 at 09:47:27AM -0800, 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
>>>>
>>>> The CB_GETATTR is given 30ms to complte. If the CB_GETATTR fails for
>>>> any reasons, the delegation is recalled and NFS4ERR_DELAY is returned
>>>> for the GETATTR from the second client.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>    fs/nfsd/nfs4state.c | 119 ++++++++++++++++++++++++++++++++++++++++----
>>>>    fs/nfsd/nfs4xdr.c   |  10 +++-
>>>>    fs/nfsd/nfsd.h      |   1 +
>>>>    fs/nfsd/state.h     |  10 +++-
>>>>    4 files changed, 127 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index d9260e77ef2d..87987515e03d 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;
>>>> @@ -1189,6 +1190,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;
>>>> @@ -3044,11 +3049,59 @@ 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);
>>>> +	struct nfs4_delegation *dp =
>>>> +			container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
>>>> +
>>>> +	nfs4_put_stid(&dp->dl_stid);
>>>> +	clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
>>>> +	wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
>>>> +}
>>>> +
>>> What happens if the client responds after the GETATTR timer elapses?
>>> Can nfsd4_cb_getattr_release over-write memory that is now being
>>> used for something else?
>> The refcount added in nfs4_cb_getattr keeps the delegation state valid
>> until it's released here by nfs4_put_stid.
> If the client never replies, does that pin the stateid?

Won't RPC timeout on waiting for reply?
This is the same behavior as when recalling a delegation,
nfsd_break_deleg_cb and nfsd4_cb_recall_release.

-Dai

>
>