Re: [PATCH 3/3] sunrpc: Add a sysfs file for adding a new xprt

Olga Kornievskaia
2025-01-14T15:09:34Z

On Mon, Jan 13, 2025 at 4:52 PM Anna Schumaker
<anna.schumaker@oracle.com> wrote:
>
> Hi Olga,
>
> On 1/13/25 9:10 AM, Olga Kornievskaia wrote:
> > On Wed, Jan 8, 2025 at 4:36 PM Anna Schumaker <anna@kernel.org> wrote:
> >>
> >> From: Anna Schumaker <anna.schumaker@oracle.com>
> >>
> >> Writing to this file will clone the 'main' xprt of an xprt_switch and
> >> add it to be used as an additional connection.
> >>
> >> Signed-off-by: Anna Schumaker <anna.schumaker@oracle.com>
> >> ---
> >> include/linux/sunrpc/xprtmultipath.h | 1 +
> >> net/sunrpc/sysfs.c | 54 ++++++++++++++++++++++++++++
> >> net/sunrpc/xprtmultipath.c | 21 +++++++++++
> >> 3 files changed, 76 insertions(+)
> >>
> >> diff --git a/include/linux/sunrpc/xprtmultipath.h b/include/linux/sunrpc/xprtmultipath.h
> >> index c0514c684b2c..c827c6ef0bc5 100644
> >> --- a/include/linux/sunrpc/xprtmultipath.h
> >> +++ b/include/linux/sunrpc/xprtmultipath.h
> >> @@ -56,6 +56,7 @@ extern void rpc_xprt_switch_add_xprt(struct rpc_xprt_switch *xps,
> >> struct rpc_xprt *xprt);
> >> extern void rpc_xprt_switch_remove_xprt(struct rpc_xprt_switch *xps,
> >> struct rpc_xprt *xprt, bool offline);
> >> +extern struct rpc_xprt *rpc_xprt_switch_get_main_xprt(struct rpc_xprt_switch *xps);
> >>
> >> extern void xprt_iter_init(struct rpc_xprt_iter *xpi,
> >> struct rpc_xprt_switch *xps);
> >> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> >> index dc3b7cd70000..ce7dae140770 100644
> >> --- a/net/sunrpc/sysfs.c
> >> +++ b/net/sunrpc/sysfs.c
> >> @@ -250,6 +250,55 @@ static ssize_t rpc_sysfs_xprt_switch_info_show(struct kobject *kobj,
> >> return ret;
> >> }
> >>
> >> +static ssize_t rpc_sysfs_xprt_switch_add_xprt_show(struct kobject *kobj,
> >> + struct kobj_attribute *attr,
> >> + char *buf)
> >> +{
> >> + return sprintf(buf, "# add one xprt to this xprt_switch\n");
> >> +}
> >> +
> >> +static ssize_t rpc_sysfs_xprt_switch_add_xprt_store(struct kobject *kobj,
> >> + struct kobj_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct rpc_xprt_switch *xprt_switch =
> >> + rpc_sysfs_xprt_switch_kobj_get_xprt(kobj);
> >> + struct xprt_create xprt_create_args;
> >> + struct rpc_xprt *xprt, *new;
> >> +
> >> + if (!xprt_switch)
> >> + return 0;
> >> +
> >> + xprt = rpc_xprt_switch_get_main_xprt(xprt_switch);
> >> + if (!xprt)
> >> + goto out;
> >> +
> >> + xprt_create_args.ident = xprt->xprt_class->ident;
> >> + xprt_create_args.net = xprt->xprt_net;
> >> + xprt_create_args.dstaddr = (struct sockaddr *)&xprt->addr;
> >> + xprt_create_args.addrlen = xprt->addrlen;
> >> + xprt_create_args.servername = xprt->servername;
> >> + xprt_create_args.bc_xprt = xprt->bc_xprt;
> >> + xprt_create_args.xprtsec = xprt->xprtsec;
> >> + xprt_create_args.connect_timeout = xprt->connect_timeout;
> >> + xprt_create_args.reconnect_timeout = xprt->max_reconnect_timeout;
> >> +
> >> + new = xprt_create_transport(&xprt_create_args);
> >> + if (IS_ERR_OR_NULL(new)) {
> >> + count = PTR_ERR(new);
> >> + goto out_put_xprt;
> >> + }
> >> +
> >> + rpc_xprt_switch_add_xprt(xprt_switch, new);
> >
> > Before adding a new transport don't we need to check that we are not
> > at or over the limit of how many connections we currently have (not
> > over nconnect limit and/or over the session trunking limit)?
>
> That's a good thought, but I'm not really seeing how to do that from inside the sunrpc code. Those are configuration values for the NFS client, and don't get passed down to sunrpc, so sunrpc has no knownledge of them. If you think that'll be a problem, I can look into adding those checks for the next posting.

Seems incorrect to allow going over a limit we enforce at the nfs
layer? So I think it is a problem.

The other thing that sticks out. Given that this is version agnostic
what happens for v3/v4 and the bc_xprt?

>
> Anna
>
> >
> >> + xprt_put(new);
> >> +
> >> +out_put_xprt:
> >> + xprt_put(xprt);
> >> +out:
> >> + xprt_switch_put(xprt_switch);
> >> + return count;
> >> +}
> >> +
> >> static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
> >> struct kobj_attribute *attr,
> >> const char *buf, size_t count)
> >> @@ -451,8 +500,13 @@ ATTRIBUTE_GROUPS(rpc_sysfs_xprt);
> >> static struct kobj_attribute rpc_sysfs_xprt_switch_info =
> >> __ATTR(xprt_switch_info, 0444, rpc_sysfs_xprt_switch_info_show, NULL);
> >>
> >> +static struct kobj_attribute rpc_sysfs_xprt_switch_add_xprt =
> >> + __ATTR(xprt_switch_add_xprt, 0644, rpc_sysfs_xprt_switch_add_xprt_show,
> >> + rpc_sysfs_xprt_switch_add_xprt_store);
> >> +
> >> static struct attribute *rpc_sysfs_xprt_switch_attrs[] = {
> >> &rpc_sysfs_xprt_switch_info.attr,
> >> + &rpc_sysfs_xprt_switch_add_xprt.attr,
> >> NULL,
> >> };
> >> ATTRIBUTE_GROUPS(rpc_sysfs_xprt_switch);
> >> diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
> >> index 720d3ba742ec..a07b81ce93c3 100644
> >> --- a/net/sunrpc/xprtmultipath.c
> >> +++ b/net/sunrpc/xprtmultipath.c
> >> @@ -92,6 +92,27 @@ void rpc_xprt_switch_remove_xprt(struct rpc_xprt_switch *xps,
> >> xprt_put(xprt);
> >> }
> >>
> >> +/**
> >> + * rpc_xprt_switch_get_main_xprt - Get the 'main' xprt for an xprt switch.
> >> + * @xps: pointer to struct rpc_xprt_switch.
> >> + */
> >> +struct rpc_xprt *rpc_xprt_switch_get_main_xprt(struct rpc_xprt_switch *xps)
> >> +{
> >> + struct rpc_xprt_iter xpi;
> >> + struct rpc_xprt *xprt;
> >> +
> >> + xprt_iter_init_listall(&xpi, xps);
> >> +
> >> + xprt = xprt_iter_get_xprt(&xpi);
> >> + while (xprt && !xprt->main) {
> >> + xprt_put(xprt);
> >> + xprt = xprt_iter_get_next(&xpi);
> >> + }
> >> +
> >> + xprt_iter_destroy(&xpi);
> >> + return xprt;
> >> +}
> >> +
> >> static DEFINE_IDA(rpc_xprtswitch_ids);
> >>
> >> void xprt_multipath_cleanup_ids(void)
> >> --
> >> 2.47.1
> >>
> >>
>