From: Olga Kornievskaia <[email protected]>
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
From: Olga Kornievskaia <[email protected]>
Add xprt_switch directory to the sysfs and create individual
xprt_swith subdirectories for multipath transport group.
Signed-off-by: Olga Kornievskaia <[email protected]>
---
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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
On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
>
>..
> @@ -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
On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <[email protected]> wrote:
>
> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > 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 <[email protected]>
> >
> >..
> > @@ -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
On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> Add xprt_switch directory to the sysfs and create individual
> xprt_swith subdirectories for multipath transport group.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> 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
[email protected]
On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
[email protected]
On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
[email protected]
On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
[email protected]
On Tue, Apr 27, 2021 at 08:12:53AM -0400, Olga Kornievskaia wrote:
> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <[email protected]> wrote:
> >
> > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <[email protected]>
> > >
> > > 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 <[email protected]>
> > >
> > >..
> > > @@ -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
On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
<[email protected]> wrote:
>
>
>
> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <[email protected]> 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 <[email protected]> wrote:
>> > >
>> > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
>> > > > From: Olga Kornievskaia <[email protected]>
>> > > >
>> > > > 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 <[email protected]>
>> > > >
>> > > >..
>> > > > @@ -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>
On Wed, May 12, 2021 at 09:49:01AM -0400, Olga Kornievskaia wrote:
> On Wed, May 12, 2021 at 9:40 AM Olga Kornievskaia <
> [email protected]> wrote:
>
> > On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> > <[email protected]> wrote:
> > > On Wed, May 12, 2021 at 6:42 AM Dan Aloni <[email protected]> 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 <[email protected]> wrote:
> > >> > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > >> > > > From: Olga Kornievskaia <[email protected]>
> > >> > > >
> > >> > > > 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 <[email protected]>
> > >> > > >
> > >> > > >..
> > >> > > > @@ -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
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 <
> > [email protected]> wrote:
> >
> > > On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> > > <[email protected]> wrote:
> > > > On Wed, May 12, 2021 at 6:42 AM Dan Aloni <[email protected]>
> > > > 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 <
> > > > > > [email protected]> wrote:
> > > > > > > On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
> > > > > > > Kornievskaia wrote:
> > > > > > > > From: Olga Kornievskaia <[email protected]>
> > > > > > > >
> > > > > > > > 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 <[email protected]>
> > > > > > > >
> > > > > > > > ..
> > > > > > > > @@ -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
[email protected]
> On May 13, 2021, at 11:13 AM, Trond Myklebust <[email protected]> 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 <
>>> [email protected]> wrote:
>>>
>>>> On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
>>>> <[email protected]> wrote:
>>>>> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <[email protected]>
>>>>> 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 <
>>>>>>> [email protected]> wrote:
>>>>>>>> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
>>>>>>>> Kornievskaia wrote:
>>>>>>>>> From: Olga Kornievskaia <[email protected]>
>>>>>>>>>
>>>>>>>>> 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 <[email protected]>
>>>>>>>>>
>>>>>>>>> ..
>>>>>>>>> @@ -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
On Thu, May 13, 2021 at 11:18 AM Chuck Lever III <[email protected]> wrote:
>
>
>
> > On May 13, 2021, at 11:13 AM, Trond Myklebust <[email protected]> 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 <
> >>> [email protected]> wrote:
> >>>
> >>>> On Wed, May 12, 2021 at 9:37 AM Olga Kornievskaia
> >>>> <[email protected]> wrote:
> >>>>> On Wed, May 12, 2021 at 6:42 AM Dan Aloni <[email protected]>
> >>>>> 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 <
> >>>>>>> [email protected]> wrote:
> >>>>>>>> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga
> >>>>>>>> Kornievskaia wrote:
> >>>>>>>>> From: Olga Kornievskaia <[email protected]>
> >>>>>>>>>
> >>>>>>>>> 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 <[email protected]>
> >>>>>>>>>
> >>>>>>>>> ..
> >>>>>>>>> @@ -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
>
>
>
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.
On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <[email protected]> wrote:
>
> On Mon, Apr 26, 2021 at 01:19:43PM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > 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 <[email protected]>
> >
> >..
> > @@ -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
On 13 May 2021, at 17:18, Olga Kornievskaia wrote:
> On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <[email protected]> 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
On Fri, May 14, 2021 at 6:17 AM Benjamin Coddington <[email protected]> wrote:
>
> On 13 May 2021, at 17:18, Olga Kornievskaia wrote:
> > On Tue, Apr 27, 2021 at 12:42 AM Dan Aloni <[email protected]> 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
>
On 14 May 2021, at 10:16, Olga Kornievskaia wrote:
> On Fri, May 14, 2021 at 6:17 AM Benjamin Coddington
> <[email protected]> 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