[PATCH 2/2] AFS: Fix a couple of problems with unlinking AFS files

Previous thread: Current git fails to compile on AMD64 for three days in a row by Florin Iucha on Thursday, May 10, 2007 - 8:16 am. (4 messages)

Next thread: Re: 2.6.21-git10/11: files getting truncated on xfs? or maybe an nlink problem? by Matt Mackall on Thursday, May 10, 2007 - 8:38 am. (5 messages)
From: David Howells
Date: Thursday, May 10, 2007 - 7:33 am

Following bug was uncovered by compiling with '-W' flag:

  CC [M]  fs/afs/write.o
fs/afs/write.c: In function ‘afs_write_back_from_locked_page’:
fs/afs/write.c:398: warning: comparison of unsigned expression >= 0 is always true

Loop variable 'n' is unsigned, so wraps around happily as far as I can
see. Trival fix attached (compile tested only).

Signed-Off-By: Mika Kukkonen <mikukkon@iki.fi>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/write.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/afs/write.c b/fs/afs/write.c
index 67ae4db..28f3751 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -395,8 +395,9 @@ static int afs_write_back_from_locked_page(struct afs_writeback *wb,
 		if (n == 0)
 			goto no_more;
 		if (pages[0]->index != start) {
-			for (n--; n >= 0; n--)
-				put_page(pages[n]);
+			do {
+				put_page(pages[--n]);
+			} while (n > 0);
 			goto no_more;
 		}
 

-

From: David Howells
Date: Thursday, May 10, 2007 - 7:33 am

Fix a couple of problems with unlinking AFS files.

 (1) The parent directory wasn't being updated properly between unlink() and
     the following lookup().

     It seems that, for some reason, invalidate_remote_inode() wasn't
     discarding the directory contents correctly, so this patch calls
     invalidate_inode_pages2() instead on non-regular files.

 (2) afs_vnode_deleted_remotely() should handle vnodes that don't have a
     source server recorded without oopsing.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/file.c  |    2 +-
 fs/afs/inode.c |   10 +++++++---
 fs/afs/super.c |    3 ++-
 fs/afs/vnode.c |   33 +++++++++++++++++++++------------
 4 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/fs/afs/file.c b/fs/afs/file.c
index 3e25795..9c0e721 100644
--- a/fs/afs/file.c
+++ b/fs/afs/file.c
@@ -236,7 +236,7 @@ static void afs_invalidatepage(struct page *page, unsigned long offset)
 {
 	int ret = 1;
 
-	kenter("{%lu},%lu", page->index, offset);
+	_enter("{%lu},%lu", page->index, offset);
 
 	BUG_ON(!PageLocked(page));
 
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 515a5d1..47f5fed 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -209,11 +209,15 @@ bad_inode:
  */
 void afs_zap_data(struct afs_vnode *vnode)
 {
-	_enter("zap data {%x:%u}", vnode->fid.vid, vnode->fid.vnode);
+	_enter("{%x:%u}", vnode->fid.vid, vnode->fid.vnode);
 
 	/* nuke all the non-dirty pages that aren't locked, mapped or being
-	 * written back */
-	invalidate_remote_inode(&vnode->vfs_inode);
+	 * written back in a regular file and completely discard the pages in a
+	 * directory or symlink */
+	if (S_ISREG(vnode->vfs_inode.i_mode))
+		invalidate_remote_inode(&vnode->vfs_inode);
+	else
+		invalidate_inode_pages2(vnode->vfs_inode.i_mapping);
 }
 
 /*
diff --git a/fs/afs/super.c b/fs/afs/super.c
index d24be33..422f532 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -488,6 +488,7 @@ static struct inode *afs_alloc_inode(struct ...
From: Andrew Morton
Date: Thursday, May 10, 2007 - 4:19 pm

On Thu, 10 May 2007 15:33:34 +0100

gcc -W finds a number of fairly scary bugs.

More than one would expect, given that it is recommended in
Documentation/SubmitChecklist, which everyone reads ;)
-

From: David Howells
Date: Friday, May 11, 2007 - 2:49 am

Do you mean in my code specifically?  Or in the kernel in general?  As far as
I can tell -W only finds an eye-glazingly large quantity of 'unused parameter'
warnings in AFS and AF_RXRPC.

David
-

From: Andrew Morton
Date: Friday, May 11, 2007 - 2:58 am

Yes, it's a shame that there doesn't seem to be a fine-grained way of
turning on -W's useful bits.

-

From: David Howells
Date: Friday, May 11, 2007 - 3:03 am

You can turn off -W's undesirable bits.  For net/rxrpc/ and fs/afs/ at least,
adding:

	CFLAGS += -W -Wno-unused-parameter

to the Makefile generates no warnings.  Perhaps this should be added to the
master Makefile.

Adding -Wsign-compare finds some stuff that I will fix.  It also finds some
stuff in the main and the networking headers.  This is a really useful option
and found some tricky bugs in CacheFiles.  I would endorse adding this
generally too.

David
-

From: David Howells
Date: Friday, May 11, 2007 - 2:57 am

Which states incorrectly:

| 22: Newly-added code has been compiled with `gcc -W'.  This will generate
|     lots of noise, but is good for finding bugs like "warning: comparison
|     between signed and unsigned".

-W does not imply -Wsign-compare, at least not on my gcc.

David
-

Previous thread: Current git fails to compile on AMD64 for three days in a row by Florin Iucha on Thursday, May 10, 2007 - 8:16 am. (4 messages)

Next thread: Re: 2.6.21-git10/11: files getting truncated on xfs? or maybe an nlink problem? by Matt Mackall on Thursday, May 10, 2007 - 8:38 am. (5 messages)