[PATCH v3 00/13] create sysfs files for changing IP address

[PATCH v3 00/13] create sysfs files for changing IP address

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

This is the same patch series that was introduced by Anna Schumaker
and slightly redone with also including some elements of the proposed
additions by Dan Alohi.

The main motivation behind is a situation where an NFS server
goes down and then comes back up with a different IP. These patches
provide a way for administrators to handle this issue by providing
a new IP address for one ore more existing transports to connect to.

Not included before is a sample output for the nconnect=2 mount:
ls /sys/kernel/sunrpc/xprt-switches/
switch-0
[root@localhost aglo]# ls /sys/kernel/sunrpc/xprt-switches/switch-0/
xprt-0-tcp  xprt-1-tcp  xprt_switch_info
[root@localhost aglo]# ls /sys/kernel/sunrpc/xprt-switches/switch-0/xprt-0-tcp/
dstaddr  xprt_info
[root@localhost aglo]# cat /sys/kernel/sunrpc/xprt-switches/switch-0/xprt-0-tcp/dstaddr 
192.168.1.68
[root@localhost aglo]# cat /sys/kernel/sunrpc/xprt-switches/switch-0/xprt-0-tcp/xprt_info 
state= CONNECTED   BOUND     
last_used=4294830258
cur_cong=0
cong_win=256
max_num_slots=65536
min_num_slots=2
num_reqs=2
binding_q_len=0
sending_q_len=0
pending_q_len=0
backlog_q_len=0
[root@localhost aglo]# cat /sys/kernel/sunrpc/xprt-switches/switch-0/xprt
xprt-0-tcp/       xprt-1-tcp/       xprt_switch_info  
[root@localhost aglo]# cat /sys/kernel/sunrpc/xprt-switches/switch-0/xprt_switch_info 
num_xprts=2
num_active=2
queue_len=0
[root@localhost aglo]# ls /sys/kernel/sunrpc/rpc-clients/
clnt-0  clnt-1
[root@localhost aglo]# ls /sys/kernel/sunrpc/rpc-clients/clnt-0
switch-0
[root@localhost aglo]# ls /sys/kernel/sunrpc/rpc-clients/clnt-1
switch-0

v3: 
--addressed the memory allocations. Patches 6,8,10 were modified
to pass in gfp_t flags into the functions. 
-- To address the allocation with the locked environment problem 
the call to rpc_sysfs_xprt_switch_xprt_setup() was removed from the
xprt_switch_add_xprt_locked() and called after but also I bumped the
refcount on xprt_switch and xprt structures being used.
-- changed patch13 to remove rcu_dereference() use in 
rpc_sysfs_xprt_switch_kobj_get_xprt() because it's not an "__rcu"
field. 

Anna Schumaker (4):
  sunrpc: Prepare xs_connect() for taking NULL tasks
  sunrpc: Create a sunrpc directory under /sys/kernel/
  sunrpc: Create a client/ subdirectory in the sunrpc sysfs
  sunrpc: Create per-rpc_clnt sysfs kobjects

Dan Aloni (2):
  sunrpc: add xprt id
  sunrpc: add IDs to multipath

Olga Kornievskaia (7):
  sunrpc: keep track of the xprt_class in rpc_xprt structure
  sunrpc: add xprt_switch direcotry to sunrpc's sysfs
  sunrpc: add a symlink from rpc-client directory to the xprt_switch
  sunrpc: add add sysfs directory per xprt under each xprt_switch
  sunrpc: add dst_attr attributes to the sysfs xprt directory
  sunrpc: provide transport info in the sysfs directory
  sunrpc: provide multipath info in the sysfs directory

 include/linux/sunrpc/clnt.h          |   1 +
 include/linux/sunrpc/xprt.h          |   5 +
 include/linux/sunrpc/xprtmultipath.h |   5 +
 net/sunrpc/Makefile                  |   2 +-
 net/sunrpc/clnt.c                    |   5 +
 net/sunrpc/sunrpc_syms.c             |  10 +
 net/sunrpc/sysfs.c                   | 481 +++++++++++++++++++++++++++
 net/sunrpc/sysfs.h                   |  42 +++
 net/sunrpc/xprt.c                    |  26 ++
 net/sunrpc/xprtmultipath.c           |  40 +++
 net/sunrpc/xprtrdma/transport.c      |   2 +
 net/sunrpc/xprtsock.c                |  11 +-
 12 files changed, 628 insertions(+), 2 deletions(-)
 create mode 100644 net/sunrpc/sysfs.c
 create mode 100644 net/sunrpc/sysfs.h

-- 
2.27.0

[PATCH v3 01/13] sunrpc: Create a sunrpc directory under /sys/kernel/

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

This is where we'll put per-rpc_client related files

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/Makefile      |  2 +-
 net/sunrpc/sunrpc_syms.c |  8 ++++++++
 net/sunrpc/sysfs.c       | 20 ++++++++++++++++++++
 net/sunrpc/sysfs.h       | 11 +++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 net/sunrpc/sysfs.c
 create mode 100644 net/sunrpc/sysfs.h

diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
index 9488600451e8..1c8de397d6ad 100644
--- a/net/sunrpc/Makefile
+++ b/net/sunrpc/Makefile
@@ -12,7 +12,7 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
 	    auth.o auth_null.o auth_unix.o \
 	    svc.o svcsock.o svcauth.o svcauth_unix.o \
 	    addr.o rpcb_clnt.o timer.o xdr.o \
-	    sunrpc_syms.o cache.o rpc_pipe.o \
+	    sunrpc_syms.o cache.o rpc_pipe.o sysfs.o \
 	    svc_xprt.o \
 	    xprtmultipath.o
 sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 236fadc4a439..3b57efc692ec 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -24,6 +24,7 @@
 #include <linux/sunrpc/xprtsock.h>
 
 #include "sunrpc.h"
+#include "sysfs.h"
 #include "netns.h"
 
 unsigned int sunrpc_net_id;
@@ -103,6 +104,10 @@ init_sunrpc(void)
 	if (err)
 		goto out4;
 
+	err = rpc_sysfs_init();
+	if (err)
+		goto out5;
+
 	sunrpc_debugfs_init();
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 	rpc_register_sysctl();
@@ -111,6 +116,8 @@ init_sunrpc(void)
 	init_socket_xprt();	/* clnt sock transport */
 	return 0;
 
+out5:
+	unregister_rpc_pipefs();
 out4:
 	unregister_pernet_subsys(&sunrpc_net_ops);
 out3:
@@ -124,6 +131,7 @@ init_sunrpc(void)
 static void __exit
 cleanup_sunrpc(void)
 {
+	rpc_sysfs_exit();
 	rpc_cleanup_clids();
 	rpcauth_remove_module();
 	cleanup_socket_xprt();
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
new file mode 100644
index 000000000000..27eda180ac5e
--- /dev/null
+++ b/net/sunrpc/sysfs.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Anna Schumaker <Anna.Schumaker@Netapp.com>
+ */
+#include <linux/kobject.h>
+
+static struct kset *rpc_sunrpc_kset;
+
+int rpc_sysfs_init(void)
+{
+	rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
+	if (!rpc_sunrpc_kset)
+		return -ENOMEM;
+	return 0;
+}
+
+void rpc_sysfs_exit(void)
+{
+	kset_unregister(rpc_sunrpc_kset);
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
new file mode 100644
index 000000000000..f181c650aab8
--- /dev/null
+++ b/net/sunrpc/sysfs.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Anna Schumaker <Anna.Schumaker@Netapp.com>
+ */
+#ifndef __SUNRPC_SYSFS_H
+#define __SUNRPC_SYSFS_H
+
+int rpc_sysfs_init(void);
+void rpc_sysfs_exit(void);
+
+#endif
-- 
2.27.0

[PATCH v3 02/13] sunrpc: Create a client/ subdirectory in the sunrpc sysfs

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

For network namespace separation.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/sysfs.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 27eda180ac5e..6be3f4cfac95 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -2,19 +2,61 @@
 /*
  * Copyright (c) 2020 Anna Schumaker <Anna.Schumaker@Netapp.com>
  */
+#include <linux/sunrpc/clnt.h>
 #include <linux/kobject.h>
 
 static struct kset *rpc_sunrpc_kset;
+static struct kobject *rpc_sunrpc_client_kobj;
+
+static void rpc_sysfs_object_release(struct kobject *kobj)
+{
+	kfree(kobj);
+}
+
+static const struct kobj_ns_type_operations *
+rpc_sysfs_object_child_ns_type(struct kobject *kobj)
+{
+	return &net_ns_type_operations;
+}
+
+static struct kobj_type rpc_sysfs_object_type = {
+	.release = rpc_sysfs_object_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.child_ns_type = rpc_sysfs_object_child_ns_type,
+};
+
+static struct kobject *rpc_sysfs_object_alloc(const char *name,
+		struct kset *kset, struct kobject *parent)
+{
+	struct kobject *kobj;
+
+	kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
+	if (kobj) {
+		kobj->kset = kset;
+		if (kobject_init_and_add(kobj, &rpc_sysfs_object_type,
+					 parent, "%s", name) == 0)
+			return kobj;
+		kobject_put(kobj);
+	}
+	return NULL;
+}
 
 int rpc_sysfs_init(void)
 {
 	rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
 	if (!rpc_sunrpc_kset)
 		return -ENOMEM;
+	rpc_sunrpc_client_kobj = rpc_sysfs_object_alloc("client", rpc_sunrpc_kset, NULL);
+	if (!rpc_sunrpc_client_kobj) {
+		kset_unregister(rpc_sunrpc_kset);
+		rpc_sunrpc_client_kobj = NULL;
+		return -ENOMEM;
+	}
 	return 0;
 }
 
 void rpc_sysfs_exit(void)
 {
+	kobject_put(rpc_sunrpc_client_kobj);
 	kset_unregister(rpc_sunrpc_kset);
 }
-- 
2.27.0

[PATCH v3 03/13] sunrpc: Create per-rpc_clnt sysfs kobjects

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

These will eventually have files placed under them for sysfs operations.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/clnt.h |  1 +
 net/sunrpc/clnt.c           |  5 +++
 net/sunrpc/sysfs.c          | 61 +++++++++++++++++++++++++++++++++++++
 net/sunrpc/sysfs.h          |  8 +++++
 4 files changed, 75 insertions(+)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 02e7a5863d28..503653720e18 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -71,6 +71,7 @@ struct rpc_clnt {
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 	struct dentry		*cl_debugfs;	/* debugfs directory */
 #endif
+	void			*cl_sysfs;	/* sysfs directory */
 	/* cl_work is only needed after cl_xpi is no longer used,
 	 * and that are of similar size
 	 */
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index c2a01125be1a..dab1abfef5cd 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -41,6 +41,7 @@
 #include <trace/events/sunrpc.h>
 
 #include "sunrpc.h"
+#include "sysfs.h"
 #include "netns.h"
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
@@ -300,6 +301,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
 	int err;
 
 	rpc_clnt_debugfs_register(clnt);
+	rpc_sysfs_client_setup(clnt, net);
 
 	pipefs_sb = rpc_get_sb_net(net);
 	if (pipefs_sb) {
@@ -327,6 +329,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
 out:
 	if (pipefs_sb)
 		rpc_put_sb_net(net);
+	rpc_sysfs_client_destroy(clnt);
 	rpc_clnt_debugfs_unregister(clnt);
 	return err;
 }
@@ -733,6 +736,7 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,
 
 	rpc_unregister_client(clnt);
 	__rpc_clnt_remove_pipedir(clnt);
+	rpc_sysfs_client_destroy(clnt);
 	rpc_clnt_debugfs_unregister(clnt);
 
 	/*
@@ -879,6 +883,7 @@ static void rpc_free_client_work(struct work_struct *work)
 	 * so they cannot be called in rpciod, so they are handled separately
 	 * here.
 	 */
+	rpc_sysfs_client_destroy(clnt);
 	rpc_clnt_debugfs_unregister(clnt);
 	rpc_free_clid(clnt);
 	rpc_clnt_remove_pipedir(clnt);
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 6be3f4cfac95..d14d54f33c65 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -4,6 +4,7 @@
  */
 #include <linux/sunrpc/clnt.h>
 #include <linux/kobject.h>
+#include "sysfs.h"
 
 static struct kset *rpc_sunrpc_kset;
 static struct kobject *rpc_sunrpc_client_kobj;
@@ -55,8 +56,68 @@ int rpc_sysfs_init(void)
 	return 0;
 }
 
+static void rpc_sysfs_client_release(struct kobject *kobj)
+{
+	struct rpc_sysfs_client *c;
+
+	c = container_of(kobj, struct rpc_sysfs_client, kobject);
+	kfree(c);
+}
+
+static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
+{
+	return container_of(kobj, struct rpc_sysfs_client, kobject)->net;
+}
+
+static struct kobj_type rpc_sysfs_client_type = {
+	.release = rpc_sysfs_client_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.namespace = rpc_sysfs_client_namespace,
+};
+
 void rpc_sysfs_exit(void)
 {
 	kobject_put(rpc_sunrpc_client_kobj);
 	kset_unregister(rpc_sunrpc_kset);
 }
+
+static struct rpc_sysfs_client *rpc_sysfs_client_alloc(struct kobject *parent,
+						       struct net *net,
+						       int clid)
+{
+	struct rpc_sysfs_client *p;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (p) {
+		p->net = net;
+		p->kobject.kset = rpc_sunrpc_kset;
+		if (kobject_init_and_add(&p->kobject, &rpc_sysfs_client_type,
+					 parent, "clnt-%d", clid) == 0)
+			return p;
+		kobject_put(&p->kobject);
+	}
+	return NULL;
+}
+
+void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net)
+{
+	struct rpc_sysfs_client *rpc_client;
+
+	rpc_client = rpc_sysfs_client_alloc(rpc_sunrpc_client_kobj, net, clnt->cl_clid);
+	if (rpc_client) {
+		clnt->cl_sysfs = rpc_client;
+		kobject_uevent(&rpc_client->kobject, KOBJ_ADD);
+	}
+}
+
+void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
+{
+	struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
+
+	if (rpc_client) {
+		kobject_uevent(&rpc_client->kobject, KOBJ_REMOVE);
+		kobject_del(&rpc_client->kobject);
+		kobject_put(&rpc_client->kobject);
+		clnt->cl_sysfs = NULL;
+	}
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index f181c650aab8..c46afc848993 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -5,7 +5,15 @@
 #ifndef __SUNRPC_SYSFS_H
 #define __SUNRPC_SYSFS_H
 
+struct rpc_sysfs_client {
+	struct kobject kobject;
+	struct net *net;
+};
+
 int rpc_sysfs_init(void);
 void rpc_sysfs_exit(void);
 
+void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net);
+void rpc_sysfs_client_destroy(struct rpc_clnt *clnt);
+
 #endif
-- 
2.27.0

[PATCH v3 04/13] sunrpc: Prepare xs_connect() for taking NULL tasks

[Olga Kornievskaia]

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

We won't have a task structure when we go to change IP addresses, so
check for one before calling the WARN_ON() to avoid crashing.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/xprtsock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 47aa47a2b07c..2bcb80c19339 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2317,7 +2317,7 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
 	unsigned long delay = 0;
 
-	WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));
+	WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport));
 
 	if (transport->sock != NULL) {
 		dprintk("RPC:       xs_connect delayed xprt %p for %lu "
-- 
2.27.0

[PATCH v3 05/13] sunrpc: add xprt id

[Olga Kornievskaia]

From: Dan Aloni <dan@kernelim.com>

This adds a unique identifier for a sunrpc transport in sysfs, which is
similarly managed to the unique IDs of clients.

Signed-off-by: Dan Aloni <dan@kernelim.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/xprt.h |  2 ++
 net/sunrpc/sunrpc_syms.c    |  1 +
 net/sunrpc/xprt.c           | 26 ++++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index d81fe8b364d0..82294d06075c 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -185,6 +185,7 @@ enum xprt_transports {
 struct rpc_xprt {
 	struct kref		kref;		/* Reference count */
 	const struct rpc_xprt_ops *ops;		/* transport methods */
+	unsigned int		id;		/* transport id */
 
 	const struct rpc_timeout *timeout;	/* timeout parms */
 	struct sockaddr_storage	addr;		/* server address */
@@ -368,6 +369,7 @@ struct rpc_xprt *	xprt_alloc(struct net *net, size_t size,
 				unsigned int num_prealloc,
 				unsigned int max_req);
 void			xprt_free(struct rpc_xprt *);
+void			xprt_cleanup_ids(void);
 
 static inline int
 xprt_enable_swap(struct rpc_xprt *xprt)
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 3b57efc692ec..b61b74c00483 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -133,6 +133,7 @@ cleanup_sunrpc(void)
 {
 	rpc_sysfs_exit();
 	rpc_cleanup_clids();
+	xprt_cleanup_ids();
 	rpcauth_remove_module();
 	cleanup_socket_xprt();
 	svc_cleanup_xprt_sock();
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index e5b5a960a69b..fd58a3a16add 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1722,6 +1722,30 @@ static void xprt_free_all_slots(struct rpc_xprt *xprt)
 	}
 }
 
+static DEFINE_IDA(rpc_xprt_ids);
+
+void xprt_cleanup_ids(void)
+{
+	ida_destroy(&rpc_xprt_ids);
+}
+
+static int xprt_alloc_id(struct rpc_xprt *xprt)
+{
+	int id;
+
+	id = ida_simple_get(&rpc_xprt_ids, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	xprt->id = id;
+	return 0;
+}
+
+static void xprt_free_id(struct rpc_xprt *xprt)
+{
+	ida_simple_remove(&rpc_xprt_ids, xprt->id);
+}
+
 struct rpc_xprt *xprt_alloc(struct net *net, size_t size,
 		unsigned int num_prealloc,
 		unsigned int max_alloc)
@@ -1734,6 +1758,7 @@ struct rpc_xprt *xprt_alloc(struct net *net, size_t size,
 	if (xprt == NULL)
 		goto out;
 
+	xprt_alloc_id(xprt);
 	xprt_init(xprt, net);
 
 	for (i = 0; i < num_prealloc; i++) {
@@ -1762,6 +1787,7 @@ void xprt_free(struct rpc_xprt *xprt)
 {
 	put_net(xprt->xprt_net);
 	xprt_free_all_slots(xprt);
+	xprt_free_id(xprt);
 	kfree_rcu(xprt, rcu);
 }
 EXPORT_SYMBOL_GPL(xprt_free);
-- 
2.27.0

[PATCH v3 06/13] sunrpc: add IDs to multipath

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

This is used to uniquely identify sunrpc multipath objects in /sys.

Signed-off-by: Dan Aloni <dan@kernelim.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/xprtmultipath.h |  4 ++++
 net/sunrpc/sunrpc_syms.c             |  1 +
 net/sunrpc/xprtmultipath.c           | 26 ++++++++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/include/linux/sunrpc/xprtmultipath.h b/include/linux/sunrpc/xprtmultipath.h
index c6cce3fbf29d..ef95a6f18ccf 100644
--- a/include/linux/sunrpc/xprtmultipath.h
+++ b/include/linux/sunrpc/xprtmultipath.h
@@ -14,6 +14,7 @@ struct rpc_xprt_switch {
 	spinlock_t		xps_lock;
 	struct kref		xps_kref;
 
+	unsigned int		xps_id;
 	unsigned int		xps_nxprts;
 	unsigned int		xps_nactive;
 	atomic_long_t		xps_queuelen;
@@ -71,4 +72,7 @@ extern struct rpc_xprt *xprt_iter_get_next(struct rpc_xprt_iter *xpi);
 
 extern bool rpc_xprt_switch_has_addr(struct rpc_xprt_switch *xps,
 		const struct sockaddr *sap);
+
+extern void xprt_multipath_cleanup_ids(void);
+
 #endif
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index b61b74c00483..691c0000e9ea 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -134,6 +134,7 @@ cleanup_sunrpc(void)
 	rpc_sysfs_exit();
 	rpc_cleanup_clids();
 	xprt_cleanup_ids();
+	xprt_multipath_cleanup_ids();
 	rpcauth_remove_module();
 	cleanup_socket_xprt();
 	svc_cleanup_xprt_sock();
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 78c075a68c04..4969a4c216f7 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -86,6 +86,30 @@ void rpc_xprt_switch_remove_xprt(struct rpc_xprt_switch *xps,
 	xprt_put(xprt);
 }
 
+static DEFINE_IDA(rpc_xprtswitch_ids);
+
+void xprt_multipath_cleanup_ids(void)
+{
+	ida_destroy(&rpc_xprtswitch_ids);
+}
+
+static int xprt_switch_alloc_id(struct rpc_xprt_switch *xps, gfp_t gfp_flags)
+{
+	int id;
+
+	id = ida_simple_get(&rpc_xprtswitch_ids, 0, 0, gfp_flags);
+	if (id < 0)
+		return id;
+
+	xps->xps_id = id;
+	return 0;
+}
+
+static void xprt_switch_free_id(struct rpc_xprt_switch *xps)
+{
+	ida_simple_remove(&rpc_xprtswitch_ids, xps->xps_id);
+}
+
 /**
  * xprt_switch_alloc - Allocate a new struct rpc_xprt_switch
  * @xprt: pointer to struct rpc_xprt
@@ -103,6 +127,7 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct rpc_xprt *xprt,
 	if (xps != NULL) {
 		spin_lock_init(&xps->xps_lock);
 		kref_init(&xps->xps_kref);
+		xprt_switch_alloc_id(xps, gfp_flags);
 		xps->xps_nxprts = xps->xps_nactive = 0;
 		atomic_long_set(&xps->xps_queuelen, 0);
 		xps->xps_net = NULL;
@@ -136,6 +161,7 @@ static void xprt_switch_free(struct kref *kref)
 			struct rpc_xprt_switch, xps_kref);
 
 	xprt_switch_free_entries(xps);
+	xprt_switch_free_id(xps);
 	kfree_rcu(xps, xps_rcu);
 }
 
-- 
2.27.0

[PATCH v3 07/13] sunrpc: keep track of the xprt_class in rpc_xprt structure

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

We need to keep track of the type for a given transport.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/xprt.h     | 2 ++
 net/sunrpc/xprtrdma/transport.c | 2 ++
 net/sunrpc/xprtsock.c           | 9 +++++++++
 3 files changed, 13 insertions(+)

diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 82294d06075c..a2edcc42e6c4 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -53,6 +53,7 @@ enum rpc_display_format_t {
 
 struct rpc_task;
 struct rpc_xprt;
+struct xprt_class;
 struct seq_file;
 struct svc_serv;
 struct net;
@@ -289,6 +290,7 @@ struct rpc_xprt {
 	atomic_t		inject_disconnect;
 #endif
 	struct rcu_head		rcu;
+	const struct xprt_class	*xprt_class;
 };
 
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
index 09953597d055..71500eb89bff 100644
--- a/net/sunrpc/xprtrdma/transport.c
+++ b/net/sunrpc/xprtrdma/transport.c
@@ -73,6 +73,7 @@ unsigned int xprt_rdma_max_inline_read = RPCRDMA_DEF_INLINE;
 unsigned int xprt_rdma_max_inline_write = RPCRDMA_DEF_INLINE;
 unsigned int xprt_rdma_memreg_strategy		= RPCRDMA_FRWR;
 int xprt_rdma_pad_optimize;
+static struct xprt_class xprt_rdma;
 
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 
@@ -349,6 +350,7 @@ xprt_setup_rdma(struct xprt_create *args)
 	/* Ensure xprt->addr holds valid server TCP (not RDMA)
 	 * address, for any side protocols which peek at it */
 	xprt->prot = IPPROTO_TCP;
+	xprt->xprt_class = &xprt_rdma;
 	xprt->addrlen = args->addrlen;
 	memcpy(&xprt->addr, sap, xprt->addrlen);
 
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 2bcb80c19339..5ff37badd335 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -91,6 +91,11 @@ static unsigned int xprt_max_resvport_limit = RPC_MAX_RESVPORT;
 
 static struct ctl_table_header *sunrpc_table_header;
 
+static struct xprt_class xs_local_transport;
+static struct xprt_class xs_udp_transport;
+static struct xprt_class xs_tcp_transport;
+static struct xprt_class xs_bc_tcp_transport;
+
 /*
  * FIXME: changing the UDP slot table size should also resize the UDP
  *        socket buffers for existing UDP transports
@@ -2777,6 +2782,7 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
 	transport = container_of(xprt, struct sock_xprt, xprt);
 
 	xprt->prot = 0;
+	xprt->xprt_class = &xs_local_transport;
 	xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
 
 	xprt->bind_timeout = XS_BIND_TO;
@@ -2846,6 +2852,7 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
 	transport = container_of(xprt, struct sock_xprt, xprt);
 
 	xprt->prot = IPPROTO_UDP;
+	xprt->xprt_class = &xs_udp_transport;
 	/* XXX: header size can vary due to auth type, IPv6, etc. */
 	xprt->max_payload = (1U << 16) - (MAX_HEADER << 3);
 
@@ -2926,6 +2933,7 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
 	transport = container_of(xprt, struct sock_xprt, xprt);
 
 	xprt->prot = IPPROTO_TCP;
+	xprt->xprt_class = &xs_tcp_transport;
 	xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
 
 	xprt->bind_timeout = XS_BIND_TO;
@@ -2999,6 +3007,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	transport = container_of(xprt, struct sock_xprt, xprt);
 
 	xprt->prot = IPPROTO_TCP;
+	xprt->xprt_class = &xs_bc_tcp_transport;
 	xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
 	xprt->timeout = &xs_tcp_default_timeout;
 
-- 
2.27.0

[PATCH v3 08/13] sunrpc: add xprt_switch direcotry to sunrpc's sysfs

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Add xprt_switch directory to the sysfs and create individual
xprt_swith subdirectories for multipath transport group.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/xprtmultipath.h |  1 +
 net/sunrpc/sysfs.c                   | 99 ++++++++++++++++++++++++++--
 net/sunrpc/sysfs.h                   | 10 +++
 net/sunrpc/xprtmultipath.c           |  4 ++
 4 files changed, 107 insertions(+), 7 deletions(-)

diff --git a/include/linux/sunrpc/xprtmultipath.h b/include/linux/sunrpc/xprtmultipath.h
index ef95a6f18ccf..47b0a85cdcfa 100644
--- a/include/linux/sunrpc/xprtmultipath.h
+++ b/include/linux/sunrpc/xprtmultipath.h
@@ -24,6 +24,7 @@ struct rpc_xprt_switch {
 
 	const struct rpc_xprt_iter_ops *xps_iter_ops;
 
+	void			*xps_sysfs;
 	struct rcu_head		xps_rcu;
 };
 
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index d14d54f33c65..03ea6d3ace95 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -7,7 +7,7 @@
 #include "sysfs.h"
 
 static struct kset *rpc_sunrpc_kset;
-static struct kobject *rpc_sunrpc_client_kobj;
+static struct kobject *rpc_sunrpc_client_kobj, *rpc_sunrpc_xprt_switch_kobj;
 
 static void rpc_sysfs_object_release(struct kobject *kobj)
 {
@@ -47,13 +47,22 @@ int rpc_sysfs_init(void)
 	rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
 	if (!rpc_sunrpc_kset)
 		return -ENOMEM;
-	rpc_sunrpc_client_kobj = rpc_sysfs_object_alloc("client", rpc_sunrpc_kset, NULL);
-	if (!rpc_sunrpc_client_kobj) {
-		kset_unregister(rpc_sunrpc_kset);
-		rpc_sunrpc_client_kobj = NULL;
-		return -ENOMEM;
-	}
+	rpc_sunrpc_client_kobj =
+		rpc_sysfs_object_alloc("rpc-clients", rpc_sunrpc_kset, NULL);
+	if (!rpc_sunrpc_client_kobj)
+		goto err_client;
+	rpc_sunrpc_xprt_switch_kobj =
+		rpc_sysfs_object_alloc("xprt-switches", rpc_sunrpc_kset, NULL);
+	if (!rpc_sunrpc_xprt_switch_kobj)
+		goto err_switch;
 	return 0;
+err_switch:
+	kobject_put(rpc_sunrpc_client_kobj);
+	rpc_sunrpc_client_kobj = NULL;
+err_client:
+	kset_unregister(rpc_sunrpc_kset);
+	rpc_sunrpc_kset = NULL;
+	return -ENOMEM;
 }
 
 static void rpc_sysfs_client_release(struct kobject *kobj)
@@ -64,20 +73,40 @@ static void rpc_sysfs_client_release(struct kobject *kobj)
 	kfree(c);
 }
 
+static void rpc_sysfs_xprt_switch_release(struct kobject *kobj)
+{
+	struct rpc_sysfs_xprt_switch *xprt_switch;
+
+	xprt_switch = container_of(kobj, struct rpc_sysfs_xprt_switch, kobject);
+	kfree(xprt_switch);
+}
+
 static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
 {
 	return container_of(kobj, struct rpc_sysfs_client, kobject)->net;
 }
 
+static const void *rpc_sysfs_xprt_switch_namespace(struct kobject *kobj)
+{
+	return container_of(kobj, struct rpc_sysfs_xprt_switch, kobject)->net;
+}
+
 static struct kobj_type rpc_sysfs_client_type = {
 	.release = rpc_sysfs_client_release,
 	.sysfs_ops = &kobj_sysfs_ops,
 	.namespace = rpc_sysfs_client_namespace,
 };
 
+static struct kobj_type rpc_sysfs_xprt_switch_type = {
+	.release = rpc_sysfs_xprt_switch_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.namespace = rpc_sysfs_xprt_switch_namespace,
+};
+
 void rpc_sysfs_exit(void)
 {
 	kobject_put(rpc_sunrpc_client_kobj);
+	kobject_put(rpc_sunrpc_xprt_switch_kobj);
 	kset_unregister(rpc_sunrpc_kset);
 }
 
@@ -99,6 +128,28 @@ static struct rpc_sysfs_client *rpc_sysfs_client_alloc(struct kobject *parent,
 	return NULL;
 }
 
+static struct rpc_sysfs_xprt_switch *
+rpc_sysfs_xprt_switch_alloc(struct kobject *parent,
+			    struct rpc_xprt_switch *xprt_switch,
+			    struct net *net,
+			    gfp_t gfp_flags)
+{
+	struct rpc_sysfs_xprt_switch *p;
+
+	p = kzalloc(sizeof(*p), gfp_flags);
+	if (p) {
+		p->net = net;
+		p->kobject.kset = rpc_sunrpc_kset;
+		if (kobject_init_and_add(&p->kobject,
+					 &rpc_sysfs_xprt_switch_type,
+					 parent, "switch-%d",
+					 xprt_switch->xps_id) == 0)
+			return p;
+		kobject_put(&p->kobject);
+	}
+	return NULL;
+}
+
 void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net)
 {
 	struct rpc_sysfs_client *rpc_client;
@@ -110,6 +161,28 @@ void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net)
 	}
 }
 
+void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch *xprt_switch,
+				 struct rpc_xprt *xprt,
+				 gfp_t gfp_flags)
+{
+	struct rpc_sysfs_xprt_switch *rpc_xprt_switch;
+	struct net *net;
+
+	if (xprt_switch->xps_net)
+		net = xprt_switch->xps_net;
+	else
+		net = xprt->xprt_net;
+	rpc_xprt_switch =
+		rpc_sysfs_xprt_switch_alloc(rpc_sunrpc_xprt_switch_kobj,
+					    xprt_switch, net, gfp_flags);
+	if (rpc_xprt_switch) {
+		xprt_switch->xps_sysfs = rpc_xprt_switch;
+		rpc_xprt_switch->xprt_switch = xprt_switch;
+		rpc_xprt_switch->xprt = xprt;
+		kobject_uevent(&rpc_xprt_switch->kobject, KOBJ_ADD);
+	}
+}
+
 void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
 {
 	struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
@@ -121,3 +194,15 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
 		clnt->cl_sysfs = NULL;
 	}
 }
+
+void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt_switch)
+{
+	struct rpc_sysfs_xprt_switch *rpc_xprt_switch = xprt_switch->xps_sysfs;
+
+	if (rpc_xprt_switch) {
+		kobject_uevent(&rpc_xprt_switch->kobject, KOBJ_REMOVE);
+		kobject_del(&rpc_xprt_switch->kobject);
+		kobject_put(&rpc_xprt_switch->kobject);
+		xprt_switch->xps_sysfs = NULL;
+	}
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index c46afc848993..52ec472bd4a9 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -10,10 +10,20 @@ struct rpc_sysfs_client {
 	struct net *net;
 };
 
+struct rpc_sysfs_xprt_switch {
+	struct kobject kobject;
+	struct net *net;
+	struct rpc_xprt_switch *xprt_switch;
+	struct rpc_xprt *xprt;
+};
+
 int rpc_sysfs_init(void);
 void rpc_sysfs_exit(void);
 
 void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net);
 void rpc_sysfs_client_destroy(struct rpc_clnt *clnt);
+void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch *xprt_switch,
+				 struct rpc_xprt *xprt, gfp_t gfp_flags);
+void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt);
 
 #endif
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 4969a4c216f7..2d73a35df9ee 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -19,6 +19,8 @@
 #include <linux/sunrpc/addr.h>
 #include <linux/sunrpc/xprtmultipath.h>
 
+#include "sysfs.h"
+
 typedef struct rpc_xprt *(*xprt_switch_find_xprt_t)(struct rpc_xprt_switch *xps,
 		const struct rpc_xprt *cur);
 
@@ -133,6 +135,7 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct rpc_xprt *xprt,
 		xps->xps_net = NULL;
 		INIT_LIST_HEAD(&xps->xps_xprt_list);
 		xps->xps_iter_ops = &rpc_xprt_iter_singular;
+		rpc_sysfs_xprt_switch_setup(xps, xprt, gfp_flags);
 		xprt_switch_add_xprt_locked(xps, xprt);
 	}
 
@@ -161,6 +164,7 @@ static void xprt_switch_free(struct kref *kref)
 			struct rpc_xprt_switch, xps_kref);
 
 	xprt_switch_free_entries(xps);
+	rpc_sysfs_xprt_switch_destroy(xps);
 	xprt_switch_free_id(xps);
 	kfree_rcu(xps, xps_rcu);
 }
-- 
2.27.0

[PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

An rpc client uses a transport switch and one ore more transports
associated with that switch. Since transports are shared among
rpc clients, create a symlink into the xprt_switch directory
instead of duplicating entries under each rpc client.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/clnt.c  |  2 +-
 net/sunrpc/sysfs.c | 25 +++++++++++++++++++++++--
 net/sunrpc/sysfs.h |  6 +++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index dab1abfef5cd..2e195623c10d 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -301,7 +301,6 @@ static int rpc_client_register(struct rpc_clnt *clnt,
 	int err;
 
 	rpc_clnt_debugfs_register(clnt);
-	rpc_sysfs_client_setup(clnt, net);
 
 	pipefs_sb = rpc_get_sb_net(net);
 	if (pipefs_sb) {
@@ -426,6 +425,7 @@ static struct rpc_clnt * rpc_new_client(const struct rpc_create_args *args,
 	/* save the nodename */
 	rpc_clnt_set_nodename(clnt, nodename);
 
+	rpc_sysfs_client_setup(clnt, xps, rpc_net_ns(clnt));
 	err = rpc_client_register(clnt, args->authflavor, args->client_name);
 	if (err)
 		goto out_no_path;
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 03ea6d3ace95..871f7c696a93 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -150,14 +150,30 @@ rpc_sysfs_xprt_switch_alloc(struct kobject *parent,
 	return NULL;
 }
 
-void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net)
+void rpc_sysfs_client_setup(struct rpc_clnt *clnt,
+			    struct rpc_xprt_switch *xprt_switch,
+			    struct net *net)
 {
 	struct rpc_sysfs_client *rpc_client;
 
-	rpc_client = rpc_sysfs_client_alloc(rpc_sunrpc_client_kobj, net, clnt->cl_clid);
+	rpc_client = rpc_sysfs_client_alloc(rpc_sunrpc_client_kobj,
+					    net, clnt->cl_clid);
 	if (rpc_client) {
+		char name[23];
+		struct rpc_sysfs_xprt_switch *xswitch =
+			(struct rpc_sysfs_xprt_switch *)xprt_switch->xps_sysfs;
+		int ret;
+
 		clnt->cl_sysfs = rpc_client;
+		rpc_client->clnt = clnt;
+		rpc_client->xprt_switch = xprt_switch;
 		kobject_uevent(&rpc_client->kobject, KOBJ_ADD);
+		snprintf(name, sizeof(name), "switch-%d", xprt_switch->xps_id);
+		ret = sysfs_create_link_nowarn(&rpc_client->kobject,
+					       &xswitch->kobject, name);
+		if (ret)
+			pr_warn("can't create link to %s in sysfs (%d)\n",
+				name, ret);
 	}
 }
 
@@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
 	struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
 
 	if (rpc_client) {
+		char name[23];
+
+		snprintf(name, sizeof(name), "switch-%d",
+			 rpc_client->xprt_switch->xps_id);
+		sysfs_remove_link(&rpc_client->kobject, name);
 		kobject_uevent(&rpc_client->kobject, KOBJ_REMOVE);
 		kobject_del(&rpc_client->kobject);
 		kobject_put(&rpc_client->kobject);
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index 52ec472bd4a9..760f0582aee5 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -8,6 +8,8 @@
 struct rpc_sysfs_client {
 	struct kobject kobject;
 	struct net *net;
+	struct rpc_clnt *clnt;
+	struct rpc_xprt_switch *xprt_switch;
 };
 
 struct rpc_sysfs_xprt_switch {
@@ -20,7 +22,9 @@ struct rpc_sysfs_xprt_switch {
 int rpc_sysfs_init(void);
 void rpc_sysfs_exit(void);
 
-void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net);
+void rpc_sysfs_client_setup(struct rpc_clnt *clnt,
+			    struct rpc_xprt_switch *xprt_switch,
+			    struct net *net);
 void rpc_sysfs_client_destroy(struct rpc_clnt *clnt);
 void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch *xprt_switch,
 				 struct rpc_xprt *xprt, gfp_t gfp_flags);
-- 
2.27.0

[PATCH v3 10/13] sunrpc: add add sysfs directory per xprt under each xprt_switch

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Add individual transport directories under each transport switch
group. For instance, for each nconnect=X connections there will be
a transport directory. Naming conventions also identifies transport
type -- xprt-<id>-<type> where type is udp, tcp, rdma, local, bc.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 include/linux/sunrpc/xprt.h |  1 +
 net/sunrpc/sysfs.c          | 86 +++++++++++++++++++++++++++++++++++++
 net/sunrpc/sysfs.h          |  9 ++++
 net/sunrpc/xprtmultipath.c  | 10 +++++
 4 files changed, 106 insertions(+)

diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index a2edcc42e6c4..1e4906759a6a 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -291,6 +291,7 @@ struct rpc_xprt {
 #endif
 	struct rcu_head		rcu;
 	const struct xprt_class	*xprt_class;
+	void			*xprt_sysfs;
 };
 
 #if defined(CONFIG_SUNRPC_BACKCHANNEL)
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 871f7c696a93..682def4293f2 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -81,6 +81,14 @@ static void rpc_sysfs_xprt_switch_release(struct kobject *kobj)
 	kfree(xprt_switch);
 }
 
+static void rpc_sysfs_xprt_switch_xprt_release(struct kobject *kobj)
+{
+	struct rpc_sysfs_xprt_switch_xprt *xprt;
+
+	xprt = container_of(kobj, struct rpc_sysfs_xprt_switch_xprt, kobject);
+	kfree(xprt);
+}
+
 static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
 {
 	return container_of(kobj, struct rpc_sysfs_client, kobject)->net;
@@ -91,6 +99,12 @@ static const void *rpc_sysfs_xprt_switch_namespace(struct kobject *kobj)
 	return container_of(kobj, struct rpc_sysfs_xprt_switch, kobject)->net;
 }
 
+static const void *rpc_sysfs_xprt_switch_xprt_namespace(struct kobject *kobj)
+{
+	return container_of(kobj, struct rpc_sysfs_xprt_switch_xprt,
+			    kobject)->net;
+}
+
 static struct kobj_type rpc_sysfs_client_type = {
 	.release = rpc_sysfs_client_release,
 	.sysfs_ops = &kobj_sysfs_ops,
@@ -103,6 +117,12 @@ static struct kobj_type rpc_sysfs_xprt_switch_type = {
 	.namespace = rpc_sysfs_xprt_switch_namespace,
 };
 
+static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
+	.release = rpc_sysfs_xprt_switch_xprt_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+	.namespace = rpc_sysfs_xprt_switch_xprt_namespace,
+};
+
 void rpc_sysfs_exit(void)
 {
 	kobject_put(rpc_sunrpc_client_kobj);
@@ -150,6 +170,40 @@ rpc_sysfs_xprt_switch_alloc(struct kobject *parent,
 	return NULL;
 }
 
+static struct rpc_sysfs_xprt_switch_xprt *
+rpc_sysfs_xprt_switch_xprt_alloc(struct kobject *parent,
+				 struct rpc_xprt *xprt,
+				 struct net *net,
+				 gfp_t gfp_flags)
+{
+	struct rpc_sysfs_xprt_switch_xprt *p;
+
+	p = kzalloc(sizeof(*p), gfp_flags);
+	if (p) {
+		char type[6];
+
+		p->net = net;
+		p->kobject.kset = rpc_sunrpc_kset;
+		if (xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)
+			snprintf(type, sizeof(type), "rdma");
+		else if (xprt->xprt_class->ident == XPRT_TRANSPORT_TCP)
+			snprintf(type, sizeof(type), "tcp");
+		else if (xprt->xprt_class->ident == XPRT_TRANSPORT_UDP)
+			snprintf(type, sizeof(type), "udp");
+		else if (xprt->xprt_class->ident == XPRT_TRANSPORT_LOCAL)
+			snprintf(type, sizeof(type), "local");
+		else if (xprt->xprt_class->ident == XPRT_TRANSPORT_BC_TCP)
+			snprintf(type, sizeof(type), "bc");
+		if (kobject_init_and_add(&p->kobject,
+					 &rpc_sysfs_xprt_switch_xprt_type,
+					 parent, "xprt-%d-%s", xprt->id,
+					 type) == 0)
+			return p;
+		kobject_put(&p->kobject);
+	}
+	return NULL;
+}
+
 void rpc_sysfs_client_setup(struct rpc_clnt *clnt,
 			    struct rpc_xprt_switch *xprt_switch,
 			    struct net *net)
@@ -199,6 +253,25 @@ void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch *xprt_switch,
 	}
 }
 
+void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch *xprt_switch,
+				      struct rpc_xprt *xprt,
+				      gfp_t gfp_flags)
+{
+	struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt;
+	struct rpc_sysfs_xprt_switch *switch_obj =
+		(struct rpc_sysfs_xprt_switch *)xprt_switch->xps_sysfs;
+
+	rpc_xprt_switch_xprt =
+		rpc_sysfs_xprt_switch_xprt_alloc(&switch_obj->kobject,
+						 xprt, xprt->xprt_net,
+						 gfp_flags);
+	if (rpc_xprt_switch_xprt) {
+		xprt->xprt_sysfs = rpc_xprt_switch_xprt;
+		rpc_xprt_switch_xprt->xprt = xprt;
+		kobject_uevent(&rpc_xprt_switch_xprt->kobject, KOBJ_ADD);
+	}
+}
+
 void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
 {
 	struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
@@ -227,3 +300,16 @@ void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt_switch)
 		xprt_switch->xps_sysfs = NULL;
 	}
 }
+
+void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt)
+{
+	struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt =
+			xprt->xprt_sysfs;
+
+	if (rpc_xprt_switch_xprt) {
+		kobject_uevent(&rpc_xprt_switch_xprt->kobject, KOBJ_REMOVE);
+		kobject_del(&rpc_xprt_switch_xprt->kobject);
+		kobject_put(&rpc_xprt_switch_xprt->kobject);
+		xprt->xprt_sysfs = NULL;
+	}
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index 760f0582aee5..b2ede4a2a82b 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -19,6 +19,12 @@ struct rpc_sysfs_xprt_switch {
 	struct rpc_xprt *xprt;
 };
 
+struct rpc_sysfs_xprt_switch_xprt {
+	struct kobject kobject;
+	struct net *net;
+	struct rpc_xprt *xprt;
+};
+
 int rpc_sysfs_init(void);
 void rpc_sysfs_exit(void);
 
@@ -29,5 +35,8 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt);
 void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch *xprt_switch,
 				 struct rpc_xprt *xprt, gfp_t gfp_flags);
 void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt);
+void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch *xprt_switch,
+				      struct rpc_xprt *xprt, gfp_t gfp_flags);
+void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt);
 
 #endif
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 2d73a35df9ee..839b20e72ffd 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -57,6 +57,11 @@ void rpc_xprt_switch_add_xprt(struct rpc_xprt_switch *xps,
 	if (xps->xps_net == xprt->xprt_net || xps->xps_net == NULL)
 		xprt_switch_add_xprt_locked(xps, xprt);
 	spin_unlock(&xps->xps_lock);
+	xprt_switch_get(xps);
+	xprt_get(xprt);
+	rpc_sysfs_xprt_switch_xprt_setup(xps, xprt, GFP_KERNEL);
+	xprt_switch_put(xps);
+	xprt_put(xprt);
 }
 
 static void xprt_switch_remove_xprt_locked(struct rpc_xprt_switch *xps,
@@ -85,6 +90,7 @@ void rpc_xprt_switch_remove_xprt(struct rpc_xprt_switch *xps,
 	spin_lock(&xps->xps_lock);
 	xprt_switch_remove_xprt_locked(xps, xprt);
 	spin_unlock(&xps->xps_lock);
+	rpc_sysfs_xprt_switch_xprt_destroy(xprt);
 	xprt_put(xprt);
 }
 
@@ -137,6 +143,9 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct rpc_xprt *xprt,
 		xps->xps_iter_ops = &rpc_xprt_iter_singular;
 		rpc_sysfs_xprt_switch_setup(xps, xprt, gfp_flags);
 		xprt_switch_add_xprt_locked(xps, xprt);
+		xprt_get(xprt);
+		rpc_sysfs_xprt_switch_xprt_setup(xps, xprt, gfp_flags);
+		xprt_put(xprt);
 	}
 
 	return xps;
@@ -152,6 +161,7 @@ static void xprt_switch_free_entries(struct rpc_xprt_switch *xps)
 				struct rpc_xprt, xprt_switch);
 		xprt_switch_remove_xprt_locked(xps, xprt);
 		spin_unlock(&xps->xps_lock);
+		rpc_sysfs_xprt_switch_xprt_destroy(xprt);
 		xprt_put(xprt);
 		spin_lock(&xps->xps_lock);
 	}
-- 
2.27.0

[PATCH v3 11/13] sunrpc: add dst_attr attributes to the sysfs xprt directory

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Allow to query and set the destination's address of a transport.
Setting of the destination address is allowed only for TCP or RDMA
based connections.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/sysfs.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 682def4293f2..076f777db3cd 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -4,6 +4,9 @@
  */
 #include <linux/sunrpc/clnt.h>
 #include <linux/kobject.h>
+#include <linux/sunrpc/addr.h>
+#include <linux/sunrpc/xprtsock.h>
+
 #include "sysfs.h"
 
 static struct kset *rpc_sunrpc_kset;
@@ -42,6 +45,66 @@ static struct kobject *rpc_sysfs_object_alloc(const char *name,
 	return NULL;
 }
 
+static inline struct rpc_xprt *
+rpc_sysfs_xprt_kobj_get_xprt(struct kobject *kobj)
+{
+	struct rpc_sysfs_xprt_switch_xprt *x = container_of(kobj,
+		struct rpc_sysfs_xprt_switch_xprt, kobject);
+
+	return xprt_get(x->xprt);
+}
+
+static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   char *buf)
+{
+	struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
+	ssize_t ret;
+
+	if (!xprt)
+		return 0;
+	if (xprt->xprt_class->ident == XPRT_TRANSPORT_LOCAL)
+		ret = sprintf(buf, "localhost");
+	else
+		ret = rpc_ntop((struct sockaddr *)&xprt->addr, buf, PAGE_SIZE);
+	buf[ret] = '\n';
+	xprt_put(xprt);
+	return ret + 1;
+}
+
+static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
+					    struct kobj_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
+	struct sockaddr *saddr;
+	int port;
+
+	if (!xprt)
+		return 0;
+	if (!(xprt->xprt_class->ident == XPRT_TRANSPORT_TCP ||
+	      xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)) {
+		xprt_put(xprt);
+		return -EOPNOTSUPP;
+	}
+
+	wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE);
+	saddr = (struct sockaddr *)&xprt->addr;
+	port = rpc_get_port(saddr);
+
+	kfree(xprt->address_strings[RPC_DISPLAY_ADDR]);
+	xprt->address_strings[RPC_DISPLAY_ADDR] = kstrndup(buf, count - 1,
+							   GFP_KERNEL);
+	xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr,
+				 sizeof(*saddr));
+	rpc_set_port(saddr, port);
+
+	xprt->ops->connect(xprt, NULL);
+	clear_bit(XPRT_LOCKED, &xprt->state);
+	xprt_put(xprt);
+	return count;
+}
+
 int rpc_sysfs_init(void)
 {
 	rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
@@ -105,6 +168,14 @@ static const void *rpc_sysfs_xprt_switch_xprt_namespace(struct kobject *kobj)
 			    kobject)->net;
 }
 
+static struct kobj_attribute rpc_sysfs_xprt_dstaddr = __ATTR(dstaddr,
+	0644, rpc_sysfs_xprt_dstaddr_show, rpc_sysfs_xprt_dstaddr_store);
+
+static struct attribute *rpc_sysfs_xprt_attrs[] = {
+	&rpc_sysfs_xprt_dstaddr.attr,
+	NULL,
+};
+
 static struct kobj_type rpc_sysfs_client_type = {
 	.release = rpc_sysfs_client_release,
 	.sysfs_ops = &kobj_sysfs_ops,
@@ -119,6 +190,7 @@ static struct kobj_type rpc_sysfs_xprt_switch_type = {
 
 static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
 	.release = rpc_sysfs_xprt_switch_xprt_release,
+	.default_attrs = rpc_sysfs_xprt_attrs,
 	.sysfs_ops = &kobj_sysfs_ops,
 	.namespace = rpc_sysfs_xprt_switch_xprt_namespace,
 };
-- 
2.27.0

[PATCH v3 12/13] sunrpc: provide transport info in the sysfs directory

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Allow to query transport's attributes. Currently showing following
fields of the rpc_xprt structure: state, last_used, cong, cwnd,
max_reqs, min_reqs, num_reqs, sizes of queues binding, sending,
pending, backlog.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/sysfs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 076f777db3cd..93d4111f6ee3 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -72,6 +72,56 @@ static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
 	return ret + 1;
 }
 
+static ssize_t rpc_sysfs_xprt_info_show(struct kobject *kobj,
+					struct kobj_attribute *attr,
+					char *buf)
+{
+	struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
+	ssize_t ret;
+	int locked, connected, connecting, close_wait, bound, binding,
+	    closing, congested, cwnd_wait, write_space;
+
+	if (!xprt)
+		return 0;
+
+	if (!xprt->state) {
+		ret = sprintf(buf, "state=CLOSED\n");
+	} else {
+		locked = test_bit(XPRT_LOCKED, &xprt->state);
+		connected = test_bit(XPRT_CONNECTED, &xprt->state);
+		connecting = test_bit(XPRT_CONNECTING, &xprt->state);
+		close_wait = test_bit(XPRT_CLOSE_WAIT, &xprt->state);
+		bound = test_bit(XPRT_BOUND, &xprt->state);
+		binding = test_bit(XPRT_BINDING, &xprt->state);
+		closing = test_bit(XPRT_CLOSING, &xprt->state);
+		congested = test_bit(XPRT_CONGESTED, &xprt->state);
+		cwnd_wait = test_bit(XPRT_CWND_WAIT, &xprt->state);
+		write_space = test_bit(XPRT_WRITE_SPACE, &xprt->state);
+
+		ret = sprintf(buf, "state=%s %s %s %s %s %s %s %s %s %s\n",
+			      locked ? "LOCKED" : "",
+			      connected ? "CONNECTED" : "",
+			      connecting ? "CONNECTING" : "",
+			      close_wait ? "CLOSE_WAIT" : "",
+			      bound ? "BOUND" : "",
+			      binding ? "BOUNDING" : "",
+			      closing ? "CLOSING" : "",
+			      congested ? "CONGESTED" : "",
+			      cwnd_wait ? "CWND_WAIT" : "",
+			      write_space ? "WRITE_SPACE" : "");
+	}
+	ret += sprintf(buf + ret, "last_used=%lu\ncur_cong=%lu\ncong_win=%lu\n"
+		       "max_num_slots=%u\nmin_num_slots=%u\nnum_reqs=%u\n"
+		       "binding_q_len=%u\nsending_q_len=%u\npending_q_len=%u\n"
+		       "backlog_q_len=%u", xprt->last_used, xprt->cong,
+		       xprt->cwnd, xprt->max_reqs, xprt->min_reqs,
+		       xprt->num_reqs, xprt->binding.qlen, xprt->sending.qlen,
+		       xprt->pending.qlen, xprt->backlog.qlen);
+	buf[ret] = '\n';
+	xprt_put(xprt);
+	return ret + 1;
+}
+
 static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
 					    struct kobj_attribute *attr,
 					    const char *buf, size_t count)
@@ -171,8 +221,12 @@ static const void *rpc_sysfs_xprt_switch_xprt_namespace(struct kobject *kobj)
 static struct kobj_attribute rpc_sysfs_xprt_dstaddr = __ATTR(dstaddr,
 	0644, rpc_sysfs_xprt_dstaddr_show, rpc_sysfs_xprt_dstaddr_store);
 
+static struct kobj_attribute rpc_sysfs_xprt_info = __ATTR(xprt_info,
+	0444, rpc_sysfs_xprt_info_show, NULL);
+
 static struct attribute *rpc_sysfs_xprt_attrs[] = {
 	&rpc_sysfs_xprt_dstaddr.attr,
+	&rpc_sysfs_xprt_info.attr,
 	NULL,
 };
 
-- 
2.27.0

[PATCH v3 13/13] sunrpc: provide multipath info in the sysfs directory

[Olga Kornievskaia]

From: Olga Kornievskaia <kolga@netapp.com>

Allow to query xrpt_switch attributes. Currently showing the following
fields of the rpc_xprt_switch structure: xps_nxprts, xps_nactive,
xps_queuelen.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 net/sunrpc/sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 93d4111f6ee3..eeac19907c43 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -54,6 +54,15 @@ rpc_sysfs_xprt_kobj_get_xprt(struct kobject *kobj)
 	return xprt_get(x->xprt);
 }
 
+static inline struct rpc_xprt_switch *
+rpc_sysfs_xprt_switch_kobj_get_xprt(struct kobject *kobj)
+{
+	struct rpc_sysfs_xprt_switch *x = container_of(kobj,
+		struct rpc_sysfs_xprt_switch, kobject);
+
+	return xprt_switch_get(x->xprt_switch);
+}
+
 static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
 					   struct kobj_attribute *attr,
 					   char *buf)
@@ -122,6 +131,24 @@ static ssize_t rpc_sysfs_xprt_info_show(struct kobject *kobj,
 	return ret + 1;
 }
 
+static ssize_t rpc_sysfs_xprt_switch_info_show(struct kobject *kobj,
+					       struct kobj_attribute *attr,
+					       char *buf)
+{
+	struct rpc_xprt_switch *xprt_switch =
+		rpc_sysfs_xprt_switch_kobj_get_xprt(kobj);
+	ssize_t ret;
+
+	if (!xprt_switch)
+		return 0;
+	ret = sprintf(buf, "num_xprts=%u\nnum_active=%u\nqueue_len=%ld",
+		      xprt_switch->xps_nxprts, xprt_switch->xps_nactive,
+		      atomic_long_read(&xprt_switch->xps_queuelen));
+	buf[ret] = '\n';
+	xprt_switch_put(xprt_switch);
+	return ret + 1;
+}
+
 static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
 					    struct kobj_attribute *attr,
 					    const char *buf, size_t count)
@@ -230,6 +257,14 @@ static struct attribute *rpc_sysfs_xprt_attrs[] = {
 	NULL,
 };
 
+static struct kobj_attribute rpc_sysfs_xprt_switch_info =
+	__ATTR(xprt_switch_info, 0444, rpc_sysfs_xprt_switch_info_show, NULL);
+
+static struct attribute *rpc_sysfs_xprt_switch_attrs[] = {
+	&rpc_sysfs_xprt_switch_info.attr,
+	NULL,
+};
+
 static struct kobj_type rpc_sysfs_client_type = {
 	.release = rpc_sysfs_client_release,
 	.sysfs_ops = &kobj_sysfs_ops,
@@ -238,6 +273,7 @@ static struct kobj_type rpc_sysfs_client_type = {
 
 static struct kobj_type rpc_sysfs_xprt_switch_type = {
 	.release = rpc_sysfs_xprt_switch_release,
+	.default_attrs = rpc_sysfs_xprt_switch_attrs,
 	.sysfs_ops = &kobj_sysfs_ops,
 	.namespace = rpc_sysfs_xprt_switch_namespace,
 };
-- 
2.27.0

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Dan Aloni]

On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> An rpc client uses a transport switch and one ore more transports
> associated with that switch. Since transports are shared among
> rpc clients, create a symlink into the xprt_switch directory
> instead of duplicating entries under each rpc client.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>
>..
> @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
>  	struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
>  
>  	if (rpc_client) {
> +		char name[23];
> +
> +		snprintf(name, sizeof(name), "switch-%d",
> +			 rpc_client->xprt_switch->xps_id);
> +		sysfs_remove_link(&rpc_client->kobject, name);

Hi Olga,

If a client can use a single switch, shouldn't the name of the symlink
be just "switch"? This is to be consistent with other symlinks in
`sysfs` such as the ones in block layer, for example in my
`/sys/block/sda`:

    bdi -> ../../../../../../../../../../../virtual/bdi/8:0
    device -> ../../../5:0:0:0


-- 
Dan Aloni

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
>
> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > An rpc client uses a transport switch and one ore more transports
> > associated with that switch. Since transports are shared among
> > rpc clients, create a symlink into the xprt_switch directory
> > instead of duplicating entries under each rpc client.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >
> >..
> > @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
> >       struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> >
> >       if (rpc_client) {
> > +             char name[23];
> > +
> > +             snprintf(name, sizeof(name), "switch-%d",
> > +                      rpc_client->xprt_switch->xps_id);
> > +             sysfs_remove_link(&rpc_client->kobject, name);
>
> Hi Olga,
>
> If a client can use a single switch, shouldn't the name of the symlink
> be just "switch"? This is to be consistent with other symlinks in
> `sysfs` such as the ones in block layer, for example in my
> `/sys/block/sda`:
>
>     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
>     device -> ../../../5:0:0:0

I think the client is written so that in the future it might have more
than one switch?

>
>
> --
> Dan Aloni

Re: [PATCH v3 08/13] sunrpc: add xprt_switch direcotry to sunrpc's sysfs

[Trond Myklebust]

On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Add xprt_switch directory to the sysfs and create individual
> xprt_swith subdirectories for multipath transport group.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  include/linux/sunrpc/xprtmultipath.h |  1 +
>  net/sunrpc/sysfs.c                   | 99
> ++++++++++++++++++++++++++--
>  net/sunrpc/sysfs.h                   | 10 +++
>  net/sunrpc/xprtmultipath.c           |  4 ++
>  4 files changed, 107 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/sunrpc/xprtmultipath.h
> b/include/linux/sunrpc/xprtmultipath.h
> index ef95a6f18ccf..47b0a85cdcfa 100644
> --- a/include/linux/sunrpc/xprtmultipath.h
> +++ b/include/linux/sunrpc/xprtmultipath.h
> @@ -24,6 +24,7 @@ struct rpc_xprt_switch {
>  
>         const struct rpc_xprt_iter_ops *xps_iter_ops;
>  
> +       void                    *xps_sysfs;

Why is this a void pointer instead of being a pointer to a struct
rpc_sysfs_xprt_switch?

>         struct rcu_head         xps_rcu;
>  };
>  
> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> index d14d54f33c65..03ea6d3ace95 100644
> --- a/net/sunrpc/sysfs.c
> +++ b/net/sunrpc/sysfs.c
> @@ -7,7 +7,7 @@
>  #include "sysfs.h"
>  
>  static struct kset *rpc_sunrpc_kset;
> -static struct kobject *rpc_sunrpc_client_kobj;
> +static struct kobject *rpc_sunrpc_client_kobj,
> *rpc_sunrpc_xprt_switch_kobj;
>  
>  static void rpc_sysfs_object_release(struct kobject *kobj)
>  {
> @@ -47,13 +47,22 @@ int rpc_sysfs_init(void)
>         rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL,
> kernel_kobj);
>         if (!rpc_sunrpc_kset)
>                 return -ENOMEM;
> -       rpc_sunrpc_client_kobj = rpc_sysfs_object_alloc("client",
> rpc_sunrpc_kset, NULL);
> -       if (!rpc_sunrpc_client_kobj) {
> -               kset_unregister(rpc_sunrpc_kset);
> -               rpc_sunrpc_client_kobj = NULL;
> -               return -ENOMEM;
> -       }
> +       rpc_sunrpc_client_kobj =
> +               rpc_sysfs_object_alloc("rpc-clients",
> rpc_sunrpc_kset, NULL);
> +       if (!rpc_sunrpc_client_kobj)
> +               goto err_client;
> +       rpc_sunrpc_xprt_switch_kobj =
> +               rpc_sysfs_object_alloc("xprt-switches",
> rpc_sunrpc_kset, NULL);
> +       if (!rpc_sunrpc_xprt_switch_kobj)
> +               goto err_switch;
>         return 0;
> +err_switch:
> +       kobject_put(rpc_sunrpc_client_kobj);
> +       rpc_sunrpc_client_kobj = NULL;
> +err_client:
> +       kset_unregister(rpc_sunrpc_kset);
> +       rpc_sunrpc_kset = NULL;
> +       return -ENOMEM;
>  }
>  
>  static void rpc_sysfs_client_release(struct kobject *kobj)
> @@ -64,20 +73,40 @@ static void rpc_sysfs_client_release(struct
> kobject *kobj)
>         kfree(c);
>  }
>  
> +static void rpc_sysfs_xprt_switch_release(struct kobject *kobj)
> +{
> +       struct rpc_sysfs_xprt_switch *xprt_switch;
> +
> +       xprt_switch = container_of(kobj, struct
> rpc_sysfs_xprt_switch, kobject);
> +       kfree(xprt_switch);
> +}
> +
>  static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
>  {
>         return container_of(kobj, struct rpc_sysfs_client, kobject)-
> >net;
>  }
>  
> +static const void *rpc_sysfs_xprt_switch_namespace(struct kobject
> *kobj)
> +{
> +       return container_of(kobj, struct rpc_sysfs_xprt_switch,
> kobject)->net;
> +}
> +
>  static struct kobj_type rpc_sysfs_client_type = {
>         .release = rpc_sysfs_client_release,
>         .sysfs_ops = &kobj_sysfs_ops,
>         .namespace = rpc_sysfs_client_namespace,
>  };
>  
> +static struct kobj_type rpc_sysfs_xprt_switch_type = {
> +       .release = rpc_sysfs_xprt_switch_release,
> +       .sysfs_ops = &kobj_sysfs_ops,
> +       .namespace = rpc_sysfs_xprt_switch_namespace,
> +};
> +
>  void rpc_sysfs_exit(void)
>  {
>         kobject_put(rpc_sunrpc_client_kobj);
> +       kobject_put(rpc_sunrpc_xprt_switch_kobj);
>         kset_unregister(rpc_sunrpc_kset);
>  }
>  
> @@ -99,6 +128,28 @@ static struct rpc_sysfs_client
> *rpc_sysfs_client_alloc(struct kobject *parent,
>         return NULL;
>  }
>  
> +static struct rpc_sysfs_xprt_switch *
> +rpc_sysfs_xprt_switch_alloc(struct kobject *parent,
> +                           struct rpc_xprt_switch *xprt_switch,
> +                           struct net *net,
> +                           gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch *p;
> +
> +       p = kzalloc(sizeof(*p), gfp_flags);
> +       if (p) {
> +               p->net = net;
> +               p->kobject.kset = rpc_sunrpc_kset;
> +               if (kobject_init_and_add(&p->kobject,
> +                                        &rpc_sysfs_xprt_switch_type,
> +                                        parent, "switch-%d",
> +                                        xprt_switch->xps_id) == 0)
> +                       return p;
> +               kobject_put(&p->kobject);
> +       }
> +       return NULL;
> +}
> +
>  void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net)
>  {
>         struct rpc_sysfs_client *rpc_client;
> @@ -110,6 +161,28 @@ void rpc_sysfs_client_setup(struct rpc_clnt
> *clnt, struct net *net)
>         }
>  }
>  
> +void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                struct rpc_xprt *xprt,
> +                                gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch *rpc_xprt_switch;
> +       struct net *net;
> +
> +       if (xprt_switch->xps_net)
> +               net = xprt_switch->xps_net;
> +       else
> +               net = xprt->xprt_net;
> +       rpc_xprt_switch =
> +               rpc_sysfs_xprt_switch_alloc(rpc_sunrpc_xprt_switch_ko
> bj,
> +                                           xprt_switch, net,
> gfp_flags);
> +       if (rpc_xprt_switch) {
> +               xprt_switch->xps_sysfs = rpc_xprt_switch;
> +               rpc_xprt_switch->xprt_switch = xprt_switch;
> +               rpc_xprt_switch->xprt = xprt;
> +               kobject_uevent(&rpc_xprt_switch->kobject, KOBJ_ADD);
> +       }
> +}
> +
>  void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
>  {
>         struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> @@ -121,3 +194,15 @@ void rpc_sysfs_client_destroy(struct rpc_clnt
> *clnt)
>                 clnt->cl_sysfs = NULL;
>         }
>  }
> +
> +void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch
> *xprt_switch)
> +{
> +       struct rpc_sysfs_xprt_switch *rpc_xprt_switch = xprt_switch-
> >xps_sysfs;
> +
> +       if (rpc_xprt_switch) {
> +               kobject_uevent(&rpc_xprt_switch->kobject,
> KOBJ_REMOVE);
> +               kobject_del(&rpc_xprt_switch->kobject);
> +               kobject_put(&rpc_xprt_switch->kobject);
> +               xprt_switch->xps_sysfs = NULL;
> +       }
> +}
> diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
> index c46afc848993..52ec472bd4a9 100644
> --- a/net/sunrpc/sysfs.h
> +++ b/net/sunrpc/sysfs.h
> @@ -10,10 +10,20 @@ struct rpc_sysfs_client {
>         struct net *net;
>  };
>  
> +struct rpc_sysfs_xprt_switch {
> +       struct kobject kobject;
> +       struct net *net;
> +       struct rpc_xprt_switch *xprt_switch;
> +       struct rpc_xprt *xprt;
> +};
> +
>  int rpc_sysfs_init(void);
>  void rpc_sysfs_exit(void);
>  
>  void rpc_sysfs_client_setup(struct rpc_clnt *clnt, struct net *net);
>  void rpc_sysfs_client_destroy(struct rpc_clnt *clnt);
> +void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                struct rpc_xprt *xprt, gfp_t
> gfp_flags);
> +void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt);
>  
>  #endif
> diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
> index 4969a4c216f7..2d73a35df9ee 100644
> --- a/net/sunrpc/xprtmultipath.c
> +++ b/net/sunrpc/xprtmultipath.c
> @@ -19,6 +19,8 @@
>  #include <linux/sunrpc/addr.h>
>  #include <linux/sunrpc/xprtmultipath.h>
>  
> +#include "sysfs.h"
> +
>  typedef struct rpc_xprt *(*xprt_switch_find_xprt_t)(struct
> rpc_xprt_switch *xps,
>                 const struct rpc_xprt *cur);
>  
> @@ -133,6 +135,7 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct
> rpc_xprt *xprt,
>                 xps->xps_net = NULL;
>                 INIT_LIST_HEAD(&xps->xps_xprt_list);
>                 xps->xps_iter_ops = &rpc_xprt_iter_singular;
> +               rpc_sysfs_xprt_switch_setup(xps, xprt, gfp_flags);
>                 xprt_switch_add_xprt_locked(xps, xprt);
>         }
>  
> @@ -161,6 +164,7 @@ static void xprt_switch_free(struct kref *kref)
>                         struct rpc_xprt_switch, xps_kref);
>  
>         xprt_switch_free_entries(xps);
> +       rpc_sysfs_xprt_switch_destroy(xps);
>         xprt_switch_free_id(xps);
>         kfree_rcu(xps, xps_rcu);
>  }

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 10/13] sunrpc: add add sysfs directory per xprt under each xprt_switch

[Trond Myklebust]

On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Add individual transport directories under each transport switch
> group. For instance, for each nconnect=X connections there will be
> a transport directory. Naming conventions also identifies transport
> type -- xprt-<id>-<type> where type is udp, tcp, rdma, local, bc.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  include/linux/sunrpc/xprt.h |  1 +
>  net/sunrpc/sysfs.c          | 86
> +++++++++++++++++++++++++++++++++++++
>  net/sunrpc/sysfs.h          |  9 ++++
>  net/sunrpc/xprtmultipath.c  | 10 +++++
>  4 files changed, 106 insertions(+)
> 
> diff --git a/include/linux/sunrpc/xprt.h
> b/include/linux/sunrpc/xprt.h
> index a2edcc42e6c4..1e4906759a6a 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -291,6 +291,7 @@ struct rpc_xprt {
>  #endif
>         struct rcu_head         rcu;
>         const struct xprt_class *xprt_class;
> +       void                    *xprt_sysfs;

Again, this should be a typed pointer.

>  };
>  
>  #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> index 871f7c696a93..682def4293f2 100644
> --- a/net/sunrpc/sysfs.c
> +++ b/net/sunrpc/sysfs.c
> @@ -81,6 +81,14 @@ static void rpc_sysfs_xprt_switch_release(struct
> kobject *kobj)
>         kfree(xprt_switch);
>  }
>  
> +static void rpc_sysfs_xprt_switch_xprt_release(struct kobject *kobj)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *xprt;
> +
> +       xprt = container_of(kobj, struct rpc_sysfs_xprt_switch_xprt,
> kobject);
> +       kfree(xprt);
> +}
> +
>  static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
>  {
>         return container_of(kobj, struct rpc_sysfs_client, kobject)-
> >net;
> @@ -91,6 +99,12 @@ static const void
> *rpc_sysfs_xprt_switch_namespace(struct kobject *kobj)
>         return container_of(kobj, struct rpc_sysfs_xprt_switch,
> kobject)->net;
>  }
>  
> +static const void *rpc_sysfs_xprt_switch_xprt_namespace(struct
> kobject *kobj)
> +{
> +       return container_of(kobj, struct rpc_sysfs_xprt_switch_xprt,
> +                           kobject)->net;
> +}
> +
>  static struct kobj_type rpc_sysfs_client_type = {
>         .release = rpc_sysfs_client_release,
>         .sysfs_ops = &kobj_sysfs_ops,
> @@ -103,6 +117,12 @@ static struct kobj_type
> rpc_sysfs_xprt_switch_type = {
>         .namespace = rpc_sysfs_xprt_switch_namespace,
>  };
>  
> +static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
> +       .release = rpc_sysfs_xprt_switch_xprt_release,
> +       .sysfs_ops = &kobj_sysfs_ops,
> +       .namespace = rpc_sysfs_xprt_switch_xprt_namespace,
> +};
> +
>  void rpc_sysfs_exit(void)
>  {
>         kobject_put(rpc_sunrpc_client_kobj);
> @@ -150,6 +170,40 @@ rpc_sysfs_xprt_switch_alloc(struct kobject
> *parent,
>         return NULL;
>  }
>  
> +static struct rpc_sysfs_xprt_switch_xprt *
> +rpc_sysfs_xprt_switch_xprt_alloc(struct kobject *parent,
> +                                struct rpc_xprt *xprt,
> +                                struct net *net,
> +                                gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *p;
> +
> +       p = kzalloc(sizeof(*p), gfp_flags);
> +       if (p) {
> +               char type[6];
> +
> +               p->net = net;
> +               p->kobject.kset = rpc_sunrpc_kset;
> +               if (xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)
> +                       snprintf(type, sizeof(type), "rdma");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_TCP)
> +                       snprintf(type, sizeof(type), "tcp");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_UDP)
> +                       snprintf(type, sizeof(type), "udp");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_LOCAL)
> +                       snprintf(type, sizeof(type), "local");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_BC_TCP)
> +                       snprintf(type, sizeof(type), "bc");
> +               if (kobject_init_and_add(&p->kobject,
> +                                       
> &rpc_sysfs_xprt_switch_xprt_type,
> +                                        parent, "xprt-%d-%s", xprt-
> >id,
> +                                        type) == 0)
> +                       return p;
> +               kobject_put(&p->kobject);
> +       }
> +       return NULL;
> +}
> +
>  void rpc_sysfs_client_setup(struct rpc_clnt *clnt,
>                             struct rpc_xprt_switch *xprt_switch,
>                             struct net *net)
> @@ -199,6 +253,25 @@ void rpc_sysfs_xprt_switch_setup(struct
> rpc_xprt_switch *xprt_switch,
>         }
>  }
>  
> +void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                     struct rpc_xprt *xprt,
> +                                     gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt;
> +       struct rpc_sysfs_xprt_switch *switch_obj =
> +               (struct rpc_sysfs_xprt_switch *)xprt_switch-
> >xps_sysfs;
> +
> +       rpc_xprt_switch_xprt =
> +               rpc_sysfs_xprt_switch_xprt_alloc(&switch_obj-
> >kobject,
> +                                                xprt, xprt-
> >xprt_net,
> +                                                gfp_flags);
> +       if (rpc_xprt_switch_xprt) {
> +               xprt->xprt_sysfs = rpc_xprt_switch_xprt;
> +               rpc_xprt_switch_xprt->xprt = xprt;
> +               kobject_uevent(&rpc_xprt_switch_xprt->kobject,
> KOBJ_ADD);
> +       }
> +}
> +
>  void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
>  {
>         struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> @@ -227,3 +300,16 @@ void rpc_sysfs_xprt_switch_destroy(struct
> rpc_xprt_switch *xprt_switch)
>                 xprt_switch->xps_sysfs = NULL;
>         }
>  }
> +
> +void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt =
> +                       xprt->xprt_sysfs;
> +
> +       if (rpc_xprt_switch_xprt) {
> +               kobject_uevent(&rpc_xprt_switch_xprt->kobject,
> KOBJ_REMOVE);
> +               kobject_del(&rpc_xprt_switch_xprt->kobject);
> +               kobject_put(&rpc_xprt_switch_xprt->kobject);
> +               xprt->xprt_sysfs = NULL;
> +       }
> +}
> diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
> index 760f0582aee5..b2ede4a2a82b 100644
> --- a/net/sunrpc/sysfs.h
> +++ b/net/sunrpc/sysfs.h
> @@ -19,6 +19,12 @@ struct rpc_sysfs_xprt_switch {
>         struct rpc_xprt *xprt;
>  };
>  
> +struct rpc_sysfs_xprt_switch_xprt {
> +       struct kobject kobject;
> +       struct net *net;
> +       struct rpc_xprt *xprt;
> +};

Can we please rename this to something that doesn't imply that it is
part of the xprt_switch? It is tied to a struct rpc_xprt rather than
the switch.

Also, why do you need a pointer to struct net here? Why not just use
xprt->xprt_net when a pointer is needed?

> +
>  int rpc_sysfs_init(void);
>  void rpc_sysfs_exit(void);
>  
> @@ -29,5 +35,8 @@ void rpc_sysfs_client_destroy(struct rpc_clnt
> *clnt);
>  void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch
> *xprt_switch,
>                                  struct rpc_xprt *xprt, gfp_t
> gfp_flags);
>  void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt);
> +void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                     struct rpc_xprt *xprt, gfp_t
> gfp_flags);
> +void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt);
>  
>  #endif
> diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
> index 2d73a35df9ee..839b20e72ffd 100644
> --- a/net/sunrpc/xprtmultipath.c
> +++ b/net/sunrpc/xprtmultipath.c
> @@ -57,6 +57,11 @@ void rpc_xprt_switch_add_xprt(struct
> rpc_xprt_switch *xps,
>         if (xps->xps_net == xprt->xprt_net || xps->xps_net == NULL)
>                 xprt_switch_add_xprt_locked(xps, xprt);
>         spin_unlock(&xps->xps_lock);
> +       xprt_switch_get(xps);
> +       xprt_get(xprt);

Do we need the get/put here? Won't the caller always hold appropriate
references?

> +       rpc_sysfs_xprt_switch_xprt_setup(xps, xprt, GFP_KERNEL);
> +       xprt_switch_put(xps);
> +       xprt_put(xprt);
>  }
>  
>  static void xprt_switch_remove_xprt_locked(struct rpc_xprt_switch
> *xps,
> @@ -85,6 +90,7 @@ void rpc_xprt_switch_remove_xprt(struct
> rpc_xprt_switch *xps,
>         spin_lock(&xps->xps_lock);
>         xprt_switch_remove_xprt_locked(xps, xprt);
>         spin_unlock(&xps->xps_lock);
> +       rpc_sysfs_xprt_switch_xprt_destroy(xprt);
>         xprt_put(xprt);
>  }
>  
> @@ -137,6 +143,9 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct
> rpc_xprt *xprt,
>                 xps->xps_iter_ops = &rpc_xprt_iter_singular;
>                 rpc_sysfs_xprt_switch_setup(xps, xprt, gfp_flags);
>                 xprt_switch_add_xprt_locked(xps, xprt);
> +               xprt_get(xprt);
> +               rpc_sysfs_xprt_switch_xprt_setup(xps, xprt,
> gfp_flags);
> +               xprt_put(xprt);
>         }
>  
>         return xps;
> @@ -152,6 +161,7 @@ static void xprt_switch_free_entries(struct
> rpc_xprt_switch *xps)
>                                 struct rpc_xprt, xprt_switch);
>                 xprt_switch_remove_xprt_locked(xps, xprt);
>                 spin_unlock(&xps->xps_lock);
> +               rpc_sysfs_xprt_switch_xprt_destroy(xprt);
>                 xprt_put(xprt);
>                 spin_lock(&xps->xps_lock);
>         }

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 11/13] sunrpc: add dst_attr attributes to the sysfs xprt directory

[Trond Myklebust]

On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Allow to query and set the destination's address of a transport.
> Setting of the destination address is allowed only for TCP or RDMA
> based connections.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  net/sunrpc/sysfs.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> index 682def4293f2..076f777db3cd 100644
> --- a/net/sunrpc/sysfs.c
> +++ b/net/sunrpc/sysfs.c
> @@ -4,6 +4,9 @@
>   */
>  #include <linux/sunrpc/clnt.h>
>  #include <linux/kobject.h>
> +#include <linux/sunrpc/addr.h>
> +#include <linux/sunrpc/xprtsock.h>
> +
>  #include "sysfs.h"
>  
>  static struct kset *rpc_sunrpc_kset;
> @@ -42,6 +45,66 @@ static struct kobject
> *rpc_sysfs_object_alloc(const char *name,
>         return NULL;
>  }
>  
> +static inline struct rpc_xprt *
> +rpc_sysfs_xprt_kobj_get_xprt(struct kobject *kobj)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *x = container_of(kobj,
> +               struct rpc_sysfs_xprt_switch_xprt, kobject);
> +
> +       return xprt_get(x->xprt);
> +}
> +
> +static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
> +                                          struct kobj_attribute
> *attr,
> +                                          char *buf)
> +{
> +       struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
> +       ssize_t ret;
> +
> +       if (!xprt)
> +               return 0;
> +       if (xprt->xprt_class->ident == XPRT_TRANSPORT_LOCAL)
> +               ret = sprintf(buf, "localhost");

This makes it look like it is a loopback socket, whereas in reality it
is a named socket. Why not just use the xprt-
>address_strings[RPC_DISPLAY_ADDR] here?

> +       else
> +               ret = rpc_ntop((struct sockaddr *)&xprt->addr, buf,
> PAGE_SIZE);
> +       buf[ret] = '\n';
> +       xprt_put(xprt);
> +       return ret + 1;
> +}
> +
> +static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
> +                                           struct kobj_attribute
> *attr,
> +                                           const char *buf, size_t
> count)
> +{
> +       struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
> +       struct sockaddr *saddr;
> +       int port;
> +
> +       if (!xprt)
> +               return 0;
> +       if (!(xprt->xprt_class->ident == XPRT_TRANSPORT_TCP ||
> +             xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)) {
> +               xprt_put(xprt);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE);
> +       saddr = (struct sockaddr *)&xprt->addr;
> +       port = rpc_get_port(saddr);
> +
> +       kfree(xprt->address_strings[RPC_DISPLAY_ADDR]);

This is not allowed. The xprt->address_strings can only be freed in a
manner that is safe w.r.t. the rcu_read_lock(). Otherwise, various
users of rpc_peeraddr2str() are prone to crash.

So if you're going to replace these strings, then you have to replace
them using something like rcu_replace_pointer(), then you have to free
them using call_rcu() or kfree_rcu}().

> +       xprt->address_strings[RPC_DISPLAY_ADDR] = kstrndup(buf, count
> - 1,
> +                                                         
> GFP_KERNEL);
> +       xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1,
> saddr,
> +                                sizeof(*saddr));
> +       rpc_set_port(saddr, port);

The above is also a bit dodgy w.r.t. rpc_peeraddr() since it is non-
atomic. However it looks like it should be OK in practice.

> +
> +       xprt->ops->connect(xprt, NULL);
> +       clear_bit(XPRT_LOCKED, &xprt->state);
> +       xprt_put(xprt);
> +       return count;
> +}
> +
>  int rpc_sysfs_init(void)
>  {
>         rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL,
> kernel_kobj);
> @@ -105,6 +168,14 @@ static const void
> *rpc_sysfs_xprt_switch_xprt_namespace(struct kobject *kobj)
>                             kobject)->net;
>  }
>  
> +static struct kobj_attribute rpc_sysfs_xprt_dstaddr =
> __ATTR(dstaddr,
> +       0644, rpc_sysfs_xprt_dstaddr_show,
> rpc_sysfs_xprt_dstaddr_store);
> +
> +static struct attribute *rpc_sysfs_xprt_attrs[] = {
> +       &rpc_sysfs_xprt_dstaddr.attr,
> +       NULL,
> +};
> +
>  static struct kobj_type rpc_sysfs_client_type = {
>         .release = rpc_sysfs_client_release,
>         .sysfs_ops = &kobj_sysfs_ops,
> @@ -119,6 +190,7 @@ static struct kobj_type
> rpc_sysfs_xprt_switch_type = {
>  
>  static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
>         .release = rpc_sysfs_xprt_switch_xprt_release,
> +       .default_attrs = rpc_sysfs_xprt_attrs,
>         .sysfs_ops = &kobj_sysfs_ops,
>         .namespace = rpc_sysfs_xprt_switch_xprt_namespace,
>  };

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 10/13] sunrpc: add add sysfs directory per xprt under each xprt_switch

[Trond Myklebust]

On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Add individual transport directories under each transport switch
> group. For instance, for each nconnect=X connections there will be
> a transport directory. Naming conventions also identifies transport
> type -- xprt-<id>-<type> where type is udp, tcp, rdma, local, bc.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  include/linux/sunrpc/xprt.h |  1 +
>  net/sunrpc/sysfs.c          | 86
> +++++++++++++++++++++++++++++++++++++
>  net/sunrpc/sysfs.h          |  9 ++++
>  net/sunrpc/xprtmultipath.c  | 10 +++++
>  4 files changed, 106 insertions(+)
> 
> diff --git a/include/linux/sunrpc/xprt.h
> b/include/linux/sunrpc/xprt.h
> index a2edcc42e6c4..1e4906759a6a 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -291,6 +291,7 @@ struct rpc_xprt {
>  #endif
>         struct rcu_head         rcu;
>         const struct xprt_class *xprt_class;
> +       void                    *xprt_sysfs;
>  };
>  
>  #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> index 871f7c696a93..682def4293f2 100644
> --- a/net/sunrpc/sysfs.c
> +++ b/net/sunrpc/sysfs.c
> @@ -81,6 +81,14 @@ static void rpc_sysfs_xprt_switch_release(struct
> kobject *kobj)
>         kfree(xprt_switch);
>  }
>  
> +static void rpc_sysfs_xprt_switch_xprt_release(struct kobject *kobj)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *xprt;
> +
> +       xprt = container_of(kobj, struct rpc_sysfs_xprt_switch_xprt,
> kobject);
> +       kfree(xprt);
> +}
> +
>  static const void *rpc_sysfs_client_namespace(struct kobject *kobj)
>  {
>         return container_of(kobj, struct rpc_sysfs_client, kobject)-
> >net;
> @@ -91,6 +99,12 @@ static const void
> *rpc_sysfs_xprt_switch_namespace(struct kobject *kobj)
>         return container_of(kobj, struct rpc_sysfs_xprt_switch,
> kobject)->net;
>  }
>  
> +static const void *rpc_sysfs_xprt_switch_xprt_namespace(struct
> kobject *kobj)
> +{
> +       return container_of(kobj, struct rpc_sysfs_xprt_switch_xprt,
> +                           kobject)->net;
> +}
> +
>  static struct kobj_type rpc_sysfs_client_type = {
>         .release = rpc_sysfs_client_release,
>         .sysfs_ops = &kobj_sysfs_ops,
> @@ -103,6 +117,12 @@ static struct kobj_type
> rpc_sysfs_xprt_switch_type = {
>         .namespace = rpc_sysfs_xprt_switch_namespace,
>  };
>  
> +static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
> +       .release = rpc_sysfs_xprt_switch_xprt_release,
> +       .sysfs_ops = &kobj_sysfs_ops,
> +       .namespace = rpc_sysfs_xprt_switch_xprt_namespace,
> +};
> +
>  void rpc_sysfs_exit(void)
>  {
>         kobject_put(rpc_sunrpc_client_kobj);
> @@ -150,6 +170,40 @@ rpc_sysfs_xprt_switch_alloc(struct kobject
> *parent,
>         return NULL;
>  }
>  
> +static struct rpc_sysfs_xprt_switch_xprt *
> +rpc_sysfs_xprt_switch_xprt_alloc(struct kobject *parent,
> +                                struct rpc_xprt *xprt,
> +                                struct net *net,
> +                                gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *p;
> +
> +       p = kzalloc(sizeof(*p), gfp_flags);
> +       if (p) {
> +               char type[6];
> +
> +               p->net = net;
> +               p->kobject.kset = rpc_sunrpc_kset;
> +               if (xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)
> +                       snprintf(type, sizeof(type), "rdma");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_TCP)
> +                       snprintf(type, sizeof(type), "tcp");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_UDP)
> +                       snprintf(type, sizeof(type), "udp");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_LOCAL)
> +                       snprintf(type, sizeof(type), "local");
> +               else if (xprt->xprt_class->ident ==
> XPRT_TRANSPORT_BC_TCP)
> +                       snprintf(type, sizeof(type), "bc");

Urgh. Can we perhaps use xprt->address_strings[RPC_DISPLAY_NETID] or
RPC_DISPLAY_PROTO? I'd really prefer that we don't make assumptions
that the current set of values for xprt->xprt_class is complete.

> +               if (kobject_init_and_add(&p->kobject,
> +                                       
> &rpc_sysfs_xprt_switch_xprt_type,
> +                                        parent, "xprt-%d-%s", xprt-
> >id,
> +                                        type) == 0)
> +                       return p;
> +               kobject_put(&p->kobject);
> +       }
> +       return NULL;
> +}
> +
>  void rpc_sysfs_client_setup(struct rpc_clnt *clnt,
>                             struct rpc_xprt_switch *xprt_switch,
>                             struct net *net)
> @@ -199,6 +253,25 @@ void rpc_sysfs_xprt_switch_setup(struct
> rpc_xprt_switch *xprt_switch,
>         }
>  }
>  
> +void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                     struct rpc_xprt *xprt,
> +                                     gfp_t gfp_flags)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt;
> +       struct rpc_sysfs_xprt_switch *switch_obj =
> +               (struct rpc_sysfs_xprt_switch *)xprt_switch-
> >xps_sysfs;
> +
> +       rpc_xprt_switch_xprt =
> +               rpc_sysfs_xprt_switch_xprt_alloc(&switch_obj-
> >kobject,
> +                                                xprt, xprt-
> >xprt_net,
> +                                                gfp_flags);
> +       if (rpc_xprt_switch_xprt) {
> +               xprt->xprt_sysfs = rpc_xprt_switch_xprt;
> +               rpc_xprt_switch_xprt->xprt = xprt;
> +               kobject_uevent(&rpc_xprt_switch_xprt->kobject,
> KOBJ_ADD);
> +       }
> +}
> +
>  void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
>  {
>         struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> @@ -227,3 +300,16 @@ void rpc_sysfs_xprt_switch_destroy(struct
> rpc_xprt_switch *xprt_switch)
>                 xprt_switch->xps_sysfs = NULL;
>         }
>  }
> +
> +void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *rpc_xprt_switch_xprt =
> +                       xprt->xprt_sysfs;
> +
> +       if (rpc_xprt_switch_xprt) {
> +               kobject_uevent(&rpc_xprt_switch_xprt->kobject,
> KOBJ_REMOVE);
> +               kobject_del(&rpc_xprt_switch_xprt->kobject);
> +               kobject_put(&rpc_xprt_switch_xprt->kobject);
> +               xprt->xprt_sysfs = NULL;
> +       }
> +}
> diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
> index 760f0582aee5..b2ede4a2a82b 100644
> --- a/net/sunrpc/sysfs.h
> +++ b/net/sunrpc/sysfs.h
> @@ -19,6 +19,12 @@ struct rpc_sysfs_xprt_switch {
>         struct rpc_xprt *xprt;
>  };
>  
> +struct rpc_sysfs_xprt_switch_xprt {
> +       struct kobject kobject;
> +       struct net *net;
> +       struct rpc_xprt *xprt;
> +};
> +
>  int rpc_sysfs_init(void);
>  void rpc_sysfs_exit(void);
>  
> @@ -29,5 +35,8 @@ void rpc_sysfs_client_destroy(struct rpc_clnt
> *clnt);
>  void rpc_sysfs_xprt_switch_setup(struct rpc_xprt_switch
> *xprt_switch,
>                                  struct rpc_xprt *xprt, gfp_t
> gfp_flags);
>  void rpc_sysfs_xprt_switch_destroy(struct rpc_xprt_switch *xprt);
> +void rpc_sysfs_xprt_switch_xprt_setup(struct rpc_xprt_switch
> *xprt_switch,
> +                                     struct rpc_xprt *xprt, gfp_t
> gfp_flags);
> +void rpc_sysfs_xprt_switch_xprt_destroy(struct rpc_xprt *xprt);
>  
>  #endif
> diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
> index 2d73a35df9ee..839b20e72ffd 100644
> --- a/net/sunrpc/xprtmultipath.c
> +++ b/net/sunrpc/xprtmultipath.c
> @@ -57,6 +57,11 @@ void rpc_xprt_switch_add_xprt(struct
> rpc_xprt_switch *xps,
>         if (xps->xps_net == xprt->xprt_net || xps->xps_net == NULL)
>                 xprt_switch_add_xprt_locked(xps, xprt);
>         spin_unlock(&xps->xps_lock);
> +       xprt_switch_get(xps);
> +       xprt_get(xprt);

Are the above references necessary?

> +       rpc_sysfs_xprt_switch_xprt_setup(xps, xprt, GFP_KERNEL);
> +       xprt_switch_put(xps);
> +       xprt_put(xprt);
>  }
>  
>  static void xprt_switch_remove_xprt_locked(struct rpc_xprt_switch
> *xps,
> @@ -85,6 +90,7 @@ void rpc_xprt_switch_remove_xprt(struct
> rpc_xprt_switch *xps,
>         spin_lock(&xps->xps_lock);
>         xprt_switch_remove_xprt_locked(xps, xprt);
>         spin_unlock(&xps->xps_lock);
> +       rpc_sysfs_xprt_switch_xprt_destroy(xprt);
>         xprt_put(xprt);
>  }
>  
> @@ -137,6 +143,9 @@ struct rpc_xprt_switch *xprt_switch_alloc(struct
> rpc_xprt *xprt,
>                 xps->xps_iter_ops = &rpc_xprt_iter_singular;
>                 rpc_sysfs_xprt_switch_setup(xps, xprt, gfp_flags);
>                 xprt_switch_add_xprt_locked(xps, xprt);
> +               xprt_get(xprt);
> +               rpc_sysfs_xprt_switch_xprt_setup(xps, xprt,
> gfp_flags);
> +               xprt_put(xprt);
>         }
>  
>         return xps;
> @@ -152,6 +161,7 @@ static void xprt_switch_free_entries(struct
> rpc_xprt_switch *xps)
>                                 struct rpc_xprt, xprt_switch);
>                 xprt_switch_remove_xprt_locked(xps, xprt);
>                 spin_unlock(&xps->xps_lock);
> +               rpc_sysfs_xprt_switch_xprt_destroy(xprt);
>                 xprt_put(xprt);
>                 spin_lock(&xps->xps_lock);
>         }

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Dan Aloni]

On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia wrote:
> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > An rpc client uses a transport switch and one ore more transports
> > > associated with that switch. Since transports are shared among
> > > rpc clients, create a symlink into the xprt_switch directory
> > > instead of duplicating entries under each rpc client.
> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > >
> > >..
> > > @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
> > >       struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> > >
> > >       if (rpc_client) {
> > > +             char name[23];
> > > +
> > > +             snprintf(name, sizeof(name), "switch-%d",
> > > +                      rpc_client->xprt_switch->xps_id);
> > > +             sysfs_remove_link(&rpc_client->kobject, name);
> >
> > Hi Olga,
> >
> > If a client can use a single switch, shouldn't the name of the symlink
> > be just "switch"? This is to be consistent with other symlinks in
> > `sysfs` such as the ones in block layer, for example in my
> > `/sys/block/sda`:
> >
> >     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
> >     device -> ../../../5:0:0:0
> 
> I think the client is written so that in the future it might have more
> than one switch?

I wonder what would be the use for that, as a switch is already collection of
xprts. Which would determine the switch to use per a new task IO?

-- 
Dan Aloni

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
>
>
> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <dan@kernelim.com> wrote:
>>
>> On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia wrote:
>> > On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
>> > >
>> > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
>> > > > From: Olga Kornievskaia <kolga@netapp.com>
>> > > >
>> > > > An rpc client uses a transport switch and one ore more transports
>> > > > associated with that switch. Since transports are shared among
>> > > > rpc clients, create a symlink into the xprt_switch directory
>> > > > instead of duplicating entries under each rpc client.
>> > > >
>> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> > > >
>> > > >..
>> > > > @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
>> > > >       struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
>> > > >
>> > > >       if (rpc_client) {
>> > > > +             char name[23];
>> > > > +
>> > > > +             snprintf(name, sizeof(name), "switch-%d",
>> > > > +                      rpc_client->xprt_switch->xps_id);
>> > > > +             sysfs_remove_link(&rpc_client->kobject, name);
>> > >
>> > > Hi Olga,
>> > >
>> > > If a client can use a single switch, shouldn't the name of the symlink
>> > > be just "switch"? This is to be consistent with other symlinks in
>> > > `sysfs` such as the ones in block layer, for example in my
>> > > `/sys/block/sda`:
>> > >
>> > >     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
>> > >     device -> ../../../5:0:0:0
>> >
>> > I think the client is written so that in the future it might have more
>> > than one switch?
>>
>> I wonder what would be the use for that, as a switch is already collection of
>> xprts. Which would determine the switch to use per a new task IO?
>
>
> I thought the switch is a collection of xprts of the same type. And if you wanted to have an RDMA connection and a TCP connection to the same server, then it would be stored under different switches? For instance we round-robin thru the transports but I don't see why we would be doing so between a TCP and an RDMA transport. But I see how a client can totally switch from an TCP based transport to an RDMA one (or a set of transports and round-robin among that set). But perhaps I'm wrong in how I'm thinking about xprt_switch and multipathing.

<looks like my reply bounced so trying to resend>

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Dan Aloni]

On Wed, May 12, 2021 at 09:49:01AM -0400, Olga Kornievskaia wrote:
> On Wed, May 12, 2021 at 9:40 AM Olga Kornievskaia <
> olga.kornievskaia@gmail.com> wrote:
> 
> > On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> > <olga.kornievskaia@gmail.com> wrote:
> > > On Wed, May 12, 2021 at 6:42 AM Dan Aloni <dan@kernelim.com> wrote:
> > >> On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia wrote:
> > >> > On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
> > >> > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > >> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > >> > > >
> > >> > > > An rpc client uses a transport switch and one ore more transports
> > >> > > > associated with that switch. Since transports are shared among
> > >> > > > rpc clients, create a symlink into the xprt_switch directory
> > >> > > > instead of duplicating entries under each rpc client.
> > >> > > >
> > >> > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > >> > > >
> > >> > > >..
> > >> > > > @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct
> > rpc_clnt *clnt)
> > >> > > >       struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> > >> > > >
> > >> > > >       if (rpc_client) {
> > >> > > > +             char name[23];
> > >> > > > +
> > >> > > > +             snprintf(name, sizeof(name), "switch-%d",
> > >> > > > +                      rpc_client->xprt_switch->xps_id);
> > >> > > > +             sysfs_remove_link(&rpc_client->kobject, name);
> > >> > >
> > >> > > Hi Olga,
> > >> > >
> > >> > > If a client can use a single switch, shouldn't the name of the
> > symlink
> > >> > > be just "switch"? This is to be consistent with other symlinks in
> > >> > > `sysfs` such as the ones in block layer, for example in my
> > >> > > `/sys/block/sda`:
> > >> > >
> > >> > >     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
> > >> > >     device -> ../../../5:0:0:0
> > >> >
> > >> > I think the client is written so that in the future it might have more
> > >> > than one switch?
> > >>
> > >> I wonder what would be the use for that, as a switch is already
> > collection of
> > >> xprts. Which would determine the switch to use per a new task IO?
> > >
> > >
> > > I thought the switch is a collection of xprts of the same type. And if
> > you wanted to have an RDMA connection and a TCP connection to the same
> > server, then it would be stored under different switches? For instance we
> > round-robin thru the transports but I don't see why we would be doing so
> > between a TCP and an RDMA transport. But I see how a client can totally
> > switch from an TCP based transport to an RDMA one (or a set of transports
> > and round-robin among that set). But perhaps I'm wrong in how I'm thinking
> > about xprt_switch and multipathing.
> >
> > <looks like my reply bounced so trying to resend>
> >
> 
> And more to answer your question, we don't have a method to switch between
> different xprt switches. But we don't have a way to specify how to mount
> with multiple types of transports. Perhaps sysfs could be/would be a way to
> switch between the two. Perhaps during session trunking discovery, the
> server can return back a list of IPs and types of transports. Say all IPs
> would be available via TCP and RDMA, then the client can create a switch
> with RDMA transports and another with TCP transports, then perhaps there
> will be a policy engine that would decide which one to choose to use to
> begin with. And then with sysfs interface would be a way to switch between
> the two if there are problems.

You raise a good point, also relevant to the ability to dynamically add
new transports on the fly with the sysfs interface - their protocol type
may be different.

Returning to the topic of multiple switches per client, I recall that a
few times it was hinted that there is an intention to have the
implementation details of xprtswitch be modified to be loadable and
pluggable with custom algorithms.  And if we are going in that
direction, I'd expect the advanced transport management and request
routing can be below the RPC client level, where we have example uses:

1) Optimizations in request routing that I've previously written about
(based on request data memory).

2) If we lift the restriction of multiple protocol types on the same
xprtswitch on one switch, then we can also allow for the implementation
'RDMA-by-default with TCP failover on standby' similar to what you
suggest, but with one switch maintaining two lists behind the scenes.

-- 
Dan Aloni

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Trond Myklebust]

On Wed, 2021-05-12 at 17:16 +0300, Dan Aloni wrote:
> On Wed, May 12, 2021 at 09:49:01AM -0400, Olga Kornievskaia wrote:
> > On Wed, May 12, 2021 at 9:40 AM Olga Kornievskaia <
> > olga.kornievskaia@gmail.com> wrote:
> > 
> > > On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> > > <olga.kornievskaia@gmail.com> wrote:
> > > > On Wed, May 12, 2021 at 6:42 AM Dan Aloni <dan@kernelim.com>
> > > > wrote:
> > > > > On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia
> > > > > wrote:
> > > > > > On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <
> > > > > > dan@kernelim.com> wrote:
> > > > > > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
> > > > > > > Kornievskaia wrote:
> > > > > > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > > > > > > 
> > > > > > > > An rpc client uses a transport switch and one ore more
> > > > > > > > transports
> > > > > > > > associated with that switch. Since transports are
> > > > > > > > shared among
> > > > > > > > rpc clients, create a symlink into the xprt_switch
> > > > > > > > directory
> > > > > > > > instead of duplicating entries under each rpc client.
> > > > > > > > 
> > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > > > > > > > 
> > > > > > > > ..
> > > > > > > > @@ -188,6 +204,11 @@ void
> > > > > > > > rpc_sysfs_client_destroy(struct
> > > rpc_clnt *clnt)
> > > > > > > >       struct rpc_sysfs_client *rpc_client = clnt-
> > > > > > > > >cl_sysfs;
> > > > > > > > 
> > > > > > > >       if (rpc_client) {
> > > > > > > > +             char name[23];
> > > > > > > > +
> > > > > > > > +             snprintf(name, sizeof(name), "switch-%d",
> > > > > > > > +                      rpc_client->xprt_switch-
> > > > > > > > >xps_id);
> > > > > > > > +             sysfs_remove_link(&rpc_client->kobject,
> > > > > > > > name);
> > > > > > > 
> > > > > > > Hi Olga,
> > > > > > > 
> > > > > > > If a client can use a single switch, shouldn't the name
> > > > > > > of the
> > > symlink
> > > > > > > be just "switch"? This is to be consistent with other
> > > > > > > symlinks in
> > > > > > > `sysfs` such as the ones in block layer, for example in
> > > > > > > my
> > > > > > > `/sys/block/sda`:
> > > > > > > 
> > > > > > >     bdi ->
> > > > > > > ../../../../../../../../../../../virtual/bdi/8:0
> > > > > > >     device -> ../../../5:0:0:0
> > > > > > 
> > > > > > I think the client is written so that in the future it
> > > > > > might have more
> > > > > > than one switch?
> > > > > 
> > > > > I wonder what would be the use for that, as a switch is
> > > > > already
> > > collection of
> > > > > xprts. Which would determine the switch to use per a new task
> > > > > IO?
> > > > 
> > > > 
> > > > I thought the switch is a collection of xprts of the same type.
> > > > And if
> > > you wanted to have an RDMA connection and a TCP connection to the
> > > same
> > > server, then it would be stored under different switches? For
> > > instance we
> > > round-robin thru the transports but I don't see why we would be
> > > doing so
> > > between a TCP and an RDMA transport. But I see how a client can
> > > totally
> > > switch from an TCP based transport to an RDMA one (or a set of
> > > transports
> > > and round-robin among that set). But perhaps I'm wrong in how I'm
> > > thinking
> > > about xprt_switch and multipathing.
> > > 
> > > <looks like my reply bounced so trying to resend>
> > > 
> > 
> > And more to answer your question, we don't have a method to switch
> > between
> > different xprt switches. But we don't have a way to specify how to
> > mount
> > with multiple types of transports. Perhaps sysfs could be/would be
> > a way to
> > switch between the two. Perhaps during session trunking discovery,
> > the
> > server can return back a list of IPs and types of transports. Say
> > all IPs
> > would be available via TCP and RDMA, then the client can create a
> > switch
> > with RDMA transports and another with TCP transports, then perhaps
> > there
> > will be a policy engine that would decide which one to choose to
> > use to
> > begin with. And then with sysfs interface would be a way to switch
> > between
> > the two if there are problems.
> 
> You raise a good point, also relevant to the ability to dynamically
> add
> new transports on the fly with the sysfs interface - their protocol
> type
> may be different.
> 
> Returning to the topic of multiple switches per client, I recall that
> a
> few times it was hinted that there is an intention to have the
> implementation details of xprtswitch be modified to be loadable and
> pluggable with custom algorithms.  And if we are going in that
> direction, I'd expect the advanced transport management and request
> routing can be below the RPC client level, where we have example
> uses:
> 
> 1) Optimizations in request routing that I've previously written
> about
> (based on request data memory).
> 
> 2) If we lift the restriction of multiple protocol types on the same
> xprtswitch on one switch, then we can also allow for the
> implementation
> 'RDMA-by-default with TCP failover on standby' similar to what you
> suggest, but with one switch maintaining two lists behind the scenes.
> 

I'm not that interested in supporting multiple switches per client, or
any setup that is asymmetric w.r.t. transport characteristics at this
time.

Any such setup is going to need a policy engine in order to decide
which RPC calls can be placed on which set of transports. That again
will end up adding a lot of complexity in the kernel itself. I've yet
to see any compelling justification for doing so.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Chuck Lever III]

> On May 13, 2021, at 11:13 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Wed, 2021-05-12 at 17:16 +0300, Dan Aloni wrote:
>> On Wed, May 12, 2021 at 09:49:01AM -0400, Olga Kornievskaia wrote:
>>> On Wed, May 12, 2021 at 9:40 AM Olga Kornievskaia <
>>> olga.kornievskaia@gmail.com> wrote:
>>> 
>>>> On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
>>>> <olga.kornievskaia@gmail.com> wrote:
>>>>> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <dan@kernelim.com>
>>>>> wrote:
>>>>>> On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia
>>>>>> wrote:
>>>>>>> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <
>>>>>>> dan@kernelim.com> wrote:
>>>>>>>> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
>>>>>>>> Kornievskaia wrote:
>>>>>>>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>>>>>>> 
>>>>>>>>> An rpc client uses a transport switch and one ore more
>>>>>>>>> transports
>>>>>>>>> associated with that switch. Since transports are
>>>>>>>>> shared among
>>>>>>>>> rpc clients, create a symlink into the xprt_switch
>>>>>>>>> directory
>>>>>>>>> instead of duplicating entries under each rpc client.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>>>>>>>> 
>>>>>>>>> ..
>>>>>>>>> @@ -188,6 +204,11 @@ void
>>>>>>>>> rpc_sysfs_client_destroy(struct
>>>> rpc_clnt *clnt)
>>>>>>>>>       struct rpc_sysfs_client *rpc_client = clnt-
>>>>>>>>>> cl_sysfs;
>>>>>>>>> 
>>>>>>>>>       if (rpc_client) {
>>>>>>>>> +             char name[23];
>>>>>>>>> +
>>>>>>>>> +             snprintf(name, sizeof(name), "switch-%d",
>>>>>>>>> +                      rpc_client->xprt_switch-
>>>>>>>>>> xps_id);
>>>>>>>>> +             sysfs_remove_link(&rpc_client->kobject,
>>>>>>>>> name);
>>>>>>>> 
>>>>>>>> Hi Olga,
>>>>>>>> 
>>>>>>>> If a client can use a single switch, shouldn't the name
>>>>>>>> of the
>>>> symlink
>>>>>>>> be just "switch"? This is to be consistent with other
>>>>>>>> symlinks in
>>>>>>>> `sysfs` such as the ones in block layer, for example in
>>>>>>>> my
>>>>>>>> `/sys/block/sda`:
>>>>>>>> 
>>>>>>>>     bdi ->
>>>>>>>> ../../../../../../../../../../../virtual/bdi/8:0
>>>>>>>>     device -> ../../../5:0:0:0
>>>>>>> 
>>>>>>> I think the client is written so that in the future it
>>>>>>> might have more
>>>>>>> than one switch?
>>>>>> 
>>>>>> I wonder what would be the use for that, as a switch is
>>>>>> already
>>>> collection of
>>>>>> xprts. Which would determine the switch to use per a new task
>>>>>> IO?
>>>>> 
>>>>> 
>>>>> I thought the switch is a collection of xprts of the same type.
>>>>> And if
>>>> you wanted to have an RDMA connection and a TCP connection to the
>>>> same
>>>> server, then it would be stored under different switches? For
>>>> instance we
>>>> round-robin thru the transports but I don't see why we would be
>>>> doing so
>>>> between a TCP and an RDMA transport. But I see how a client can
>>>> totally
>>>> switch from an TCP based transport to an RDMA one (or a set of
>>>> transports
>>>> and round-robin among that set). But perhaps I'm wrong in how I'm
>>>> thinking
>>>> about xprt_switch and multipathing.
>>>> 
>>>> <looks like my reply bounced so trying to resend>
>>>> 
>>> 
>>> And more to answer your question, we don't have a method to switch
>>> between
>>> different xprt switches. But we don't have a way to specify how to
>>> mount
>>> with multiple types of transports. Perhaps sysfs could be/would be
>>> a way to
>>> switch between the two. Perhaps during session trunking discovery,
>>> the
>>> server can return back a list of IPs and types of transports. Say
>>> all IPs
>>> would be available via TCP and RDMA, then the client can create a
>>> switch
>>> with RDMA transports and another with TCP transports, then perhaps
>>> there
>>> will be a policy engine that would decide which one to choose to
>>> use to
>>> begin with. And then with sysfs interface would be a way to switch
>>> between
>>> the two if there are problems.
>> 
>> You raise a good point, also relevant to the ability to dynamically
>> add
>> new transports on the fly with the sysfs interface - their protocol
>> type
>> may be different.
>> 
>> Returning to the topic of multiple switches per client, I recall that
>> a
>> few times it was hinted that there is an intention to have the
>> implementation details of xprtswitch be modified to be loadable and
>> pluggable with custom algorithms.  And if we are going in that
>> direction, I'd expect the advanced transport management and request
>> routing can be below the RPC client level, where we have example
>> uses:
>> 
>> 1) Optimizations in request routing that I've previously written
>> about
>> (based on request data memory).
>> 
>> 2) If we lift the restriction of multiple protocol types on the same
>> xprtswitch on one switch, then we can also allow for the
>> implementation
>> 'RDMA-by-default with TCP failover on standby' similar to what you
>> suggest, but with one switch maintaining two lists behind the scenes.
>> 
> 
> I'm not that interested in supporting multiple switches per client, or
> any setup that is asymmetric w.r.t. transport characteristics at this
> time.
> 
> Any such setup is going to need a policy engine in order to decide
> which RPC calls can be placed on which set of transports. That again
> will end up adding a lot of complexity in the kernel itself. I've yet
> to see any compelling justification for doing so.

I agree -- SMB multi-channel allows TCP+RDMA configurations, and its
tough to decide how to distribute work across connections and NICs
that have such vastly different performance characteristics.

I would like to see us crawling and walking before trying to run.


--
Chuck Lever

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

On Thu, May 13, 2021 at 11:18 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On May 13, 2021, at 11:13 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Wed, 2021-05-12 at 17:16 +0300, Dan Aloni wrote:
> >> On Wed, May 12, 2021 at 09:49:01AM -0400, Olga Kornievskaia wrote:
> >>> On Wed, May 12, 2021 at 9:40 AM Olga Kornievskaia <
> >>> olga.kornievskaia@gmail.com> wrote:
> >>>
> >>>> On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> >>>> <olga.kornievskaia@gmail.com> wrote:
> >>>>> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <dan@kernelim.com>
> >>>>> wrote:
> >>>>>> On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia
> >>>>>> wrote:
> >>>>>>> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <
> >>>>>>> dan@kernelim.com> wrote:
> >>>>>>>> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
> >>>>>>>> Kornievskaia wrote:
> >>>>>>>>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>>>>>>>
> >>>>>>>>> An rpc client uses a transport switch and one ore more
> >>>>>>>>> transports
> >>>>>>>>> associated with that switch. Since transports are
> >>>>>>>>> shared among
> >>>>>>>>> rpc clients, create a symlink into the xprt_switch
> >>>>>>>>> directory
> >>>>>>>>> instead of duplicating entries under each rpc client.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>>>>>>>>
> >>>>>>>>> ..
> >>>>>>>>> @@ -188,6 +204,11 @@ void
> >>>>>>>>> rpc_sysfs_client_destroy(struct
> >>>> rpc_clnt *clnt)
> >>>>>>>>>       struct rpc_sysfs_client *rpc_client = clnt-
> >>>>>>>>>> cl_sysfs;
> >>>>>>>>>
> >>>>>>>>>       if (rpc_client) {
> >>>>>>>>> +             char name[23];
> >>>>>>>>> +
> >>>>>>>>> +             snprintf(name, sizeof(name), "switch-%d",
> >>>>>>>>> +                      rpc_client->xprt_switch-
> >>>>>>>>>> xps_id);
> >>>>>>>>> +             sysfs_remove_link(&rpc_client->kobject,
> >>>>>>>>> name);
> >>>>>>>>
> >>>>>>>> Hi Olga,
> >>>>>>>>
> >>>>>>>> If a client can use a single switch, shouldn't the name
> >>>>>>>> of the
> >>>> symlink
> >>>>>>>> be just "switch"? This is to be consistent with other
> >>>>>>>> symlinks in
> >>>>>>>> `sysfs` such as the ones in block layer, for example in
> >>>>>>>> my
> >>>>>>>> `/sys/block/sda`:
> >>>>>>>>
> >>>>>>>>     bdi ->
> >>>>>>>> ../../../../../../../../../../../virtual/bdi/8:0
> >>>>>>>>     device -> ../../../5:0:0:0
> >>>>>>>
> >>>>>>> I think the client is written so that in the future it
> >>>>>>> might have more
> >>>>>>> than one switch?
> >>>>>>
> >>>>>> I wonder what would be the use for that, as a switch is
> >>>>>> already
> >>>> collection of
> >>>>>> xprts. Which would determine the switch to use per a new task
> >>>>>> IO?
> >>>>>
> >>>>>
> >>>>> I thought the switch is a collection of xprts of the same type.
> >>>>> And if
> >>>> you wanted to have an RDMA connection and a TCP connection to the
> >>>> same
> >>>> server, then it would be stored under different switches? For
> >>>> instance we
> >>>> round-robin thru the transports but I don't see why we would be
> >>>> doing so
> >>>> between a TCP and an RDMA transport. But I see how a client can
> >>>> totally
> >>>> switch from an TCP based transport to an RDMA one (or a set of
> >>>> transports
> >>>> and round-robin among that set). But perhaps I'm wrong in how I'm
> >>>> thinking
> >>>> about xprt_switch and multipathing.
> >>>>
> >>>> <looks like my reply bounced so trying to resend>
> >>>>
> >>>
> >>> And more to answer your question, we don't have a method to switch
> >>> between
> >>> different xprt switches. But we don't have a way to specify how to
> >>> mount
> >>> with multiple types of transports. Perhaps sysfs could be/would be
> >>> a way to
> >>> switch between the two. Perhaps during session trunking discovery,
> >>> the
> >>> server can return back a list of IPs and types of transports. Say
> >>> all IPs
> >>> would be available via TCP and RDMA, then the client can create a
> >>> switch
> >>> with RDMA transports and another with TCP transports, then perhaps
> >>> there
> >>> will be a policy engine that would decide which one to choose to
> >>> use to
> >>> begin with. And then with sysfs interface would be a way to switch
> >>> between
> >>> the two if there are problems.
> >>
> >> You raise a good point, also relevant to the ability to dynamically
> >> add
> >> new transports on the fly with the sysfs interface - their protocol
> >> type
> >> may be different.
> >>
> >> Returning to the topic of multiple switches per client, I recall that
> >> a
> >> few times it was hinted that there is an intention to have the
> >> implementation details of xprtswitch be modified to be loadable and
> >> pluggable with custom algorithms.  And if we are going in that
> >> direction, I'd expect the advanced transport management and request
> >> routing can be below the RPC client level, where we have example
> >> uses:
> >>
> >> 1) Optimizations in request routing that I've previously written
> >> about
> >> (based on request data memory).
> >>
> >> 2) If we lift the restriction of multiple protocol types on the same
> >> xprtswitch on one switch, then we can also allow for the
> >> implementation
> >> 'RDMA-by-default with TCP failover on standby' similar to what you
> >> suggest, but with one switch maintaining two lists behind the scenes.
> >>
> >
> > I'm not that interested in supporting multiple switches per client, or
> > any setup that is asymmetric w.r.t. transport characteristics at this
> > time.
> >
> > Any such setup is going to need a policy engine in order to decide
> > which RPC calls can be placed on which set of transports. That again
> > will end up adding a lot of complexity in the kernel itself. I've yet
> > to see any compelling justification for doing so.
>
> I agree -- SMB multi-channel allows TCP+RDMA configurations, and its
> tough to decide how to distribute work across connections and NICs
> that have such vastly different performance characteristics.
>
> I would like to see us crawling and walking before trying to run.

Sounds good folks. I'll remove the multiple switches from the sysfs
infrastructure. v7 coming up.

>
>
> --
> Chuck Lever
>
>
>

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Tom Talpey]

On 5/13/2021 11:18 AM, Chuck Lever III wrote:
> I agree -- SMB multi-channel allows TCP+RDMA configurations, and its
> tough to decide how to distribute work across connections and NICs
> that have such vastly different performance characteristics.

Purely FYI on this point - SMB does not attempt to perform operations
over links which differ in transport protocol nor link speed. It
prefers RDMA over any TCP, and higher link speed over lower. When
multiple tiers exist, it relegates lower-tier connections to standby
status.

Most implementations have various tweaks to prefer or to poison
certain transports or connection associations, but the basics
are always to use homogeneous links.

Tom.

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
>
> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > An rpc client uses a transport switch and one ore more transports
> > associated with that switch. Since transports are shared among
> > rpc clients, create a symlink into the xprt_switch directory
> > instead of duplicating entries under each rpc client.
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >
> >..
> > @@ -188,6 +204,11 @@ void rpc_sysfs_client_destroy(struct rpc_clnt *clnt)
> >       struct rpc_sysfs_client *rpc_client = clnt->cl_sysfs;
> >
> >       if (rpc_client) {
> > +             char name[23];
> > +
> > +             snprintf(name, sizeof(name), "switch-%d",
> > +                      rpc_client->xprt_switch->xps_id);
> > +             sysfs_remove_link(&rpc_client->kobject, name);
>
> Hi Olga,
>
> If a client can use a single switch, shouldn't the name of the symlink
> be just "switch"? This is to be consistent with other symlinks in
> `sysfs` such as the ones in block layer, for example in my
> `/sys/block/sda`:
>
>     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
>     device -> ../../../5:0:0:0
>

Jumping back to this comment because now that I went back to try to
modify the code I'm having doubts.

We still need numbering of xprt switches because they are different
for different mounts. So xprt_switches directory would still have
switch-0 for say a mount to server A and then switch-0 for a mount to
server B.  While yes I see that for a given rpc client that's making a
link into a xprt_switches directory will only have 1 link. And "yes"
the name of the link could be "switch". But isn't it more informative
to keep this to be the same name as the name of the directory under
the xprt_switches?

>
> --
> Dan Aloni

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Benjamin Coddington]

On 13 May 2021, at 17:18, Olga Kornievskaia wrote:
> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
>> If a client can use a single switch, shouldn't the name of the symlink
>> be just "switch"? This is to be consistent with other symlinks in
>> `sysfs` such as the ones in block layer, for example in my
>> `/sys/block/sda`:
>>
>>     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
>>     device -> ../../../5:0:0:0
>>
>
> Jumping back to this comment because now that I went back to try to
> modify the code I'm having doubts.
>
> We still need numbering of xprt switches because they are different
> for different mounts. So xprt_switches directory would still have
> switch-0 for say a mount to server A and then switch-0 for a mount to
> server B.  While yes I see that for a given rpc client that's making a
> link into a xprt_switches directory will only have 1 link. And "yes"
> the name of the link could be "switch". But isn't it more informative
> to keep this to be the same name as the name of the directory under
> the xprt_switches?

The information isn't lost, as the symlink points to the specific switch.
Not using a number in the symlink name informs that there will only be one
switch for each client and makes it more deterministic for users and
software to navigate.

Ben

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Olga Kornievskaia]

On Fri, May 14, 2021 at 6:17 AM Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 13 May 2021, at 17:18, Olga Kornievskaia wrote:
> > On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <dan@kernelim.com> wrote:
> >> If a client can use a single switch, shouldn't the name of the symlink
> >> be just "switch"? This is to be consistent with other symlinks in
> >> `sysfs` such as the ones in block layer, for example in my
> >> `/sys/block/sda`:
> >>
> >>     bdi -> ../../../../../../../../../../../virtual/bdi/8:0
> >>     device -> ../../../5:0:0:0
> >>
> >
> > Jumping back to this comment because now that I went back to try to
> > modify the code I'm having doubts.
> >
> > We still need numbering of xprt switches because they are different
> > for different mounts. So xprt_switches directory would still have
> > switch-0 for say a mount to server A and then switch-0 for a mount to
> > server B.  While yes I see that for a given rpc client that's making a
> > link into a xprt_switches directory will only have 1 link. And "yes"
> > the name of the link could be "switch". But isn't it more informative
> > to keep this to be the same name as the name of the directory under
> > the xprt_switches?
>
> The information isn't lost, as the symlink points to the specific switch.
> Not using a number in the symlink name informs that there will only be one
> switch for each client and makes it more deterministic for users and
> software to navigate.

What will be lost is that when you look at the xprt_switches directory
and see switch-1... switch-10 subdirectory, there is no way to tell
which rpc client uses which switch. Because each client-1 directory
will only have an entry saying "switch".

Anyway, I submitted the new version but I think it's not as good as
the original.

>
> Ben
>

Re: [PATCH v3 09/13] sunrpc: add a symlink from rpc-client directory to the xprt_switch

[Benjamin Coddington]

On 14 May 2021, at 10:16, Olga Kornievskaia wrote:
> On Fri, May 14, 2021 at 6:17 AM Benjamin Coddington 
> <bcodding@redhat.com> wrote:
>> The information isn't lost, as the symlink points to the specific 
>> switch.
>> Not using a number in the symlink name informs that there will only 
>> be one
>> switch for each client and makes it more deterministic for users and
>> software to navigate.
>
> What will be lost is that when you look at the xprt_switches directory
> and see switch-1... switch-10 subdirectory, there is no way to tell
> which rpc client uses which switch. Because each client-1 directory
> will only have an entry saying "switch".
>
> Anyway, I submitted the new version but I think it's not as good as
> the original.

Hmm, ok - will we ever need to traverse objects in that direction 
though?
I'm thinking that operations on xprts/rpcs will always start from a 
mount
perspective from the admin, but really the root object is struct 
nfs_server,
or bdi.  I'm thinking of something like:

     /sys/fs/nfs/<bdi>/rpc_clnt -> ../../net/sunrpc/clnt-0
     /sys/fs/nfs/<bdi>/volumes
     ...

I suppose though you could have something monitoring the xprts, and upon
finding a particular state would want to navigate back up to see what 
client
is affected.  At that point you'd have to read all the symlinks for all 
the
rpc_clients.

Ben