tmpfs: incr. link-count on directory rename

Hi Christoph, hi all!

When I move a directory into another on a tmpfs filesystem, the link-count of the new parent directory isn't getting incremented. (which leads to find getting hickup, which eg. lets dpkg produce a "bzip2" binary package without binaries, generally not a nice thing)

test:

isax@home:/tmp/test$ mkdir t1 
isax@home:/tmp/test$ mkdir t2 
isax@home:/tmp/test$ mv t2 t1 
isax@home:/tmp/test$ ls -lRa 
.: 
total 0 
drwxr-xr-x    5 isax     isax            0 Feb 13 23:05 . 
drwxrwxrwt   13 root     root            0 Feb 13 23:04 .. 
drwxr-xr-x    2 isax     isax            0 Feb 13 23:05 t1 

./t1:
total 0

drwxr-xr-x    2 isax     isax            0 Feb 13 23:05 . 
drwxr-xr-x    5 isax     isax            0 Feb 13 23:05 .. 
drwxr-xr-x    2 isax     isax            0 Feb 13 23:05 t2 

./t1/t2:
total 0

drwxr-xr-x    2 isax     isax            0 Feb 13 23:05 . 
drwxr-xr-x    2 isax     isax            0 Feb 13 23:05 .. 

the link count of "t1", "t1/." and "t1/t2/.." should be "3", not "2". The following patch seems to work fine for me and is tested on my machine running debian's 2.4.17-1 and user-mode-linux 2.5.1-1. This is my first patch to the kernel, so I suppose there is a really huge mistake in my patch I don't see now...

@@ -1085,6 +1085,9 @@
 {
        int error = -ENOTEMPTY;
 
+       if (S_ISDIR(old_dentry->d_inode->i_mode)) {
+               new_dir->i_nlink++;
+       }
        if (shmem_empty(new_dentry)) {
                struct inode *inode = new_dentry->d_inode;
                if (inode) {

uli martens

Yeah, that optimization actually breaks on so many filesystems (AFS/Coda/iso9660/msdos/any filesystem which contains more than 65538 subdirectories in a directory/you name it) that they even provided a flag to disable it (-noleaf), which in my opinion should always be the default setting.

Another trick on the 'filesystem side' of things is to set the directory linkcount to 1. Basically find always subtracts 2 and then as long as the count is not zero, it will stat the entry to see if it's a directory. So a directory linkcount of 1 will 'underflow' and find will work correctly for the first 65535 sub-directories.

It's been on my todo list to use this fix in Coda as we have some link count issues when we're flipping entries between 'faked' symlinks for mountpoints/conflicts and real directories.

Jan

@@ -1100,6 +1100,10 @@
 		error = 0;
 		old_dentry->d_inode->i_ctime = old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME;
 	}
+	if (!error && S_ISDIR(old_dentry->d_inode->i_mode)) {
+	    old_dir->i_nlink--;
+	    new_dir->i_nlink++;
+	}
 	return error;
 }
 

Oops, you're right, the linkcount of old_dir isn't decremented at the moment, too. I will test your patch this evening, but I think it looks better than mine... ;)

uli martens

Hi Uli,

I prefer this one.

Linus, Marcelo, please apply.

The patch fixes the refcounting of directories on renames in tmpfs.

Greetings
Christoph

diff -uNr 18-rc1/mm/shmem.c c/mm/shmem.c
@@ -1083,19 +1083,25 @@
  */
 static int shmem_rename(struct inode * old_dir, struct dentry *old_dentry, struct inode * new_dir,struct dentry *new_dentry)
 {
-	int error = -ENOTEMPTY;
+	struct inode *inode;
 
-	if (shmem_empty(new_dentry)) {
-		struct inode *inode = new_dentry->d_inode;
-		if (inode) {
-			inode->i_ctime = CURRENT_TIME;
-			inode->i_nlink--;
-			dput(new_dentry);
-		}
-		error = 0;
-		old_dentry->d_inode->i_ctime = old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME;
+	if (!shmem_empty(new_dentry)) 
+		return -ENOTEMPTY;
+
+	inode = new_dentry->d_inode;
+	if (inode) {
+		inode->i_ctime = CURRENT_TIME;
+		inode->i_nlink--;
+		dput(new_dentry);
+	}
+	inode = old_dentry->d_inode;
+	if (S_ISDIR(inode->i_mode)) {
+		old_dir->i_nlink--;
+		new_dir->i_nlink++;
 	}
-	return error;
+
+	inode->i_ctime = old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME;
+	return 0;
 }
 
 static int shmem_symlink(struct inode * dir, struct dentry *dentry, const char * symname)