| Message ID | 20220620152407.63127-3-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handling session trunking group membership | expand |
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. In addition, a function like this that is intended for use by a different layer needs a proper kerneldoc comment so that we know what the API is for, and what it does. > + > struct connect_timeout_data { > unsigned long connect_timeout; > unsigned long reconnect_timeout;
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 > >
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); + struct connect_timeout_data { unsigned long connect_timeout; unsigned long reconnect_timeout;