NFS: Stop sillyname renames and unmounts from racing

PATCH In review Steve Dickson 4 participants 6 messages Nov 3, 2007 – Nov 8, 2007

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2007-11-03 11:13 UTC #0 lore raw

PATCH 1/1 NFS: Stop sillyname renames and unmounts from racing

The following patch stops NFS sillyname renames and umounts from racing. I have a test script does the following:
1) start nfs server
2) mount loopback
3) open file in background
4) remove file
5) stop nfs server
6) kill -9 process which has file open
7) restart nfs server
8) umount looback mount.

After umount I got the "VFS: Busy inodes after unmount" message because the processing of the rename has not finished.

Below is a patch that the uses the new silly_count mechanism to synchronize sillyname processing and umounts. The patch introduces a nfs_put_super() routine that waits until the nfsi->silly_count count is zero.

A side-effect of finding and waiting for all the inode to find the sillyname processing, is I need to traverse the sb->s_inodes list in the supper block. To do that safely the inode_lock spin lock has to be held. So for modules to be able to "see" that lock I needed to EXPORT_SYMBOL_GPL() it.

Any objections to exporting the inode_lock spin lock? If so, how should modules _safely_ access the s_inode list?

steved.

Author: Steve Dickson <[email protected]>
Date:   Wed Oct 31 12:19:26 2007 -0400

Close a unlink/sillyname rename and umount race by added a nfs_put_super routine that will run through all the inode currently on the super block, waiting for those that are in the middle of a sillyname rename or removal.

This patch stop the infamous "VFS: Busy inodes after unmount... " warning during umounts.

Signed-off-by Steve Dickson <[email protected]>

fs/inode.c +1 -0

index ed35383..da9034a 100644
@@ -81,6 +81,7 @@ static struct hlist_head *inode_hashtable __read_mostly;
  * the i_state of an inode while it is in use..
  */
 DEFINE_SPINLOCK(inode_lock);
+EXPORT_SYMBOL_GPL(inode_lock);
 
 /*
  * iprune_mutex provides exclusion between the kswapd or try_to_free_pages

fs/nfs/super.c +1 -0

index fa517ae..2ac3c34 100644
@@ -48,6 +48,7 @@
 #include <linux/nfs_xdr.h>
 #include <linux/magic.h>
 #include <linux/parser.h>
+#include <linux/writeback.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>

@@ -202,6 +203,7 @@ static int nfs_get_sb(struct file_system_type *,

int, const char *, void *, stru
static int nfs_xdev_get_sb(struct file_system_type *fs_type,
int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
static void nfs_kill_super(struct super_block *);
+static void nfs_put_super(struct super_block *);

static struct file_system_type nfs_fs_type = {
.owner = THIS_MODULE,
@@ -223,6 +225,7 @@ static const struct super_operations nfs_sops = {
.alloc_inode = nfs_alloc_inode,
.destroy_inode = nfs_destroy_inode,
.write_inode = nfs_write_inode,
+ .put_super = nfs_put_super,
.statfs = nfs_statfs,
.clear_inode = nfs_clear_inode,
.umount_begin = nfs_umount_begin,
@@ -1767,6 +1770,30 @@ static void nfs4_kill_super(struct super_block *sb)
nfs_free_server(server);
}

+void nfs_put_super(struct super_block *sb)
+{
+ struct inode *inode;
+ struct nfs_inode *nfsi;
+ /*
+ * Make sure there are no outstanding renames
+ */
+relock:
+ spin_lock(&inode_lock);
+ list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+ nfsi = NFS_I(inode);
+ if (atomic_read(&nfsi->silly_count) > 0) {
+ /* Keep this inode around during the wait */
+ atomic_inc(&inode->i_count);
+ spin_unlock(&inode_lock);
+ wait_event(nfsi->waitqueue,
+ atomic_read(&nfsi->silly_count) == 1);
+ iput(inode);
+ goto relock;
+ }
+ }
+ spin_unlock(&inode_lock);
+}
+
/*
* Clone an NFS4 server record on xdev traversal (FSID-change) */

Andrew Morton

2007-11-06 05:06 UTC #1 lore raw

Quoted 1 line · On Sat, 03 Nov 2007 07:09:25 -0400 Steve Dickson <[email protected]> wrote:

The following patch stops NFS sillyname renames and umounts from racing.

(appropriate cc's added)

Quoted 49 lines

I have a test script does the following:
1) start nfs server
2) mount loopback
3) open file in background
4) remove file
5) stop nfs server
6) kill -9 process which has file open
7) restart nfs server
8) umount looback mount.
After umount I got the "VFS: Busy inodes after unmount" message because the processing of the rename has not finished.
Below is a patch that the uses the new silly_count mechanism to synchronize sillyname processing and umounts. The patch introduces a nfs_put_super() routine that waits until the nfsi->silly_count count is zero.
A side-effect of finding and waiting for all the inode to find the sillyname processing, is I need to traverse the sb->s_inodes list in the supper block. To do that safely the inode_lock spin lock has to be held. So for modules to be able to "see" that lock I needed to EXPORT_SYMBOL_GPL() it.
Any objections to exporting the inode_lock spin lock? If so, how should modules _safely_ access the s_inode list?
steved.
Author: Steve Dickson <[email protected]>
Date:   Wed Oct 31 12:19:26 2007 -0400
Close a unlink/sillyname rename and umount race by added a nfs_put_super routine that will run through all the inode currently on the super block, waiting for those that are in the middle of a sillyname rename or removal.
This patch stop the infamous "VFS: Busy inodes after unmount... " warning during umounts.
Signed-off-by Steve Dickson <[email protected]>
fs/inode.c +0 -0
index ed35383..da9034a 100644

fs/inode.c static struct hlist_head *inode_hashtable __read_mostly; · line 84

Quoted 4 lines

fs/inode.c +1 -0

@@ -81,6 +81,7 @@ static struct hlist_head *inode_hashtable __read_mostly;
   * the i_state of an inode while it is in use..
   */
  DEFINE_SPINLOCK(inode_lock);
+EXPORT_SYMBOL_GPL(inode_lock);

That's going to make hch unhappy.

Your email client is performing space-stuffing. See http://mbligh.org/linuxdocs/Email/Clients/Thunderbird

Quoted 11 lines

fs/inode.c +1 -0

  static struct file_system_type nfs_fs_type = {
  	.owner		= THIS_MODULE,

@@ -223,6 +225,7 @@ static const struct super_operations nfs_sops = {
  	.alloc_inode	= nfs_alloc_inode,
  	.destroy_inode	= nfs_destroy_inode,
  	.write_inode	= nfs_write_inode,
+	.put_super	= nfs_put_super,
  	.statfs		= nfs_statfs,
  	.clear_inode	= nfs_clear_inode,
  	.umount_begin	= nfs_umount_begin,

fs/inode.c static void nfs4_kill_super(struct super_block *sb) · line 1773

Quoted 4 lines

fs/inode.c +1 -0

@@ -1767,6 +1770,30 @@ static void nfs4_kill_super(struct super_block *sb)
  	nfs_free_server(server);
  }
 
+void nfs_put_super(struct super_block *sb)

This was (correctly) declared to be static. We should define it that way too (I didn't know you could do this, actually).

Quoted 12 lines

fs/inode.c +12 -0

+{
+	struct inode *inode;
+	struct nfs_inode *nfsi;
+	/*
+	 * Make sure there are no outstanding renames
+	 */
+relock:
+	spin_lock(&inode_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		nfsi = NFS_I(inode);
+		if (atomic_read(&nfsi->silly_count) > 0) {
+			/* Keep this inode around  during the wait */

fs/inode.c static void nfs4_kill_super(struct super_block *sb) · line 1795

Quoted 10 lines

fs/inode.c +10 -0

+			atomic_inc(&inode->i_count);
+			spin_unlock(&inode_lock);
+			wait_event(nfsi->waitqueue,
+				atomic_read(&nfsi->silly_count) == 1);
+			iput(inode);
+			goto relock;
+		}
+	}
+	spin_unlock(&inode_lock);
+}

That's an O(n^2) search. If it is at all possible to hit a catastrophic slowdown in here, you can bet that someone out there will indeed hit it in real life.

I'm too lazy to look, but we might need to check things like I_FREEING and I_CLEAR before taking a ref on this inode.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Alexander Viro

2007-11-06 05:15 UTC #2 lore raw

Quoted 4 lines · Andrew Morton

Quoted 2 lines
Any objections to exporting the inode_lock spin lock? If so, how should modules _safely_ access the s_inode list?
That's going to make hch unhappy.

That's going to make me just as unhappy, especially since it's pointless; instead of the entire sorry mess we should just bump sb->s_active to pin the superblock down (we know that it's active at that point, so it's just an atomic_inc(); no games with locking, etc., are needed) and call deactivate_super() on the way out. And deactivate_super() is exported already.

Benny Halevy

2007-11-06 08:24 UTC #4 lore raw

In reply to Andrew Morton

Quoted 111 lines · Andrew Morton

Quoted 1 line · On Sat, 03 Nov 2007 07:09:25 -0400 Steve Dickson <[email protected]> wrote:
The following patch stops NFS sillyname renames and umounts from racing.
(appropriate cc's added)
Quoted 49 lines
I have a test script does the following:
1) start nfs server
2) mount loopback
3) open file in background
4) remove file
5) stop nfs server
6) kill -9 process which has file open
7) restart nfs server
8) umount looback mount.
After umount I got the "VFS: Busy inodes after unmount" message because the processing of the rename has not finished.
Below is a patch that the uses the new silly_count mechanism to synchronize sillyname processing and umounts. The patch introduces a nfs_put_super() routine that waits until the nfsi->silly_count count is zero.
A side-effect of finding and waiting for all the inode to find the sillyname processing, is I need to traverse the sb->s_inodes list in the supper block. To do that safely the inode_lock spin lock has to be held. So for modules to be able to "see" that lock I needed to EXPORT_SYMBOL_GPL() it.
Any objections to exporting the inode_lock spin lock? If so, how should modules _safely_ access the s_inode list?
steved.
Author: Steve Dickson <[email protected]>
Date:   Wed Oct 31 12:19:26 2007 -0400
Close a unlink/sillyname rename and umount race by added a nfs_put_super routine that will run through all the inode currently on the super block, waiting for those that are in the middle of a sillyname rename or removal.
This patch stop the infamous "VFS: Busy inodes after unmount... " warning during umounts.
Signed-off-by Steve Dickson <[email protected]>
fs/inode.c +0 -0
index ed35383..da9034a 100644
fs/inode.c static struct hlist_head *inode_hashtable __read_mostly; · line 84
Quoted 4 lines
fs/inode.c +1 -0
@@ -81,6 +81,7 @@ static struct hlist_head *inode_hashtable __read_mostly;
   * the i_state of an inode while it is in use..
   */
  DEFINE_SPINLOCK(inode_lock);
+EXPORT_SYMBOL_GPL(inode_lock);
That's going to make hch unhappy.
Your email client is performing space-stuffing. See http://mbligh.org/linuxdocs/Email/Clients/Thunderbird
Quoted 11 lines
fs/inode.c +1 -0
  static struct file_system_type nfs_fs_type = {
  	.owner		= THIS_MODULE,
@@ -223,6 +225,7 @@ static const struct super_operations nfs_sops = {
  	.alloc_inode	= nfs_alloc_inode,
  	.destroy_inode	= nfs_destroy_inode,
  	.write_inode	= nfs_write_inode,
+	.put_super	= nfs_put_super,
  	.statfs		= nfs_statfs,
  	.clear_inode	= nfs_clear_inode,
  	.umount_begin	= nfs_umount_begin,
fs/inode.c static void nfs4_kill_super(struct super_block *sb) · line 1773
Quoted 4 lines
fs/inode.c +1 -0
@@ -1767,6 +1770,30 @@ static void nfs4_kill_super(struct super_block *sb)
  	nfs_free_server(server);
  }
 
+void nfs_put_super(struct super_block *sb)
This was (correctly) declared to be static. We should define it that way too (I didn't know you could do this, actually).
Quoted 12 lines
fs/inode.c +12 -0
+{
+	struct inode *inode;
+	struct nfs_inode *nfsi;
+	/*
+	 * Make sure there are no outstanding renames
+	 */
+relock:
+	spin_lock(&inode_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		nfsi = NFS_I(inode);
+		if (atomic_read(&nfsi->silly_count) > 0) {
+			/* Keep this inode around  during the wait */
fs/inode.c static void nfs4_kill_super(struct super_block *sb) · line 1795
Quoted 10 lines
fs/inode.c +10 -0
+			atomic_inc(&inode->i_count);
+			spin_unlock(&inode_lock);
+			wait_event(nfsi->waitqueue,
+				atomic_read(&nfsi->silly_count) == 1);
+			iput(inode);
+			goto relock;
+		}
+	}
+	spin_unlock(&inode_lock);
+}
That's an O(n^2) search. If it is at all possible to hit a catastrophic slowdown in here, you can bet that someone out there will indeed hit it in real life.

Quoted 2 lines

I'm too lazy to look, but we might need to check things like I_FREEING and I_CLEAR before taking a ref on this inode.

It'd be very nice if the silly renamed inodes (with silly_count > 1) were moved to a different list in the first pass, under the inode_lock, and then waited on until silly_count <= 1 in a second pass only on the filtered list. This will provide you with O(1).

Alexander Viro

2007-11-06 08:50 UTC #5 lore raw

Quoted 4 lines · Benny Halevy

It'd be very nice if the silly renamed inodes (with silly_count > 1) were moved to a different list in the first pass, under the inode_lock, and then waited on until silly_count <= 1 in a second pass only on the filtered list. This will provide you with O(1).

It's absolutely pointless, starting with any kind of searching for inodes, etc. If you want fs shutdown _not_ to happen until async activity of that kind is over, don't reinvent the sodding wheels, just tell VFS that you are holding an active reference to superblock. End of story.

Steve Dickson

author

2007-11-08 09:05 UTC #3 lore raw

In reply to Alexander Viro

Quoted 4 lines · Alexander Viro

Quoted 2 lines · Andrew Morton
Quoted 2 lines
Any objections to exporting the inode_lock spin lock? If so, how should modules _safely_ access the s_inode list?

Quoted 8 lines

Quoted 1 line
That's going to make hch unhappy.
That's going to make me just as unhappy, especially since it's pointless; instead of the entire sorry mess we should just bump sb->s_active to pin the superblock down (we know that it's active at that point, so it's just an atomic_inc(); no games with locking, etc., are needed) and call deactivate_super() on the way out. And deactivate_super() is exported already.

Even though Al suggestion did indeed fix the race and was incredibility simple (adding two lines of code), it turns out calling deactivate_super() from the rpciod kernel daemon is not good, since it can cause deadlocks with other async tasks.

Following a suggestion from Trond, I decided to go back to the original idea of using put_super, but keeping all the changes inside the NFS client code.

So the following patch adds an active/deactive mechanism to the nfs_server structure (very similar to the s_active/deactivate_super() mechanism) that any async opt (in this case silly renames) can use to stop unmount for occurring before they are finished.

This means there is no need to expose any spin locks, no need for an O(n2) search and all the changes are contained inside the NFS client code.

steved.

[Note: I did make the proposed corrections to my email client, so hopefully space-stuffing will be fixed. ]

Author: Steve Dickson <[email protected]>

Added an active/deactive mechanism to the nfs_server structure allowing async operations to hold off umount until the operations are done.

Signed-off-by Steve Dickson <[email protected]>

fs/nfs/client.c +4 -0

index 70587f3..2ecf726 100644
@@ -593,6 +593,10 @@ static int nfs_init_server(struct nfs_server *server,
 	server->namelen  = data->namlen;
 	/* Create a client RPC handle for the NFSv3 ACL management interface */
 	nfs_init_server_aclclient(server);
+
+	init_waitqueue_head(&server->active_wq);
+	atomic_set(&server->active, 0);
+
 	dprintk("<-- nfs_init_server() = 0 [new %p]\n", clp);
 	return 0;

fs/nfs/super.c +13 -0

index fa517ae..e07ac96 100644
@@ -202,6 +202,7 @@ static int nfs_get_sb(struct file_system_type *, int, const char *, void *, stru
 static int nfs_xdev_get_sb(struct file_system_type *fs_type,
 		int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
 static void nfs_kill_super(struct super_block *);
+static void nfs_put_super(struct super_block *);
 
 static struct file_system_type nfs_fs_type = {
 	.owner		= THIS_MODULE,

@@ -223,6 +224,7 @@ static const struct super_operations nfs_sops = {
 	.alloc_inode	= nfs_alloc_inode,
 	.destroy_inode	= nfs_destroy_inode,
 	.write_inode	= nfs_write_inode,
+	.put_super	= nfs_put_super,
 	.statfs		= nfs_statfs,
 	.clear_inode	= nfs_clear_inode,
 	.umount_begin	= nfs_umount_begin,

@@ -1767,6 +1769,17 @@ static void nfs4_kill_super(struct super_block *sb)
 	nfs_free_server(server);
 }
 
+static void nfs_put_super(struct super_block *sb)
+{
+	struct nfs_server *server = NFS_SB(sb);
+	/*
+	 * Make sure there are no outstanding ops to this server.
+	 * If so, wait for them to finish before allowing the
+	 * unmount to continue.
+	 */
+	wait_event(server->active_wq, atomic_read(&server->active) == 0);
+}
+
 /*
  * Clone an NFS4 server record on xdev traversal (FSID-change)
  */

fs/nfs/unlink.c +2 -0

index 233ad38..cf12a24 100644
@@ -29,6 +29,7 @@ struct nfs_unlinkdata {
 static void
 nfs_free_unlinkdata(struct nfs_unlinkdata *data)
 {
+	nfs_sb_deactive(NFS_SERVER(data->dir));
 	iput(data->dir);
 	put_rpccred(data->cred);
 	kfree(data->args.name.name);

@@ -151,6 +152,7 @@ static int nfs_do_call_unlink(struct dentry *parent, struct inode *dir, struct n
 		nfs_dec_sillycount(dir);
 		return 0;
 	}
+	nfs_sb_active(NFS_SERVER(dir));
 	data->args.fh = NFS_FH(dir);
 	nfs_fattr_init(&data->res.dir_attr);

include/linux/nfs_fs_sb.h +17 -0

index 0cac49b..6ef3af8 100644
@@ -4,6 +4,8 @@
 #include <linux/list.h>
 #include <linux/backing-dev.h>
 
+#include <asm/atomic.h>
+
 struct nfs_iostats;
 
 /*

@@ -110,8 +112,23 @@ struct nfs_server {
 						   filesystem */
 #endif
 	void (*destroy)(struct nfs_server *);
+
+	atomic_t active; /* Keep trace of any activity to this server */
+	wait_queue_head_t active_wq;  /* Wait for any activity to stop  */
 };
 
+static inline void 
+nfs_sb_active(struct nfs_server *server)
+{
+	atomic_inc(&server->active);
+}
+static inline void 
+nfs_sb_deactive(struct nfs_server *server)
+{
+	if (atomic_dec_and_test(&server->active))
+		wake_up(&server->active_wq);
+}
+
 /* Server capabilities */
 #define NFS_CAP_READDIRPLUS	(1U << 0)
 #define NFS_CAP_HARDLINKS	(1U << 1)