Re: linux 2.4.18-5 nfs/rpc problem

[David Jeffery <david_jeffery@adaptec.com>]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

Eduard,

I have also seen this problem with both Red Hat and stock kernels.  I've
got some patches That I'm testing to try and fix the problem for me, but
it takes a while for my setup to reproduce the problem.  How easy is it
for you to trigger?

I've attached two patches for 2.4.22 that I'm currently testing.  The
first (nfsiput.patch) is to fix the deadlocked processes you see by
moving the iput() call to happen after the waited on request is
unlocked.

But that simple fix is incomplete.  It creates another problem that
generates a BUG() in iput() as now instead of deadlocking, nfs can
continue on and end up calling iput() on a cleared inode.  The second
patch (nfsigrab.patch) checks for a failure when calling igrab() and
aborts an async write and instead converts it to a sync write which I
believe should fix the problem.  But, I haven't triggered it yet so this
patch is effectively untested.

If someone who knows nfs/vfs better could sanity check what I'm doing,
it would be appreciated.

David Jeffery

[-- Attachment #2: nfsiput.patch --]
[-- Type: text/plain, Size: 3053 bytes --]

--- write.c.orig	Thu Sep 25 07:33:34 2003
+++ write.c	Fri Sep 26 07:09:32 2003
@@ -318,14 +318,15 @@
 /*
  * Insert a write request into an inode
  */
-static inline void
+static inline int
 nfs_inode_remove_request(struct nfs_page *req)
 {
 	struct inode *inode;
+	int need_iput = 0;
 	spin_lock(&nfs_wreq_lock);
 	if (list_empty(&req->wb_hash)) {
 		spin_unlock(&nfs_wreq_lock);
-		return;
+		return 0;
 	}
 	if (!NFS_WBACK_BUSY(req))
 		printk(KERN_ERR "NFS: unlocked request attempted unhashed!\n");
@@ -335,13 +336,12 @@
 	inode->u.nfs_i.npages--;
 	if ((inode->u.nfs_i.npages == 0) != list_empty(&inode->u.nfs_i.writeback))
 		printk(KERN_ERR "NFS: desynchronized value of nfs_i.npages.\n");
-	if (list_empty(&inode->u.nfs_i.writeback)) {
-		spin_unlock(&nfs_wreq_lock);
-		iput(inode);
-	} else
-		spin_unlock(&nfs_wreq_lock);
+	if (list_empty(&inode->u.nfs_i.writeback))
+		need_iput = 1;
+	spin_unlock(&nfs_wreq_lock);
 	nfs_clear_request(req);
 	nfs_release_request(req);
+	return need_iput;
 }
 
 /*
@@ -1049,6 +1049,7 @@
 	 */
 	nfs_write_attributes(inode, resp->fattr);
 	while (!list_empty(&data->pages)) {
+		int need_iput = 0;
 		req = nfs_list_entry(data->pages.next);
 		nfs_list_remove_request(req);
 		page = req->wb_page;
@@ -1064,14 +1065,14 @@
 			SetPageError(page);
 			if (req->wb_file)
 				req->wb_file->f_error = task->tk_status;
-			nfs_inode_remove_request(req);
+			need_iput = nfs_inode_remove_request(req);
 			dprintk(", error = %d\n", task->tk_status);
 			goto next;
 		}
 
 #ifdef CONFIG_NFS_V3
 		if (argp->stable != NFS_UNSTABLE || resp->verf->committed == NFS_FILE_SYNC) {
-			nfs_inode_remove_request(req);
+			need_iput = nfs_inode_remove_request(req);
 			dprintk(" OK\n");
 			goto next;
 		}
@@ -1080,10 +1081,12 @@
 		nfs_mark_request_commit(req);
 		dprintk(" marked for commit\n");
 #else
-		nfs_inode_remove_request(req);
+		need_iput = nfs_inode_remove_request(req);
 #endif
 	next:
 		nfs_unlock_request(req);
+		if(need_iput)
+			iput(inode);
 	}
 }
 
@@ -1203,6 +1206,7 @@
 
 	nfs_write_attributes(inode, resp->fattr);
 	while (!list_empty(&data->pages)) {
+		int need_iput = 0;
 		req = nfs_list_entry(data->pages.next);
 		nfs_list_remove_request(req);
 
@@ -1214,7 +1218,7 @@
 		if (task->tk_status < 0) {
 			if (req->wb_file)
 				req->wb_file->f_error = task->tk_status;
-			nfs_inode_remove_request(req);
+			need_iput = nfs_inode_remove_request(req);
 			dprintk(", error = %d\n", task->tk_status);
 			goto next;
 		}
@@ -1223,7 +1227,7 @@
 		 * returned by the server against all stored verfs. */
 		if (!memcmp(req->wb_verf.verifier, data->verf.verifier, sizeof(data->verf.verifier))) {
 			/* We have a match */
-			nfs_inode_remove_request(req);
+			need_iput = nfs_inode_remove_request(req);
 			dprintk(" OK\n");
 			goto next;
 		}
@@ -1232,6 +1236,8 @@
 		nfs_mark_request_dirty(req);
 	next:
 		nfs_unlock_request(req);
+		if(need_iput)
+			iput(inode);
 	}
 }
 #endif

[-- Attachment #3: nfsigrab.patch --]
[-- Type: text/plain, Size: 1961 bytes --]

--- linux-2.4.22/fs/nfs/write.c.fix4.notfinished	Tue Oct  7 08:15:56 2003
+++ linux-2.4.22/fs/nfs/write.c	Tue Oct  7 09:14:16 2003
@@ -248,6 +248,12 @@
 		err = nfs_writepage_async(NULL, inode, page, 0, offset);
 		if (err >= 0)
 			err = 0;
+		else if(err == -EDEADLK){
+			printk(KERN_WARNING "NFS DEBUG: EDEADLK returned, doing nfs_writepage_sync\n");
+			err = nfs_writepage_sync(NULL, inode, page, 0, offset);
+			if (err == offset)
+				err = 0;
+		}
 	} else {
 		err = nfs_writepage_sync(NULL, inode, page, 0, offset); 
 		if (err == offset)
@@ -292,19 +298,22 @@
  *	 & co. for the 'write append' case. For 2.5 we may want to consider
  *	 some form of hashing so as to perform well on random writes.
  */
-static inline void
+static inline int
 nfs_inode_add_request(struct inode *inode, struct nfs_page *req)
 {
 	struct list_head *pos, *head;
 	unsigned long pg_idx = page_index(req->wb_page);
 
 	if (!list_empty(&req->wb_hash))
-		return;
+		return 0;
 	if (!NFS_WBACK_BUSY(req))
 		printk(KERN_ERR "NFS: unlocked request attempted hashed!\n");
 	head = &inode->u.nfs_i.writeback;
 	if (list_empty(head))
-		igrab(inode);
+		if(!igrab(inode)){
+			printk(KERN_WARNING "NFS DEBUG: igrab failed!  Aborting async writepage.\n");
+			return -EDEADLK;
+		}
 	list_for_each_prev(pos, head) {
 		struct nfs_page *entry = nfs_inode_wb_entry(pos);
 		if (page_index(entry->wb_page) < pg_idx)
@@ -313,6 +322,7 @@
 	inode->u.nfs_i.npages++;
 	list_add(&req->wb_hash, pos);
 	req->wb_count++;
+	return 0;
 }
 
 /*
@@ -659,9 +669,15 @@
 		}
 
 		if (new) {
+			int error;
 			nfs_lock_request_dontget(new);
-			nfs_inode_add_request(inode, new);
+			error = nfs_inode_add_request(inode, new);
 			spin_unlock(&nfs_wreq_lock);
+			if(error < 0){
+				nfs_unlock_request(new);
+				nfs_release_request(new);
+				return ERR_PTR(error);
+			}
 			nfs_mark_request_dirty(new);
 			return new;
 		}