Skip to contentSkip to conversation

spinlock recursion on inode number mismatches

PATCH In review Steve Dickson 2 participants 3 messages Nov 17, 2005 – Nov 18, 2005

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Steve Dickson

author
#0 lore raw

PATCH 1/1 spinlock recursion on inode number mismatches

When compiling over NFS using a 2.6.14 kernel, the following spinlock recursion BUG popped:

nfs_update_inode: inode number mismatch expected (0:16/0x1dee71a), got (0:16/0x100000000000000)
nfs_update_inode: inode number mismatch expected (0:16/0x1dee71a), got (0:16/0x1dee719)
nfs_update_inode: inode number mismatch expected (0:16/0x1dee71a), got (0:16/0x0)
nfs_update_inode: inode number mismatch expected (0:16/0x1dee71a), got (0:16/0x1dee719)

BUG: spinlock recursion on CPU#0, bhc/22635 (Not tainted)
 lock: db7294e8, .magic: dead4ead, .owner: bhc/22635, .owner_cpu: 0
 [<c01f53b3>] spin_bug+0xa3/0xd7
 [<c01f551e>] _raw_spin_lock+0x68/0x6a
 [<f8db1151>] nfs_zap_caches+0x1a/0xaa [nfs]
 [<f8db2445>] nfs_update_inode+0x9f/0x618 [nfs]
 [<c033be8c>] _spin_unlock_irq+0x5/0x7
 [<f8db236b>] nfs_post_op_update_inode+0x2c/0x67 [nfs]
 [<f8dba716>] nfs3_proc_remove+0x9e/0xd7 [nfs]
 [<f8daed5a>] nfs_safe_remove+0x68/0xc4 [nfs]
 [<f8daee5e>] nfs_unlink+0xa8/0x10e [nfs]
 [<c017c8df>] vfs_unlink+0x19f/0x1a6
 [<c017c9a3>] sys_unlink+0xbd/0x13e
 [<c033cee2>] do_page_fault+0x262/0x650
 [<c016cb1d>] do_sync_read+0x0/0x116
 [<c01040a5>] syscall_call+0x7/0xb
Kernel panic - not syncing: bad locking
 [<c01250a8>] panic+0x45/0x1c5
 [<c01f53e7>] __spin_lock_debug+0x0/0xcf
 [<c01f551e>] _raw_spin_lock+0x68/0x6a
 [<f8db1151>] nfs_zap_caches+0x1a/0xaa [nfs]
 [<f8db2445>] nfs_update_inode+0x9f/0x618 [nfs]
 [<c033be8c>] _spin_unlock_irq+0x5/0x7
 [<f8db236b>] nfs_post_op_update_inode+0x2c/0x67 [nfs]
 [<f8dba716>] nfs3_proc_remove+0x9e/0xd7 [nfs]
 [<f8daed5a>] nfs_safe_remove+0x68/0xc4 [nfs]
 [<f8daee5e>] nfs_unlink+0xa8/0x10e [nfs]
 [<c017c8df>] vfs_unlink+0x19f/0x1a6
 [<c017c9a3>] sys_unlink+0xbd/0x13e
 [<c033cee2>] do_page_fault+0x262/0x650
 [<c016cb1d>] do_sync_read+0x0/0x116
 [<c01040a5>] syscall_call+0x7/0xb

The attached patch solve the problem by the problem by moving the call to nfs_invalidate_inode() out of nfs_invalidate_inode().

Commments?

steved.

The following patch stop a spinlock recursion BUG() from popping by moving the call to nfs_invalidate_inode() out of nfs_update_inode(). The nfs_invalidate_inode() is now down only when nfs_update_inode() returns -ESTALE.

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

----------------------------------------------

fs/nfs/inode.c renamed from fs/nfs/inode.c.orig +6 -6
@@ -1129,6 +1129,8 @@ __nfs_revalidate_inode(struct nfs_server
 		dfprintk(PAGECACHE, "nfs_revalidate_inode: (%s/%Ld) refresh failed, error=%d\n",
 			 inode->i_sb->s_id,
 			 (long long)NFS_FILEID(inode), status);
+		if (status == -ESTALE)
+			nfs_invalidate_inode(inode);
 		goto out;
 	}
 	cache_validity = nfsi->cache_validity;

@@ -1355,6 +1357,8 @@ int nfs_refresh_inode(struct inode *inod
 		status = nfs_check_inode_attributes(inode, fattr);
 
 	spin_unlock(&inode->i_lock);
+	if (status == -ESTALE)
+		nfs_invalidate_inode(inode);
 	return status;
 }
 

@@ -1382,6 +1386,8 @@ int nfs_post_op_update_inode(struct inod
 	nfsi->cache_change_attribute = jiffies;
 out:
 	spin_unlock(&inode->i_lock);
+	if (status == -ESTALE)
+		nfs_invalidate_inode(inode);
 	return status;
 }
 

@@ -1528,12 +1534,6 @@ static int nfs_update_inode(struct inode
 	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
 			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
 #endif
-	/*
-	 * No need to worry about unhashing the dentry, as the
-	 * lookup validation will know that the inode is bad.
-	 * (But we fall through to invalidate the caches.)
-	 */
-	nfs_invalidate_inode(inode);
  out_err:
 	set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
 	return -ESTALE;
#1 lore raw
Quoted 2 lines · Steve Dickson

The attached patch solve the problem by the problem by moving the call to nfs_invalidate_inode() out of nfs_invalidate_inode().

Hmm... That will cause us to call make_bad_inode on all errors in nfs_update_inode().

Given the sort of things we have in NFSv4 these days (delegations etc) that need to be cleaned up on inode release, I'm not sure we should ever be calling make_bad_inode().

How about something like the appended patch instead?

Cheers,
Trond
-------
NFS: Fix a spinlock recursion inside nfs_update_inode()

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/inode.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)
fs/nfs/inode.c +12 -14
index 6391d89..aaab1a5 100644
@@ -643,14 +643,11 @@ static int nfs_show_options(struct seq_f
 /*
  * Invalidate the local caches
  */
-void
-nfs_zap_caches(struct inode *inode)
+static void nfs_zap_caches_locked(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 	int mode = inode->i_mode;
 
-	spin_lock(&inode->i_lock);
-
 	NFS_ATTRTIMEO(inode) = NFS_MINATTRTIMEO(inode);
 	NFS_ATTRTIMEO_UPDATE(inode) = jiffies;
 

@@ -659,7 +656,12 @@ nfs_zap_caches(struct inode *inode)
 		nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL|NFS_INO_REVAL_PAGECACHE;
 	else
 		nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL|NFS_INO_REVAL_PAGECACHE;
+}
 
+void nfs_zap_caches(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	nfs_zap_caches_locked(inode);
 	spin_unlock(&inode->i_lock);
 }
 

@@ -676,16 +678,13 @@ static void nfs_zap_acl_cache(struct ino
 }
 
 /*
- * Invalidate, but do not unhash, the inode
+ * Invalidate, but do not unhash, the inode.
+ * NB: must be called with inode->i_lock held!
  */
-static void
-nfs_invalidate_inode(struct inode *inode)
+static void nfs_invalidate_inode(struct inode *inode)
 {
-	umode_t save_mode = inode->i_mode;
-
-	make_bad_inode(inode);
-	inode->i_mode = save_mode;
-	nfs_zap_caches(inode);
+	set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+	nfs_zap_caches_locked(inode);
 }
 
 struct nfs_find_desc {

@@ -1528,14 +1527,13 @@ static int nfs_update_inode(struct inode
 	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
 			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
 #endif
+ out_err:
 	/*
 	 * No need to worry about unhashing the dentry, as the
 	 * lookup validation will know that the inode is bad.
 	 * (But we fall through to invalidate the caches.)
 	 */
 	nfs_invalidate_inode(inode);
- out_err:
-	set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
 	return -ESTALE;
 }

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc. Get Certified Today Register for a JBoss Training Course. Free Certification Exam for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#2 lore raw
Quoted 2 lines · Trond Myklebust

Hmm... That will cause us to call make_bad_inode on all errors in nfs_update_inode().

Good point... I didn't see that...

Quoted 2 lines

How about something like the appended patch instead?

Yes... creating away for nfs_zap_caches() to be called while holding the inode spinlock does in deed fix the problem... nice work... thanks!

steved.

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc. Get Certified Today Register for a JBoss Training Course. Free Certification Exam for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs