v2:
- don't take xprt lock unless we need to manipulate the memalloc flag
- add new xprt operations for swap enable/disable
This series is a (small) overhaul of the swap-over-NFS code. The main
impetus is to fix the problem reported by Jerome Marchand. We currently
hold the rcu_read_lock when calling xs_swapper and that's just plain
wrong. The first patch in this series should fix that problem, and also
clean up a bit of a layering violation.
The other focus of this set is to change how the swapper refcounting
works. Right now, it's only tracked in the rpc_xprt, and there seem to
be some gaps in its coverage -- places where we should taking or
dropping references but aren't. This changes it so that the clnt tracks
the number of swapfiles that it has, and the xprt tracks the number of
"swappable" clients.
It also ensures that we only call sk_set_memalloc once per socket. I
believe that's the correct thing to do as the main reason for the
memalloc_socks counter is to track whether we have _any_
memalloc-enabled sockets.
There is still some work to be done here as I think there remains some
potential for races between swapon/swapoff, and reconnect or migration
events. That will take some careful thought that I haven't the time to
spend on at the moment. I don't think this set will make those races
any worse though.
Jeff Layton (5):
sunrpc: keep a count of swapfiles associated with the rpc_clnt
sunrpc: make xprt->swapper an atomic_t
sunrpc: if we're closing down a socket, clear memalloc on it first
sunrpc: lock xprt before trying to set memalloc on the sockets
sunrpc: turn swapper_enable/disable functions into rpc_xprt_ops
fs/nfs/file.c | 11 +-----
include/linux/sunrpc/clnt.h | 1 +
include/linux/sunrpc/sched.h | 16 ++++++++
include/linux/sunrpc/xprt.h | 17 ++++++++-
net/sunrpc/clnt.c | 67 +++++++++++++++++++++++++++-----
net/sunrpc/xprtrdma/transport.c | 15 +++++++-
net/sunrpc/xprtsock.c | 84 +++++++++++++++++++++++++++++++++--------
7 files changed, 174 insertions(+), 37 deletions(-)
--
2.4.2
Split xs_swapper into enable/disable functions and eliminate the
"enable" flag.
Currently, it's racy if you have multiple swapon/swapoff operations
running in parallel over the same xprt. Also fix it so that we only
set it to a memalloc socket on a 0->1 transition and only clear it
on a 1->0 transition.
Cc: Mel Gorman <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
include/linux/sunrpc/xprt.h | 5 +++--
net/sunrpc/clnt.c | 4 ++--
net/sunrpc/xprtsock.c | 38 +++++++++++++++++++++++++-------------
3 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 8b93ef53df3c..26b1624128ec 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -180,7 +180,7 @@ struct rpc_xprt {
atomic_t num_reqs; /* total slots */
unsigned long state; /* transport state */
unsigned char resvport : 1; /* use a reserved port */
- unsigned int swapper; /* we're swapping over this
+ atomic_t swapper; /* we're swapping over this
transport */
unsigned int bind_index; /* bind function index */
@@ -345,7 +345,8 @@ void xprt_release_rqst_cong(struct rpc_task *task);
void xprt_disconnect_done(struct rpc_xprt *xprt);
void xprt_force_disconnect(struct rpc_xprt *xprt);
void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
-int xs_swapper(struct rpc_xprt *xprt, int enable);
+int xs_swapper_enable(struct rpc_xprt *xprt);
+void xs_swapper_disable(struct rpc_xprt *xprt);
bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
void xprt_unlock_connect(struct rpc_xprt *, void *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 383cb778179f..804a75e71e84 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2492,7 +2492,7 @@ retry:
goto retry;
}
- ret = xs_swapper(xprt, 1);
+ ret = xs_swapper_enable(xprt);
xprt_put(xprt);
}
return ret;
@@ -2519,7 +2519,7 @@ retry:
goto retry;
}
- xs_swapper(xprt, 0);
+ xs_swapper_disable(xprt);
xprt_put(xprt);
}
}
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index b29703996028..a2861bbfd319 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1966,31 +1966,43 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
xprt);
- if (xprt->swapper)
+ if (atomic_read(&xprt->swapper))
sk_set_memalloc(transport->inet);
}
/**
- * xs_swapper - Tag this transport as being used for swap.
+ * xs_swapper_enable - Tag this transport as being used for swap.
* @xprt: transport to tag
- * @enable: enable/disable
*
+ * Take a reference to this transport on behalf of the rpc_clnt, and
+ * optionally mark it for swapping if it wasn't already.
*/
-int xs_swapper(struct rpc_xprt *xprt, int enable)
+int
+xs_swapper_enable(struct rpc_xprt *xprt)
{
struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
xprt);
- int err = 0;
- if (enable) {
- xprt->swapper++;
- xs_set_memalloc(xprt);
- } else if (xprt->swapper) {
- xprt->swapper--;
- sk_clear_memalloc(transport->inet);
- }
+ if (atomic_inc_return(&xprt->swapper) == 1)
+ sk_set_memalloc(transport->inet);
+ return 0;
+}
- return err;
+/**
+ * xs_swapper_disable - Untag this transport as being used for swap.
+ * @xprt: transport to tag
+ *
+ * Drop a "swapper" reference to this xprt on behalf of the rpc_clnt. If the
+ * swapper refcount goes to 0, untag the socket as a memalloc socket.
+ */
+void
+xs_swapper_disable(struct rpc_xprt *xprt)
+{
+ struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
+ xprt);
+
+ if (atomic_dec_and_test(&xprt->swapper))
+ sk_clear_memalloc(transport->inet);
}
#else
static void xs_set_memalloc(struct rpc_xprt *xprt)
--
2.4.2
Jerome reported seeing a warning pop when working with a swapfile on
NFS. The nfs_swap_activate can end up calling sk_set_memalloc while
holding the rcu_read_lock and that function can sleep.
To fix that, we need to take a reference to the xprt while holding the
rcu_read_lock, set the socket up for swapping and then drop that
reference. But, xprt_put is not exported and having NFS deal with the
underlying xprt is a bit of layering violation anyway.
Fix this by adding a set of activate/deactivate functions that take a
rpc_clnt pointer instead of an rpc_xprt, and have nfs_swap_activate and
nfs_swap_deactivate call those.
Also, add a per-rpc_clnt atomic counter to keep track of the number of
active swapfiles associated with it. When the counter does a 0->1
transition, we enable swapping on the xprt, when we do a 1->0 transition
we disable swapping on it.
This also allows us to be a bit more selective with the RPC_TASK_SWAPPER
flag. If non-swapper and swapper clnts are sharing a xprt, then we only
need to flag the tasks from the swapper clnt with that flag.
Acked-by: Mel Gorman <[email protected]>
Reported-by: Jerome Marchand <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/file.c | 11 ++------
include/linux/sunrpc/clnt.h | 1 +
include/linux/sunrpc/sched.h | 16 +++++++++++
net/sunrpc/clnt.c | 67 ++++++++++++++++++++++++++++++++++++++------
net/sunrpc/xprtsock.c | 1 -
5 files changed, 77 insertions(+), 19 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 8b8d83a526ce..7b26840ccfe1 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -559,25 +559,18 @@ static int nfs_launder_page(struct page *page)
static int nfs_swap_activate(struct swap_info_struct *sis, struct file *file,
sector_t *span)
{
- int ret;
struct rpc_clnt *clnt = NFS_CLIENT(file->f_mapping->host);
*span = sis->pages;
- rcu_read_lock();
- ret = xs_swapper(rcu_dereference(clnt->cl_xprt), 1);
- rcu_read_unlock();
-
- return ret;
+ return rpc_clnt_swap_activate(clnt);
}
static void nfs_swap_deactivate(struct file *file)
{
struct rpc_clnt *clnt = NFS_CLIENT(file->f_mapping->host);
- rcu_read_lock();
- xs_swapper(rcu_dereference(clnt->cl_xprt), 0);
- rcu_read_unlock();
+ rpc_clnt_swap_deactivate(clnt);
}
#endif
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 598ba80ec30c..131032f15cc1 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -56,6 +56,7 @@ struct rpc_clnt {
struct rpc_rtt * cl_rtt; /* RTO estimator data */
const struct rpc_timeout *cl_timeout; /* Timeout strategy */
+ atomic_t cl_swapper; /* swapfile count */
int cl_nodelen; /* nodename length */
char cl_nodename[UNX_MAXNODENAME+1];
struct rpc_pipe_dir_head cl_pipedir_objects;
diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 5f1e6bd4c316..50472d716e72 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -269,4 +269,20 @@ static inline void rpc_assign_waitqueue_name(struct rpc_wait_queue *q,
}
#endif
+#if IS_ENABLED(CONFIG_SUNRPC_SWAP)
+int rpc_clnt_swap_activate(struct rpc_clnt *clnt);
+void rpc_clnt_swap_deactivate(struct rpc_clnt *clnt);
+#else
+static inline int
+rpc_clnt_swap_activate(struct rpc_clnt *clnt)
+{
+ return 0;
+}
+
+static inline void
+rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
+{
+}
+#endif /* CONFIG_SUNRPC_SWAP */
+
#endif /* _LINUX_SUNRPC_SCHED_H_ */
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index e6ce1517367f..383cb778179f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -891,15 +891,8 @@ void rpc_task_set_client(struct rpc_task *task, struct rpc_clnt *clnt)
task->tk_flags |= RPC_TASK_SOFT;
if (clnt->cl_noretranstimeo)
task->tk_flags |= RPC_TASK_NO_RETRANS_TIMEOUT;
- if (sk_memalloc_socks()) {
- struct rpc_xprt *xprt;
-
- rcu_read_lock();
- xprt = rcu_dereference(clnt->cl_xprt);
- if (xprt->swapper)
- task->tk_flags |= RPC_TASK_SWAPPER;
- rcu_read_unlock();
- }
+ if (atomic_read(&clnt->cl_swapper))
+ task->tk_flags |= RPC_TASK_SWAPPER;
/* Add to the client's list of all tasks */
spin_lock(&clnt->cl_lock);
list_add_tail(&task->tk_task, &clnt->cl_tasks);
@@ -2476,3 +2469,59 @@ void rpc_show_tasks(struct net *net)
spin_unlock(&sn->rpc_client_lock);
}
#endif
+
+#if IS_ENABLED(CONFIG_SUNRPC_SWAP)
+int
+rpc_clnt_swap_activate(struct rpc_clnt *clnt)
+{
+ int ret = 0;
+ struct rpc_xprt *xprt;
+
+ if (atomic_inc_return(&clnt->cl_swapper) == 1) {
+retry:
+ rcu_read_lock();
+ xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
+ rcu_read_unlock();
+ if (!xprt) {
+ /*
+ * If we didn't get a reference, then we likely are
+ * racing with a migration event. Wait for a grace
+ * period and try again.
+ */
+ synchronize_rcu();
+ goto retry;
+ }
+
+ ret = xs_swapper(xprt, 1);
+ xprt_put(xprt);
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(rpc_clnt_swap_activate);
+
+void
+rpc_clnt_swap_deactivate(struct rpc_clnt *clnt)
+{
+ struct rpc_xprt *xprt;
+
+ if (atomic_dec_if_positive(&clnt->cl_swapper) == 0) {
+retry:
+ rcu_read_lock();
+ xprt = xprt_get(rcu_dereference(clnt->cl_xprt));
+ rcu_read_unlock();
+ if (!xprt) {
+ /*
+ * If we didn't get a reference, then we likely are
+ * racing with a migration event. Wait for a grace
+ * period and try again.
+ */
+ synchronize_rcu();
+ goto retry;
+ }
+
+ xs_swapper(xprt, 0);
+ xprt_put(xprt);
+ }
+}
+EXPORT_SYMBOL_GPL(rpc_clnt_swap_deactivate);
+#endif /* CONFIG_SUNRPC_SWAP */
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 66891e32c5e3..b29703996028 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1992,7 +1992,6 @@ int xs_swapper(struct rpc_xprt *xprt, int enable)
return err;
}
-EXPORT_SYMBOL_GPL(xs_swapper);
#else
static void xs_set_memalloc(struct rpc_xprt *xprt)
{
--
2.4.2
We currently increment the memalloc_socks counter if we have a xprt that
is associated with a swapfile. That socket can be replaced however
during a reconnect event, and the memalloc_socks counter is never
decremented if that occurs.
When tearing down a xprt socket, check to see if the xprt is set up for
swapping and sk_clear_memalloc before releasing the socket if so.
Acked-by: Mel Gorman <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/xprtsock.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index a2861bbfd319..359446442112 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -827,6 +827,9 @@ static void xs_reset_transport(struct sock_xprt *transport)
if (sk == NULL)
return;
+ if (atomic_read(&transport->xprt.swapper))
+ sk_clear_memalloc(sk);
+
write_lock_bh(&sk->sk_callback_lock);
transport->inet = NULL;
transport->sock = NULL;
--
2.4.2
RDMA xprts don't have a sock_xprt, but an rdma_xprt, so the
xs_swapper_enable/disable functions will likely oops when fed an RDMA
xprt. Turn these functions into rpc_xprt_ops so that that doesn't
occur. For now the RDMA versions are no-ops.
Cc: Chuck Lever <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
include/linux/sunrpc/xprt.h | 16 ++++++++++++++--
net/sunrpc/clnt.c | 4 ++--
net/sunrpc/xprtrdma/transport.c | 15 ++++++++++++++-
net/sunrpc/xprtsock.c | 31 +++++++++++++++++++++++++------
4 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index 26b1624128ec..7eb58610eb94 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -133,6 +133,8 @@ struct rpc_xprt_ops {
void (*close)(struct rpc_xprt *xprt);
void (*destroy)(struct rpc_xprt *xprt);
void (*print_stats)(struct rpc_xprt *xprt, struct seq_file *seq);
+ int (*enable_swap)(struct rpc_xprt *xprt);
+ void (*disable_swap)(struct rpc_xprt *xprt);
};
/*
@@ -327,6 +329,18 @@ static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *
return p + xprt->tsh_size;
}
+static inline int
+xprt_enable_swap(struct rpc_xprt *xprt)
+{
+ return xprt->ops->enable_swap(xprt);
+}
+
+static inline void
+xprt_disable_swap(struct rpc_xprt *xprt)
+{
+ xprt->ops->disable_swap(xprt);
+}
+
/*
* Transport switch helper functions
*/
@@ -345,8 +359,6 @@ void xprt_release_rqst_cong(struct rpc_task *task);
void xprt_disconnect_done(struct rpc_xprt *xprt);
void xprt_force_disconnect(struct rpc_xprt *xprt);
void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
-int xs_swapper_enable(struct rpc_xprt *xprt);
-void xs_swapper_disable(struct rpc_xprt *xprt);
bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
void xprt_unlock_connect(struct rpc_xprt *, void *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 804a75e71e84..60d1835edb26 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2492,7 +2492,7 @@ retry:
goto retry;
}
- ret = xs_swapper_enable(xprt);
+ ret = xprt_enable_swap(xprt);
xprt_put(xprt);
}
return ret;
@@ -2519,7 +2519,7 @@ retry:
goto retry;
}
- xs_swapper_disable(xprt);
+ xprt_disable_swap(xprt);
xprt_put(xprt);
}
}
diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
index 54f23b1be986..e7a157754095 100644
--- a/net/sunrpc/xprtrdma/transport.c
+++ b/net/sunrpc/xprtrdma/transport.c
@@ -682,6 +682,17 @@ static void xprt_rdma_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
r_xprt->rx_stats.bad_reply_count);
}
+static int
+xprt_rdma_enable_swap(struct rpc_xprt *xprt)
+{
+ return 0;
+}
+
+static void
+xprt_rdma_disable_swap(struct rpc_xprt *xprt)
+{
+}
+
/*
* Plumbing for rpc transport switch and kernel module
*/
@@ -700,7 +711,9 @@ static struct rpc_xprt_ops xprt_rdma_procs = {
.send_request = xprt_rdma_send_request,
.close = xprt_rdma_close,
.destroy = xprt_rdma_destroy,
- .print_stats = xprt_rdma_print_stats
+ .print_stats = xprt_rdma_print_stats,
+ .enable_swap = xprt_rdma_enable_swap,
+ .disable_swap = xprt_rdma_disable_swap,
};
static struct xprt_class xprt_rdma = {
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 16aa5dad41b2..b8aaf20aea96 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1985,14 +1985,14 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
}
/**
- * xs_swapper_enable - Tag this transport as being used for swap.
+ * xs_enable_swap - Tag this transport as being used for swap.
* @xprt: transport to tag
*
* Take a reference to this transport on behalf of the rpc_clnt, and
* optionally mark it for swapping if it wasn't already.
*/
-int
-xs_swapper_enable(struct rpc_xprt *xprt)
+static int
+xs_enable_swap(struct rpc_xprt *xprt)
{
struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
@@ -2007,14 +2007,14 @@ xs_swapper_enable(struct rpc_xprt *xprt)
}
/**
- * xs_swapper_disable - Untag this transport as being used for swap.
+ * xs_disable_swap - Untag this transport as being used for swap.
* @xprt: transport to tag
*
* Drop a "swapper" reference to this xprt on behalf of the rpc_clnt. If the
* swapper refcount goes to 0, untag the socket as a memalloc socket.
*/
-void
-xs_swapper_disable(struct rpc_xprt *xprt)
+static void
+xs_disable_swap(struct rpc_xprt *xprt)
{
struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
@@ -2030,6 +2030,17 @@ xs_swapper_disable(struct rpc_xprt *xprt)
static void xs_set_memalloc(struct rpc_xprt *xprt)
{
}
+
+static int
+xs_enable_swap(struct rpc_xprt *xprt)
+{
+ return 0;
+}
+
+static void
+xs_disable_swap(struct rpc_xprt *xprt)
+{
+}
#endif
static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
@@ -2496,6 +2507,8 @@ static struct rpc_xprt_ops xs_local_ops = {
.close = xs_close,
.destroy = xs_destroy,
.print_stats = xs_local_print_stats,
+ .enable_swap = xs_enable_swap,
+ .disable_swap = xs_disable_swap,
};
static struct rpc_xprt_ops xs_udp_ops = {
@@ -2515,6 +2528,8 @@ static struct rpc_xprt_ops xs_udp_ops = {
.close = xs_close,
.destroy = xs_destroy,
.print_stats = xs_udp_print_stats,
+ .enable_swap = xs_enable_swap,
+ .disable_swap = xs_disable_swap,
};
static struct rpc_xprt_ops xs_tcp_ops = {
@@ -2531,6 +2546,8 @@ static struct rpc_xprt_ops xs_tcp_ops = {
.close = xs_tcp_shutdown,
.destroy = xs_destroy,
.print_stats = xs_tcp_print_stats,
+ .enable_swap = xs_enable_swap,
+ .disable_swap = xs_disable_swap,
};
/*
@@ -2548,6 +2565,8 @@ static struct rpc_xprt_ops bc_tcp_ops = {
.close = bc_close,
.destroy = bc_destroy,
.print_stats = xs_tcp_print_stats,
+ .enable_swap = xs_enable_swap,
+ .disable_swap = xs_disable_swap,
};
static int xs_init_anyaddr(const int family, struct sockaddr *sap)
--
2.4.2
It's possible that we could race with a call to xs_reset_transport, in
which case the xprt->inet pointer could be zeroed out while we're
accessing it. Lock the xprt before we try to set memalloc on it.
Cc: Mel Gorman <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/xprtsock.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 359446442112..16aa5dad41b2 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1964,11 +1964,22 @@ static void xs_local_connect(struct rpc_xprt *xprt, struct rpc_task *task)
}
#ifdef CONFIG_SUNRPC_SWAP
+/*
+ * Note that this should be called with XPRT_LOCKED held (or when we otherwise
+ * know that we have exclusive access to the socket), to guard against
+ * races with xs_reset_transport.
+ */
static void xs_set_memalloc(struct rpc_xprt *xprt)
{
struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
xprt);
+ /*
+ * If there's no sock, then we have nothing to set. The
+ * reconnecting process will get it for us.
+ */
+ if (!transport->inet)
+ return;
if (atomic_read(&xprt->swapper))
sk_set_memalloc(transport->inet);
}
@@ -1983,11 +1994,15 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
int
xs_swapper_enable(struct rpc_xprt *xprt)
{
- struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
- xprt);
+ struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
- if (atomic_inc_return(&xprt->swapper) == 1)
- sk_set_memalloc(transport->inet);
+ if (atomic_inc_return(&xprt->swapper) != 1)
+ return 0;
+ if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE))
+ return -ERESTARTSYS;
+ if (xs->inet)
+ sk_set_memalloc(xs->inet);
+ xprt_release_xprt(xprt, NULL);
return 0;
}
@@ -2001,11 +2016,15 @@ xs_swapper_enable(struct rpc_xprt *xprt)
void
xs_swapper_disable(struct rpc_xprt *xprt)
{
- struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
- xprt);
+ struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
- if (atomic_dec_and_test(&xprt->swapper))
- sk_clear_memalloc(transport->inet);
+ if (!atomic_dec_and_test(&xprt->swapper))
+ return;
+ if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE))
+ return;
+ if (xs->inet)
+ sk_clear_memalloc(xs->inet);
+ xprt_release_xprt(xprt, NULL);
}
#else
static void xs_set_memalloc(struct rpc_xprt *xprt)
--
2.4.2
On Wed, Jun 3, 2015 at 10:43 AM, Jeff Layton <[email protected]> wrote:
> RDMA xprts don't have a sock_xprt, but an rdma_xprt, so the
> xs_swapper_enable/disable functions will likely oops when fed an RDMA
> xprt. Turn these functions into rpc_xprt_ops so that that doesn't
> occur. For now the RDMA versions are no-ops.
>
> Cc: Chuck Lever <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> include/linux/sunrpc/xprt.h | 16 ++++++++++++++--
> net/sunrpc/clnt.c | 4 ++--
> net/sunrpc/xprtrdma/transport.c | 15 ++++++++++++++-
> net/sunrpc/xprtsock.c | 31 +++++++++++++++++++++++++------
> 4 files changed, 55 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> index 26b1624128ec..7eb58610eb94 100644
> --- a/include/linux/sunrpc/xprt.h
> +++ b/include/linux/sunrpc/xprt.h
> @@ -133,6 +133,8 @@ struct rpc_xprt_ops {
> void (*close)(struct rpc_xprt *xprt);
> void (*destroy)(struct rpc_xprt *xprt);
> void (*print_stats)(struct rpc_xprt *xprt, struct seq_file *seq);
> + int (*enable_swap)(struct rpc_xprt *xprt);
> + void (*disable_swap)(struct rpc_xprt *xprt);
> };
>
> /*
> @@ -327,6 +329,18 @@ static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *
> return p + xprt->tsh_size;
> }
>
> +static inline int
> +xprt_enable_swap(struct rpc_xprt *xprt)
> +{
> + return xprt->ops->enable_swap(xprt);
> +}
> +
> +static inline void
> +xprt_disable_swap(struct rpc_xprt *xprt)
> +{
> + xprt->ops->disable_swap(xprt);
> +}
> +
> /*
> * Transport switch helper functions
> */
> @@ -345,8 +359,6 @@ void xprt_release_rqst_cong(struct rpc_task *task);
> void xprt_disconnect_done(struct rpc_xprt *xprt);
> void xprt_force_disconnect(struct rpc_xprt *xprt);
> void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
> -int xs_swapper_enable(struct rpc_xprt *xprt);
> -void xs_swapper_disable(struct rpc_xprt *xprt);
>
> bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
> void xprt_unlock_connect(struct rpc_xprt *, void *);
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 804a75e71e84..60d1835edb26 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2492,7 +2492,7 @@ retry:
> goto retry;
> }
>
> - ret = xs_swapper_enable(xprt);
> + ret = xprt_enable_swap(xprt);
> xprt_put(xprt);
> }
> return ret;
> @@ -2519,7 +2519,7 @@ retry:
> goto retry;
> }
>
> - xs_swapper_disable(xprt);
> + xprt_disable_swap(xprt);
> xprt_put(xprt);
> }
> }
> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
> index 54f23b1be986..e7a157754095 100644
> --- a/net/sunrpc/xprtrdma/transport.c
> +++ b/net/sunrpc/xprtrdma/transport.c
> @@ -682,6 +682,17 @@ static void xprt_rdma_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
> r_xprt->rx_stats.bad_reply_count);
> }
>
> +static int
> +xprt_rdma_enable_swap(struct rpc_xprt *xprt)
> +{
> + return 0;
Shouldn't the function be returning an error here? What does swapon
expect if the device you are trying to enable doesn't support swap?
> +}
> +
> +static void
> +xprt_rdma_disable_swap(struct rpc_xprt *xprt)
> +{
> +}
> +
> /*
> * Plumbing for rpc transport switch and kernel module
> */
> @@ -700,7 +711,9 @@ static struct rpc_xprt_ops xprt_rdma_procs = {
> .send_request = xprt_rdma_send_request,
> .close = xprt_rdma_close,
> .destroy = xprt_rdma_destroy,
> - .print_stats = xprt_rdma_print_stats
> + .print_stats = xprt_rdma_print_stats,
> + .enable_swap = xprt_rdma_enable_swap,
> + .disable_swap = xprt_rdma_disable_swap,
> };
>
> static struct xprt_class xprt_rdma = {
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 16aa5dad41b2..b8aaf20aea96 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1985,14 +1985,14 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
> }
>
> /**
> - * xs_swapper_enable - Tag this transport as being used for swap.
> + * xs_enable_swap - Tag this transport as being used for swap.
> * @xprt: transport to tag
> *
> * Take a reference to this transport on behalf of the rpc_clnt, and
> * optionally mark it for swapping if it wasn't already.
> */
> -int
> -xs_swapper_enable(struct rpc_xprt *xprt)
> +static int
> +xs_enable_swap(struct rpc_xprt *xprt)
> {
> struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
>
> @@ -2007,14 +2007,14 @@ xs_swapper_enable(struct rpc_xprt *xprt)
> }
>
> /**
> - * xs_swapper_disable - Untag this transport as being used for swap.
> + * xs_disable_swap - Untag this transport as being used for swap.
> * @xprt: transport to tag
> *
> * Drop a "swapper" reference to this xprt on behalf of the rpc_clnt. If the
> * swapper refcount goes to 0, untag the socket as a memalloc socket.
> */
> -void
> -xs_swapper_disable(struct rpc_xprt *xprt)
> +static void
> +xs_disable_swap(struct rpc_xprt *xprt)
> {
> struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
>
> @@ -2030,6 +2030,17 @@ xs_swapper_disable(struct rpc_xprt *xprt)
> static void xs_set_memalloc(struct rpc_xprt *xprt)
> {
> }
> +
> +static int
> +xs_enable_swap(struct rpc_xprt *xprt)
> +{
> + return 0;
Ditto.
> +}
> +
> +static void
> +xs_disable_swap(struct rpc_xprt *xprt)
> +{
> +}
> #endif
>
> static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> @@ -2496,6 +2507,8 @@ static struct rpc_xprt_ops xs_local_ops = {
> .close = xs_close,
> .destroy = xs_destroy,
> .print_stats = xs_local_print_stats,
> + .enable_swap = xs_enable_swap,
> + .disable_swap = xs_disable_swap,
> };
>
> static struct rpc_xprt_ops xs_udp_ops = {
> @@ -2515,6 +2528,8 @@ static struct rpc_xprt_ops xs_udp_ops = {
> .close = xs_close,
> .destroy = xs_destroy,
> .print_stats = xs_udp_print_stats,
> + .enable_swap = xs_enable_swap,
> + .disable_swap = xs_disable_swap,
> };
>
> static struct rpc_xprt_ops xs_tcp_ops = {
> @@ -2531,6 +2546,8 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> .close = xs_tcp_shutdown,
> .destroy = xs_destroy,
> .print_stats = xs_tcp_print_stats,
> + .enable_swap = xs_enable_swap,
> + .disable_swap = xs_disable_swap,
> };
>
> /*
> @@ -2548,6 +2565,8 @@ static struct rpc_xprt_ops bc_tcp_ops = {
> .close = bc_close,
> .destroy = bc_destroy,
> .print_stats = xs_tcp_print_stats,
> + .enable_swap = xs_enable_swap,
> + .disable_swap = xs_disable_swap,
> };
>
> static int xs_init_anyaddr(const int family, struct sockaddr *sap)
> --
> 2.4.2
>
On Wed, 3 Jun 2015 10:48:10 -0400
Trond Myklebust <[email protected]> wrote:
> On Wed, Jun 3, 2015 at 10:43 AM, Jeff Layton <[email protected]> wrote:
> > RDMA xprts don't have a sock_xprt, but an rdma_xprt, so the
> > xs_swapper_enable/disable functions will likely oops when fed an RDMA
> > xprt. Turn these functions into rpc_xprt_ops so that that doesn't
> > occur. For now the RDMA versions are no-ops.
> >
> > Cc: Chuck Lever <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > include/linux/sunrpc/xprt.h | 16 ++++++++++++++--
> > net/sunrpc/clnt.c | 4 ++--
> > net/sunrpc/xprtrdma/transport.c | 15 ++++++++++++++-
> > net/sunrpc/xprtsock.c | 31 +++++++++++++++++++++++++------
> > 4 files changed, 55 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> > index 26b1624128ec..7eb58610eb94 100644
> > --- a/include/linux/sunrpc/xprt.h
> > +++ b/include/linux/sunrpc/xprt.h
> > @@ -133,6 +133,8 @@ struct rpc_xprt_ops {
> > void (*close)(struct rpc_xprt *xprt);
> > void (*destroy)(struct rpc_xprt *xprt);
> > void (*print_stats)(struct rpc_xprt *xprt, struct seq_file *seq);
> > + int (*enable_swap)(struct rpc_xprt *xprt);
> > + void (*disable_swap)(struct rpc_xprt *xprt);
> > };
> >
> > /*
> > @@ -327,6 +329,18 @@ static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *
> > return p + xprt->tsh_size;
> > }
> >
> > +static inline int
> > +xprt_enable_swap(struct rpc_xprt *xprt)
> > +{
> > + return xprt->ops->enable_swap(xprt);
> > +}
> > +
> > +static inline void
> > +xprt_disable_swap(struct rpc_xprt *xprt)
> > +{
> > + xprt->ops->disable_swap(xprt);
> > +}
> > +
> > /*
> > * Transport switch helper functions
> > */
> > @@ -345,8 +359,6 @@ void xprt_release_rqst_cong(struct rpc_task *task);
> > void xprt_disconnect_done(struct rpc_xprt *xprt);
> > void xprt_force_disconnect(struct rpc_xprt *xprt);
> > void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
> > -int xs_swapper_enable(struct rpc_xprt *xprt);
> > -void xs_swapper_disable(struct rpc_xprt *xprt);
> >
> > bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
> > void xprt_unlock_connect(struct rpc_xprt *, void *);
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index 804a75e71e84..60d1835edb26 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -2492,7 +2492,7 @@ retry:
> > goto retry;
> > }
> >
> > - ret = xs_swapper_enable(xprt);
> > + ret = xprt_enable_swap(xprt);
> > xprt_put(xprt);
> > }
> > return ret;
> > @@ -2519,7 +2519,7 @@ retry:
> > goto retry;
> > }
> >
> > - xs_swapper_disable(xprt);
> > + xprt_disable_swap(xprt);
> > xprt_put(xprt);
> > }
> > }
> > diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
> > index 54f23b1be986..e7a157754095 100644
> > --- a/net/sunrpc/xprtrdma/transport.c
> > +++ b/net/sunrpc/xprtrdma/transport.c
> > @@ -682,6 +682,17 @@ static void xprt_rdma_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
> > r_xprt->rx_stats.bad_reply_count);
> > }
> >
> > +static int
> > +xprt_rdma_enable_swap(struct rpc_xprt *xprt)
> > +{
> > + return 0;
>
> Shouldn't the function be returning an error here? What does swapon
> expect if the device you are trying to enable doesn't support swap?
>
Chuck suggested making these no-ops for RDMA for now. I'm fine with
returning an error, but is it really an error? Maybe RDMA doesn't need
any special setup for swapping?
> > +}
> > +
> > +static void
> > +xprt_rdma_disable_swap(struct rpc_xprt *xprt)
> > +{
> > +}
> > +
> > /*
> > * Plumbing for rpc transport switch and kernel module
> > */
> > @@ -700,7 +711,9 @@ static struct rpc_xprt_ops xprt_rdma_procs = {
> > .send_request = xprt_rdma_send_request,
> > .close = xprt_rdma_close,
> > .destroy = xprt_rdma_destroy,
> > - .print_stats = xprt_rdma_print_stats
> > + .print_stats = xprt_rdma_print_stats,
> > + .enable_swap = xprt_rdma_enable_swap,
> > + .disable_swap = xprt_rdma_disable_swap,
> > };
> >
> > static struct xprt_class xprt_rdma = {
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 16aa5dad41b2..b8aaf20aea96 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -1985,14 +1985,14 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
> > }
> >
> > /**
> > - * xs_swapper_enable - Tag this transport as being used for swap.
> > + * xs_enable_swap - Tag this transport as being used for swap.
> > * @xprt: transport to tag
> > *
> > * Take a reference to this transport on behalf of the rpc_clnt, and
> > * optionally mark it for swapping if it wasn't already.
> > */
> > -int
> > -xs_swapper_enable(struct rpc_xprt *xprt)
> > +static int
> > +xs_enable_swap(struct rpc_xprt *xprt)
> > {
> > struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
> >
> > @@ -2007,14 +2007,14 @@ xs_swapper_enable(struct rpc_xprt *xprt)
> > }
> >
> > /**
> > - * xs_swapper_disable - Untag this transport as being used for swap.
> > + * xs_disable_swap - Untag this transport as being used for swap.
> > * @xprt: transport to tag
> > *
> > * Drop a "swapper" reference to this xprt on behalf of the rpc_clnt. If the
> > * swapper refcount goes to 0, untag the socket as a memalloc socket.
> > */
> > -void
> > -xs_swapper_disable(struct rpc_xprt *xprt)
> > +static void
> > +xs_disable_swap(struct rpc_xprt *xprt)
> > {
> > struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
> >
> > @@ -2030,6 +2030,17 @@ xs_swapper_disable(struct rpc_xprt *xprt)
> > static void xs_set_memalloc(struct rpc_xprt *xprt)
> > {
> > }
> > +
> > +static int
> > +xs_enable_swap(struct rpc_xprt *xprt)
> > +{
> > + return 0;
>
> Ditto.
>
This just mirrors what the existing code already does. When swap over
NFS is Kconfig'ed off, it returns 0 here. AIUI, swapon will then fail
at the NFS layer though, so you'd never see this.
> > +}
> > +
> > +static void
> > +xs_disable_swap(struct rpc_xprt *xprt)
> > +{
> > +}
> > #endif
> >
> > static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> > @@ -2496,6 +2507,8 @@ static struct rpc_xprt_ops xs_local_ops = {
> > .close = xs_close,
> > .destroy = xs_destroy,
> > .print_stats = xs_local_print_stats,
> > + .enable_swap = xs_enable_swap,
> > + .disable_swap = xs_disable_swap,
> > };
> >
> > static struct rpc_xprt_ops xs_udp_ops = {
> > @@ -2515,6 +2528,8 @@ static struct rpc_xprt_ops xs_udp_ops = {
> > .close = xs_close,
> > .destroy = xs_destroy,
> > .print_stats = xs_udp_print_stats,
> > + .enable_swap = xs_enable_swap,
> > + .disable_swap = xs_disable_swap,
> > };
> >
> > static struct rpc_xprt_ops xs_tcp_ops = {
> > @@ -2531,6 +2546,8 @@ static struct rpc_xprt_ops xs_tcp_ops = {
> > .close = xs_tcp_shutdown,
> > .destroy = xs_destroy,
> > .print_stats = xs_tcp_print_stats,
> > + .enable_swap = xs_enable_swap,
> > + .disable_swap = xs_disable_swap,
> > };
> >
> > /*
> > @@ -2548,6 +2565,8 @@ static struct rpc_xprt_ops bc_tcp_ops = {
> > .close = bc_close,
> > .destroy = bc_destroy,
> > .print_stats = xs_tcp_print_stats,
> > + .enable_swap = xs_enable_swap,
> > + .disable_swap = xs_disable_swap,
> > };
> >
> > static int xs_init_anyaddr(const int family, struct sockaddr *sap)
> > --
> > 2.4.2
> >
--
Jeff Layton <[email protected]>
On Jun 3, 2015, at 11:01 AM, Jeff Layton <[email protected]> wrote:
> On Wed, 3 Jun 2015 10:48:10 -0400
> Trond Myklebust <[email protected]> wrote:
>
>> On Wed, Jun 3, 2015 at 10:43 AM, Jeff Layton <[email protected]> wrote:
>>> RDMA xprts don't have a sock_xprt, but an rdma_xprt, so the
>>> xs_swapper_enable/disable functions will likely oops when fed an RDMA
>>> xprt. Turn these functions into rpc_xprt_ops so that that doesn't
>>> occur. For now the RDMA versions are no-ops.
>>>
>>> Cc: Chuck Lever <[email protected]>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> include/linux/sunrpc/xprt.h | 16 ++++++++++++++--
>>> net/sunrpc/clnt.c | 4 ++--
>>> net/sunrpc/xprtrdma/transport.c | 15 ++++++++++++++-
>>> net/sunrpc/xprtsock.c | 31 +++++++++++++++++++++++++------
>>> 4 files changed, 55 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
>>> index 26b1624128ec..7eb58610eb94 100644
>>> --- a/include/linux/sunrpc/xprt.h
>>> +++ b/include/linux/sunrpc/xprt.h
>>> @@ -133,6 +133,8 @@ struct rpc_xprt_ops {
>>> void (*close)(struct rpc_xprt *xprt);
>>> void (*destroy)(struct rpc_xprt *xprt);
>>> void (*print_stats)(struct rpc_xprt *xprt, struct seq_file *seq);
>>> + int (*enable_swap)(struct rpc_xprt *xprt);
>>> + void (*disable_swap)(struct rpc_xprt *xprt);
>>> };
>>>
>>> /*
>>> @@ -327,6 +329,18 @@ static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *
>>> return p + xprt->tsh_size;
>>> }
>>>
>>> +static inline int
>>> +xprt_enable_swap(struct rpc_xprt *xprt)
>>> +{
>>> + return xprt->ops->enable_swap(xprt);
>>> +}
>>> +
>>> +static inline void
>>> +xprt_disable_swap(struct rpc_xprt *xprt)
>>> +{
>>> + xprt->ops->disable_swap(xprt);
>>> +}
>>> +
>>> /*
>>> * Transport switch helper functions
>>> */
>>> @@ -345,8 +359,6 @@ void xprt_release_rqst_cong(struct rpc_task *task);
>>> void xprt_disconnect_done(struct rpc_xprt *xprt);
>>> void xprt_force_disconnect(struct rpc_xprt *xprt);
>>> void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
>>> -int xs_swapper_enable(struct rpc_xprt *xprt);
>>> -void xs_swapper_disable(struct rpc_xprt *xprt);
>>>
>>> bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
>>> void xprt_unlock_connect(struct rpc_xprt *, void *);
>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> index 804a75e71e84..60d1835edb26 100644
>>> --- a/net/sunrpc/clnt.c
>>> +++ b/net/sunrpc/clnt.c
>>> @@ -2492,7 +2492,7 @@ retry:
>>> goto retry;
>>> }
>>>
>>> - ret = xs_swapper_enable(xprt);
>>> + ret = xprt_enable_swap(xprt);
>>> xprt_put(xprt);
>>> }
>>> return ret;
>>> @@ -2519,7 +2519,7 @@ retry:
>>> goto retry;
>>> }
>>>
>>> - xs_swapper_disable(xprt);
>>> + xprt_disable_swap(xprt);
>>> xprt_put(xprt);
>>> }
>>> }
>>> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
>>> index 54f23b1be986..e7a157754095 100644
>>> --- a/net/sunrpc/xprtrdma/transport.c
>>> +++ b/net/sunrpc/xprtrdma/transport.c
>>> @@ -682,6 +682,17 @@ static void xprt_rdma_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
>>> r_xprt->rx_stats.bad_reply_count);
>>> }
>>>
>>> +static int
>>> +xprt_rdma_enable_swap(struct rpc_xprt *xprt)
>>> +{
>>> + return 0;
>>
>> Shouldn't the function be returning an error here? What does swapon
>> expect if the device you are trying to enable doesn't support swap?
>>
>
>
> Chuck suggested making these no-ops for RDMA for now.
I did indeed. What I meant was that you needn?t worry too much right now
about how swap-on-NFS/RDMA is supposed to work, just make it not crash, and
someone (maybe me) will look at it later to ensure it is working correctly
and then we can claim it is supported. Sorry I was not clear.
> I'm fine with
> returning an error, but is it really an error? Maybe RDMA doesn't need
> any special setup for swapping?
This sounds a little snarky, but we don?t know for sure that nothing is
needed until it is tested and reviewed. I think it?s reasonable to assume
it doesn?t work 100% until we have positive confirmation that it does work.
Maybe add a comment to that effect in these new xprt methods? And I would
have it return something like ENOSYS.
Likewise, consider the same return code here:
+#if IS_ENABLED(CONFIG_SUNRPC_SWAP)
+int rpc_clnt_swap_activate(struct rpc_clnt *clnt);
+void rpc_clnt_swap_deactivate(struct rpc_clnt *clnt);
+#else
+static inline int
+rpc_clnt_swap_activate(struct rpc_clnt *clnt)
+{
+ return 0;
^^^^
+}
I?m not familiar enough with the swapon administrative interface to know if
?swapping on this device is not supported? is a reasonable and expected
failure mode for swapon. So maybe I?m just full of turtles.
>
>>> +}
>>> +
>>> +static void
>>> +xprt_rdma_disable_swap(struct rpc_xprt *xprt)
>>> +{
>>> +}
>>> +
>>> /*
>>> * Plumbing for rpc transport switch and kernel module
>>> */
>>> @@ -700,7 +711,9 @@ static struct rpc_xprt_ops xprt_rdma_procs = {
>>> .send_request = xprt_rdma_send_request,
>>> .close = xprt_rdma_close,
>>> .destroy = xprt_rdma_destroy,
>>> - .print_stats = xprt_rdma_print_stats
>>> + .print_stats = xprt_rdma_print_stats,
>>> + .enable_swap = xprt_rdma_enable_swap,
>>> + .disable_swap = xprt_rdma_disable_swap,
>>> };
>>>
>>> static struct xprt_class xprt_rdma = {
>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>> index 16aa5dad41b2..b8aaf20aea96 100644
>>> --- a/net/sunrpc/xprtsock.c
>>> +++ b/net/sunrpc/xprtsock.c
>>> @@ -1985,14 +1985,14 @@ static void xs_set_memalloc(struct rpc_xprt *xprt)
>>> }
>>>
>>> /**
>>> - * xs_swapper_enable - Tag this transport as being used for swap.
>>> + * xs_enable_swap - Tag this transport as being used for swap.
>>> * @xprt: transport to tag
>>> *
>>> * Take a reference to this transport on behalf of the rpc_clnt, and
>>> * optionally mark it for swapping if it wasn't already.
>>> */
>>> -int
>>> -xs_swapper_enable(struct rpc_xprt *xprt)
>>> +static int
>>> +xs_enable_swap(struct rpc_xprt *xprt)
>>> {
>>> struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
>>>
>>> @@ -2007,14 +2007,14 @@ xs_swapper_enable(struct rpc_xprt *xprt)
>>> }
>>>
>>> /**
>>> - * xs_swapper_disable - Untag this transport as being used for swap.
>>> + * xs_disable_swap - Untag this transport as being used for swap.
>>> * @xprt: transport to tag
>>> *
>>> * Drop a "swapper" reference to this xprt on behalf of the rpc_clnt. If the
>>> * swapper refcount goes to 0, untag the socket as a memalloc socket.
>>> */
>>> -void
>>> -xs_swapper_disable(struct rpc_xprt *xprt)
>>> +static void
>>> +xs_disable_swap(struct rpc_xprt *xprt)
>>> {
>>> struct sock_xprt *xs = container_of(xprt, struct sock_xprt, xprt);
>>>
>>> @@ -2030,6 +2030,17 @@ xs_swapper_disable(struct rpc_xprt *xprt)
>>> static void xs_set_memalloc(struct rpc_xprt *xprt)
>>> {
>>> }
>>> +
>>> +static int
>>> +xs_enable_swap(struct rpc_xprt *xprt)
>>> +{
>>> + return 0;
>>
>> Ditto.
>>
>
> This just mirrors what the existing code already does. When swap over
> NFS is Kconfig'ed off, it returns 0 here. AIUI, swapon will then fail
> at the NFS layer though, so you'd never see this.
>
>>> +}
>>> +
>>> +static void
>>> +xs_disable_swap(struct rpc_xprt *xprt)
>>> +{
>>> +}
>>> #endif
>>>
>>> static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
>>> @@ -2496,6 +2507,8 @@ static struct rpc_xprt_ops xs_local_ops = {
>>> .close = xs_close,
>>> .destroy = xs_destroy,
>>> .print_stats = xs_local_print_stats,
>>> + .enable_swap = xs_enable_swap,
>>> + .disable_swap = xs_disable_swap,
>>> };
>>>
>>> static struct rpc_xprt_ops xs_udp_ops = {
>>> @@ -2515,6 +2528,8 @@ static struct rpc_xprt_ops xs_udp_ops = {
>>> .close = xs_close,
>>> .destroy = xs_destroy,
>>> .print_stats = xs_udp_print_stats,
>>> + .enable_swap = xs_enable_swap,
>>> + .disable_swap = xs_disable_swap,
>>> };
>>>
>>> static struct rpc_xprt_ops xs_tcp_ops = {
>>> @@ -2531,6 +2546,8 @@ static struct rpc_xprt_ops xs_tcp_ops = {
>>> .close = xs_tcp_shutdown,
>>> .destroy = xs_destroy,
>>> .print_stats = xs_tcp_print_stats,
>>> + .enable_swap = xs_enable_swap,
>>> + .disable_swap = xs_disable_swap,
>>> };
>>>
>>> /*
>>> @@ -2548,6 +2565,8 @@ static struct rpc_xprt_ops bc_tcp_ops = {
>>> .close = bc_close,
>>> .destroy = bc_destroy,
>>> .print_stats = xs_tcp_print_stats,
>>> + .enable_swap = xs_enable_swap,
>>> + .disable_swap = xs_disable_swap,
>>> };
>>>
>>> static int xs_init_anyaddr(const int family, struct sockaddr *sap)
>>> --
>>> 2.4.2
>>>
>
>
> --
> Jeff Layton <[email protected]>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Wed, 3 Jun 2015 13:07:34 -0400
Chuck Lever <[email protected]> wrote:
>
> On Jun 3, 2015, at 11:01 AM, Jeff Layton <[email protected]> wrote:
>
> > On Wed, 3 Jun 2015 10:48:10 -0400
> > Trond Myklebust <[email protected]> wrote:
> >
> >> On Wed, Jun 3, 2015 at 10:43 AM, Jeff Layton <[email protected]> wrote:
> >>> RDMA xprts don't have a sock_xprt, but an rdma_xprt, so the
> >>> xs_swapper_enable/disable functions will likely oops when fed an RDMA
> >>> xprt. Turn these functions into rpc_xprt_ops so that that doesn't
> >>> occur. For now the RDMA versions are no-ops.
> >>>
> >>> Cc: Chuck Lever <[email protected]>
> >>> Signed-off-by: Jeff Layton <[email protected]>
> >>> ---
> >>> include/linux/sunrpc/xprt.h | 16 ++++++++++++++--
> >>> net/sunrpc/clnt.c | 4 ++--
> >>> net/sunrpc/xprtrdma/transport.c | 15 ++++++++++++++-
> >>> net/sunrpc/xprtsock.c | 31 +++++++++++++++++++++++++------
> >>> 4 files changed, 55 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
> >>> index 26b1624128ec..7eb58610eb94 100644
> >>> --- a/include/linux/sunrpc/xprt.h
> >>> +++ b/include/linux/sunrpc/xprt.h
> >>> @@ -133,6 +133,8 @@ struct rpc_xprt_ops {
> >>> void (*close)(struct rpc_xprt *xprt);
> >>> void (*destroy)(struct rpc_xprt *xprt);
> >>> void (*print_stats)(struct rpc_xprt *xprt, struct seq_file *seq);
> >>> + int (*enable_swap)(struct rpc_xprt *xprt);
> >>> + void (*disable_swap)(struct rpc_xprt *xprt);
> >>> };
> >>>
> >>> /*
> >>> @@ -327,6 +329,18 @@ static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *
> >>> return p + xprt->tsh_size;
> >>> }
> >>>
> >>> +static inline int
> >>> +xprt_enable_swap(struct rpc_xprt *xprt)
> >>> +{
> >>> + return xprt->ops->enable_swap(xprt);
> >>> +}
> >>> +
> >>> +static inline void
> >>> +xprt_disable_swap(struct rpc_xprt *xprt)
> >>> +{
> >>> + xprt->ops->disable_swap(xprt);
> >>> +}
> >>> +
> >>> /*
> >>> * Transport switch helper functions
> >>> */
> >>> @@ -345,8 +359,6 @@ void xprt_release_rqst_cong(struct rpc_task *task);
> >>> void xprt_disconnect_done(struct rpc_xprt *xprt);
> >>> void xprt_force_disconnect(struct rpc_xprt *xprt);
> >>> void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie);
> >>> -int xs_swapper_enable(struct rpc_xprt *xprt);
> >>> -void xs_swapper_disable(struct rpc_xprt *xprt);
> >>>
> >>> bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *);
> >>> void xprt_unlock_connect(struct rpc_xprt *, void *);
> >>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >>> index 804a75e71e84..60d1835edb26 100644
> >>> --- a/net/sunrpc/clnt.c
> >>> +++ b/net/sunrpc/clnt.c
> >>> @@ -2492,7 +2492,7 @@ retry:
> >>> goto retry;
> >>> }
> >>>
> >>> - ret = xs_swapper_enable(xprt);
> >>> + ret = xprt_enable_swap(xprt);
> >>> xprt_put(xprt);
> >>> }
> >>> return ret;
> >>> @@ -2519,7 +2519,7 @@ retry:
> >>> goto retry;
> >>> }
> >>>
> >>> - xs_swapper_disable(xprt);
> >>> + xprt_disable_swap(xprt);
> >>> xprt_put(xprt);
> >>> }
> >>> }
> >>> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
> >>> index 54f23b1be986..e7a157754095 100644
> >>> --- a/net/sunrpc/xprtrdma/transport.c
> >>> +++ b/net/sunrpc/xprtrdma/transport.c
> >>> @@ -682,6 +682,17 @@ static void xprt_rdma_print_stats(struct rpc_xprt *xprt, struct seq_file *seq)
> >>> r_xprt->rx_stats.bad_reply_count);
> >>> }
> >>>
> >>> +static int
> >>> +xprt_rdma_enable_swap(struct rpc_xprt *xprt)
> >>> +{
> >>> + return 0;
> >>
> >> Shouldn't the function be returning an error here? What does swapon
> >> expect if the device you are trying to enable doesn't support swap?
> >>
> >
> >
> > Chuck suggested making these no-ops for RDMA for now.
>
> I did indeed. What I meant was that you needn’t worry too much right now
> about how swap-on-NFS/RDMA is supposed to work, just make it not crash, and
> someone (maybe me) will look at it later to ensure it is working correctly
> and then we can claim it is supported. Sorry I was not clear.
>
> > I'm fine with
> > returning an error, but is it really an error? Maybe RDMA doesn't need
> > any special setup for swapping?
>
> This sounds a little snarky, but we don’t know for sure that nothing is
> needed until it is tested and reviewed. I think it’s reasonable to assume
> it doesn’t work 100% until we have positive confirmation that it does work.
>
> Maybe add a comment to that effect in these new xprt methods? And I would
> have it return something like ENOSYS.
>
> Likewise, consider the same return code here:
>
> +#if IS_ENABLED(CONFIG_SUNRPC_SWAP)
> +int rpc_clnt_swap_activate(struct rpc_clnt *clnt);
> +void rpc_clnt_swap_deactivate(struct rpc_clnt *clnt);
> +#else
> +static inline int
> +rpc_clnt_swap_activate(struct rpc_clnt *clnt)
> +{
> + return 0;
> ^^^^
> +}
>
> I’m not familiar enough with the swapon administrative interface to know if
> “swapping on this device is not supported” is a reasonable and expected
> failure mode for swapon. So maybe I’m just full of turtles.
>
No worries. I'm fine with returning an error if this stuff is disabled.
The manpage seems to indicate that EINVAL is the right error code to
use, but I'll see if I can verify that.
I'll need to look over the code a little more...
--
Jeff Layton <[email protected]>