On Mon, 2012-07-09 at 17:45 -0400, Chuck Lever wrote:
> 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?
I mean that calling FREE_STATEID in cases where it isn't strictly needed
because the stateid is already invalid is safe. Not calling it when it
is needed ties up resources unnecessarily on the server.
--
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�����٥
