2022-06-20 15:34:37

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 00/12] Handling session trunking group membership

From: Olga Kornievskaia <[email protected]>

Client needs to handle session trunking group membership changes that
occur when a particular server leaves an established trunked group.
This results in a server sending a NFS4ERR_BAD_SESSION because that
server no longer has session's state.

This series proposes to deal with that situation in two fold. First
on DESTROY_SESSION, the client will offline all trunked connections
it has established to the server. Then on CREATE_SESSION it will
iterate thru offlined connections only and probe them again for
session trunking. If session trunking conditions still hold then
such transport would be made active again otherwise it will be
deleted from the trunked group.

Olga Kornievskaia (12):
SUNRPC expose functions for offline remote xprt functionality
SUNRPC add function to offline remove trunkable transports
NFSv4.1 offline trunkable transports on DESTROY_SESSION
SUNRPC create an iterator to list only OFFLINE xprts
SUNRPC parameterize rpc_clnt_iterate_for_each_xprt with iterator init
function
SUNRPC enable back offline transports in trunking discovery
SUNRPC create an rpc function that allows xprt removal from rpc_clnt
NFSv4.1 remove xprt from xprt_switch if session trunking test fails
SUNRPC restructure rpc_clnt_setup_test_and_add_xprt
SUNRPC export xprt_iter_rewind function
SUNRPC create a function that probes only offline transports
NFSv4.1 probe offline transports for trunking on session creation

fs/nfs/nfs4proc.c | 18 ++-
include/linux/sunrpc/clnt.h | 7 +-
include/linux/sunrpc/xprt.h | 3 +
include/linux/sunrpc/xprtmultipath.h | 7 +-
net/sunrpc/clnt.c | 204 ++++++++++++++++++++++-----
net/sunrpc/debugfs.c | 3 +-
net/sunrpc/stats.c | 2 +-
net/sunrpc/sysfs.c | 28 +---
net/sunrpc/xprt.c | 35 +++++
net/sunrpc/xprtmultipath.c | 109 +++++++++++---
10 files changed, 338 insertions(+), 78 deletions(-)

--
2.27.0


2022-06-20 15:34:42

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 10/12] SUNRPC export xprt_iter_rewind function

From: Olga Kornievskaia <[email protected]>

Make xprt_iter_rewind callable outside of xprtmultipath.c

Signed-off-by: Olga Kornievskaia <[email protected]>
---
include/linux/sunrpc/xprtmultipath.h | 2 ++
net/sunrpc/xprtmultipath.c | 1 -
2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/sunrpc/xprtmultipath.h b/include/linux/sunrpc/xprtmultipath.h
index 9fff0768d942..c0514c684b2c 100644
--- a/include/linux/sunrpc/xprtmultipath.h
+++ b/include/linux/sunrpc/xprtmultipath.h
@@ -68,6 +68,8 @@ extern void xprt_iter_init_listoffline(struct rpc_xprt_iter *xpi,

extern void xprt_iter_destroy(struct rpc_xprt_iter *xpi);

+extern void xprt_iter_rewind(struct rpc_xprt_iter *xpi);
+
extern struct rpc_xprt_switch *xprt_iter_xchg_switch(
struct rpc_xprt_iter *xpi,
struct rpc_xprt_switch *newswitch);
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 41ec46e5f1a3..154bb6cf27ad 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -479,7 +479,6 @@ struct rpc_xprt *xprt_iter_next_entry_offline(struct rpc_xprt_iter *xpi)
* Resets xpi to ensure that it points to the first entry in the list
* of transports.
*/
-static
void xprt_iter_rewind(struct rpc_xprt_iter *xpi)
{
rcu_read_lock();
--
2.27.0

2022-06-20 15:34:42

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 05/12] SUNRPC parameterize rpc_clnt_iterate_for_each_xprt with iterator init function

From: Olga Kornievskaia <[email protected]>

Allow for rpc_clnt_iterate_for_each_xprt() to take in an iterator
initialization function if no function passed in a default initiator
is used.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 2 +-
include/linux/sunrpc/clnt.h | 1 +
net/sunrpc/clnt.c | 17 ++++++++++++-----
net/sunrpc/debugfs.c | 2 +-
net/sunrpc/stats.c | 2 +-
5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cf898bea3bfd..5e4c32924347 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8532,7 +8532,7 @@ int nfs4_proc_bind_conn_to_session(struct nfs_client *clp, const struct cred *cr
.clp = clp,
.cred = cred,
};
- return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient,
+ return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient, NULL,
nfs4_proc_bind_conn_to_session_callback, &data);
}

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index e74a0740603b..20aed14fe222 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -213,6 +213,7 @@ const char *rpc_peeraddr2str(struct rpc_clnt *, enum rpc_display_format_t);
int rpc_localaddr(struct rpc_clnt *, struct sockaddr *, size_t);

int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
+ int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter *),
int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void *),
void *data);

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 410bd6c352ad..b26267606de0 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -817,6 +817,8 @@ int rpc_clnt_xprt_iter_offline_init(struct rpc_clnt *clnt,
/**
* rpc_clnt_iterate_for_each_xprt - Apply a function to all transports
* @clnt: pointer to client
+ * @setup: an optional iterator init function to use, if none supplied
+ * default rpc_clnt_xprt_iter_init() iterator is used
* @fn: function to apply
* @data: void pointer to function data
*
@@ -826,13 +828,17 @@ int rpc_clnt_xprt_iter_offline_init(struct rpc_clnt *clnt,
* On error, the iteration stops, and the function returns the error value.
*/
int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
+ int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter *),
int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void *),
void *data)
{
struct rpc_xprt_iter xpi;
int ret;

- ret = rpc_clnt_xprt_iter_init(clnt, &xpi);
+ if (!setup)
+ ret = rpc_clnt_xprt_iter_init(clnt, &xpi);
+ else
+ ret = setup(clnt, &xpi);
if (ret)
return ret;
for (;;) {
@@ -3052,7 +3058,8 @@ static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,

void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void *data)
{
- rpc_clnt_iterate_for_each_xprt(clnt, rpc_xprt_offline_destroy, data);
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL, rpc_xprt_offline_destroy,
+ data);
}
EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);

@@ -3084,7 +3091,7 @@ rpc_set_connect_timeout(struct rpc_clnt *clnt,
.connect_timeout = connect_timeout,
.reconnect_timeout = reconnect_timeout,
};
- rpc_clnt_iterate_for_each_xprt(clnt,
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL,
rpc_xprt_set_connect_timeout,
&timeout);
}
@@ -3181,7 +3188,7 @@ rpc_clnt_swap_activate(struct rpc_clnt *clnt)
while (clnt != clnt->cl_parent)
clnt = clnt->cl_parent;
if (atomic_inc_return(&clnt->cl_swapper) == 1)
- return rpc_clnt_iterate_for_each_xprt(clnt,
+ return rpc_clnt_iterate_for_each_xprt(clnt, NULL,
rpc_clnt_swap_activate_callback, NULL);
return 0;
}
@@ -3200,7 +3207,7 @@ void
rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
{
if (atomic_dec_if_positive(&clnt->cl_swapper) == 0)
- rpc_clnt_iterate_for_each_xprt(clnt,
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL,
rpc_clnt_swap_deactivate_callback, NULL);
}
EXPORT_SYMBOL_GPL(rpc_clnt_swap_deactivate);
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index 7dc9cc929bfd..ab60b4d3deb2 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -160,7 +160,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs, clnt,
&tasks_fops);

- rpc_clnt_iterate_for_each_xprt(clnt, do_xprt_debugfs, &xprtnum);
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs, &xprtnum);
}

void
diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index 52908f9e6eab..e50f73a4aca5 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -258,7 +258,7 @@ void rpc_clnt_show_stats(struct seq_file *seq, struct rpc_clnt *clnt)
seq_printf(seq, "p/v: %u/%u (%s)\n",
clnt->cl_prog, clnt->cl_vers, clnt->cl_program->name);

- rpc_clnt_iterate_for_each_xprt(clnt, do_print_stats, seq);
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats, seq);

seq_printf(seq, "\tper-op statistics\n");
for (op = 0; op < maxproc; op++) {
--
2.27.0

2022-06-20 15:34:43

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 04/12] SUNRPC create an iterator to list only OFFLINE xprts

From: Olga Kornievskaia <[email protected]>

Create a new iterator helper that will go thru the all the transports
in the switch and return transports that are marked OFFLINE.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
include/linux/sunrpc/xprtmultipath.h | 3 +
net/sunrpc/clnt.c | 19 +++++-
net/sunrpc/xprtmultipath.c | 98 +++++++++++++++++++++++++---
3 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/include/linux/sunrpc/xprtmultipath.h b/include/linux/sunrpc/xprtmultipath.h
index bbb8a5fa0816..688ca7eb1d01 100644
--- a/include/linux/sunrpc/xprtmultipath.h
+++ b/include/linux/sunrpc/xprtmultipath.h
@@ -63,6 +63,9 @@ extern void xprt_iter_init(struct rpc_xprt_iter *xpi,
extern void xprt_iter_init_listall(struct rpc_xprt_iter *xpi,
struct rpc_xprt_switch *xps);

+extern void xprt_iter_init_listoffline(struct rpc_xprt_iter *xpi,
+ struct rpc_xprt_switch *xps);
+
extern void xprt_iter_destroy(struct rpc_xprt_iter *xpi);

extern struct rpc_xprt_switch *xprt_iter_xchg_switch(
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 544b55a3aa20..410bd6c352ad 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -785,7 +785,9 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,
EXPORT_SYMBOL_GPL(rpc_switch_client_transport);

static
-int rpc_clnt_xprt_iter_init(struct rpc_clnt *clnt, struct rpc_xprt_iter *xpi)
+int _rpc_clnt_xprt_iter_init(struct rpc_clnt *clnt, struct rpc_xprt_iter *xpi,
+ void func(struct rpc_xprt_iter *xpi,
+ struct rpc_xprt_switch *xps))
{
struct rpc_xprt_switch *xps;

@@ -794,11 +796,24 @@ int rpc_clnt_xprt_iter_init(struct rpc_clnt *clnt, struct rpc_xprt_iter *xpi)
rcu_read_unlock();
if (xps == NULL)
return -EAGAIN;
- xprt_iter_init_listall(xpi, xps);
+ func(xpi, xps);
xprt_switch_put(xps);
return 0;
}

+static
+int rpc_clnt_xprt_iter_init(struct rpc_clnt *clnt, struct rpc_xprt_iter *xpi)
+{
+ return _rpc_clnt_xprt_iter_init(clnt, xpi, xprt_iter_init_listall);
+}
+
+static
+int rpc_clnt_xprt_iter_offline_init(struct rpc_clnt *clnt,
+ struct rpc_xprt_iter *xpi)
+{
+ return _rpc_clnt_xprt_iter_init(clnt, xpi, xprt_iter_init_listoffline);
+}
+
/**
* rpc_clnt_iterate_for_each_xprt - Apply a function to all transports
* @clnt: pointer to client
diff --git a/net/sunrpc/xprtmultipath.c b/net/sunrpc/xprtmultipath.c
index 1693f81aae37..4374cd6acc55 100644
--- a/net/sunrpc/xprtmultipath.c
+++ b/net/sunrpc/xprtmultipath.c
@@ -27,6 +27,7 @@ typedef struct rpc_xprt *(*xprt_switch_find_xprt_t)(struct rpc_xprt_switch *xps,
static const struct rpc_xprt_iter_ops rpc_xprt_iter_singular;
static const struct rpc_xprt_iter_ops rpc_xprt_iter_roundrobin;
static const struct rpc_xprt_iter_ops rpc_xprt_iter_listall;
+static const struct rpc_xprt_iter_ops rpc_xprt_iter_listoffline;

static void xprt_switch_add_xprt_locked(struct rpc_xprt_switch *xps,
struct rpc_xprt *xprt)
@@ -248,6 +249,18 @@ struct rpc_xprt *xprt_switch_find_first_entry(struct list_head *head)
return NULL;
}

+static
+struct rpc_xprt *xprt_switch_find_first_entry_offline(struct list_head *head)
+{
+ struct rpc_xprt *pos;
+
+ list_for_each_entry_rcu(pos, head, xprt_switch) {
+ if (!xprt_is_active(pos))
+ return pos;
+ }
+ return NULL;
+}
+
static
struct rpc_xprt *xprt_iter_first_entry(struct rpc_xprt_iter *xpi)
{
@@ -259,8 +272,8 @@ struct rpc_xprt *xprt_iter_first_entry(struct rpc_xprt_iter *xpi)
}

static
-struct rpc_xprt *xprt_switch_find_current_entry(struct list_head *head,
- const struct rpc_xprt *cur)
+struct rpc_xprt *_xprt_switch_find_current_entry(struct list_head *head,
+ const struct rpc_xprt *cur, bool find_active)
{
struct rpc_xprt *pos;
bool found = false;
@@ -268,14 +281,25 @@ struct rpc_xprt *xprt_switch_find_current_entry(struct list_head *head,
list_for_each_entry_rcu(pos, head, xprt_switch) {
if (cur == pos)
found = true;
- if (found && xprt_is_active(pos))
+ if (found && ((find_active && xprt_is_active(pos)) ||
+ (!find_active && xprt_is_active(pos))))
return pos;
}
return NULL;
}

static
-struct rpc_xprt *xprt_iter_current_entry(struct rpc_xprt_iter *xpi)
+struct rpc_xprt *xprt_switch_find_current_entry(struct list_head *head,
+ const struct rpc_xprt *cur)
+{
+ return _xprt_switch_find_current_entry(head, cur, true);
+}
+
+static
+struct rpc_xprt * _xprt_iter_current_entry(struct rpc_xprt_iter *xpi,
+ struct rpc_xprt *first_entry(struct list_head *head),
+ struct rpc_xprt *current_entry(struct list_head *head,
+ const struct rpc_xprt *cur))
{
struct rpc_xprt_switch *xps = rcu_dereference(xpi->xpi_xpswitch);
struct list_head *head;
@@ -284,8 +308,30 @@ struct rpc_xprt *xprt_iter_current_entry(struct rpc_xprt_iter *xpi)
return NULL;
head = &xps->xps_xprt_list;
if (xpi->xpi_cursor == NULL || xps->xps_nxprts < 2)
- return xprt_switch_find_first_entry(head);
- return xprt_switch_find_current_entry(head, xpi->xpi_cursor);
+ return first_entry(head);
+ return current_entry(head, xpi->xpi_cursor);
+}
+
+static
+struct rpc_xprt *xprt_iter_current_entry(struct rpc_xprt_iter *xpi)
+{
+ return _xprt_iter_current_entry(xpi, xprt_switch_find_first_entry,
+ xprt_switch_find_current_entry);
+}
+
+static
+struct rpc_xprt *xprt_switch_find_current_entry_offline(struct list_head *head,
+ const struct rpc_xprt *cur)
+{
+ return _xprt_switch_find_current_entry(head, cur, false);
+}
+
+static
+struct rpc_xprt *xprt_iter_current_entry_offline(struct rpc_xprt_iter *xpi)
+{
+ return _xprt_iter_current_entry(xpi,
+ xprt_switch_find_first_entry_offline,
+ xprt_switch_find_current_entry_offline);
}

bool rpc_xprt_switch_has_addr(struct rpc_xprt_switch *xps,
@@ -310,7 +356,7 @@ bool rpc_xprt_switch_has_addr(struct rpc_xprt_switch *xps,

static
struct rpc_xprt *xprt_switch_find_next_entry(struct list_head *head,
- const struct rpc_xprt *cur)
+ const struct rpc_xprt *cur, bool check_active)
{
struct rpc_xprt *pos, *prev = NULL;
bool found = false;
@@ -318,7 +364,12 @@ struct rpc_xprt *xprt_switch_find_next_entry(struct list_head *head,
list_for_each_entry_rcu(pos, head, xprt_switch) {
if (cur == prev)
found = true;
- if (found && xprt_is_active(pos))
+ /* for request to return active transports return only
+ * active, for request to return offline transports
+ * return only offline
+ */
+ if (found && ((check_active && xprt_is_active(pos)) ||
+ (!check_active && !xprt_is_active(pos))))
return pos;
prev = pos;
}
@@ -355,7 +406,7 @@ struct rpc_xprt *__xprt_switch_find_next_entry_roundrobin(struct list_head *head
{
struct rpc_xprt *ret;

- ret = xprt_switch_find_next_entry(head, cur);
+ ret = xprt_switch_find_next_entry(head, cur, true);
if (ret != NULL)
return ret;
return xprt_switch_find_first_entry(head);
@@ -397,7 +448,14 @@ static
struct rpc_xprt *xprt_switch_find_next_entry_all(struct rpc_xprt_switch *xps,
const struct rpc_xprt *cur)
{
- return xprt_switch_find_next_entry(&xps->xps_xprt_list, cur);
+ return xprt_switch_find_next_entry(&xps->xps_xprt_list, cur, true);
+}
+
+static
+struct rpc_xprt *xprt_switch_find_next_entry_offline(struct rpc_xprt_switch *xps,
+ const struct rpc_xprt *cur)
+{
+ return xprt_switch_find_next_entry(&xps->xps_xprt_list, cur, false);
}

static
@@ -407,6 +465,13 @@ struct rpc_xprt *xprt_iter_next_entry_all(struct rpc_xprt_iter *xpi)
xprt_switch_find_next_entry_all);
}

+static
+struct rpc_xprt *xprt_iter_next_entry_offline(struct rpc_xprt_iter *xpi)
+{
+ return xprt_iter_next_entry_multiple(xpi,
+ xprt_switch_find_next_entry_offline);
+}
+
/*
* xprt_iter_rewind - Resets the xprt iterator
* @xpi: pointer to rpc_xprt_iter
@@ -460,6 +525,12 @@ void xprt_iter_init_listall(struct rpc_xprt_iter *xpi,
__xprt_iter_init(xpi, xps, &rpc_xprt_iter_listall);
}

+void xprt_iter_init_listoffline(struct rpc_xprt_iter *xpi,
+ struct rpc_xprt_switch *xps)
+{
+ __xprt_iter_init(xpi, xps, &rpc_xprt_iter_listoffline);
+}
+
/**
* xprt_iter_xchg_switch - Atomically swap out the rpc_xprt_switch
* @xpi: pointer to rpc_xprt_iter
@@ -574,3 +645,10 @@ const struct rpc_xprt_iter_ops rpc_xprt_iter_listall = {
.xpi_xprt = xprt_iter_current_entry,
.xpi_next = xprt_iter_next_entry_all,
};
+
+static
+const struct rpc_xprt_iter_ops rpc_xprt_iter_listoffline = {
+ .xpi_rewind = xprt_iter_default_rewind,
+ .xpi_xprt = xprt_iter_current_entry_offline,
+ .xpi_next = xprt_iter_next_entry_offline,
+};
--
2.27.0

2022-06-20 15:34:52

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 11/12] SUNRPC create a function that probes only offline transports

From: Olga Kornievskaia <[email protected]>

For only offline transports, attempt to check connectivity via
a NULL call and, if that succeeds, call a provided session trunking
detection function.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 2 +-
include/linux/sunrpc/clnt.h | 3 ++-
net/sunrpc/clnt.c | 47 +++++++++++++++++++++++++++++++++----
net/sunrpc/debugfs.c | 3 ++-
net/sunrpc/stats.c | 2 +-
5 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 152da2bc5100..00778f351283 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8533,7 +8533,7 @@ int nfs4_proc_bind_conn_to_session(struct nfs_client *clp, const struct cred *cr
.cred = cred,
};
return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient, NULL,
- nfs4_proc_bind_conn_to_session_callback, &data);
+ nfs4_proc_bind_conn_to_session_callback, &data, false);
}

/*
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index ac1024da86c5..a0160b83d4a4 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -215,7 +215,7 @@ int rpc_localaddr(struct rpc_clnt *, struct sockaddr *, size_t);
int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter *),
int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void *),
- void *data);
+ void *data, bool do_rewind);

int rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
struct rpc_xprt_switch *xps,
@@ -236,6 +236,7 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
struct rpc_xprt *,
void *);
void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void *);
+void rpc_probe_trunked_xprts(struct rpc_clnt *, void *);

const char *rpc_proc_name(const struct rpc_task *task);

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6b04b29bf842..348d0772c91d 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -830,7 +830,7 @@ int rpc_clnt_xprt_iter_offline_init(struct rpc_clnt *clnt,
int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter *),
int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void *),
- void *data)
+ void *data, bool do_rewind)
{
struct rpc_xprt_iter xpi;
int ret;
@@ -850,6 +850,9 @@ int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
xprt_put(xprt);
if (ret < 0)
break;
+
+ if (do_rewind)
+ xprt_iter_rewind(&xpi);
}
xprt_iter_destroy(&xpi);
return ret;
@@ -3032,6 +3035,40 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
}
EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);

+static int rpc_xprt_probe_trunked(struct rpc_clnt *clnt,
+ struct rpc_xprt *xprt,
+ void *data)
+{
+ struct rpc_xprt_switch *xps;
+ struct rpc_xprt *main_xprt;
+ int status = 0;
+
+ xprt_get(xprt);
+
+ rcu_read_lock();
+ main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
+ xps = xprt_switch_get(rcu_dereference(clnt->cl_xpi.xpi_xpswitch));
+ status = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
+ (struct sockaddr *)&main_xprt->addr);
+ rcu_read_unlock();
+ xprt_put(main_xprt);
+ if (status || !test_bit(XPRT_OFFLINE, &xprt->state))
+ goto out;
+
+ status = rpc_clnt_add_xprt_helper(clnt, xprt, data);
+out:
+ xprt_put(xprt);
+ xprt_switch_put(xps);
+ return status;
+}
+
+void rpc_probe_trunked_xprts(struct rpc_clnt *clnt, void *data)
+{
+ rpc_clnt_iterate_for_each_xprt(clnt, rpc_clnt_xprt_iter_offline_init,
+ rpc_xprt_probe_trunked, data, true);
+}
+EXPORT_SYMBOL_GPL(rpc_probe_trunked_xprts);
+
static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
struct rpc_xprt *xprt,
void *data)
@@ -3071,7 +3108,7 @@ static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void *data)
{
rpc_clnt_iterate_for_each_xprt(clnt, NULL, rpc_xprt_offline_destroy,
- data);
+ data, false);
}
EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);

@@ -3105,7 +3142,7 @@ rpc_set_connect_timeout(struct rpc_clnt *clnt,
};
rpc_clnt_iterate_for_each_xprt(clnt, NULL,
rpc_xprt_set_connect_timeout,
- &timeout);
+ &timeout, false);
}
EXPORT_SYMBOL_GPL(rpc_set_connect_timeout);

@@ -3228,7 +3265,7 @@ rpc_clnt_swap_activate(struct rpc_clnt *clnt)
clnt = clnt->cl_parent;
if (atomic_inc_return(&clnt->cl_swapper) == 1)
return rpc_clnt_iterate_for_each_xprt(clnt, NULL,
- rpc_clnt_swap_activate_callback, NULL);
+ rpc_clnt_swap_activate_callback, NULL, false);
return 0;
}
EXPORT_SYMBOL_GPL(rpc_clnt_swap_activate);
@@ -3247,7 +3284,7 @@ rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
{
if (atomic_dec_if_positive(&clnt->cl_swapper) == 0)
rpc_clnt_iterate_for_each_xprt(clnt, NULL,
- rpc_clnt_swap_deactivate_callback, NULL);
+ rpc_clnt_swap_deactivate_callback, NULL, false);
}
EXPORT_SYMBOL_GPL(rpc_clnt_swap_deactivate);
#endif /* CONFIG_SUNRPC_SWAP */
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index ab60b4d3deb2..9c700bad1ec5 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -160,7 +160,8 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs, clnt,
&tasks_fops);

- rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs, &xprtnum);
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs, &xprtnum,
+ false);
}

void
diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
index e50f73a4aca5..60e2d738a8f1 100644
--- a/net/sunrpc/stats.c
+++ b/net/sunrpc/stats.c
@@ -258,7 +258,7 @@ void rpc_clnt_show_stats(struct seq_file *seq, struct rpc_clnt *clnt)
seq_printf(seq, "p/v: %u/%u (%s)\n",
clnt->cl_prog, clnt->cl_vers, clnt->cl_program->name);

- rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats, seq);
+ rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats, seq, false);

seq_printf(seq, "\tper-op statistics\n");
for (op = 0; op < maxproc; op++) {
--
2.27.0

2022-06-20 15:35:01

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 02/12] SUNRPC add function to offline remove trunkable transports

From: Olga Kornievskaia <[email protected]>

Iterate thru available transports in the xprt_switch for all
trunkable transports offline and possibly remote them as well.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
include/linux/sunrpc/clnt.h | 1 +
net/sunrpc/clnt.c | 42 +++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 90501404fa49..e74a0740603b 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -234,6 +234,7 @@ int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
struct rpc_xprt_switch *,
struct rpc_xprt *,
void *);
+void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void *);

const char *rpc_proc_name(const struct rpc_task *task);

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e2c6eca0271b..544b55a3aa20 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2999,6 +2999,48 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
}
EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);

+static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
+ struct rpc_xprt *xprt,
+ void *data)
+{
+ struct rpc_xprt *main_xprt;
+ struct rpc_xprt_switch *xps;
+ int err = 0;
+ int *offline_destroy = (int *)data;
+
+ xprt_get(xprt);
+
+ rcu_read_lock();
+ main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
+ xps = xprt_switch_get(rcu_dereference(clnt->cl_xpi.xpi_xpswitch));
+ err = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
+ (struct sockaddr *)&main_xprt->addr);
+ rcu_read_unlock();
+ xprt_put(main_xprt);
+ if (err)
+ goto out;
+
+ if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE)) {
+ err = -EINTR;
+ goto out;
+ }
+ xprt_set_offline_locked(xprt, xps);
+ if (*offline_destroy)
+ xprt_delete_locked(xprt, xps);
+
+ xprt_release_write(xprt, NULL);
+out:
+ xprt_put(xprt);
+ xprt_switch_put(xps);
+ return err;
+}
+
+void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void *data)
+{
+ rpc_clnt_iterate_for_each_xprt(clnt, rpc_xprt_offline_destroy, data);
+}
+EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);
+
struct connect_timeout_data {
unsigned long connect_timeout;
unsigned long reconnect_timeout;
--
2.27.0

2022-06-20 15:35:15

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 08/12] NFSv4.1 remove xprt from xprt_switch if session trunking test fails

From: Olga Kornievskaia <[email protected]>

If we are doing a session trunking test and it fails for the transport,
then remove this transport from the xprt_switch group.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5e4c32924347..152da2bc5100 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8917,6 +8917,9 @@ void nfs4_test_session_trunk(struct rpc_clnt *clnt, struct rpc_xprt *xprt,

if (status == 0)
rpc_clnt_xprt_switch_add_xprt(clnt, xprt);
+ else if (rpc_clnt_xprt_switch_has_addr(clnt,
+ (struct sockaddr *)&xprt->addr))
+ rpc_clnt_xprt_switch_remove_xprt(clnt, xprt);

rpc_put_task(task);
}
--
2.27.0

2022-06-20 15:35:22

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 12/12] NFSv4.1 probe offline transports for trunking on session creation

From: Olga Kornievskaia <[email protected]>

Once the session is established call into the SUNRPC layer to check
if any offlined trunking connections should be re-enabled.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 00778f351283..6bf21bc8c074 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -9244,6 +9244,13 @@ int nfs4_proc_create_session(struct nfs_client *clp, const struct cred *cred)
int status;
unsigned *ptr;
struct nfs4_session *session = clp->cl_session;
+ struct nfs4_add_xprt_data xprtdata = {
+ .clp = clp,
+ };
+ struct rpc_add_xprt_test rpcdata = {
+ .add_xprt_test = clp->cl_mvops->session_trunk,
+ .data = &xprtdata,
+ };

dprintk("--> %s clp=%p session=%p\n", __func__, clp, session);

@@ -9260,6 +9267,7 @@ int nfs4_proc_create_session(struct nfs_client *clp, const struct cred *cred)
ptr = (unsigned *)&session->sess_id.data[0];
dprintk("%s client>seqid %d sessionid %u:%u:%u:%u\n", __func__,
clp->cl_seqid, ptr[0], ptr[1], ptr[2], ptr[3]);
+ rpc_probe_trunked_xprts(clp->cl_rpcclient, &rpcdata);
out:
return status;
}
--
2.27.0

2022-06-20 15:35:24

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 03/12] NFSv4.1 offline trunkable transports on DESTROY_SESSION

From: Olga Kornievskaia <[email protected]>

When session is destroy, some of the transports might no longer be
valid trunks for the new session. Offline existing transports.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c0fdcf8c0032..cf898bea3bfd 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -9273,7 +9273,7 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
.rpc_argp = session,
.rpc_cred = cred,
};
- int status = 0;
+ int status = 0, offline = 0;

/* session is still being setup */
if (!test_and_clear_bit(NFS4_SESSION_ESTABLISHED, &session->session_state))
@@ -9286,6 +9286,7 @@ int nfs4_proc_destroy_session(struct nfs4_session *session,
if (status)
dprintk("NFS: Got error %d from the server on DESTROY_SESSION. "
"Session has been destroyed regardless...\n", status);
+ rpc_clnt_manage_trunked_xprts(session->clp->cl_rpcclient, &offline);
return status;
}

--
2.27.0

2022-06-20 15:35:27

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v1 06/12] SUNRPC enable back offline transports in trunking discovery

From: Olga Kornievskaia <[email protected]>

When we are adding a transport to a xprt_switch that's already on
the list but has been marked OFFLINE, then make the state ONLINE
since it's been tested now.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
include/linux/sunrpc/clnt.h | 1 +
net/sunrpc/clnt.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 20aed14fe222..319bcd3a3593 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -243,6 +243,7 @@ void rpc_clnt_xprt_switch_put(struct rpc_clnt *);
void rpc_clnt_xprt_switch_add_xprt(struct rpc_clnt *, struct rpc_xprt *);
bool rpc_clnt_xprt_switch_has_addr(struct rpc_clnt *clnt,
const struct sockaddr *sap);
+void rpc_clnt_xprt_set_online(struct rpc_clnt *clnt, struct rpc_xprt *xprt);
void rpc_cleanup_clids(void);

static inline int rpc_reply_expected(struct rpc_task *task)
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index b26267606de0..1cbd598f596c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -3105,8 +3105,22 @@ void rpc_clnt_xprt_switch_put(struct rpc_clnt *clnt)
}
EXPORT_SYMBOL_GPL(rpc_clnt_xprt_switch_put);

+void rpc_clnt_xprt_set_online(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
+{
+ struct rpc_xprt_switch *xps;
+
+ rcu_read_lock();
+ xps = rcu_dereference(clnt->cl_xpi.xpi_xpswitch);
+ rcu_read_unlock();
+ xprt_set_online_locked(xprt, xps);
+}
+
void rpc_clnt_xprt_switch_add_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
{
+ if (rpc_clnt_xprt_switch_has_addr(clnt,
+ (const struct sockaddr *)&xprt->addr)) {
+ return rpc_clnt_xprt_set_online(clnt, xprt);
+ }
rcu_read_lock();
rpc_xprt_switch_add_xprt(rcu_dereference(clnt->cl_xpi.xpi_xpswitch),
xprt);
--
2.27.0

2022-07-12 15:27:42

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v1 02/12] SUNRPC add function to offline remove trunkable transports

On Mon, 2022-06-20 at 11:23 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> Iterate thru available transports in the xprt_switch for all
> trunkable transports offline and possibly remote them as well.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
>  include/linux/sunrpc/clnt.h |  1 +
>  net/sunrpc/clnt.c           | 42
> +++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/include/linux/sunrpc/clnt.h
> b/include/linux/sunrpc/clnt.h
> index 90501404fa49..e74a0740603b 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -234,6 +234,7 @@
> int         rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
>                         struct rpc_xprt_switch *,
>                         struct rpc_xprt *,
>                         void *);
> +void           rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void
> *);
>  
>  const char *rpc_proc_name(const struct rpc_task *task);
>  
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index e2c6eca0271b..544b55a3aa20 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2999,6 +2999,48 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);
>  
> +static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
> +                                   struct rpc_xprt *xprt,
> +                                   void *data)
> +{
> +       struct rpc_xprt *main_xprt;
> +       struct rpc_xprt_switch *xps;
> +       int err = 0;
> +       int *offline_destroy = (int *)data;
> +
> +       xprt_get(xprt);
> +
> +       rcu_read_lock();
> +       main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
> +       xps = xprt_switch_get(rcu_dereference(clnt-
> >cl_xpi.xpi_xpswitch));
> +       err = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
> +                               (struct sockaddr *)&main_xprt->addr);
> +       rcu_read_unlock();
> +       xprt_put(main_xprt);
> +       if (err)
> +               goto out;
> +
> +       if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED,
> TASK_KILLABLE)) {
> +               err = -EINTR;
> +               goto out;
> +       }
> +       xprt_set_offline_locked(xprt, xps);
> +       if (*offline_destroy)
> +               xprt_delete_locked(xprt, xps);
> +
> +       xprt_release_write(xprt, NULL);
> +out:
> +       xprt_put(xprt);
> +       xprt_switch_put(xps);
> +       return err;
> +}
> +
> +void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void
> *data)
> +{
> +       rpc_clnt_iterate_for_each_xprt(clnt,
> rpc_xprt_offline_destroy, data);
> +}
> +EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);

Why is this function taking a 'void *' argument when
rpc_xprt_offline_destroy() won't accept anything other than an 'int *'.
It would be much cleaner to have 2 separate functions, neither or which
need more than one argument. Then you can hide the pointer to the 'int'
in each function and avoid exposing it as part of the API.

In addition, a function like this that is intended for use by a
different layer needs a proper kerneldoc comment so that we know what
the API is for, and what it does.

> +
>  struct connect_timeout_data {
>         unsigned long connect_timeout;
>         unsigned long reconnect_timeout;

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


2022-07-12 15:59:23

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v1 05/12] SUNRPC parameterize rpc_clnt_iterate_for_each_xprt with iterator init function

On Mon, 2022-06-20 at 11:24 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> Allow for rpc_clnt_iterate_for_each_xprt() to take in an iterator
> initialization function if no function passed in a default initiator
> is used.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
>  fs/nfs/nfs4proc.c           |  2 +-
>  include/linux/sunrpc/clnt.h |  1 +
>  net/sunrpc/clnt.c           | 17 ++++++++++++-----
>  net/sunrpc/debugfs.c        |  2 +-
>  net/sunrpc/stats.c          |  2 +-
>  5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index cf898bea3bfd..5e4c32924347 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -8532,7 +8532,7 @@ int nfs4_proc_bind_conn_to_session(struct
> nfs_client *clp, const struct cred *cr
>                 .clp = clp,
>                 .cred = cred,
>         };
> -       return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient,
> +       return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient,
> NULL,
>                         nfs4_proc_bind_conn_to_session_callback,
> &data);
>  }
>  
> diff --git a/include/linux/sunrpc/clnt.h
> b/include/linux/sunrpc/clnt.h
> index e74a0740603b..20aed14fe222 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -213,6 +213,7 @@ const char  *rpc_peeraddr2str(struct rpc_clnt *,
> enum rpc_display_format_t);
>  int            rpc_localaddr(struct rpc_clnt *, struct sockaddr *,
> size_t);
>  
>  int            rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
> +                       int (*setup)(struct rpc_clnt *, struct
> rpc_xprt_iter *),
>                         int (*fn)(struct rpc_clnt *, struct rpc_xprt
> *, void *),
>                         void *data);
>  
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 410bd6c352ad..b26267606de0 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -817,6 +817,8 @@ int rpc_clnt_xprt_iter_offline_init(struct
> rpc_clnt *clnt,
>  /**
>   * rpc_clnt_iterate_for_each_xprt - Apply a function to all
> transports
>   * @clnt: pointer to client
> + * @setup: an optional iterator init function to use, if none
> supplied
> + *   default rpc_clnt_xprt_iter_init() iterator is used
>   * @fn: function to apply
>   * @data: void pointer to function data
>   *
> @@ -826,13 +828,17 @@ int rpc_clnt_xprt_iter_offline_init(struct
> rpc_clnt *clnt,
>   * On error, the iteration stops, and the function returns the error
> value.
>   */
>  int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
> +               int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter
> *),
>                 int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void
> *),
>                 void *data)
>  {
>         struct rpc_xprt_iter xpi;
>         int ret;
>  
> -       ret = rpc_clnt_xprt_iter_init(clnt, &xpi);
> +       if (!setup)
> +               ret = rpc_clnt_xprt_iter_init(clnt, &xpi);
> +       else
> +               ret = setup(clnt, &xpi);

Please create an internal function that is not exported outside the RPC
layer for this. It would be fine to share code with
rpc_clnt_iterate_for_each_xprt() where appropriate, but we should not
expose an API that expects the caller to have intimate knowledge of the
rpc client internals.

>         if (ret)
>                 return ret;
>         for (;;) {
> @@ -3052,7 +3058,8 @@ static int rpc_xprt_offline_destroy(struct
> rpc_clnt *clnt,
>  
>  void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void
> *data)
>  {
> -       rpc_clnt_iterate_for_each_xprt(clnt,
> rpc_xprt_offline_destroy, data);
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL,
> rpc_xprt_offline_destroy,
> +                       data);
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);
>  
> @@ -3084,7 +3091,7 @@ rpc_set_connect_timeout(struct rpc_clnt *clnt,
>                 .connect_timeout = connect_timeout,
>                 .reconnect_timeout = reconnect_timeout,
>         };
> -       rpc_clnt_iterate_for_each_xprt(clnt,
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL,
>                         rpc_xprt_set_connect_timeout,
>                         &timeout);
>  }
> @@ -3181,7 +3188,7 @@ rpc_clnt_swap_activate(struct rpc_clnt *clnt)
>         while (clnt != clnt->cl_parent)
>                 clnt = clnt->cl_parent;
>         if (atomic_inc_return(&clnt->cl_swapper) == 1)
> -               return rpc_clnt_iterate_for_each_xprt(clnt,
> +               return rpc_clnt_iterate_for_each_xprt(clnt, NULL,
>                                 rpc_clnt_swap_activate_callback,
> NULL);
>         return 0;
>  }
> @@ -3200,7 +3207,7 @@ void
>  rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
>  {
>         if (atomic_dec_if_positive(&clnt->cl_swapper) == 0)
> -               rpc_clnt_iterate_for_each_xprt(clnt,
> +               rpc_clnt_iterate_for_each_xprt(clnt, NULL,
>                                 rpc_clnt_swap_deactivate_callback,
> NULL);
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_swap_deactivate);
> diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> index 7dc9cc929bfd..ab60b4d3deb2 100644
> --- a/net/sunrpc/debugfs.c
> +++ b/net/sunrpc/debugfs.c
> @@ -160,7 +160,7 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>         debugfs_create_file("tasks", S_IFREG | 0400, clnt-
> >cl_debugfs, clnt,
>                             &tasks_fops);
>  
> -       rpc_clnt_iterate_for_each_xprt(clnt, do_xprt_debugfs,
> &xprtnum);
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs,
> &xprtnum);
>  }
>  
>  void
> diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
> index 52908f9e6eab..e50f73a4aca5 100644
> --- a/net/sunrpc/stats.c
> +++ b/net/sunrpc/stats.c
> @@ -258,7 +258,7 @@ void rpc_clnt_show_stats(struct seq_file *seq,
> struct rpc_clnt *clnt)
>         seq_printf(seq, "p/v: %u/%u (%s)\n",
>                         clnt->cl_prog, clnt->cl_vers, clnt-
> >cl_program->name);
>  
> -       rpc_clnt_iterate_for_each_xprt(clnt, do_print_stats, seq);
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats,
> seq);
>  
>         seq_printf(seq, "\tper-op statistics\n");
>         for (op = 0; op < maxproc; op++) {

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


2022-07-12 16:02:43

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v1 11/12] SUNRPC create a function that probes only offline transports

On Mon, 2022-06-20 at 11:24 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> For only offline transports, attempt to check connectivity via
> a NULL call and, if that succeeds, call a provided session trunking
> detection function.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
>  fs/nfs/nfs4proc.c           |  2 +-
>  include/linux/sunrpc/clnt.h |  3 ++-
>  net/sunrpc/clnt.c           | 47 +++++++++++++++++++++++++++++++++--
> --
>  net/sunrpc/debugfs.c        |  3 ++-
>  net/sunrpc/stats.c          |  2 +-
>  5 files changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 152da2bc5100..00778f351283 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -8533,7 +8533,7 @@ int nfs4_proc_bind_conn_to_session(struct
> nfs_client *clp, const struct cred *cr
>                 .cred = cred,
>         };
>         return rpc_clnt_iterate_for_each_xprt(clp->cl_rpcclient,
> NULL,
> -                       nfs4_proc_bind_conn_to_session_callback,
> &data);
> +                       nfs4_proc_bind_conn_to_session_callback,
> &data, false);
>  }
>  
>  /*
> diff --git a/include/linux/sunrpc/clnt.h
> b/include/linux/sunrpc/clnt.h
> index ac1024da86c5..a0160b83d4a4 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -215,7 +215,7 @@ int         rpc_localaddr(struct rpc_clnt *,
> struct sockaddr *, size_t);
>  int            rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
>                         int (*setup)(struct rpc_clnt *, struct
> rpc_xprt_iter *),
>                         int (*fn)(struct rpc_clnt *, struct rpc_xprt
> *, void *),
> -                       void *data);
> +                       void *data, bool do_rewind);
>  
>  int            rpc_clnt_test_and_add_xprt(struct rpc_clnt *clnt,
>                         struct rpc_xprt_switch *xps,
> @@ -236,6 +236,7 @@
> int         rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
>                         struct rpc_xprt *,
>                         void *);
>  void           rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void
> *);
> +void           rpc_probe_trunked_xprts(struct rpc_clnt *, void *);
>  
>  const char *rpc_proc_name(const struct rpc_task *task);
>  
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 6b04b29bf842..348d0772c91d 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -830,7 +830,7 @@ int rpc_clnt_xprt_iter_offline_init(struct
> rpc_clnt *clnt,
>  int rpc_clnt_iterate_for_each_xprt(struct rpc_clnt *clnt,
>                 int (*setup)(struct rpc_clnt *, struct rpc_xprt_iter
> *),
>                 int (*fn)(struct rpc_clnt *, struct rpc_xprt *, void
> *),
> -               void *data)
> +               void *data, bool do_rewind)
>  {
>         struct rpc_xprt_iter xpi;
>         int ret;
> @@ -850,6 +850,9 @@ int rpc_clnt_iterate_for_each_xprt(struct
> rpc_clnt *clnt,
>                 xprt_put(xprt);
>                 if (ret < 0)
>                         break;
> +
> +               if (do_rewind)
> +                       xprt_iter_rewind(&xpi);

This really needs to be another separate function. You are not
iterating over each xprt here, you are always looking at the first
valid entry.

>         }
>         xprt_iter_destroy(&xpi);
>         return ret;
> @@ -3032,6 +3035,40 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);
>  
> +static int rpc_xprt_probe_trunked(struct rpc_clnt *clnt,
> +                                 struct rpc_xprt *xprt,
> +                                 void *data)
> +{
> +       struct rpc_xprt_switch *xps;
> +       struct rpc_xprt *main_xprt;
> +       int status = 0;
> +
> +       xprt_get(xprt);
> +
> +       rcu_read_lock();
> +       main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
> +       xps = xprt_switch_get(rcu_dereference(clnt-
> >cl_xpi.xpi_xpswitch));
> +       status = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
> +                               (struct sockaddr *)&main_xprt->addr);
> +       rcu_read_unlock();
> +       xprt_put(main_xprt);
> +       if (status || !test_bit(XPRT_OFFLINE, &xprt->state))
> +               goto out;
> +
> +       status = rpc_clnt_add_xprt_helper(clnt, xprt, data);
> +out:
> +       xprt_put(xprt);
> +       xprt_switch_put(xps);
> +       return status;
> +}
> +
> +void rpc_probe_trunked_xprts(struct rpc_clnt *clnt, void *data)

'data' is not a 'void *'. You patch 447574a510fc ("SUNRPC restructure
rpc_clnt_setup_test_and_add_xprt") means that only a 'struct
rpc_add_xprt_test' argument is allowed here.

> +{
> +       rpc_clnt_iterate_for_each_xprt(clnt,
> rpc_clnt_xprt_iter_offline_init,
> +                       rpc_xprt_probe_trunked, data, true);
> +}
> +EXPORT_SYMBOL_GPL(rpc_probe_trunked_xprts);
> +
>  static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
>                                     struct rpc_xprt *xprt,
>                                     void *data)
> @@ -3071,7 +3108,7 @@ static int rpc_xprt_offline_destroy(struct
> rpc_clnt *clnt,
>  void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void
> *data)
>  {
>         rpc_clnt_iterate_for_each_xprt(clnt, NULL,
> rpc_xprt_offline_destroy,
> -                       data);
> +                       data, false);
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);
>  
> @@ -3105,7 +3142,7 @@ rpc_set_connect_timeout(struct rpc_clnt *clnt,
>         };
>         rpc_clnt_iterate_for_each_xprt(clnt, NULL,
>                         rpc_xprt_set_connect_timeout,
> -                       &timeout);
> +                       &timeout, false);
>  }
>  EXPORT_SYMBOL_GPL(rpc_set_connect_timeout);
>  
> @@ -3228,7 +3265,7 @@ rpc_clnt_swap_activate(struct rpc_clnt *clnt)
>                 clnt = clnt->cl_parent;
>         if (atomic_inc_return(&clnt->cl_swapper) == 1)
>                 return rpc_clnt_iterate_for_each_xprt(clnt, NULL,
> -                               rpc_clnt_swap_activate_callback,
> NULL);
> +                               rpc_clnt_swap_activate_callback,
> NULL, false);
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_swap_activate);
> @@ -3247,7 +3284,7 @@ rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
>  {
>         if (atomic_dec_if_positive(&clnt->cl_swapper) == 0)
>                 rpc_clnt_iterate_for_each_xprt(clnt, NULL,
> -                               rpc_clnt_swap_deactivate_callback,
> NULL);
> +                               rpc_clnt_swap_deactivate_callback,
> NULL, false);
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_swap_deactivate);
>  #endif /* CONFIG_SUNRPC_SWAP */
> diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> index ab60b4d3deb2..9c700bad1ec5 100644
> --- a/net/sunrpc/debugfs.c
> +++ b/net/sunrpc/debugfs.c
> @@ -160,7 +160,8 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>         debugfs_create_file("tasks", S_IFREG | 0400, clnt-
> >cl_debugfs, clnt,
>                             &tasks_fops);
>  
> -       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs,
> &xprtnum);
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_xprt_debugfs,
> &xprtnum,
> +                       false);
>  }
>  
>  void
> diff --git a/net/sunrpc/stats.c b/net/sunrpc/stats.c
> index e50f73a4aca5..60e2d738a8f1 100644
> --- a/net/sunrpc/stats.c
> +++ b/net/sunrpc/stats.c
> @@ -258,7 +258,7 @@ void rpc_clnt_show_stats(struct seq_file *seq,
> struct rpc_clnt *clnt)
>         seq_printf(seq, "p/v: %u/%u (%s)\n",
>                         clnt->cl_prog, clnt->cl_vers, clnt-
> >cl_program->name);
>  
> -       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats,
> seq);
> +       rpc_clnt_iterate_for_each_xprt(clnt, NULL, do_print_stats,
> seq, false);
>  
>         seq_printf(seq, "\tper-op statistics\n");
>         for (op = 0; op < maxproc; op++) {

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


2022-07-12 16:08:54

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH v1 02/12] SUNRPC add function to offline remove trunkable transports

On Tue, Jul 12, 2022 at 11:24 AM Trond Myklebust
<[email protected]> wrote:
>
> On Mon, 2022-06-20 at 11:23 -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <[email protected]>
> >
> > Iterate thru available transports in the xprt_switch for all
> > trunkable transports offline and possibly remote them as well.
> >
> > Signed-off-by: Olga Kornievskaia <[email protected]>
> > ---
> > include/linux/sunrpc/clnt.h | 1 +
> > net/sunrpc/clnt.c | 42
> > +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/include/linux/sunrpc/clnt.h
> > b/include/linux/sunrpc/clnt.h
> > index 90501404fa49..e74a0740603b 100644
> > --- a/include/linux/sunrpc/clnt.h
> > +++ b/include/linux/sunrpc/clnt.h
> > @@ -234,6 +234,7 @@
> > int rpc_clnt_setup_test_and_add_xprt(struct rpc_clnt *,
> > struct rpc_xprt_switch *,
> > struct rpc_xprt *,
> > void *);
> > +void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *, void
> > *);
> >
> > const char *rpc_proc_name(const struct rpc_task *task);
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index e2c6eca0271b..544b55a3aa20 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -2999,6 +2999,48 @@ int rpc_clnt_add_xprt(struct rpc_clnt *clnt,
> > }
> > EXPORT_SYMBOL_GPL(rpc_clnt_add_xprt);
> >
> > +static int rpc_xprt_offline_destroy(struct rpc_clnt *clnt,
> > + struct rpc_xprt *xprt,
> > + void *data)
> > +{
> > + struct rpc_xprt *main_xprt;
> > + struct rpc_xprt_switch *xps;
> > + int err = 0;
> > + int *offline_destroy = (int *)data;
> > +
> > + xprt_get(xprt);
> > +
> > + rcu_read_lock();
> > + main_xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
> > + xps = xprt_switch_get(rcu_dereference(clnt-
> > >cl_xpi.xpi_xpswitch));
> > + err = rpc_cmp_addr_port((struct sockaddr *)&xprt->addr,
> > + (struct sockaddr *)&main_xprt->addr);
> > + rcu_read_unlock();
> > + xprt_put(main_xprt);
> > + if (err)
> > + goto out;
> > +
> > + if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED,
> > TASK_KILLABLE)) {
> > + err = -EINTR;
> > + goto out;
> > + }
> > + xprt_set_offline_locked(xprt, xps);
> > + if (*offline_destroy)
> > + xprt_delete_locked(xprt, xps);
> > +
> > + xprt_release_write(xprt, NULL);
> > +out:
> > + xprt_put(xprt);
> > + xprt_switch_put(xps);
> > + return err;
> > +}
> > +
> > +void rpc_clnt_manage_trunked_xprts(struct rpc_clnt *clnt, void
> > *data)
> > +{
> > + rpc_clnt_iterate_for_each_xprt(clnt,
> > rpc_xprt_offline_destroy, data);
> > +}
> > +EXPORT_SYMBOL_GPL(rpc_clnt_manage_trunked_xprts);
>
> Why is this function taking a 'void *' argument when
> rpc_xprt_offline_destroy() won't accept anything other than an 'int *'.
> It would be much cleaner to have 2 separate functions, neither or which
> need more than one argument. Then you can hide the pointer to the 'int'
> in each function and avoid exposing it as part of the API.

I could remove the void * altogether. As the following code only
offlines the transports. I wrote this function to be generic to be
able to do either if the need arises. It wasn't clear to me what you
meant by "have 2 separate functions". If you mean one for offline and
another for destroy, then perhaps that removes that need too. However,
if we were to have a generic one then since the majority of the code
is the same I don't see how having 2 functions is better.

> In addition, a function like this that is intended for use by
> different layer needs a proper kerneldoc comment so that we know what
> the API is for, and what it does.

Will add a comment above the function to explain what it does.

>
> > +
> > struct connect_timeout_data {
> > unsigned long connect_timeout;
> > unsigned long reconnect_timeout;
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
>
>