On Tue, Jul 12, 2022 at 11:24 AM Trond Myklebust
<trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 2022-06-20 at 11:23 -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
>
>
