remove deadlock risk with loop-back mounted NFS filesystems

If requests are being sent to the local host, then NFS will need to take care to avoid deadlocks.

So keep track when accepting a connection or sending a UDP request and set a flag in the svc_xprt when the peer connected to is local.

The interface rpc_is_foreign() is provided to check is a given client is connected to a foreign server. When it returns zero it is either not connected or connected to a local server and in either case greater care is needed.

Signed-off-by NeilBrown <[email protected]>

---
 include/linux/sunrpc/clnt.h |    1 +
 include/linux/sunrpc/xprt.h |    1 +
 net/sunrpc/clnt.c           |   25 +++++++++++++++++++++++++
 net/sunrpc/xprtsock.c       |    9 +++++++++
 4 files changed, 36 insertions(+)

include/linux/sunrpc/clnt.h +1 -0

index 70736b98c721..cd79b2a28ceb 100644
@@ -175,6 +175,7 @@ void		rpc_force_rebind(struct rpc_clnt *);
 size_t		rpc_peeraddr(struct rpc_clnt *, struct sockaddr *, size_t);
 const char	*rpc_peeraddr2str(struct rpc_clnt *, enum rpc_display_format_t);
 int		rpc_localaddr(struct rpc_clnt *, struct sockaddr *, size_t);
+int		rpc_is_foreign(struct rpc_clnt *);
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_SUNRPC_CLNT_H */

include/linux/sunrpc/xprt.h +0 -0

index fcbfe8783243..6a9dffcb9d3f 100644

Quoted 29 lines · NeilBrown

If requests are being sent to the local host, then NFS will need to take care to avoid deadlocks.

So keep track when accepting a connection or sending a UDP request and set a flag in the svc_xprt when the peer connected to is local.

The interface rpc_is_foreign() is provided to check is a given client is connected to a foreign server. When it returns zero it is either not connected or connected to a local server and in either case greater care is needed.

Signed-off-by NeilBrown <[email protected]>

---
 include/linux/sunrpc/clnt.h |    1 +
 include/linux/sunrpc/xprt.h |    1 +
 net/sunrpc/clnt.c           |   25 +++++++++++++++++++++++++
 net/sunrpc/xprtsock.c       |    9 +++++++++
 4 files changed, 36 insertions(+)

include/linux/sunrpc/clnt.h +1 -0

index 70736b98c721..cd79b2a28ceb 100644
@@ -175,6 +175,7 @@ void		rpc_force_rebind(struct rpc_clnt *);
 size_t		rpc_peeraddr(struct rpc_clnt *, struct sockaddr *, size_t);
 const char	*rpc_peeraddr2str(struct rpc_clnt *, enum rpc_display_format_t);
 int		rpc_localaddr(struct rpc_clnt *, struct sockaddr *, size_t);
+int		rpc_is_foreign(struct rpc_clnt *);
 

include/linux/sunrpc/xprt.h int xs_swapper(struct rpc_xprt *xprt, int enable); · line 361

Quoted 12 lines

include/linux/sunrpc/clnt.h +0 -0

 #endif /* __KERNEL__ */
 #endif /* _LINUX_SUNRPC_CLNT_H */

include/linux/sunrpc/xprt.h +1 -0

index fcbfe8783243..6a9dffcb9d3f 100644
@@ -357,6 +357,7 @@ int			xs_swapper(struct rpc_xprt *xprt, int enable);
 #define XPRT_CONNECTION_ABORT	(7)
 #define XPRT_CONNECTION_CLOSE	(8)
 #define XPRT_CONGESTED		(9)
+#define XPRT_LOCAL		(10)
 

Can we please rename that to XPRT_LOOPBACK or something along those lines? To me XPRT_LOCAL looks a little too close for comfort to AF_LOCAL, which is also a supported transport.

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

Quoted 44 lines · NeilBrown

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

Quoted 5 lines

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

This puts way too much RPC connection logic in the NFS layer: we really should not have to care. Is there any reason why we could not just use RPC_TASK_SOFTCONN to have the commit fail if the connection is broken?

Note that all this has to come with a huge WARNING: all your data may be lost even if you think you just fsync()ed it. Is there any difference between doing this and using the 'async' export option on the knfsd side?

Quoted 51 lines · On Wed, 20 Aug 2014 20:45:16 -0400 Trond Myklebust…

Quoted 44 lines · NeilBrown

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

Quoted 5 lines

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

Quoted 3 lines

This puts way too much RPC connection logic in the NFS layer: we really should not have to care. Is there any reason why we could not just use RPC_TASK_SOFTCONN to have the commit fail if the connection is broken?

I tried to keep as much in the RPC layer as I could....

There really is a big difference between talking to an 'nfsd' on the same machine and talking to one on a different machine. In the first case you need to be cautious of deadlocks, in the second you don't. That is a difference that matters to NFS, not to RPC.

I guess we could always have a timeout, even when connected to a remote server. We would just end up blocking somewhere else when the server was not responding.

I don't think SOFTCONN is really appropriate. We don't want the COMMIT to stop being retried. We just don't want the memory reclaim code to block waiting for the COMMIT.

Quoted 6 lines

Note that all this has to come with a huge WARNING: all your data may be lost even if you think you just fsync()ed it. Is there any difference between doing this and using the 'async' export option on the knfsd side?

I don't think this patch risks losing data at all - that certainly isn't the intent.
This patch only affects the behaviour of ->releasepage, and only allows it to fail instead of block. It is perfectly acceptable for releasepage to fail and it doesn't result in data loss. It just means that the page is busy.

Quoted 80 lines · NeilBrown

Quoted 51 lines · On Wed, 20 Aug 2014 20:45:16 -0400 Trond Myklebust…

Quoted 44 lines · NeilBrown

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

Quoted 5 lines

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

Quoted 3 lines

This puts way too much RPC connection logic in the NFS layer: we really should not have to care. Is there any reason why we could not just use RPC_TASK_SOFTCONN to have the commit fail if the connection is broken?

I tried to keep as much in the RPC layer as I could....

There really is a big difference between talking to an 'nfsd' on the same machine and talking to one on a different machine. In the first case you need to be cautious of deadlocks, in the second you don't. That is a difference that matters to NFS, not to RPC.

I guess we could always have a timeout, even when connected to a remote server. We would just end up blocking somewhere else when the server was not responding.

I don't think SOFTCONN is really appropriate. We don't want the COMMIT to stop being retried. We just don't want the memory reclaim code to block waiting for the COMMIT.

Quoted 6 lines

Note that all this has to come with a huge WARNING: all your data may be lost even if you think you just fsync()ed it. Is there any difference between doing this and using the 'async' export option on the knfsd side?

Quoted 6 lines

I don't think this patch risks losing data at all - that certainly isn't the intent.
This patch only affects the behaviour of ->releasepage, and only allows it to fail instead of block. It is perfectly acceptable for releasepage to fail and it doesn't result in data loss. It just means that the page is busy.

So why not just change the behaviour of ->releasepage() to always initiate a non-blocking commit and then wait up to 1 second for the PG_private bit to be released?

Quoted 88 lines · On Wed, 20 Aug 2014 21:42:55 -0400 Trond Myklebust…

Quoted 86 lines · NeilBrown

Quoted 51 lines · On Wed, 20 Aug 2014 20:45:16 -0400 Trond Myklebust…

Quoted 44 lines · NeilBrown

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

Quoted 5 lines

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

Quoted 3 lines

This puts way too much RPC connection logic in the NFS layer: we really should not have to care. Is there any reason why we could not just use RPC_TASK_SOFTCONN to have the commit fail if the connection is broken?

I tried to keep as much in the RPC layer as I could....

There really is a big difference between talking to an 'nfsd' on the same machine and talking to one on a different machine. In the first case you need to be cautious of deadlocks, in the second you don't. That is a difference that matters to NFS, not to RPC.

I guess we could always have a timeout, even when connected to a remote server. We would just end up blocking somewhere else when the server was not responding.

I don't think SOFTCONN is really appropriate. We don't want the COMMIT to stop being retried. We just don't want the memory reclaim code to block waiting for the COMMIT.

Quoted 6 lines

Note that all this has to come with a huge WARNING: all your data may be lost even if you think you just fsync()ed it. Is there any difference between doing this and using the 'async' export option on the knfsd side?

I don't think this patch risks losing data at all - that certainly isn't the intent.
This patch only affects the behaviour of ->releasepage, and only allows it to fail instead of block. It is perfectly acceptable for releasepage to fail and it doesn't result in data loss. It just means that the page is busy.

Quoted 4 lines

So why not just change the behaviour of ->releasepage() to always initiate a non-blocking commit and then wait up to 1 second for the PG_private bit to be released?

Would that work?
PG_private seems to be set when the page is being written, not when the COMMIT is happening.

Quoted 88 lines · On Wed, 20 Aug 2014 21:42:55 -0400 Trond Myklebust…

Quoted 80 lines · NeilBrown

Quoted 51 lines · On Wed, 20 Aug 2014 20:45:16 -0400 Trond Myklebust…

Quoted 44 lines · NeilBrown

Support for loop-back mounted NFS filesystems is useful when NFS is used to access shared storage in a high-availability cluster.

If the node running the NFS server fails, some other node can mount the filesystem and start providing NFS service. If that node already had the filesystem NFS mounted, it will now have it loop-back mounted.

nfsd can suffer a deadlock when allocating memory and entering direct reclaim.
While direct reclaim does not write to the NFS filesystem it can send and wait for a COMMIT through nfs_release_page().

This patch modifies nfs_release_page() and the functions it calls so that if the COMMIT is sent to the local host (i.e. is routed over the loop-back interface) then nfs_release_page() will not wait forever for that COMMIT to complete. This is achieved using a new flag FLUSH_COND_CONNECTED. When this is set the flush is conditional and will only wait if the client is connected to a non-local server.

Aborting early should be safe as kswapd uses the same code but never waits for the COMMIT.

Always aborting early could upset the balance of memory management so when the commit was sent to the local host we still wait but with a 100ms timeout. This is enough to significantly slow down any process that is allocating lots of memory and often long enough to let the commit complete.

In those rare cases where it is nfsd, or something that nfsd is waiting for, that is calling nfs_release_page(), 100ms is not so long that throughput will be seriously affected.

When fail-over happens a remote (foreign) client will first become disconnected and then turn into a local client.
To prevent deadlocks from happening at this point, we still have a timeout when the COMMIT has been sent to a remote client. In this case the timeout is longer (1 second).

So when a node that has mounted a remote filesystem loses the connection, nfs_release_page() will stop blocking and start failing. Memory allocators will then be able to make progress without blocking in NFS. Any process which writes to a file will still be blocked in balance_dirty_pages().

Quoted 5 lines

This patch makes use of the new 'private' field in "struct wait_bit_key" to store the start time of a commit, so the 'action' function called by __wait_on_bit() knows how much longer it is appropriate to wait.

Quoted 3 lines

This puts way too much RPC connection logic in the NFS layer: we really should not have to care. Is there any reason why we could not just use RPC_TASK_SOFTCONN to have the commit fail if the connection is broken?

I tried to keep as much in the RPC layer as I could....

There really is a big difference between talking to an 'nfsd' on the same machine and talking to one on a different machine. In the first case you need to be cautious of deadlocks, in the second you don't. That is a difference that matters to NFS, not to RPC.

I guess we could always have a timeout, even when connected to a remote server. We would just end up blocking somewhere else when the server was not responding.

I don't think SOFTCONN is really appropriate. We don't want the COMMIT to stop being retried. We just don't want the memory reclaim code to block waiting for the COMMIT.

Quoted 6 lines

Note that all this has to come with a huge WARNING: all your data may be lost even if you think you just fsync()ed it. Is there any difference between doing this and using the 'async' export option on the knfsd side?

Quoted 6 lines

I don't think this patch risks losing data at all - that certainly isn't the intent.
This patch only affects the behaviour of ->releasepage, and only allows it to fail instead of block. It is perfectly acceptable for releasepage to fail and it doesn't result in data loss. It just means that the page is busy.

Quoted 4 lines

So why not just change the behaviour of ->releasepage() to always initiate a non-blocking commit and then wait up to 1 second for the PG_private bit to be released?

Would that work?
PG_private seems to be set when the page is being written, not when the COMMIT is happening.
That is marked with the NFS_INO_COMMIT flag.

So what my code does is wait a little while for NFS_INO_COMMIT to be released.

I tried to leave the flow largely unchanged for a non-local server and only introduced a timeout for a local server. That probably creates the complexity that is bothering you(?).