On Mon, 2012-11-12 at 16:08 -0500, Andy Adamson wrote:
> On Mon, Nov 12, 2012 at 3:51 PM, Bryan Schumaker <bjschuma@xxxxxxxxxx> wrote:
> > On 11/12/2012 03:49 PM, Andy Adamson wrote:
> >> On Mon, Nov 12, 2012 at 3:29 PM, Myklebust, Trond
> >> <Trond.Myklebust@xxxxxxxxxx> wrote:
> >>> On Mon, 2012-11-12 at 15:22 -0500, Andy Adamson wrote:
> >>>> On Mon, Nov 12, 2012 at 1:35 PM, <bjschuma@xxxxxxxxxx> wrote:
> >>>>> From: Bryan Schumaker <bjschuma@xxxxxxxxxx>
> >>>>>
> >>>>> During recovery the NFS4_SESSION_DRAINING flag might be set on the
> >>>>> client structure. This can cause lease renewal to abort when
> >>>
> >>> Not lease renewal. It is lease verification (i.e. checking that we have
> >>> a valid lease and session) that will deadlock.
> >>>
> >>>>> nfs41_setup_sequence sees that we are doing recovery. As a result, the
> >>>>> client never recovers and all activity with the NFS server halts.
> >>>>
> >>>>
> >>>> When does this happen? Say the session is draining, and an RPC returns
> >>>> from one of the compounds such as nfs_open, nfs_locku etc whose
> >>>> rpc_call_done routine calls renew_lease after freeing it's slot. Like
> >>>> all calls on a draining session, the renew_lease sleeps on the session
> >>>> slot_tbl_waitq - is this what you mean by "causes lease renewal to
> >>>> abort"? How does this cause the client to never recover?
> >>>>
> >>>> The only other call to renew_lease is from the state manager itself
> >>>> which runs one function at a time, and will not call renew_lease until
> >>>> the recovery is over....
> >>>>
> >>>> What am I missing....?
> >>>
> >>> nfs4_check_lease() is run exclusively by the state manager thread in
> >>> order to check that the clientid (and session id on NFSv4.1) are valid.
> >>> It will deadlock the state manager thread if NFS4_SESSION_DRAINING is
> >>> already set.
> >>
> >> OK. NFS4_SESSION_DRAINING is also set exclusively by the state manager
> >> thread. Why is the state manager told to check the lease when it's
> >> also draining a session?
>
> Here is the call in the state manager.
>
> if (test_and_clear_bit(NFS4CLNT_BIND_CONN_TO_SESSION,
> &clp->cl_state)) {
> section = "bind conn to session";
> status = nfs4_bind_conn_to_session(clp);
> if (status < 0)
> goto out_error;
> continue;
> }
>
> nfs4_bind_conn_to_session calls nfs4_begin_drain_session. The error
> case is fine - nfs4_end_drain_session is called in out_error.
>
> But the continue when nfs4_bind_conn_to_session succeeds can send the
> state manager to process other flags (such as NFS4CLNT_CHECK_LEASE)
> without a call to nfs4_end_drain_session.
>
> That looks like a bug to me. The same can occur with
> NFS4CLNT_RECALL_SLOT.
So what?
> Why not just call nfs4_end_drain_session prior
> to the continue, or perhaps remove the continue and hit the
> nfs4_end_drain_session call further down in the state manager loop?
Calling nfs4_end_drain_session in order to allow unprivileged operations
to proceed, when we're not even sure that we still have a lease, makes
no sense. The whole point here is to block those operations until the
state manager thread has recovered the lease, session, open stateids and
lock stateids.
--
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�����٥
