Re: [PATCH v1 02/12] SUNRPC add function to offline remove trunkable transports

From: Olga Kornievskaia <olga.kornievskaia@gmail.com>
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>
Subject: Re: [PATCH v1 02/12] SUNRPC add function to offline remove trunkable transports
Date: Tue, 12 Jul 2022 12:07:13 -0400	[thread overview]
Message-ID: <CAN-5tyH0=SoQ-fnBY1MrjipEvky2MVxoETL2-a1PLMQNi+CQFA@mail.gmail.com> (raw)
In-Reply-To: <ad8fdf73ef81e405b7fb0e07bff6ac41d562658e.camel@hammerspace.com>

On Tue, Jul 12, 2022 at 11:24 AM Trond Myklebust
<trondmy@hammerspace.com> wrote:
>
> On Mon, 2022-06-20 at 11:23 -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Iterate thru available transports in the xprt_switch for all
> > trunkable transports offline and possibly remote them as well.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  include/linux/sunrpc/clnt.h |  1 +
> >  net/sunrpc/clnt.c           | 42
> > +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/include/linux/sunrpc/clnt.h
> > b/include/linux/sunrpc/clnt.h
> > index 90501404fa49..e74a0740603b 100644
> > --- a/include/linux/sunrpc/clnt.h
> > +++ b/include/linux/sunrpc/clnt.h
> > @@ -234,6 +234,7 @@
> > int         rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
> >                         struct rpc_xprt_switch *,
> >                         struct rpc_xprt *,
> >                         void *);
> > +void           rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void
> > *);
> >
> >  const char *rpc_proc_name(const struct rpc_task *task);
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index e2c6eca0271b..544b55a3aa20 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -2999,6 +2999,48 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
> >  }
> >  EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);
> >
> > +static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
> > +                                   struct rpc_xprt *xprt,
> > +                                   void *data)
> > +{
> > +       struct rpc_xprt *main_xprt;
> > +       struct rpc_xprt_switch *xps;
> > +       int err = 0;
> > +       int *offline_destroy = (int *)data;
> > +
> > +       xprt_get(xprt);
> > +
> > +       rcu_read_lock();
> > +       main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
> > +       xps = xprt_switch_get(rcu_dereference(clnt-
> > >cl_xpi.xpi_xpswitch));
> > +       err = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
> > +                               (struct sockaddr *)&main_xprt->addr);
> > +       rcu_read_unlock();
> > +       xprt_put(main_xprt);
> > +       if (err)
> > +               goto out;
> > +
> > +       if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED,
> > TASK_KILLABLE)) {
> > +               err = -EINTR;
> > +               goto out;
> > +       }
> > +       xprt_set_offline_locked(xprt, xps);
> > +       if (*offline_destroy)
> > +               xprt_delete_locked(xprt, xps);
> > +
> > +       xprt_release_write(xprt, NULL);
> > +out:
> > +       xprt_put(xprt);
> > +       xprt_switch_put(xps);
> > +       return err;
> > +}
> > +
> > +void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void
> > *data)
> > +{
> > +       rpc_clnt_iterate_for_each_xprt(clnt,
> > rpc_xprt_offline_destroy, data);
> > +}
> > +EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);
>
> Why is this function taking a 'void *' argument when
> rpc_xprt_offline_destroy() won't accept anything other than an 'int *'.
> It would be much cleaner to have 2 separate functions, neither or which
> need more than one argument. Then you can hide the pointer to the 'int'
> in each function and avoid exposing it as part of the API.

I could remove the void * altogether. As the following code only
offlines the transports. I wrote this function to be generic to be
able to do either if the need arises. It wasn't clear to me what you
meant by "have 2 separate functions". If you mean one for offline and
another for destroy, then perhaps that removes that need too. However,
if we were to have a generic one then since the majority of the code
is the same I don't see how having 2 functions is better.

> In addition, a function like this that is intended for use by
> different layer needs a proper kerneldoc comment so that we know what
> the API is for, and what it does.

Will add a comment above the function to explain what it does.

>
> > +
> >  struct connect_timeout_data {
> >         unsigned long connect_timeout;
> >         unsigned long reconnect_timeout;
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>