Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200424190856.30292-1-olga.kornievskaia@gmail.com> <dc483bc0dea9eab6b3fab1568fd18f710c818b39.camel@hammerspace.com>
In-Reply-To: <dc483bc0dea9eab6b3fab1568fd18f710c818b39.camel@hammerspace.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Fri, 24 Apr 2020 17:10:50 -0400
Message-ID: <CAN-5tyG3OeRevMzLEBxOiFSOjxxL-GWvDGrDNh6ieCmu10KpUw@mail.gmail.com>
Subject: Re: [PATCH 1/1] NFSv4.1: fix handling of backchannel binding in BIND_CONN_TO_SESSION
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 24, 2020 at 4:51 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> Hi Olga
>
> On Fri, 2020-04-24 at 15:08 -0400, Olga Kornievskaia wrote:
> > Currently, if the client sends BIND_CONN_TO_SESSION with
> > NFS4_CDFC4_FORE_OR_BOTH but only gets NFS4_CDFS4_FORE back it ignores
> > that it wasn't able to enable a backchannel.
> >
> > To make sure, the client sends BIND_CONN_TO_SESSION as the first
> > operation on the connections (ie., no other session compounds haven't
> > been sent before), and if the client's request to bind the
> > backchannel
> > is not satisfied, then reset the connection and retry.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4proc.c           | 6 ++++++
> >  include/linux/sunrpc/clnt.h | 5 +++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 512afb1..69a5487 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -7891,6 +7891,7 @@ static int nfs4_check_cl_exchange_flags(u32
> > flags)
> >  nfs4_bind_one_conn_to_session_done(struct rpc_task *task, void
> > *calldata)
> >  {
> >       struct nfs41_bind_conn_to_session_args *args = task-
> > >tk_msg.rpc_argp;
> > +     struct nfs41_bind_conn_to_session_res *res = task-
> > >tk_msg.rpc_resp;
> >       struct nfs_client *clp = args->client;
> >
> >       switch (task->tk_status) {
> > @@ -7899,6 +7900,11 @@ static int nfs4_check_cl_exchange_flags(u32
> > flags)
> >               nfs4_schedule_session_recovery(clp->cl_session,
> >                               task->tk_status);
> >       }
> > +     if (args->dir == NFS4_CDFC4_FORE_OR_BOTH &&
> > +                     res->dir != NFS4_CDFS4_BOTH) {
> > +             rpc_task_close_connection(task);
> > +             task->tk_status = -NFS4ERR_DELAY;
>
> We don't have any error handling for NFS4ERR_DELAY in this function, so
> if your intention is to replay the RPC call, then maybe you want to
> replace this line with a call to rpc_restart_call()?

What happens is nfs4_proc_bind_conn_to_session() is errored with
ERR_DELAY which goes back into nfs4state.c which has a case for
ERR_DELAY which resets the bit and does the call again. But I can try
the rpc_restart_call().

> However it would be good to ensure that we only do this once or twice
> so that we don't loop forever. Maybe also add a variable to the struct
> nfs41_bind_conn_to_session_args that can set the number of retries
> remaining?

If I recode using rpc_restart_call, then I can add something to the
args. Otherwise, number of retries would need to be handled some other
way that I'd need to think of.

Thank you for all the help.

>
> > +     }
> >  }
> >
> >  static const struct rpc_call_ops nfs4_bind_one_conn_to_session_ops =
> > {
> > diff --git a/include/linux/sunrpc/clnt.h
> > b/include/linux/sunrpc/clnt.h
> > index ca7e108..cc20a08 100644
> > --- a/include/linux/sunrpc/clnt.h
> > +++ b/include/linux/sunrpc/clnt.h
> > @@ -236,4 +236,9 @@ static inline int rpc_reply_expected(struct
> > rpc_task *task)
> >               (task->tk_msg.rpc_proc->p_decode != NULL);
> >  }
> >
> > +static inline void rpc_task_close_connection(struct rpc_task *task)
> > +{
> > +     if (task->tk_xprt)
> > +             xprt_force_disconnect(task->tk_xprt);
> > +}
> >  #endif /* _LINUX_SUNRPC_CLNT_H */
>
> Thanks
>   Trond
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>