Re: [PATCH 2/2] nfsv41: handle current stateid on open and close

[Benny Halevy <bhalevy@tonian.com>]

On 2011-12-06 04:08, J. Bruce Fields wrote:
> On Sun, Dec 04, 2011 at 02:42:11PM +0200, Benny Halevy wrote:
>> On 2011-12-04 14:03, tigran.mkrtchyan@desy.de wrote:
>>> From: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
>>>
>>>
>>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
>>> ---
>>>  fs/nfsd/nfs4proc.c  |    6 ++++++
>>>  fs/nfsd/nfs4state.c |   26 ++++++++++++++++++++------
>>>  2 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index fa38336..535aed2 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -400,6 +400,12 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>  	 */
>>>  	status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
>>>  	WARN_ON(status && open->op_created);
>>> +
>>> +	if(status)
>>> +		goto out;
>>> +
>>> +	/* set current state id */
>>> +	memcpy(&cstate->current_stateid, &open->op_stateid, sizeof(stateid_t));
> 
> That comment is a bit redundant.
> 
>> Since this should be done for all stateid-returning operations
>> I think that a cleaner approach could be to mark those as such in
>> nfsd4_ops by providing a per-op function to return the operation's
>> stateid.  You can then call this method from nfsd4_proc_compound()
>> after the call to nfsd4_encode_operation() and when status == 0.
> 
> So the choice is between
> 
> +       memcpy(&cstate->current_stateid, &open->op_stateid,
> sizeof(stateid_t));
> 
> and
> 
> +       static void get_open_stateid(stateid_t *s)
> +       {
> +               memcpy(s, open->op_stateid);
> +       }
> +
> +       [OP_OPEN] = {
> +               ...
> +               .op_get_stateid = get_open_stateid,
> +               ...
> +       }
> 
> ?
> 
> I'm not so sure.

The point is to copy the result stateid into the current_stateid
in a centralized place: nfsd4_proc_compound() and do that for all
stateid-modifying operations.

> 
> Anyway, thanks Tigran for looking at this.
> 
> Do we want to guarantee that the client can't expire as long as a
> compound references the stateid?  I think that's the case.

The client can't time out while the 4.1 compound is in progress, see commit d768298.
Are you thinking of explicit expiration of the client?
We may unhash the client and keep using it while it's referenced
so that's not a problem.  As far as the stateid goes, we're copying the
value of the stateid, not pointing to any stateid structure.  If the
actual state was destroyed, we will detect that when the current_stateid
is used by any successive operation and we cannot find the state using
the stateid.

Benny

> 
> --b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html