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

[Benny Halevy <bhalevy@tonian.com>]

On 2011-12-06 20:24, Tigran Mkrtchyan wrote:
> Ok, I think  I know what to do. I will update patches later today or tomorrw.

Great!  Thanks much for picking this up!

Benny

> 
> Tigran.
> 
> On Tue, Dec 6, 2011 at 3:32 PM, Benny Halevy <bhalevy@tonian.com> wrote:
>> On 2011-12-06 15:40, J. Bruce Fields wrote:
>>> On Tue, Dec 06, 2011 at 02:31:14PM +0100, Tigran Mkrtchyan wrote:
>>>> On Tue, Dec 6, 2011 at 12:26 PM, Benny Halevy <bhalevy@tonian.com> wrote:
>>>>> 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.
>>>>
>>>> Sounds good, nevertheless all operations call differently resulting
>>>> stateid which make it's not possible to use a unified schema.
>>
>> That was the point of introducing a per-op method to get the value.
>>
>>>
>>> Could we just remove all (or most) of those different fields and replace
>>> them by cstate->current_stateid?
>>
>> Hmm, that will require changing the corresponding xdr result encoding
>> routines to use the new field and all other use sites, but I don't think
>> of a good reason why this won't work.
>>
>> 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
>> --
>> 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
> --
> 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