LinuxLists.cc - [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

[permalink] [raw]

Subject: [PATCH v2 3/5] sunrpc: Create per-rpc_clnt sysfs kobjects

From: Anna Schumaker <[email protected]>

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

Signed-off-by: Anna Schumaker <[email protected]>
---
include/linux/sunrpc/clnt.h | 1 +
net/sunrpc/clnt.c | 5 ++++
net/sunrpc/sysfs.c | 60 +++++++++++++++++++++++++++++++++++++
net/sunrpc/sysfs.h | 8 +++++
4 files changed, 74 insertions(+)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 02e7a5863d28..503653720e18 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -71,6 +71,7 @@ struct rpc_clnt {
#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
struct dentry *cl_debugfs; /* debugfs directory */
#endif
+ void *cl_sysfs; /* sysfs directory */
/* cl_work is only needed after cl_xpi is no longer used,
* and that are of similar size
*/
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 612f0a641f4c..02905eae5c0a 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -41,6 +41,7 @@
#include <trace/events/sunrpc.h>

#include "sunrpc.h"
+#include "sysfs.h"
#include "netns.h"

#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
@@ -300,6 +301,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
int err;

rpc_clnt_debugfs_register(clnt);
+ rpc_netns_sysfs_setup(clnt, net);

pipefs_sb = rpc_get_sb_net(net);
if (pipefs_sb) {
@@ -327,6 +329,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
out:
if (pipefs_sb)
rpc_put_sb_net(net);
+ rpc_netns_sysfs_destroy(clnt);
rpc_clnt_debugfs_unregister(clnt);
return err;
}
@@ -733,6 +736,7 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,

rpc_unregister_client(clnt);
__rpc_clnt_remove_pipedir(clnt);
+ rpc_netns_sysfs_destroy(clnt);
rpc_clnt_debugfs_unregister(clnt);

/*
@@ -879,6 +883,7 @@ static void rpc_free_client_work(struct work_struct *work)
* so they cannot be called in rpciod, so they are handled separately
* here.
*/
+ rpc_netns_sysfs_destroy(clnt);
rpc_clnt_debugfs_unregister(clnt);
rpc_free_clid(clnt);
rpc_clnt_remove_pipedir(clnt);
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index ad6a5de5927c..42a690f8bb52 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -4,6 +4,7 @@
*/
#include <linux/sunrpc/clnt.h>
#include <linux/kobject.h>
+#include "sysfs.h"

struct kobject *rpc_client_kobj;
static struct kset *rpc_client_kset;
@@ -54,8 +55,67 @@ int rpc_sysfs_init(void)
return 0;
}

+static void rpc_netns_client_release(struct kobject *kobj)
+{
+ struct rpc_netns_client *c;
+
+ c = container_of(kobj, struct rpc_netns_client, kobject);
+ kfree(c);
+}
+
+static const void *rpc_netns_client_namespace(struct kobject *kobj)
+{
+ return container_of(kobj, struct rpc_netns_client, kobject)->net;
+}
+
+static struct kobj_type rpc_netns_client_type = {
+ .release = rpc_netns_client_release,
+ .sysfs_ops = &kobj_sysfs_ops,
+ .namespace = rpc_netns_client_namespace,
+};
+
void rpc_sysfs_exit(void)
{
kobject_put(rpc_client_kobj);
kset_unregister(rpc_client_kset);
}
+
+static struct rpc_netns_client *rpc_netns_client_alloc(struct kobject *parent,
+ struct net *net, int clid)
+{
+ struct rpc_netns_client *p;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (p) {
+ p->net = net;
+ p->kobject.kset = rpc_client_kset;
+ if (kobject_init_and_add(&p->kobject, &rpc_netns_client_type,
+ parent, "%d", clid) == 0)
+ return p;
+ kobject_put(&p->kobject);
+ }
+ return NULL;
+}
+
+void rpc_netns_sysfs_setup(struct rpc_clnt *clnt, struct net *net)
+{
+ struct rpc_netns_client *rpc_client;
+
+ rpc_client = rpc_netns_client_alloc(rpc_client_kobj, net, clnt->cl_clid);
+ if (rpc_client) {
+ clnt->cl_sysfs = rpc_client;
+ kobject_uevent(&rpc_client->kobject, KOBJ_ADD);
+ }
+}
+
+void rpc_netns_sysfs_destroy(struct rpc_clnt *clnt)
+{
+ struct rpc_netns_client *rpc_client = clnt->cl_sysfs;
+
+ if (rpc_client) {
+ kobject_uevent(&rpc_client->kobject, KOBJ_REMOVE);
+ kobject_del(&rpc_client->kobject);
+ kobject_put(&rpc_client->kobject);
+ clnt->cl_sysfs = NULL;
+ }
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index 93c3cd220506..279a836594e7 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -5,9 +5,17 @@
#ifndef __SUNRPC_SYSFS_H
#define __SUNRPC_SYSFS_H

+struct rpc_netns_client {
+ struct kobject kobject;
+ struct net *net;
+};
+
extern struct kobject *rpc_client_kobj;

extern int rpc_sysfs_init(void);
extern void rpc_sysfs_exit(void);

+void rpc_netns_sysfs_setup(struct rpc_clnt *clnt, struct net *net);
+void rpc_netns_sysfs_destroy(struct rpc_clnt *clnt);
+
#endif
--
2.29.2

2021-02-02 18:47:52

[permalink] [raw]

Subject: [PATCH v2 1/5] sunrpc: Create a sunrpc directory under /sys/kernel/

From: Anna Schumaker <[email protected]>

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

Signed-off-by: Anna Schumaker <[email protected]>
---
v2: Move directory from /sys/net/sunrpc/ to /sys/kernel/sunrpc/
---
net/sunrpc/Makefile | 2 +-
net/sunrpc/sunrpc_syms.c | 8 ++++++++
net/sunrpc/sysfs.c | 20 ++++++++++++++++++++
net/sunrpc/sysfs.h | 13 +++++++++++++
4 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 net/sunrpc/sysfs.c
create mode 100644 net/sunrpc/sysfs.h

diff --git a/net/sunrpc/Makefile b/net/sunrpc/Makefile
index 9488600451e8..1c8de397d6ad 100644
--- a/net/sunrpc/Makefile
+++ b/net/sunrpc/Makefile
@@ -12,7 +12,7 @@ sunrpc-y := clnt.o xprt.o socklib.o xprtsock.o sched.o \
auth.o auth_null.o auth_unix.o \
svc.o svcsock.o svcauth.o svcauth_unix.o \
addr.o rpcb_clnt.o timer.o xdr.o \
- sunrpc_syms.o cache.o rpc_pipe.o \
+ sunrpc_syms.o cache.o rpc_pipe.o sysfs.o \
svc_xprt.o \
xprtmultipath.o
sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 236fadc4a439..3b57efc692ec 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -24,6 +24,7 @@
#include <linux/sunrpc/xprtsock.h>

#include "sunrpc.h"
+#include "sysfs.h"
#include "netns.h"

unsigned int sunrpc_net_id;
@@ -103,6 +104,10 @@ init_sunrpc(void)
if (err)
goto out4;

+ err = rpc_sysfs_init();
+ if (err)
+ goto out5;
+
sunrpc_debugfs_init();
#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
rpc_register_sysctl();
@@ -111,6 +116,8 @@ init_sunrpc(void)
init_socket_xprt(); /* clnt sock transport */
return 0;

+out5:
+ unregister_rpc_pipefs();
out4:
unregister_pernet_subsys(&sunrpc_net_ops);
out3:
@@ -124,6 +131,7 @@ init_sunrpc(void)
static void __exit
cleanup_sunrpc(void)
{
+ rpc_sysfs_exit();
rpc_cleanup_clids();
rpcauth_remove_module();
cleanup_socket_xprt();
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
new file mode 100644
index 000000000000..fbb8db50eb29
--- /dev/null
+++ b/net/sunrpc/sysfs.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Anna Schumaker <[email protected]>
+ */
+#include <linux/kobject.h>
+
+static struct kset *rpc_client_kset;
+
+int rpc_sysfs_init(void)
+{
+ rpc_client_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
+ if (!rpc_client_kset)
+ return -ENOMEM;
+ return 0;
+}
+
+void rpc_sysfs_exit(void)
+{
+ kset_unregister(rpc_client_kset);
+}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
new file mode 100644
index 000000000000..93c3cd220506
--- /dev/null
+++ b/net/sunrpc/sysfs.h
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Anna Schumaker <[email protected]>
+ */
+#ifndef __SUNRPC_SYSFS_H
+#define __SUNRPC_SYSFS_H
+
+extern struct kobject *rpc_client_kobj;
+
+extern int rpc_sysfs_init(void);
+extern void rpc_sysfs_exit(void);
+
+#endif
--
2.29.2

2021-02-02 18:48:31

[permalink] [raw]

Subject: [PATCH v2 2/5] sunrpc: Create a net/ subdirectory in the sunrpc sysfs

From: Anna Schumaker <[email protected]>

For network namespace separation.

Signed-off-by: Anna Schumaker <[email protected]>
---
net/sunrpc/sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index fbb8db50eb29..ad6a5de5927c 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -2,19 +2,60 @@
/*
* Copyright (c) 2020 Anna Schumaker <[email protected]>
*/
+#include <linux/sunrpc/clnt.h>
#include <linux/kobject.h>

+struct kobject *rpc_client_kobj;
static struct kset *rpc_client_kset;

+static void rpc_netns_object_release(struct kobject *kobj)
+{
+ kfree(kobj);
+}
+
+static const struct kobj_ns_type_operations *rpc_netns_object_child_ns_type(
+ struct kobject *kobj)
+{
+ return &net_ns_type_operations;
+}
+
+static struct kobj_type rpc_netns_object_type = {
+ .release = rpc_netns_object_release,
+ .sysfs_ops = &kobj_sysfs_ops,
+ .child_ns_type = rpc_netns_object_child_ns_type,
+};
+
+static struct kobject *rpc_netns_object_alloc(const char *name,
+ struct kset *kset, struct kobject *parent)
+{
+ struct kobject *kobj;
+ kobj = kzalloc(sizeof(*kobj), GFP_KERNEL);
+ if (kobj) {
+ kobj->kset = kset;
+ if (kobject_init_and_add(kobj, &rpc_netns_object_type,
+ parent, "%s", name) == 0)
+ return kobj;
+ kobject_put(kobj);
+ }
+ return NULL;
+}
+
int rpc_sysfs_init(void)
{
rpc_client_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj);
if (!rpc_client_kset)
return -ENOMEM;
+ rpc_client_kobj = rpc_netns_object_alloc("net", rpc_client_kset, NULL);
+ if (!rpc_client_kobj) {
+ kset_unregister(rpc_client_kset);
+ rpc_client_kset = NULL;
+ return -ENOMEM;
+ }
return 0;
}

void rpc_sysfs_exit(void)
{
+ kobject_put(rpc_client_kobj);
kset_unregister(rpc_client_kset);
}
--
2.29.2

2021-02-02 18:48:57

[permalink] [raw]

Subject: [PATCH v2 4/5] sunrpc: Prepare xs_connect() for taking NULL tasks

From: Anna Schumaker <[email protected]>

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

Signed-off-by: Anna Schumaker <[email protected]>
---
net/sunrpc/xprtsock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index c56a66cdf4ac..250abf1aa018 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
unsigned long delay = 0;

- WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));
+ if (task)
+ WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));

if (transport->sock != NULL) {
dprintk("RPC: xs_connect delayed xprt %p for %lu "
--
2.29.2

2021-02-02 18:49:21

[permalink] [raw]

Subject: [PATCH v2 5/5] sunrpc: Create a per-rpc_clnt file for managing the destination IP address

From: Anna Schumaker <[email protected]>

Reading the file displays the current destination address, and writing
to it allows users to change the address.

And since we're using IP here, restrict to only creating sysfs files for
TCP and RDMA connections.

Signed-off-by: Anna Schumaker <[email protected]>
---
v2: Change filename and related functions from "address" to "dstaddr"
Combine patches for reading and writing to this file
---
net/sunrpc/sysfs.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
net/sunrpc/sysfs.h | 1 +
2 files changed, 48 insertions(+)

diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 42a690f8bb52..8b01b4df64ee 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -3,6 +3,8 @@
* Copyright (c) 2020 Anna Schumaker <[email protected]>
*/
#include <linux/sunrpc/clnt.h>
+#include <linux/sunrpc/addr.h>
+#include <linux/sunrpc/xprtsock.h>
#include <linux/kobject.h>
#include "sysfs.h"

@@ -55,6 +57,37 @@ int rpc_sysfs_init(void)
return 0;
}

+static ssize_t rpc_netns_dstaddr_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct rpc_netns_client *c = container_of(kobj,
+ struct rpc_netns_client, kobject);
+ struct rpc_clnt *clnt = c->clnt;
+ struct rpc_xprt *xprt = rcu_dereference(clnt->cl_xprt);
+
+ return rpc_ntop((struct sockaddr *)&xprt->addr, buf, PAGE_SIZE);
+}
+
+static ssize_t rpc_netns_dstaddr_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t count)
+{
+ struct rpc_netns_client *c = container_of(kobj,
+ struct rpc_netns_client, kobject);
+ struct rpc_clnt *clnt = c->clnt;
+ struct rpc_xprt *xprt = rcu_dereference(clnt->cl_xprt);
+ struct sockaddr *saddr = (struct sockaddr *)&xprt->addr;
+ int port = rpc_get_port(saddr);
+
+ xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, sizeof(*saddr));
+ rpc_set_port(saddr, port);
+
+ kfree(xprt->address_strings[RPC_DISPLAY_ADDR]);
+ xprt->address_strings[RPC_DISPLAY_ADDR] = kstrndup(buf, count - 1, GFP_KERNEL);
+
+ xprt->ops->connect(xprt, NULL);
+ return count;
+}
+
static void rpc_netns_client_release(struct kobject *kobj)
{
struct rpc_netns_client *c;
@@ -68,8 +101,17 @@ static const void *rpc_netns_client_namespace(struct kobject *kobj)
return container_of(kobj, struct rpc_netns_client, kobject)->net;
}

+static struct kobj_attribute rpc_netns_client_dstaddr = __ATTR(dstaddr,
+ 0644, rpc_netns_dstaddr_show, rpc_netns_dstaddr_store);
+
+static struct attribute *rpc_netns_client_attrs[] = {
+ &rpc_netns_client_dstaddr.attr,
+ NULL,
+};
+
static struct kobj_type rpc_netns_client_type = {
.release = rpc_netns_client_release,
+ .default_attrs = rpc_netns_client_attrs,
.sysfs_ops = &kobj_sysfs_ops,
.namespace = rpc_netns_client_namespace,
};
@@ -100,10 +142,15 @@ static struct rpc_netns_client *rpc_netns_client_alloc(struct kobject *parent,
void rpc_netns_sysfs_setup(struct rpc_clnt *clnt, struct net *net)
{
struct rpc_netns_client *rpc_client;
+ struct rpc_xprt *xprt = rcu_dereference(clnt->cl_xprt);
+
+ if (!(xprt->prot & (IPPROTO_TCP | XPRT_TRANSPORT_RDMA)))
+ return;

rpc_client = rpc_netns_client_alloc(rpc_client_kobj, net, clnt->cl_clid);
if (rpc_client) {
clnt->cl_sysfs = rpc_client;
+ rpc_client->clnt = clnt;
kobject_uevent(&rpc_client->kobject, KOBJ_ADD);
}
}
diff --git a/net/sunrpc/sysfs.h b/net/sunrpc/sysfs.h
index 279a836594e7..137a12c87954 100644
--- a/net/sunrpc/sysfs.h
+++ b/net/sunrpc/sysfs.h
@@ -8,6 +8,7 @@
struct rpc_netns_client {
struct kobject kobject;
struct net *net;
+ struct rpc_clnt *clnt;
};

extern struct kobject *rpc_client_kobj;
--
2.29.2

2021-02-02 18:55:29

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

You're welcome! I'll try to remember to CC him on future versions

On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever <[email protected]> wrote:
>
> I want to ensure Dan is aware of this work. Thanks for posting, Anna!
>
> > On Feb 2, 2021, at 1:42 PM, [email protected] wrote:
> >
> > From: Anna Schumaker <[email protected]>
> >
> > It's possible for an NFS server to go down but come back up with a
> > different IP address. These patches provide a way for administrators to
> > handle this issue by providing a new IP address for xprt sockets to
> > connect to.
> >
> > Chuck has suggested some ideas for future work that could also use this
> > interface, such as:
> > - srcaddr: To move between network devices on the client
> > - type: "tcp", "rdma", "local"
> > - bound: 0 for autobind, or the result of the most recent rpcbind query
> > - connected: either true or false
> > - last: read-only timestamp of the last operation to use the transport
> > - device: A symlink to the physical network device
> >
> > Changes in v2:
> > - Put files under /sys/kernel/sunrpc/ instead of /sys/net/sunrpc/
> > - Rename file from "address" to "dstaddr"
> >
> > Thoughts?
> > Anna
> >
> >
> > Anna Schumaker (5):
> > sunrpc: Create a sunrpc directory under /sys/kernel/
> > sunrpc: Create a net/ subdirectory in the sunrpc sysfs
> > sunrpc: Create per-rpc_clnt sysfs kobjects
> > sunrpc: Prepare xs_connect() for taking NULL tasks
> > sunrpc: Create a per-rpc_clnt file for managing the destination IP
> > address
> >
> > include/linux/sunrpc/clnt.h | 1 +
> > net/sunrpc/Makefile | 2 +-
> > net/sunrpc/clnt.c | 5 ++
> > net/sunrpc/sunrpc_syms.c | 8 ++
> > net/sunrpc/sysfs.c | 168 ++++++++++++++++++++++++++++++++++++
> > net/sunrpc/sysfs.h | 22 +++++
> > net/sunrpc/xprtsock.c | 3 +-
> > 7 files changed, 207 insertions(+), 2 deletions(-)
> > create mode 100644 net/sunrpc/sysfs.c
> > create mode 100644 net/sunrpc/sysfs.h
> >
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
>

2021-02-02 19:24:29

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

I want to ensure Dan is aware of this work. Thanks for posting, Anna!

> On Feb 2, 2021, at 1:42 PM, [email protected] wrote:
>
> From: Anna Schumaker <[email protected]>
>
> It's possible for an NFS server to go down but come back up with a
> different IP address. These patches provide a way for administrators to
> handle this issue by providing a new IP address for xprt sockets to
> connect to.
>
> Chuck has suggested some ideas for future work that could also use this
> interface, such as:
> - srcaddr: To move between network devices on the client
> - type: "tcp", "rdma", "local"
> - bound: 0 for autobind, or the result of the most recent rpcbind query
> - connected: either true or false
> - last: read-only timestamp of the last operation to use the transport
> - device: A symlink to the physical network device
>
> Changes in v2:
> - Put files under /sys/kernel/sunrpc/ instead of /sys/net/sunrpc/
> - Rename file from "address" to "dstaddr"
>
> Thoughts?
> Anna
>
>
> Anna Schumaker (5):
> sunrpc: Create a sunrpc directory under /sys/kernel/
> sunrpc: Create a net/ subdirectory in the sunrpc sysfs
> sunrpc: Create per-rpc_clnt sysfs kobjects
> sunrpc: Prepare xs_connect() for taking NULL tasks
> sunrpc: Create a per-rpc_clnt file for managing the destination IP
> address
>
> include/linux/sunrpc/clnt.h | 1 +
> net/sunrpc/Makefile | 2 +-
> net/sunrpc/clnt.c | 5 ++
> net/sunrpc/sunrpc_syms.c | 8 ++
> net/sunrpc/sysfs.c | 168 ++++++++++++++++++++++++++++++++++++
> net/sunrpc/sysfs.h | 22 +++++
> net/sunrpc/xprtsock.c | 3 +-
> 7 files changed, 207 insertions(+), 2 deletions(-)
> create mode 100644 net/sunrpc/sysfs.c
> create mode 100644 net/sunrpc/sysfs.h
>
> --
> 2.29.2
>

--
Chuck Lever

2021-02-02 19:28:13

by Dan Aloni

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
> You're welcome! I'll try to remember to CC him on future versions
> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever <[email protected]> wrote:
> >
> > I want to ensure Dan is aware of this work. Thanks for posting, Anna!

Thanks Anna and Chuck. I'm accessing and monitoring the mailing list via
NNTP and I'm also on #linux-nfs for chatting (da-x).

I see srcaddr was already discussed, so the patches I'm planning to send
next will be based on the latest version of your patchset and concern
multipath.

What I'm going for is the following:

- Expose transports that are reachable from xprtmultipath. Each in its
own sub-directory, with an interface and status representation similar
to the top directory.
- A way to add/remove transports.
- Inspiration for coding this is various other things in the kernel that
use configfs, perhaps it can be used here too.

Also, what do you think would be a straightforward way for a userspace
program to find what sunrpc client id serves a mountpoint? If we add an
ioctl for the mountdir AFAIK it would be the first one that the NFS
client supports, so I wonder if there's a better interface that can work
for that.

--
Dan Aloni

2021-02-02 19:52:42

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

> On Feb 2, 2021, at 2:24 PM, Dan Aloni <[email protected]> wrote:
>
> On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
>> You're welcome! I'll try to remember to CC him on future versions
>> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever <[email protected]> wrote:
>>>
>>> I want to ensure Dan is aware of this work. Thanks for posting, Anna!
>
> Thanks Anna and Chuck. I'm accessing and monitoring the mailing list via
> NNTP and I'm also on #linux-nfs for chatting (da-x).
>
> I see srcaddr was already discussed, so the patches I'm planning to send
> next will be based on the latest version of your patchset and concern
> multipath.
>
> What I'm going for is the following:
>
> - Expose transports that are reachable from xprtmultipath. Each in its
> own sub-directory, with an interface and status representation similar
> to the top directory.
> - A way to add/remove transports.
> - Inspiration for coding this is various other things in the kernel that
> use configfs, perhaps it can be used here too.
>
> Also, what do you think would be a straightforward way for a userspace
> program to find what sunrpc client id serves a mountpoint? If we add an
> ioctl for the mountdir AFAIK it would be the first one that the NFS
> client supports, so I wonder if there's a better interface that can work
> for that.

Has the new mount API been merged? That provides a way to open
a mountpoint and get a file descriptor for it, and then write
commands to it.

--
Chuck Lever

2021-02-02 19:54:50

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On Tue, Feb 2, 2021 at 2:48 PM Chuck Lever <[email protected]> wrote:
>
>
>
> > On Feb 2, 2021, at 2:24 PM, Dan Aloni <[email protected]> wrote:
> >
> > On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
> >> You're welcome! I'll try to remember to CC him on future versions
> >> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever <[email protected]> wrote:
> >>>
> >>> I want to ensure Dan is aware of this work. Thanks for posting, Anna!
> >
> > Thanks Anna and Chuck. I'm accessing and monitoring the mailing list via
> > NNTP and I'm also on #linux-nfs for chatting (da-x).
> >
> > I see srcaddr was already discussed, so the patches I'm planning to send
> > next will be based on the latest version of your patchset and concern
> > multipath.
> >
> > What I'm going for is the following:
> >
> > - Expose transports that are reachable from xprtmultipath. Each in its
> > own sub-directory, with an interface and status representation similar
> > to the top directory.
> > - A way to add/remove transports.
> > - Inspiration for coding this is various other things in the kernel that
> > use configfs, perhaps it can be used here too.

Sounds good! I'm looking forward to seeing them

> >
> > Also, what do you think would be a straightforward way for a userspace
> > program to find what sunrpc client id serves a mountpoint? If we add an
> > ioctl for the mountdir AFAIK it would be the first one that the NFS
> > client supports, so I wonder if there's a better interface that can work
> > for that.
>
> Has the new mount API been merged? That provides a way to open
> a mountpoint and get a file descriptor for it, and then write
> commands to it.

I'm pretty sure it was merged a release or two ago (at least, that's
when the fs_context patches went in)

Anna
>
>
> --
> Chuck Lever
>
>
>

2021-02-02 19:55:03

by Benjamin Coddington

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On 2 Feb 2021, at 14:24, Dan Aloni wrote:

> On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
>> You're welcome! I'll try to remember to CC him on future versions
>> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever <[email protected]> wrote:
>>>
>>> I want to ensure Dan is aware of this work. Thanks for posting, Anna!
>
> Thanks Anna and Chuck. I'm accessing and monitoring the mailing list via
> NNTP and I'm also on #linux-nfs for chatting (da-x).
>
> I see srcaddr was already discussed, so the patches I'm planning to send
> next will be based on the latest version of your patchset and concern
> multipath.
>
> What I'm going for is the following:
>
> - Expose transports that are reachable from xprtmultipath. Each in its
> own sub-directory, with an interface and status representation similar
> to the top directory.
> - A way to add/remove transports.
> - Inspiration for coding this is various other things in the kernel that
> use configfs, perhaps it can be used here too.
>
> Also, what do you think would be a straightforward way for a userspace
> program to find what sunrpc client id serves a mountpoint? If we add an
> ioctl for the mountdir AFAIK it would be the first one that the NFS
> client supports, so I wonder if there's a better interface that can work
> for that.

I'm a fan of adding an ioctl interface for userspace, but I think we'd
better avoid using NFS itself because it would be nice to someday implement
an NFS "shutdown" for non-responsive servers, but sending any ioctl to the
mountpoint could revalidate it, and we'd hang on the GETATTR.

Maybe we can figure out a way to expose the superblock via sysfs for each
mount.

Ben

2021-02-03 00:53:22

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] sunrpc: Prepare xs_connect() for taking NULL tasks

On Tue, 2021-02-02 at 13:42 -0500, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> We won't have a task structure when we go to change IP addresses, so
> check for one before calling the WARN_ON() to avoid crashing.
>
> Signed-off-by: Anna Schumaker <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index c56a66cdf4ac..250abf1aa018 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt,
> struct rpc_task *task)
>         struct sock_xprt *transport = container_of(xprt, struct
> sock_xprt, xprt);
>         unsigned long delay = 0;
>
> -       WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));
> +       if (task)
> +               WARN_ON_ONCE(!xprt_lock_connect(xprt, task,
> transport));

Nit: That's the same as
WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport));

>
>         if (transport->sock != NULL) {
>                 dprintk("RPC:       xs_connect delayed xprt %p for
> %lu "

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2021-02-03 00:53:30

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] sunrpc: Prepare xs_connect() for taking NULL tasks

On Tue, 2021-02-02 at 16:49 -0500, Trond Myklebust wrote:
> On Tue, 2021-02-02 at 13:42 -0500, [email protected] wrote:
> > From: Anna Schumaker <[email protected]>
> >
> > We won't have a task structure when we go to change IP addresses,
> > so
> > check for one before calling the WARN_ON() to avoid crashing.
> >
> > Signed-off-by: Anna Schumaker <[email protected]>
> > ---
> > net/sunrpc/xprtsock.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index c56a66cdf4ac..250abf1aa018 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt,
> > struct rpc_task *task)
> >         struct sock_xprt *transport = container_of(xprt, struct
> > sock_xprt, xprt);
> >         unsigned long delay = 0;
> >
> > -       WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));
> > +       if (task)
> > +               WARN_ON_ONCE(!xprt_lock_connect(xprt, task,
> > transport));
>
> Nit: That's the same as
>    WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport));
>
> >
> >         if (transport->sock != NULL) {
> >                 dprintk("RPC:       xs_connect delayed xprt %p for
> > %lu "
>

So, the problem with this patch is that you're deliberately
circumventing the process of locking the socket. That's not going to
work.
What you could do is follow the example set by xprt_destroy() and
xs_enable_swap(), to call wait_on_bit_lock() when there is no task.
That should work, but you'd better make sure that your process holds a
reference to the xprt->kref before doing that, or else you could easily
end up deadlocking with xprt_destroy().

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2021-02-03 00:56:32

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

> On Feb 2, 2021, at 5:17 PM, Trond Myklebust <[email protected]> wrote:
>
> On Tue, 2021-02-02 at 14:49 -0500, Benjamin Coddington wrote:
>> On 2 Feb 2021, at 14:24, Dan Aloni wrote:
>>
>>> On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
>>>> You're welcome! I'll try to remember to CC him on future versions
>>>> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever
>>>> <[email protected]> wrote:
>>>>>
>>>>> I want to ensure Dan is aware of this work. Thanks for posting,
>>>>> Anna!
>>>
>>> Thanks Anna and Chuck. I'm accessing and monitoring the mailing
>>> list via
>>> NNTP and I'm also on #linux-nfs for chatting (da-x).
>>>
>>> I see srcaddr was already discussed, so the patches I'm planning to
>>> send
>>> next will be based on the latest version of your patchset and
>>> concern
>>> multipath.
>>>
>>> What I'm going for is the following:
>>>
>>> - Expose transports that are reachable from xprtmultipath. Each in
>>> its
>>> own sub-directory, with an interface and status representation
>>> similar
>>> to the top directory.
>>> - A way to add/remove transports.
>>> - Inspiration for coding this is various other things in the kernel
>>> that
>>> use configfs, perhaps it can be used here too.
>>>
>>> Also, what do you think would be a straightforward way for a
>>> userspace
>>> program to find what sunrpc client id serves a mountpoint? If we
>>> add an
>>> ioctl for the mountdir AFAIK it would be the first one that the NFS
>>> client supports, so I wonder if there's a better interface that can
>>> work
>>> for that.
>>
>> I'm a fan of adding an ioctl interface for userspace, but I think
>> we'd
>> better avoid using NFS itself because it would be nice to someday
>> implement
>> an NFS "shutdown" for non-responsive servers, but sending any ioctl
>> to the
>> mountpoint could revalidate it, and we'd hang on the GETATTR.
>>
>> Maybe we can figure out a way to expose the superblock via sysfs for
>> each
>> mount.
>
> Right. There is potential functionality here that we do not need or
> even want to expose via the mount interface. Being able to cancel all
> the hung RPC calls in an RPC queue, for instance, is not something you
> want to do through fsopen() and friends.

I thought we were talking only about an ioctl or fsopen cmd that
identifies the transports that are associated with an NFS mount.

Ostensibly a read-only use of that API.

--
Chuck Lever

2021-02-03 00:56:35

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On Tue, 2021-02-02 at 14:49 -0500, Benjamin Coddington wrote:
> On 2 Feb 2021, at 14:24, Dan Aloni wrote:
>
> > On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
> > > You're welcome! I'll try to remember to CC him on future versions
> > > On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever
> > > <[email protected]> wrote:
> > > >
> > > > I want to ensure Dan is aware of this work. Thanks for posting,
> > > > Anna!
> >
> > Thanks Anna and Chuck. I'm accessing and monitoring the mailing
> > list via
> > NNTP and I'm also on #linux-nfs for chatting (da-x).
> >
> > I see srcaddr was already discussed, so the patches I'm planning to
> > send
> > next will be based on the latest version of your patchset and
> > concern
> > multipath.
> >
> > What I'm going for is the following:
> >
> > - Expose transports that are reachable from xprtmultipath. Each in
> > its
> > own sub-directory, with an interface and status representation
> > similar
> > to the top directory.
> > - A way to add/remove transports.
> > - Inspiration for coding this is various other things in the kernel
> > that
> > use configfs, perhaps it can be used here too.
> >
> > Also, what do you think would be a straightforward way for a
> > userspace
> > program to find what sunrpc client id serves a mountpoint? If we
> > add an
> > ioctl for the mountdir AFAIK it would be the first one that the NFS
> > client supports, so I wonder if there's a better interface that can
> > work
> > for that.
>
> I'm a fan of adding an ioctl interface for userspace, but I think
> we'd
> better avoid using NFS itself because it would be nice to someday
> implement
> an NFS "shutdown" for non-responsive servers, but sending any ioctl
> to the
> mountpoint could revalidate it, and we'd hang on the GETATTR.
>
> Maybe we can figure out a way to expose the superblock via sysfs for
> each
> mount.

Right. There is potential functionality here that we do not need or
even want to expose via the mount interface. Being able to cancel all
the hung RPC calls in an RPC queue, for instance, is not something you
want to do through fsopen() and friends.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2021-02-03 00:57:00

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

> On Feb 2, 2021, at 5:24 PM, Trond Myklebust <[email protected]> wrote:
>
> On Tue, 2021-02-02 at 22:21 +0000, Chuck Lever wrote:
>>
>>
>>> On Feb 2, 2021, at 5:17 PM, Trond Myklebust <
>>> [email protected]> wrote:
>>>
>>> On Tue, 2021-02-02 at 14:49 -0500, Benjamin Coddington wrote:
>>>> On 2 Feb 2021, at 14:24, Dan Aloni wrote:
>>>>
>>>>> On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
>>>>>> You're welcome! I'll try to remember to CC him on future
>>>>>> versions
>>>>>> On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> I want to ensure Dan is aware of this work. Thanks for
>>>>>>> posting,
>>>>>>> Anna!
>>>>>
>>>>> Thanks Anna and Chuck. I'm accessing and monitoring the mailing
>>>>> list via
>>>>> NNTP and I'm also on #linux-nfs for chatting (da-x).
>>>>>
>>>>> I see srcaddr was already discussed, so the patches I'm
>>>>> planning to
>>>>> send
>>>>> next will be based on the latest version of your patchset and
>>>>> concern
>>>>> multipath.
>>>>>
>>>>> What I'm going for is the following:
>>>>>
>>>>> - Expose transports that are reachable from xprtmultipath. Each
>>>>> in
>>>>> its
>>>>> own sub-directory, with an interface and status
>>>>> representation
>>>>> similar
>>>>> to the top directory.
>>>>> - A way to add/remove transports.
>>>>> - Inspiration for coding this is various other things in the
>>>>> kernel
>>>>> that
>>>>> use configfs, perhaps it can be used here too.
>>>>>
>>>>> Also, what do you think would be a straightforward way for a
>>>>> userspace
>>>>> program to find what sunrpc client id serves a mountpoint? If
>>>>> we
>>>>> add an
>>>>> ioctl for the mountdir AFAIK it would be the first one that the
>>>>> NFS
>>>>> client supports, so I wonder if there's a better interface that
>>>>> can
>>>>> work
>>>>> for that.
>>>>
>>>> I'm a fan of adding an ioctl interface for userspace, but I think
>>>> we'd
>>>> better avoid using NFS itself because it would be nice to someday
>>>> implement
>>>> an NFS "shutdown" for non-responsive servers, but sending any
>>>> ioctl
>>>> to the
>>>> mountpoint could revalidate it, and we'd hang on the GETATTR.
>>>>
>>>> Maybe we can figure out a way to expose the superblock via sysfs
>>>> for
>>>> each
>>>> mount.
>>>
>>> Right. There is potential functionality here that we do not need or
>>> even want to expose via the mount interface. Being able to cancel
>>> all
>>> the hung RPC calls in an RPC queue, for instance, is not something
>>> you
>>> want to do through fsopen() and friends.
>>
>> I thought we were talking only about an ioctl or fsopen cmd that
>> identifies the transports that are associated with an NFS mount.
>>
>> Ostensibly a read-only use of that API.
>>
>
> I'll let Anna chime in with the details of her use case, but my
> understanding has always been that this would be a read/write interface
> for changing the properties of those transports on the fly.

Agreed, but Dan's looking for a way to match up an NFS mount to the
/sys directories that Anna is adding to do those manipulations.

So, fsopen() or ioctl() would identify the transports, and then
Anna's API would enable an appropriately privileged user to
change the properties as you indicated. Two separate steps.

If the new API already provides a mechanism to determine which
transports to adjust, then we won't need an ioctl/fsopen at all.

--
Chuck Lever

2021-02-03 00:57:29

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On Tue, 2021-02-02 at 22:21 +0000, Chuck Lever wrote:
>
>
> > On Feb 2, 2021, at 5:17 PM, Trond Myklebust <
> > [email protected]> wrote:
> >
> > On Tue, 2021-02-02 at 14:49 -0500, Benjamin Coddington wrote:
> > > On 2 Feb 2021, at 14:24, Dan Aloni wrote:
> > >
> > > > On Tue, Feb 02, 2021 at 01:52:10PM -0500, Anna Schumaker wrote:
> > > > > You're welcome! I'll try to remember to CC him on future
> > > > > versions
> > > > > On Tue, Feb 2, 2021 at 1:51 PM Chuck Lever
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > I want to ensure Dan is aware of this work. Thanks for
> > > > > > posting,
> > > > > > Anna!
> > > >
> > > > Thanks Anna and Chuck. I'm accessing and monitoring the mailing
> > > > list via
> > > > NNTP and I'm also on #linux-nfs for chatting (da-x).
> > > >
> > > > I see srcaddr was already discussed, so the patches I'm
> > > > planning to
> > > > send
> > > > next will be based on the latest version of your patchset and
> > > > concern
> > > > multipath.
> > > >
> > > > What I'm going for is the following:
> > > >
> > > > - Expose transports that are reachable from xprtmultipath. Each
> > > > in
> > > > its
> > > > own sub-directory, with an interface and status
> > > > representation
> > > > similar
> > > > to the top directory.
> > > > - A way to add/remove transports.
> > > > - Inspiration for coding this is various other things in the
> > > > kernel
> > > > that
> > > > use configfs, perhaps it can be used here too.
> > > >
> > > > Also, what do you think would be a straightforward way for a
> > > > userspace
> > > > program to find what sunrpc client id serves a mountpoint? If
> > > > we
> > > > add an
> > > > ioctl for the mountdir AFAIK it would be the first one that the
> > > > NFS
> > > > client supports, so I wonder if there's a better interface that
> > > > can
> > > > work
> > > > for that.
> > >
> > > I'm a fan of adding an ioctl interface for userspace, but I think
> > > we'd
> > > better avoid using NFS itself because it would be nice to someday
> > > implement
> > > an NFS "shutdown" for non-responsive servers, but sending any
> > > ioctl
> > > to the
> > > mountpoint could revalidate it, and we'd hang on the GETATTR.
> > >
> > > Maybe we can figure out a way to expose the superblock via sysfs
> > > for
> > > each
> > > mount.
> >
> > Right. There is potential functionality here that we do not need or
> > even want to expose via the mount interface. Being able to cancel
> > all
> > the hung RPC calls in an RPC queue, for instance, is not something
> > you
> > want to do through fsopen() and friends.
>
> I thought we were talking only about an ioctl or fsopen cmd that
> identifies the transports that are associated with an NFS mount.
>
> Ostensibly a read-only use of that API.
>

I'll let Anna chime in with the details of her use case, but my
understanding has always been that this would be a read/write interface
for changing the properties of those transports on the fly.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]

2021-02-03 21:22:50

by Dan Aloni

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] SUNRPC: Create sysfs files for changing IP

On Tue, Feb 02, 2021 at 02:49:38PM -0500, Benjamin Coddington wrote:
> On 2 Feb 2021, at 14:24, Dan Aloni wrote:
> > Also, what do you think would be a straightforward way for a userspace
> > program to find what sunrpc client id serves a mountpoint? If we add an
> > ioctl for the mountdir AFAIK it would be the first one that the NFS
> > client supports, so I wonder if there's a better interface that can work
> > for that.
>
> I'm a fan of adding an ioctl interface for userspace, but I think we'd
> better avoid using NFS itself because it would be nice to someday implement
> an NFS "shutdown" for non-responsive servers, but sending any ioctl to the
> mountpoint could revalidate it, and we'd hang on the GETATTR.

For that, I was looking into using openat2() with the very recently
added RESOLVE_CACHED flag. However from some experimentation I see that it
still sleeps on the unresponsive mount in nfs_weak_revalidate(), and the
latter cannot tell whether LOOKUP_CACHED flag was passed to
d_weak_revalidate().

> Maybe we can figure out a way to expose the superblock via sysfs for each
> mount.

Essentially this is what fspick() syscall lets you do. I imagine that it
can be implemented entirely under fs/nfs, using fsconfig() from under a
FSCONFIG_SET_STRING passing a special string such as
"report-clients-ids", causing a list of sunrpc client IDs to get written
to the fs_context log.

However even with this interface we may still need to verify that the
path lookup that `fspick` does using `user_path_at` is not blocking on
non-responsive NFS mounts.

--
Dan Aloni

2021-02-05 18:39:05