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

[Anna Schumaker <anna.schumaker@oracle.com>]

On 1/9/25 10:10 AM, Benjamin Coddington wrote:
> On 8 Jan 2025, at 16:36, Anna Schumaker 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.
> 
> I like this too!  I'd prefer it was ./add_xprt instead of
> ./xprt_switch_add_xprt, since the directory already gives the context that
> you're operating on the xprt_switch.

I'd prefer that too, actually. I don't know what I was thinking going with the longer name, and I've made that change for v2.

> 
> After happily adding a bunch of xprts, I did have to look up the source to
> re-learn how to remove them which wasn't obvious from the sysfs structure.
> You have to write "offline", then "remove" to the xprt_state file.  This
> made me wish there was just a ./xprt-N-tcp/del_xprt that would do both of
> those..

`rpcctl xprt remove` will already do both of those in one step, if that helps. But I can look at adding in 'del_xprt' if you still think it would help.

> 
> .. and in stark contrast to my previous preference on less dynamic sysfs, I
> think that the ./del_xprt shouldn't appear for the "main" xprt (since you
> can't remove/delete them).
> 
> .. all just thoughts, these look good!

Thanks!
Anna

> 
>>
>> 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);
>> +	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
>