[PATCH 0/4] sunrpc: clean up "swapper" xprt handling

[PATCH 0/4] sunrpc: clean up "swapper" xprt handling

[Jeff Layton]

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 (4):
  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

 fs/nfs/file.c                | 11 ++------
 include/linux/sunrpc/clnt.h  |  1 +
 include/linux/sunrpc/sched.h | 16 +++++++++++
 include/linux/sunrpc/xprt.h  |  5 ++--
 net/sunrpc/clnt.c            | 67 ++++++++++++++++++++++++++++++++++++++------
 net/sunrpc/xprtsock.c        | 59 +++++++++++++++++++++++++++++---------
 6 files changed, 125 insertions(+), 34 deletions(-)

-- 
2.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/4] sunrpc: keep a count of swapfiles associated with the rpc_clnt

[Jeff Layton]

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.

Cc: Mel Gorman <mgorman@suse.de>
Reported-by: Jerome Marchand <jmarchan@redhat.com>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] sunrpc: keep a count of swapfiles associated with the rpc_clnt

[Mel Gorman]

On Sat, May 30, 2015 at 08:03:10AM -0400, Jeff Layton wrote:
> 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.
> 
> Cc: Mel Gorman <mgorman@suse.de>
> Reported-by: Jerome Marchand <jmarchan@redhat.com>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/4] sunrpc: make xprt->swapper an atomic_t

[Jeff Layton]

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 <mgorman@suse.de>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] sunrpc: make xprt->swapper an atomic_t

[Chuck Lever]

Hi Jeff-

On May 30, 2015, at 8:03 AM, Jeff Layton <jlayton@poochiereds.net> wrote:

> 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 <mgorman@suse.de>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> ---
> 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);
> 	}
> }

Seems like xs_swapper() is specific to socket-based transports.

There’s no struct sock * to use as an argument with RDMA, so xs_swapper()
would probably dereference garbage if “swapon” was invoked on a
proto=rdma mount point.

Should these new functions be made members of the rpc_xprt_ops? The
RDMA version of the methods can be made no-ops for now.


> 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.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] sunrpc: make xprt->swapper an atomic_t

[Jeff Layton]

On Sat, 30 May 2015 13:55:44 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Hi Jeff-
> 
> On May 30, 2015, at 8:03 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> > 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 <mgorman@suse.de>
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> > 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);
> > 	}
> > }
> 
> Seems like xs_swapper() is specific to socket-based transports.
> 
> There’s no struct sock * to use as an argument with RDMA, so xs_swapper()
> would probably dereference garbage if “swapon” was invoked on a
> proto=rdma mount point.
> 
> Should these new functions be made members of the rpc_xprt_ops? The
> RDMA version of the methods can be made no-ops for now.
> 

Oh, right -- rdma uses rpcrdma_xprt instead. Yeah, adding a new ops to
handle this sounds like the right thing to do. I'll roll that into the
v2 set.

Thanks!
Jeff

> 
> > 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.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-nfs" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 


-- 
Jeff Layton <jlayton@poochiereds.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Jeff Layton]

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.

Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Mel Gorman]

On Sat, May 30, 2015 at 08:03:12AM -0400, Jeff Layton wrote:
> 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.
> 
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Mel Gorman]

On Sat, May 30, 2015 at 08:03:12AM -0400, Jeff Layton wrote:
> 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.
> 
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Jeff Layton]

On Tue, 2 Jun 2015 13:40:26 +0100
Mel Gorman <mgorman@suse.de> wrote:

> On Sat, May 30, 2015 at 08:03:12AM -0400, Jeff Layton wrote:
> > 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.
> > 
> > Cc: Mel Gorman <mgorman@suse.de>
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> 
> Acked-by: Mel Gorman <mgorman@suse.de>
> 

Thanks Mel,

I should also mention that I see this warning pop when working with
swapfiles on NFS. This trace is with this patchset, but I see a similar
one without it:

[   74.232485] ------------[ cut here ]------------
[   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 sk_clear_memalloc+0x51/0x80()
[   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk virtio_pci ata_generic virtio_ring pata_acpi virtio
[   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
[   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   74.245546]  0000000000000000 0000000079e69e31 ffff8800d066bde8 ffffffff8179263d
[   74.246786]  0000000000000000 0000000000000000 ffff8800d066be28 ffffffff8109e6fa
[   74.248175]  0000000000000000 ffff880118d48000 ffff8800d58f5c08 ffff880036e380a8
[   74.249483] Call Trace:
[   74.249872]  [<ffffffff8179263d>] dump_stack+0x45/0x57
[   74.250703]  [<ffffffff8109e6fa>] warn_slowpath_common+0x8a/0xc0
[   74.251655]  [<ffffffff8109e82a>] warn_slowpath_null+0x1a/0x20
[   74.252585]  [<ffffffff81661241>] sk_clear_memalloc+0x51/0x80
[   74.253519]  [<ffffffffa0116c72>] xs_disable_swap+0x42/0x80 [sunrpc]
[   74.254537]  [<ffffffffa01109de>] rpc_clnt_swap_deactivate+0x7e/0xc0 [sunrpc]
[   74.255610]  [<ffffffffa03e4fd7>] nfs_swap_deactivate+0x27/0x30 [nfs]
[   74.256582]  [<ffffffff811e99d4>] destroy_swap_extents+0x74/0x80
[   74.257496]  [<ffffffff811ecb52>] SyS_swapoff+0x222/0x5c0
[   74.258318]  [<ffffffff81023f27>] ? syscall_trace_leave+0xc7/0x140
[   74.259253]  [<ffffffff81798dae>] system_call_fastpath+0x12/0x71
[   74.260158] ---[ end trace 2530722966429f10 ]---

...that comes from this in sk_clear_memalloc:

        /*
         * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
         * progress of swapping. However, if SOCK_MEMALLOC is cleared while
         * it has rmem allocations there is a risk that the user of the
         * socket cannot make forward progress due to exceeding the rmem
         * limits. By rights, sk_clear_memalloc() should only be called
         * on sockets being torn down but warn and reset the accounting if
         * that assumption breaks.
         */
        if (WARN_ON(sk->sk_forward_alloc))
                sk_mem_reclaim(sk);

Is it wrong to call sk_clear_memalloc on swapoff? Should we try to keep
it set up as a memalloc socket on the last swapoff and just wait until
the socket is being freed to clear it? If so, then maybe the right
thing to do is to call sk_clear_memalloc in __sk_free or somewhere
similar if it's set up for memalloc?

-- 
Jeff Layton <jlayton@poochiereds.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Mel Gorman]

On Wed, Jun 03, 2015 at 10:32:00AM -0400, Jeff Layton wrote:
> On Tue, 2 Jun 2015 13:40:26 +0100
> Mel Gorman <mgorman@suse.de> wrote:
> 
> > On Sat, May 30, 2015 at 08:03:12AM -0400, Jeff Layton wrote:
> > > 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.
> > > 
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > 
> > Acked-by: Mel Gorman <mgorman@suse.de>
> > 
> 
> Thanks Mel,
> 
> I should also mention that I see this warning pop when working with
> swapfiles on NFS. This trace is with this patchset, but I see a similar
> one without it:
> 
> [   74.232485] ------------[ cut here ]------------
> [   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 sk_clear_memalloc+0x51/0x80()
> [   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk virtio_pci ata_generic virtio_ring pata_acpi virtio
> [   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
> [   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [   74.245546]  0000000000000000 0000000079e69e31 ffff8800d066bde8 ffffffff8179263d
> [   74.246786]  0000000000000000 0000000000000000 ffff8800d066be28 ffffffff8109e6fa
> [   74.248175]  0000000000000000 ffff880118d48000 ffff8800d58f5c08 ffff880036e380a8
> [   74.249483] Call Trace:
> [   74.249872]  [<ffffffff8179263d>] dump_stack+0x45/0x57
> [   74.250703]  [<ffffffff8109e6fa>] warn_slowpath_common+0x8a/0xc0
> [   74.251655]  [<ffffffff8109e82a>] warn_slowpath_null+0x1a/0x20
> [   74.252585]  [<ffffffff81661241>] sk_clear_memalloc+0x51/0x80
> [   74.253519]  [<ffffffffa0116c72>] xs_disable_swap+0x42/0x80 [sunrpc]
> [   74.254537]  [<ffffffffa01109de>] rpc_clnt_swap_deactivate+0x7e/0xc0 [sunrpc]
> [   74.255610]  [<ffffffffa03e4fd7>] nfs_swap_deactivate+0x27/0x30 [nfs]
> [   74.256582]  [<ffffffff811e99d4>] destroy_swap_extents+0x74/0x80
> [   74.257496]  [<ffffffff811ecb52>] SyS_swapoff+0x222/0x5c0
> [   74.258318]  [<ffffffff81023f27>] ? syscall_trace_leave+0xc7/0x140
> [   74.259253]  [<ffffffff81798dae>] system_call_fastpath+0x12/0x71
> [   74.260158] ---[ end trace 2530722966429f10 ]---
> 
> ...that comes from this in sk_clear_memalloc:
> 
>         /*
>          * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
>          * progress of swapping. However, if SOCK_MEMALLOC is cleared while
>          * it has rmem allocations there is a risk that the user of the
>          * socket cannot make forward progress due to exceeding the rmem
>          * limits. By rights, sk_clear_memalloc() should only be called
>          * on sockets being torn down but warn and reset the accounting if
>          * that assumption breaks.
>          */
>         if (WARN_ON(sk->sk_forward_alloc))
>                 sk_mem_reclaim(sk);
> 
> Is it wrong to call sk_clear_memalloc on swapoff? Should we try to keep
> it set up as a memalloc socket on the last swapoff and just wait until
> the socket is being freed to clear it? If so, then maybe the right
> thing to do is to call sk_clear_memalloc in __sk_free or somewhere
> similar if it's set up for memalloc?
>

I think it is perfectly reasonable to remove the warning after your
series. When I had it in mind, I was primarily thinking of the shutdown
case and a single swap file. With your series applied, the disabling of
swap is called at the correct time. So, something like this to tack on
to the end of your series?

---8<---
net, swap: Remove a warning and clarify why sk_mem_reclaim is required when deactivating swap

Jeff Layton reported the following;

 [   74.232485] ------------[ cut here ]------------
 [   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 sk_clear_memalloc+0x51/0x80()
 [   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk virtio_pci ata_generic virtio_ring pata_acpi virtio
 [   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
 [   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 [   74.245546]  0000000000000000 0000000079e69e31 ffff8800d066bde8 ffffffff8179263d
 [   74.246786]  0000000000000000 0000000000000000 ffff8800d066be28 ffffffff8109e6fa
 [   74.248175]  0000000000000000 ffff880118d48000 ffff8800d58f5c08 ffff880036e380a8
 [   74.249483] Call Trace:
 [   74.249872]  [<ffffffff8179263d>] dump_stack+0x45/0x57
 [   74.250703]  [<ffffffff8109e6fa>] warn_slowpath_common+0x8a/0xc0
 [   74.251655]  [<ffffffff8109e82a>] warn_slowpath_null+0x1a/0x20
 [   74.252585]  [<ffffffff81661241>] sk_clear_memalloc+0x51/0x80
 [   74.253519]  [<ffffffffa0116c72>] xs_disable_swap+0x42/0x80 [sunrpc]
 [   74.254537]  [<ffffffffa01109de>] rpc_clnt_swap_deactivate+0x7e/0xc0 [sunrpc]
 [   74.255610]  [<ffffffffa03e4fd7>] nfs_swap_deactivate+0x27/0x30 [nfs]
 [   74.256582]  [<ffffffff811e99d4>] destroy_swap_extents+0x74/0x80
 [   74.257496]  [<ffffffff811ecb52>] SyS_swapoff+0x222/0x5c0
 [   74.258318]  [<ffffffff81023f27>] ? syscall_trace_leave+0xc7/0x140
 [   74.259253]  [<ffffffff81798dae>] system_call_fastpath+0x12/0x71
 [   74.260158] ---[ end trace 2530722966429f10 ]---

The warning in question was unnecessary but with Jeff's series the rules
are also clearer.  This patch removes the warning and updates the comment
to explain why sk_mem_reclaim() may still be called.

Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 net/core/sock.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 71e3e5f1eaa0..1ebf706b5847 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -354,14 +354,12 @@ void sk_clear_memalloc(struct sock *sk)
 
 	/*
 	 * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
-	 * progress of swapping. However, if SOCK_MEMALLOC is cleared while
-	 * it has rmem allocations there is a risk that the user of the
-	 * socket cannot make forward progress due to exceeding the rmem
-	 * limits. By rights, sk_clear_memalloc() should only be called
-	 * on sockets being torn down but warn and reset the accounting if
-	 * that assumption breaks.
+	 * progress of swapping. SOCK_MEMALLOC may be cleared while
+	 * it has rmem allocations due to the last swapfile being deactivated
+	 * but there is a risk that the socket is unusable due to exceeding
+	 * the rmem limits. Reclaim the reserves and obey rmem limits again.
 	 */
-	if (WARN_ON(sk->sk_forward_alloc))
+	if (sk->sk_forward_alloc)
 		sk_mem_reclaim(sk);
 }
 EXPORT_SYMBOL_GPL(sk_clear_memalloc);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] sunrpc: if we're closing down a socket, clear memalloc on it first

[Jeff Layton]

On Thu, 4 Jun 2015 14:08:30 +0100
Mel Gorman <mgorman@suse.de> wrote:

> On Wed, Jun 03, 2015 at 10:32:00AM -0400, Jeff Layton wrote:
> > On Tue, 2 Jun 2015 13:40:26 +0100
> > Mel Gorman <mgorman@suse.de> wrote:
> > 
> > > On Sat, May 30, 2015 at 08:03:12AM -0400, Jeff Layton wrote:
> > > > 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.
> > > > 
> > > > Cc: Mel Gorman <mgorman@suse.de>
> > > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > > 
> > > Acked-by: Mel Gorman <mgorman@suse.de>
> > > 
> > 
> > Thanks Mel,
> > 
> > I should also mention that I see this warning pop when working with
> > swapfiles on NFS. This trace is with this patchset, but I see a similar
> > one without it:
> > 
> > [   74.232485] ------------[ cut here ]------------
> > [   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 sk_clear_memalloc+0x51/0x80()
> > [   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk virtio_pci ata_generic virtio_ring pata_acpi virtio
> > [   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
> > [   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> > [   74.245546]  0000000000000000 0000000079e69e31 ffff8800d066bde8 ffffffff8179263d
> > [   74.246786]  0000000000000000 0000000000000000 ffff8800d066be28 ffffffff8109e6fa
> > [   74.248175]  0000000000000000 ffff880118d48000 ffff8800d58f5c08 ffff880036e380a8
> > [   74.249483] Call Trace:
> > [   74.249872]  [<ffffffff8179263d>] dump_stack+0x45/0x57
> > [   74.250703]  [<ffffffff8109e6fa>] warn_slowpath_common+0x8a/0xc0
> > [   74.251655]  [<ffffffff8109e82a>] warn_slowpath_null+0x1a/0x20
> > [   74.252585]  [<ffffffff81661241>] sk_clear_memalloc+0x51/0x80
> > [   74.253519]  [<ffffffffa0116c72>] xs_disable_swap+0x42/0x80 [sunrpc]
> > [   74.254537]  [<ffffffffa01109de>] rpc_clnt_swap_deactivate+0x7e/0xc0 [sunrpc]
> > [   74.255610]  [<ffffffffa03e4fd7>] nfs_swap_deactivate+0x27/0x30 [nfs]
> > [   74.256582]  [<ffffffff811e99d4>] destroy_swap_extents+0x74/0x80
> > [   74.257496]  [<ffffffff811ecb52>] SyS_swapoff+0x222/0x5c0
> > [   74.258318]  [<ffffffff81023f27>] ? syscall_trace_leave+0xc7/0x140
> > [   74.259253]  [<ffffffff81798dae>] system_call_fastpath+0x12/0x71
> > [   74.260158] ---[ end trace 2530722966429f10 ]---
> > 
> > ...that comes from this in sk_clear_memalloc:
> > 
> >         /*
> >          * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
> >          * progress of swapping. However, if SOCK_MEMALLOC is cleared while
> >          * it has rmem allocations there is a risk that the user of the
> >          * socket cannot make forward progress due to exceeding the rmem
> >          * limits. By rights, sk_clear_memalloc() should only be called
> >          * on sockets being torn down but warn and reset the accounting if
> >          * that assumption breaks.
> >          */
> >         if (WARN_ON(sk->sk_forward_alloc))
> >                 sk_mem_reclaim(sk);
> > 
> > Is it wrong to call sk_clear_memalloc on swapoff? Should we try to keep
> > it set up as a memalloc socket on the last swapoff and just wait until
> > the socket is being freed to clear it? If so, then maybe the right
> > thing to do is to call sk_clear_memalloc in __sk_free or somewhere
> > similar if it's set up for memalloc?
> >
> 
> I think it is perfectly reasonable to remove the warning after your
> series. When I had it in mind, I was primarily thinking of the shutdown
> case and a single swap file. With your series applied, the disabling of
> swap is called at the correct time. So, something like this to tack on
> to the end of your series?
> 
> ---8<---
> net, swap: Remove a warning and clarify why sk_mem_reclaim is required when deactivating swap
> 
> Jeff Layton reported the following;
> 
>  [   74.232485] ------------[ cut here ]------------
>  [   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 sk_clear_memalloc+0x51/0x80()
>  [   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk virtio_pci ata_generic virtio_ring pata_acpi virtio
>  [   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
>  [   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
>  [   74.245546]  0000000000000000 0000000079e69e31 ffff8800d066bde8 ffffffff8179263d
>  [   74.246786]  0000000000000000 0000000000000000 ffff8800d066be28 ffffffff8109e6fa
>  [   74.248175]  0000000000000000 ffff880118d48000 ffff8800d58f5c08 ffff880036e380a8
>  [   74.249483] Call Trace:
>  [   74.249872]  [<ffffffff8179263d>] dump_stack+0x45/0x57
>  [   74.250703]  [<ffffffff8109e6fa>] warn_slowpath_common+0x8a/0xc0
>  [   74.251655]  [<ffffffff8109e82a>] warn_slowpath_null+0x1a/0x20
>  [   74.252585]  [<ffffffff81661241>] sk_clear_memalloc+0x51/0x80
>  [   74.253519]  [<ffffffffa0116c72>] xs_disable_swap+0x42/0x80 [sunrpc]
>  [   74.254537]  [<ffffffffa01109de>] rpc_clnt_swap_deactivate+0x7e/0xc0 [sunrpc]
>  [   74.255610]  [<ffffffffa03e4fd7>] nfs_swap_deactivate+0x27/0x30 [nfs]
>  [   74.256582]  [<ffffffff811e99d4>] destroy_swap_extents+0x74/0x80
>  [   74.257496]  [<ffffffff811ecb52>] SyS_swapoff+0x222/0x5c0
>  [   74.258318]  [<ffffffff81023f27>] ? syscall_trace_leave+0xc7/0x140
>  [   74.259253]  [<ffffffff81798dae>] system_call_fastpath+0x12/0x71
>  [   74.260158] ---[ end trace 2530722966429f10 ]---
> 
> The warning in question was unnecessary but with Jeff's series the rules
> are also clearer.  This patch removes the warning and updates the comment
> to explain why sk_mem_reclaim() may still be called.
> 
> Signed-off-by: Mel Gorman <mgorman@suse.de>
> ---
>  net/core/sock.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 71e3e5f1eaa0..1ebf706b5847 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -354,14 +354,12 @@ void sk_clear_memalloc(struct sock *sk)
>  
>  	/*
>  	 * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
> -	 * progress of swapping. However, if SOCK_MEMALLOC is cleared while
> -	 * it has rmem allocations there is a risk that the user of the
> -	 * socket cannot make forward progress due to exceeding the rmem
> -	 * limits. By rights, sk_clear_memalloc() should only be called
> -	 * on sockets being torn down but warn and reset the accounting if
> -	 * that assumption breaks.
> +	 * progress of swapping. SOCK_MEMALLOC may be cleared while
> +	 * it has rmem allocations due to the last swapfile being deactivated
> +	 * but there is a risk that the socket is unusable due to exceeding
> +	 * the rmem limits. Reclaim the reserves and obey rmem limits again.
>  	 */
> -	if (WARN_ON(sk->sk_forward_alloc))
> +	if (sk->sk_forward_alloc)
>  		sk_mem_reclaim(sk);
>  }
>  EXPORT_SYMBOL_GPL(sk_clear_memalloc);

Sure, sounds reasonable. If I need to do a respin of the series, I'll
roll this into it. Otherwise you or I can just send it as a separate
patch afterward.

Thanks,
-- 
Jeff Layton <jlayton@poochiereds.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/4] sunrpc: lock xprt before trying to set memalloc on the sockets

[Jeff Layton]

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 <mgorman@suse.de>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 net/sunrpc/xprtsock.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 359446442112..91770ccab608 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);
 }
@@ -1986,8 +1997,11 @@ xs_swapper_enable(struct rpc_xprt *xprt)
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
 			xprt);
 
+	if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE))
+		return -ERESTARTSYS;
 	if (atomic_inc_return(&xprt->swapper) == 1)
 		sk_set_memalloc(transport->inet);
+	xprt_release_xprt(xprt, NULL);
 	return 0;
 }
 
@@ -2004,8 +2018,11 @@ xs_swapper_disable(struct rpc_xprt *xprt)
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
 			xprt);
 
+	if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_UNINTERRUPTIBLE))
+		return;
 	if (atomic_dec_and_test(&xprt->swapper))
 		sk_clear_memalloc(transport->inet);
+	xprt_release_xprt(xprt, NULL);
 }
 #else
 static void xs_set_memalloc(struct rpc_xprt *xprt)
-- 
2.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] sunrpc: lock xprt before trying to set memalloc on the sockets

[Jeff Layton]

On Sat, 30 May 2015 08:03:13 -0400
Jeff Layton <jlayton@poochiereds.net> wrote:

> 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 <mgorman@suse.de>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> ---
>  net/sunrpc/xprtsock.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 359446442112..91770ccab608 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);
>  }
> @@ -1986,8 +1997,11 @@ xs_swapper_enable(struct rpc_xprt *xprt)
>  	struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
>  			xprt);
>  
> +	if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE))
> +		return -ERESTARTSYS;
>  	if (atomic_inc_return(&xprt->swapper) == 1)
>  		sk_set_memalloc(transport->inet);

Oh, we need to check that transport->inet is not NULL before we call
sk_set/clear_memalloc on it. I'll respin with that fix once I give
everyone a chance to comment on the rest...

> +	xprt_release_xprt(xprt, NULL);
>  	return 0;
>  }
>  
> @@ -2004,8 +2018,11 @@ xs_swapper_disable(struct rpc_xprt *xprt)
>  	struct sock_xprt *transport = container_of(xprt, struct sock_xprt,
>  			xprt);
>  
> +	if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_UNINTERRUPTIBLE))
> +		return;
>  	if (atomic_dec_and_test(&xprt->swapper))
>  		sk_clear_memalloc(transport->inet);
> +	xprt_release_xprt(xprt, NULL);
>  }
>  #else
>  static void xs_set_memalloc(struct rpc_xprt *xprt)


-- 
Jeff Layton <jlayton@poochiereds.net>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>