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.
> >>>> 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...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
