On Tue, Dec 6, 2011 at 12:26 PM, Benny Halevy <bhalevy@xxxxxxxxxx> 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@xxxxxxx wrote:
>>>> From: Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
>>>>
>>>>
>>>> Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@xxxxxxx>
>>>> ---
>>>> 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.
Regards,
Tigran.
>
>>
>> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
