On Jul 9, 2012, at 5:31 PM, Myklebust, Trond wrote:
> On Mon, 2012-07-09 at 16:37 -0400, Chuck Lever wrote:
>> On Jul 9, 2012, at 4:35 PM, Myklebust, Trond wrote:
>>
>>> On Mon, 2012-07-09 at 16:19 -0400, Chuck Lever wrote:
>>>> On Jul 9, 2012, at 4:15 PM, Chuck Lever wrote:
>>>>
>>>>>
>>>>> On Jul 9, 2012, at 4:03 PM, Myklebust, Trond wrote:
>>>>>
>>>>>> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote:
>>>>>>> Before beginning state recovery, the state manager zaps open and lock
>>>>>>> state for each open file, thus the "state->flags & flags" test in
>>>>>>> nfs41_{open,lock}_expired() always fails during reclaim. But open
>>>>>>> recovery is still needed for these files.
>>>>>>>
>>>>>>> To force a call to nfs4_open_expired(), change the default return
>>>>>>> value for nfs41_check_expired_stateid() to force open recovery, and
>>>>>>> the default return value for nfs41_check_locks() to force lock
>>>>>>> recovery, if the requested flags are clear. Fix suggested by Bryan
>>>>>>> Schumaker.
>>>>>>
>>>>>> This makes absolutely no sense...
>>>>>>
>>>>>> The point of these function should be to test if the stateid is still
>>>>>> valid. If it is, then we need no recovery.
>>>>>> If the stateid is _not_ valid, then we free it, and then decide if
>>>>>> recovery is needed. The only exception to that rule is if TEST_STATEID
>>>>>> returns NFS4ERR_BAD_STATEID, (in which case we don't need to free the
>>>>>> stateid but just recover immediately).
>>>>>
>>>>> So you'd like to change the sense of the switch statement in my earlier patch:
>>>>>
>>>>> NFS4_OK:
>>>>> do nothing
>>>>> NFS4ERR_BAD_STATEID:
>>>>> just open
>>>>> default:
>>>>> free the state ID, then open
>>>>>
>>>>> I still don't think we need a FREE_STATEID call for NFS4ERR_EXPIRED, for example. It seems to me the only cases where FREE_STATEID is needed is the _REVOKED cases.
>
> I'm not sure about what the rules are for NFS4ERR_EXPIRED. As far as I'm
> aware, NFS4ERR_EXPIRED is NFSv4.0-speak for *_STATE_REVOKED; returning
> it as part of a TEST_STATEID makes little sense to me, so I'm taking the
> conservative approach to it.
Based on your review comments, we have an opportunity to make this error handling more clear, which I'd like to clarify before redriving the patch.
I'm not quite sure what "conservative approach" means. Do you want to include NFS4ERR_EXPIRED with the _REVOKED arms, treat it as evidence of a broken server, or ignore it entirely?
Do you want to leave the default: arm as it is, or do you have a preference for adding additional explicit error codes in the switch statement? Are there cases which report that something has gone awry versus cases which simply report the state ID is rotten? My feeling when I wrote this was to get the switch statement in there, and then we can introduce more error codes as needed.
>
>>>>>> IOW: The state->flags and lsp->ls_flags tests should just be removed.
>>>>>
>>>>> Bryan had a reason to leave those tests in, but I don't remember what it was.
>>>>
>>>> Ah.
>>>>
>>>> I think it was that if those flags are set, then the client assumes the server still has that state, thus no recovery actions should be done.
>>>
>>> ??? If the server is still holding the state, then the TEST_STATEID will
>>> return '0'. Why do we need extra tests?
>>
>> This is Bryan's design. But my understanding is that recovery always visits every state ID, and these flags prevent tests from going to the server for all but the state ID to be recovered.
>>
>> Is that correct, Bryan?
>
> OK. I think I understand what the issue is.
>
> If we call nfs4_state_start_reclaim_nograce() (due to a lease purge or a
> NFS4ERR_EXPIRED on nfs4_check_lease()), then that resets the sequence
> ids and does a full reset of all state. In that case, we do indeed not
> want to call TEST_STATEID, since the best it can do is return
> NFS4ERR_OLD_STATEID.
>
> OK. This needs some comments in the code...
I can add comments in both the patch description and in the code itself.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
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
