nfs_refresh_inode: inode number mismatch

Hi,

[Previously sent to nfs@sourceforge with no response]

I'm using a frankenstein kernel, 2.4.21-rc3 with some -ac bits, and 2.5.69 NFS+RPC backported to it. Like the CITI kernel (for krb5), but a little more aggressive on the bits backported. For the purpose of this email, I think the code I have questions with is similar or even identical from 2.4.21->2.5.69. I can reproduce this problem on a RH 2.4.20-9smp kernel.

Consider these two shells running on the same machine:

1 2

cd /nfs cd /nfs
mkdir t
echo foo > t/foo
less t/foo
[less waits for input]
rm -rf t
'v'
[vi tries to access tmp/foo]

At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints the "inode number mismatch" error. AFAICT, this is just noise, but the noise is driving me crazy. :-)

Now, if sequence 2 is run on a different machine, there is no error! So that hints to me that the local cache just needs to be cleared, perhaps in nfs_rmdir() or maybe in nfs_unlink()/nfs_safe_remove(). I've tried a few things, but I'm not familiar enough with the code and am making slow progress. I can suppress this error by testing for 'unlinked but open' in __nfs_refresh_inode:

        if (NFS_FILEID(inode) != fattr->fileid) {
		if (inode->i_nlink)	/* quiet if inode DNE anymore */
			printk(...)
	}

Do you think this is safe? Some minimal logs:

kernel: NFS: dentry_delete(t/.nfs01c7d70600000001, 2) | renamed file
kernel: NFS: delete_inode(e/29873926) | unlink of renamed foo

kernel: NFS: refresh_inode(e/29873923 ct=1 info=0x6) | accessing t/
kernel: nfs_refresh_inode: inode number mismatch
kernel: expected (0xe/0x1c7d703), got (0xe/0xe63bc2)
kernel: NFS: dentry_delete(fsstress/t, 0)
kernel: NFS: delete_inode(e/29873923)

and then access calls beginning at the root. I apologize for the likely uselessness of the above logs. I can email some annotated logs if desired, but the problem is very easy to reproduce, so I'll hold off for now.

This problem only exists for nfsv3. This problem doesn't occur if there is a third process also holding foo open (note that the directory does get removed, just no kernel error when trying to access it).

The 2.2 kernel doesn't have this problem, because (apparently) it doesn't allow you to unlink a .nfsXXX file while it's open (and therefore you cannot remove the dir).

Which made me look around (2.5.69): In nfs_silly_rename(), the new dentry (sdentry) gets a d_count of 1. Doesn't this indicate that no one is holding this file open? (which then tells nfs_unlink() to just call nfs_safe_remove() rather than nfs_silly_rename()) Is that really desirable? Even if I set the d_count to match what the previous dentry->d_count had, and avoid calling dput(sdentry), on the next run through nfs_unlink() the d_count is 1 and it just goes to nfs_safe_remove(). I think that clearly, I don't understand what the d_move() is for. (My guess is to avoid nfs_async_unlink() getting passed a dentry which we are actually about to get rid of, but I haven't wrapped my head around the dcache yet.)

Then I noticed that the DCACHE_NFSFS_RENAMED seems a little racy. nfs_async_unlink() sets this and when the call completes, nfs_complete_unlink() resets it. So while it's being deleted, if an rm -rf quickly picks up the .nfs name before the async unlink returns, it won't get removed. But if the nfs call completes first, it does get removed. Is the intention just to prevent removal of the .nfs file until the old file is removed on the server? What's the benefit of this?

So, even with that error message quieted, fsstress reports lots of inode mismatches. I am in the process of trying to piece together a simple reproducible sequence of NFS calls.

This is against a netapp server, although I can't see how the server would matter.

Thanks for any advice, guidance, or hopefully fixes! BTW, I'm interested to hear what tools folks use to stress the NFS client.

/fc

> Hi, [Previously sent to nfs@sourceforge with no response]

> I'm using a frankenstein kernel, 2.4.21-rc3 with some -ac bits, > and 2.5.69 NFS+RPC backported to it. Like the CITI kernel (for > krb5), but a little more aggressive on the bits backported. > For the purpose of this email, I think the code I have > questions with is similar or even identical from
> 2.4.21->2.5.69. I can reproduce this problem on a RH > 2.4.20-9smp kernel.

> Consider these two shells running on the same machine:

> 1 2

> cd /nfs cd /nfs mkdir t echo foo > t/foo less t/foo
> [less waits for input]
> rm -rf t
> 'v'
> [vi tries to access tmp/foo]

> At this point, fs/nfs/inode.c:__nfs_refresh_inode() prints the > "inode number mismatch" error. AFAICT, this is just noise, but > the noise is driving me crazy. :-)

Inode number mismatch points to either an an obvious server error (it is not providing unique filehandles) or corruption of the fattr struct that was passed to nfs_refresh_inode().

Cheers,
Trond

Clearly it's not the former. No way a netapp filer is going to have this problem. I can't imagine *any* nfs server having this problem.

Could you take another look at the specific case I cited? At the time I try to access the file, the path to it no longer exists. No information on this file should exist.

/fc

> Could you take another look at the specific case I cited? At > the time I try to access the file, the path to it no longer > exists. No information on this file should exist.

I cannot duplicate.

Cheers,
Trond

There's a very common cause on embedded boards that don't have real-time clocks. Without a clock the client uses the same XID on every run, leading to lots of these messages. Is your clock broken?

- Adrian Cox
http://www.humboldt.co.uk/

BTDT.

If this is the case, you need to ensure that you don't reboot the client before the servers XID cache times out the XID numbers. For Linux knfsd, that's around 2 minutes.

Russell King ([email protected])                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

> If this is the case, you need to ensure that you don't reboot > the client before the servers XID cache times out the XID > numbers. For Linux knfsd, that's around 2 minutes.

Note that older versions of knfsd didn't time out their replay cache at all...

Cheers,
Trond

OK, I've studied this a lot more and think I've figured it out. I think calling it a race is incorrect, but I don't know how to succintly describe the problem otherwise.

Let's look at the first attachment, a test case which shows the problem. When run against a Solaris NFS server, the readdir()/unlink() loop returns ".", "..", "foo", NULL, and the rmdir fails. I suspect a Linux NFS server does the same. But when run against a netapp, it returns ".", "..", "foo", ".nfs...", NULL, ie it picks up the sillyrenamed file immediately, and this allows the rmdir to succeed.

When foo is unlinked, nfs_unlink() does a sillyrename, this puts the dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache. This causes a problem, because dentry->d_parent->d_inode is now guaranteed to remain stale. (OK, I'm not really sure about this last part.)

Then readdir() returns the new .nfs file, this creates a NEW dentry (we just moved the first one to nfs_delete_queue and did not create a negative dentry) which now has d_count==1 so instead of sillyrename we just remove it (but note, we actually have this file open). Then rmdir succeeeds.

Now, we have a dentry on nfs_delete_queue which a) has already been unlinked and b) whose dentry->d_parent DNE and dentry->d_parent->d_inode DNE. Of course this will cause confusion later!

Note that if a process does a drive by on the .nfs file (another round of unlinked-but-open) before we unlink it, we would sillyrename it again. We'd now have two different dentry's on the delete queue for the same file. (One of them would just leak, I think--possible local DoS?)

Also note that in vfs_unlink(), we do a d_delete() after the i_op->unlink(), I think this guarantees that no call to sillyrename will ever be passed a dentry with the DCACHE_NFSFS_RENAMED flag set (it gets set in sillyrename, but then unhashed; the next time through vfs_unlink() we have a new dentry with no flags set). The DCACHE_NFSFS_RENAMED bit looks to be a holdover from 2.2, where the dentry doesn't get unhashed.

I have 3 proposals for fixes, I've implemented 2 of them and patches are attached against both 2.4.21-rc7 and 2.5.70.

1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is responsible for doing a d_delete(), in 2.4 it happens in the VFS and I think it was just an oversight that the 2.4 VFS doesn't consider sillyrename (considering the code and comments that are cruft).

This preserves the unlinked-but-open semantic, but breaks rmdir. So it's not a clear winner from a semantics POV. dentry->d_count is always correct, which sounds like a plus.

The patch to make this work is utterly simple, which is a big plus.

2a) In nfs_unlink(), if not sillyrenaming, look for the file on
nfs_delete_queue. If present, remove it (since it's now extra).

This has the advantage of preserving the 'rm -rf' semantic, but breaks unlinked-but-open since the parent dir can go away, and dentry->d_count is not guaranteed to be correct.

I've implemented but not included this.

2b) Since this is really only a problem when the parent dir goes away,
do the same as above but only scan the queue in nfs_rmdir(), and mark any entries whose d_parent is "us".

I've included this in favor of (2a) because it's simpler and should give better performance in the common case.

3) In nfs_complete_unlink(), do a d_lookup() before waking the rpc task. If an entry is found, go head and schedule the rpc. If no entry is found, or a negative entry is found, just remove from the queue.

I couldn't get this to work.

With the #2 patch, I've taken the liberty of cleaning up stale comments. How annoying they were while trying to understand the code. :-( I would have done the same for the #1 patch, but it's so small that I didn't want to otherwise "pollute" it. If you decide to go with the #1 patch, at least do look at the #2 patch for comment fixups.

If you need more data to convince yourself of this bug, I'm definitely able to provide more data, just tell me what you'd like to see. But I hope I've adequately described it.

/fc

/*
* nfsbug.c
* exercises a bug in Linux 2.4/2.5 client NFS
* If the parent directory of an unlinked-but-open file goes away,
* kernel reports "inode mismatch".
*
* nfsbug <nfsdir>
*/

#include <stdio.h>
#include <string.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <dirent.h>
#include <unistd.h>

int
main(int argc, char *argv[])

{
    char path[2048];
    char file[2048];
    int fd;
    int v = 1, r;
    struct dirent *d;
    DIR *dir;

    if (argc != 2)
	exit(1);

    /* create a dir "t" under <nfsdir> */
    sprintf(path, "%s/%s", argv[1], "t");
    if (mkdir(path, 0777) < 0)
	exit(1);

    /* create a file "foo" under <nfsdir>/t */
    sprintf(file, "%s/%s", path, "foo");
    if ((fd = open(file, O_CREAT, 0666)) < 0)
	exit(1);

    /*
     * ok, we're now holding the file open, do an rm -rf.
     * We have to readdir() because unlinking <file> over NFS does a rename
     * (otherwise we could just unlink it and then do rmdir).
     * The readdir() returns foo, then the renamed file (which, when unlinked,
     * really gets unlinked, not renamed).
     */
    if ((dir = opendir(path)) == NULL)
	exit(1);
    while (d = readdir(dir)) {
	if (!strcmp(".", d->d_name) || !strcmp("..", d->d_name))
	    continue;
	sprintf(file, "%s/%s", path, d->d_name);
	r = unlink(file);
	if (v && r == 0)
	    printf("unlinked %s\n", file);
	else if (v && r < 0)
	    printf("failed to unlink %s\n", file);
    }
    if (rmdir(path) < 0)
	printf("failed to rmdir %s\n", path);
    else
	printf("rmdir-ed %s\n", path);

    /* exit, which tries to unlink the .nfs file again */
    printf("exit: ");
    fgets((char *)&r, 2, stdin);

    exit(0);
}

@@ -1482,13 +1482,14 @@ int vfs_unlink(struct inode *dir, struct
 				lock_kernel();
 				error = dir->i_op->unlink(dir, dentry);
 				unlock_kernel();
-				if (!error)
+				if (!error &&
+				    !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 					d_delete(dentry);
 			}
 		}
 	}
 	up(&dir->i_zombie);
-	if (!error)
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 		inode_dir_notify(dir, DN_DELETE);
 	return error;
 }

@@ -1631,7 +1631,9 @@ int vfs_unlink(struct inode *dir, struct
 			error = dir->i_op->unlink(dir, dentry);
 	}
 	up(&dentry->d_inode->i_sem);
-	if (!error) {
+
+	/* We don't d_delete() NFS sillyrenamed files--they still exist. */
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED)) {
 		d_delete(dentry);
 		inode_dir_notify(dir, DN_DELETE);
 	}

diff -uNrp linux-2.4.21-rc7/fs/nfs/dir.c linux-2.4.21-rc7-silly/fs/nfs/dir.c
@@ -738,8 +738,10 @@ static int nfs_rmdir(struct inode *dir, 
 
 	nfs_zap_caches(dir);
 	error = NFS_PROTO(dir)->rmdir(dir, &dentry->d_name);
-	if (!error)
+	if (!error) {
+		nfs_async_unlink_dequeue(dentry);
 		dentry->d_inode->i_nlink = 0;
+	}
 
 	return error;
 }

@@ -772,8 +774,14 @@ dentry->d_parent->d_name.name, dentry->d
 	 * We don't allow a dentry to be silly-renamed twice.
 	 */
 	error = -EBUSY;
-	if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
+	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
+#ifdef NFS_PARANOIA
+		printk("NFS: silly-rename %s/%s with DCACHE_NFSFS_RENAMED "
+		       "already set?\n",
+		       dentry->d_parent->d_name.name, dentry->d_name.name);
+#endif
 		goto out;
+	}
 
 	sprintf(silly, ".nfs%*.*lx",
 		i_inosize, i_inosize, dentry->d_inode->i_ino);

diff -uNrp linux-2.4.21-rc7/include/linux/nfs_fs.h linux-2.4.21-rc7-silly/include/linux/nfs_fs.h
@@ -198,6 +198,7 @@ extern int nfs_lock(struct file *, int, 
  */
 extern int  nfs_async_unlink(struct dentry *);
 extern void nfs_complete_unlink(struct dentry *);
+extern void nfs_async_unlink_dequeue(struct dentry *);
 
 /*
  * linux/fs/nfs/write.c

diff -uNrp linux-2.5.70/fs/nfs/dir.c linux-2.5.70-silly/fs/nfs/dir.c
@@ -894,8 +894,10 @@ static int nfs_rmdir(struct inode *dir, 
 	lock_kernel();
 	nfs_zap_caches(dir);
 	error = NFS_PROTO(dir)->rmdir(dir, &dentry->d_name);
-	if (!error)
+	if (!error) {
+		nfs_async_unlink_dequeue(dentry);
 		dentry->d_inode->i_nlink = 0;
+	}
 	unlock_kernel();
 
 	return error;

@@ -925,8 +927,14 @@ dentry->d_parent->d_name.name, dentry->d
 	 * We don't allow a dentry to be silly-renamed twice.
 	 */
 	error = -EBUSY;
-	if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
+	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
+#ifdef NFS_PARANOIA
+		printk("NFS: silly-rename %s/%s with DCACHE_NFSFS_RENAMED "
+		       "already set?\n",
+		       dentry->d_parent->d_name.name, dentry->d_name.name);
+#endif
 		goto out;
+	}
 
 	sprintf(silly, ".nfs%*.*lx",
 		i_inosize, i_inosize, dentry->d_inode->i_ino);

@@ -959,7 +967,6 @@ dentry->d_parent->d_name.name, dentry->d
 		nfs_renew_times(dentry);
 		d_move(dentry, sdentry);
 		error = nfs_async_unlink(dentry);
- 		/* If we return 0 we don't unlink */
 	}
 	dput(sdentry);
 out:

@@ -1000,10 +1007,13 @@ out:
 	return error;
 }
 
-/*  We do silly rename. In case sillyrename() returns -EBUSY, the inode
- *  belongs to an active ".nfs..." file and we return -EBUSY.
+/*
+ * We do silly rename if the dentry says there are active users.
+ * Otherwise we unlink.
  *
- *  If sillyrename() returns 0, we do nothing, otherwise we unlink.
+ * Note that the dentry->d_count may lie; once a file is sillyrenamed
+ * it is unhashed and so a future lookup repopulates the dcache and the
+ * new dentry doesn't reflect the previous dentry->d_count.
  */
 static int nfs_unlink(struct inode *dir, struct dentry *dentry)
 {

diff -uNrp linux-2.5.70/include/linux/nfs_fs.h linux-2.5.70-silly/include/linux/nfs_fs.h
@@ -294,6 +294,7 @@ extern int nfs_lock(struct file *, int, 
  */
 extern int  nfs_async_unlink(struct dentry *);
 extern void nfs_complete_unlink(struct dentry *);
+extern void nfs_async_unlink_dequeue(struct dentry *);
 
 /*
  * linux/fs/nfs/write.c

Forgot to mention, this affects NFSv3 only. Maybe because NFSv2 doesn't have the cookie verifier and so readdir() always restarts? Something like that.

/fc

Good job.

I think your #1 is "obviously correct", but the fact that it breaks rmdir sounds like a bummer. However, since it only breaks rmdir when silly-renames exist - and since silly-renames should only happen when you have a file descriptor still open - I'd be inclined to say that this is the right behaviour.

This sounds like a hack, even if it happens to work.

I dunno. I'm personally inclined to prefer (1), since that seems to just fix a bug in the VFS layer and doesn't introduce any conceptual complexity. But I think I'd let Trond make the final decision, I don't hate (2b) enough to say "over my dead body!".

Trond?

Linus

> I think your #1 is "obviously correct", but the fact that it > breaks rmdir sounds like a bummer. However, since it only > breaks rmdir when silly-renames exist - and since silly-renames > should only happen when you have a file descriptor still open - > I'd be inclined to say that this is the right behaviour.

I agree.

If people prefer 'rm -rf' correctness instead of unlinked-but-open, then we could do that by changing the behaviour of 'unlink' on a silly-deleted filed. Currently it returns EBUSY, but we could just as well have it complete the unlink, and mark the inode as being stale...

Cheers,
Trond

Actually, *currently* it unlinks. :-) That's the problem.

/fc

> On Mon, Jun 09, 2003 at 06:40:43PM +0200, Trond Myklebust > wrote:
>> If people prefer 'rm -rf' correctness instead of >> unlinked-but-open, then we could do that by changing the >> behaviour of 'unlink' on a silly-deleted filed. Currently it >> returns EBUSY, but we could just as well have it complete the >> unlink, and mark the inode as being stale...

> Actually, *currently* it unlinks. :-) That's the problem.

No. The problem is that it aliases the dentry, and so it unlinks incorrectly...

Cheers,
Trond

????

What does hashed state have to ->d_parent?

b) is bogus. Unhashing does nothing to ->d_parent.

Two different dentries for the same file is obviously not a problem...

... and AFAICS it opens a huge can of worms with races in NFS unlink/rename.

Sigh... I'll look through that code and try to reconstruct the analysis. It used to be a very big mess and there was fairly subtle logics around unhashing/checks for d_count/etc. involved in fixing ;-/ And there was a lot of changes since then. Oh, well...

Because now d_parent can be d_deleted (no children) and then go away. Then won't dentry->d_parent be wrong? What happens if d_parent becomes a negative d_entry v. disappearing entirely.

Like I said, I didn't study this part well enough to be sure even what I'm talking about, much less the conclusion. :-)

Except that d_parent can now disappear, right? Because it no longer knows it has children. Or is that wrong.

It is if d_count goes to 0 on one of them and the inode is then unlinked. But the other dentry remains and again tries to unlink when its d_count goes to 0. Over NFS the fh includes the generation and so you can't accidentally delete what only looks like the same file, but what happens in the local fs?

Also, the real problem is that something goes wrong with d_parent and NFS tries to refresh an inode that it should really know doesn't exist anymore. That's why my #2 patch only executes during a rmdir.

I may not have 100% accurately captured the problem, but the effect is easy to see.

As for the last sentence about the DoS, I can trivially construct a program which just keeps creating dentries on the nfs_delete_queue that will never go away. Because the original dentry gets unhashed and when the file is looked up again it doesn't have the flags, so it gets sillyrenamed again... and again ... and again. The nfs_delete_queue processing stops at the first hit.

AFAICS it solves at least one of them. I actually prefer this #1 patch to my #2 patch, I just posted the other for completeness. It's obviously correct to leave the d_entry alone, since you did not actually remove it from the directory. And d_count and d_flags accurately represents the state, and nfs_unlink does treat that correctly. I'm new to the kernel, so please educate me where that is wrong.

That would be very cool if you do that. There are other problems besides the one I described, I haven't figured them out enough to post yet. But I don't see why you wouldn't think the one liner above isn't correct.

/fc

Why? It still has our dentry refering to it and contributing into its ->d_count.

->d_parent will not become negative.

Please, take a look at the way normal links work.

Now, _that_ is interesting. What are you actually seeing there? Note that unhashing doesn't change _any_ ->d_count - hash chains are not counted in it and ->d_parent is not changed.

ahh, ok, thanks. (Thanks also to Trond for nudging me in the correct direction!) I will look into that some more, then.

But anyway you do agree that the one line fix is correct? You may have gotten thrown off track because of my somewhat broken description.

/fc

As far as I can see, it doesn't solve the real problem. E.g. it allows you to create all sorts of fun for async-unlink code in NFS - the thing pretty much assumes that victim dentry is not going anywhere.

> On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
>> When foo is unlinked, nfs_unlink() does a sillyrename, this >> puts the dentry on nfs_delete_queue, and (in the VFS) unhashes >> it from the dcache. This causes a problem, because >> dentry->d_parent->d_inode is now guaranteed to remain stale. >> (OK, I'm not really sure about this last part.)

> ????

Al,

AFAICS the problem is the following:

- NFS sillyrenames dentry 1
- Upon return from nfs_unlink(), VFS unhashes dentry 1

- Upon next lookup, VFS+NFS conspire to create aliased dentry 2 to sillyrenamed file
- Upon last close of files associated with dentry 1, NFS completes sillyrename. File is unlinked on server.
- Aliased dentry 2 is still around, but it is now pointing to stale

    fh.

IOW we just want to prevent VFS from unhashing the dentry in the first
place: dentry aliasing cannot work together with sillyrename.

Cheers,
Trond

Aliasing could be dealt with. They would have the same inode, so it's easy to detect. The real problem is different: what happens if I take silly-renamed file and rename it away? You suddenly get ->dir and ->dentry if your nfs_unlinkdata having nothing to do with each other.

_If_ we want to be able to work with silly-renamed dentry, we need much more careful async unlink. Your current code assumes that these dentries
won't go anywhere. AFAICS, dcache will not get into inconsistent state,
but it will have very little to do with state of server...

dentry only contains the inode, not the fh. On the server, the inode can go away and come back as a new fh, but with the same inode. Is that detectable (would comparison hooks have to be added?)? Although, I guess the inode is enough; you can do an NFS_I(inode)->fh to get the fh, but I wouldn't guess you'd want that in the VFS. Bah, here I go again ... forgive me if that's nonsense.

You could disallow rename if DCACHE_NFSFS_RENAMED is set. That would be easy and makes sense as an "ok" thing to do. I mean, if you're not going to allow unlinking of a sillyrenamed file, you may as well disallow rename as well.

If that's not desirable (again, seems ok to me ... speaking as just a user) then hey, in rename you just need to check the nfs_delete_queue.

OK, where else besides rename would the dentry change?

/fc

Wow, it's clear to me now :-) that this is another place I'm seeing NFS problems.

...

I can answer this myself: link. (Is that correct?) Anywhere else?

/fc

You wouldn't need to do that anywhere near VFS - all relevant code is inside NFS and yes, there we _are_ allowed to look at ->fh ;-)

FWIW, we could probably simply do the following: have nfs_lookup() return ERR_PTR(-EINVAL) if it notices that it's about to give us such alias. IOW, no access to such guys at all - if it's going to die, we refuse to do anything with it. I'll try to do that variant when I get some sleep - I'd rather not mess with anything in that area until I'm completely awake...

Sounds ok to me, except that Linus says

so anyway, please find attached a 2.4.21-rc7 and 2.5.70 patch which prevents removal or rename of unlinked-but-open files. You can see the rename bug by doing something like

mkdir d1 d2
hold a file open in d1 and rm it; it gets sillyrenamed move sillyrenamed file to d2
rmdir d1
close file => "inode number mismatch" (data->dir isn't "live", ie it doesn't follow the rename, and d1 is gone)

/fc

@@ -1482,13 +1482,14 @@ int vfs_unlink(struct inode *dir, struct
 				lock_kernel();
 				error = dir->i_op->unlink(dir, dentry);
 				unlock_kernel();
-				if (!error)
+				if (!error &&
+				    !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 					d_delete(dentry);
 			}
 		}
 	}
 	up(&dir->i_zombie);
-	if (!error)
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 		inode_dir_notify(dir, DN_DELETE);
 	return error;
 }

@@ -1830,6 +1831,10 @@ int vfs_rename(struct inode *old_dir, st
 	       struct inode *new_dir, struct dentry *new_dentry)
 {
 	int error;
+
+	if (old_dentry->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
+
 	if (S_ISDIR(old_dentry->d_inode->i_mode))
 		error = vfs_rename_dir(old_dir,old_dentry,new_dir,new_dentry);
 	else

@@ -1631,7 +1631,9 @@ int vfs_unlink(struct inode *dir, struct
 			error = dir->i_op->unlink(dir, dentry);
 	}
 	up(&dentry->d_inode->i_sem);
-	if (!error) {
+
+	/* We don't d_delete() NFS sillyrenamed files--they still exist. */
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED)) {
 		d_delete(dentry);
 		inode_dir_notify(dir, DN_DELETE);
 	}

@@ -1949,6 +1951,10 @@ int vfs_rename(struct inode *old_dir, st
 
 	if (old_dentry->d_inode == new_dentry->d_inode)
  		return 0;
+
+	/* Don't allow sillyrenamed files to move; messes up async_unlink */
+	if (old_dentry->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
  
 	error = may_delete(old_dir, old_dentry, is_dir);
 	if (error)

Minor adjustment. It's functionally the same but prevents a spurious printk (fs/nfs/dir.c:1199 in 2.5.70).

Works for me.

/fc

@@ -1482,13 +1482,14 @@ int vfs_unlink(struct inode *dir, struct
 				lock_kernel();
 				error = dir->i_op->unlink(dir, dentry);
 				unlock_kernel();
-				if (!error)
+				if (!error &&
+				    !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 					d_delete(dentry);
 			}
 		}
 	}
 	up(&dir->i_zombie);
-	if (!error)
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED))
 		inode_dir_notify(dir, DN_DELETE);
 	return error;
 }

@@ -1830,6 +1831,11 @@ int vfs_rename(struct inode *old_dir, st
 	       struct inode *new_dir, struct dentry *new_dentry)
 {
 	int error;
+
+	if (old_dentry->d_flags & DCACHE_NFSFS_RENAMED ||
+	    new_dentry->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
+
 	if (S_ISDIR(old_dentry->d_inode->i_mode))
 		error = vfs_rename_dir(old_dir,old_dentry,new_dir,new_dentry);
 	else

@@ -1631,7 +1631,9 @@ int vfs_unlink(struct inode *dir, struct
 			error = dir->i_op->unlink(dir, dentry);
 	}
 	up(&dentry->d_inode->i_sem);
-	if (!error) {
+
+	/* We don't d_delete() NFS sillyrenamed files--they still exist. */
+	if (!error && !(dentry->d_flags & DCACHE_NFSFS_RENAMED)) {
 		d_delete(dentry);
 		inode_dir_notify(dir, DN_DELETE);
 	}

@@ -1949,6 +1951,11 @@ int vfs_rename(struct inode *old_dir, st
 
 	if (old_dentry->d_inode == new_dentry->d_inode)
  		return 0;
+
+	/* Don't allow sillyrenamed files to move; messes up async_unlink */
+	if (old_dentry->d_flags & DCACHE_NFSFS_RENAMED ||
+	    new_dentry->d_flags & DCACHE_NFSFS_RENAMED)
+		return -EBUSY;
  
 	error = may_delete(old_dir, old_dentry, is_dir);
 	if (error)

> Aliasing could be dealt with. They would have the same inode, > so it's easy to detect.

I suppose we could... Is the procedure that nfs_lookup() should first rehash the dropped dentry, then return it instead of NULL?

> The real problem is different: what happens if I take > silly-renamed file and rename it away? You suddenly get ->dir > and ->dentry if your nfs_unlinkdata having nothing to do with > each other.

->dir is the important one here, as it provides the filehandle. We only use ->dentry in order to give us a name/string for the NFSPROC_REMOVE call.

> AFAICS, dcache will not get into inconsistent state, but it > will have very little to do with state of server...

But that's the best we can do in any scenario. NFS does not ever give you a guarantee that someone won't screw things up on the server, nor does it give you any failsafe mechanism for detecting it. That's why operation atomicity is such an issue with the current kernel, and is why I'm so eager to push the intent patches into 2.6.

Sure sillyrename fails miserably in the atomicity department. There's bugger all we can do about that: if you are suggesting we should rely on some sort of revalidation before we unlink, then that's just as race prone as what we have now.

Cheers,
Trond

It _is_ a problem. It does the wrong thing on any subsequent directory operation (move or unlink).

Multiple aliased dentries have never been ok, unless the filesystem explicitly handles them and invalidates them (ie ntfs/fat kind of things).

Linus

Amusingly, we hit _exactly_ this same problem a couple of days ago in Lustre because we were trying to use RW locks instead of the single directory i_sem to improve large (10M files) directory lookup performance, and it is hard to hit and even harder to detect.

We've gone back to using i_sem to protect the actual dentry instantiation in lookup for now (we have a separate DLM for doing RW locking of the directory for create/rename/unlink), but will be looking at ways to allow this in the filesystem again at some time in the future. Allowing RW locking for local filesystem lookups would probably also be a win.

My 5-minute theory to fix this is to re-check the inode dentry alias list when you are doing d_instantiate() (with dcache lock held) for another dentry with the same hash (and further the same name if the hashes match), but that would behave poorly when there are large numbers of links to a single file. It _might_ be safe to check the first N entries on the i_dentry list, where N ~= num_cpus, assuming that we could only have N racy dentry lookups going at one time. Even so, the boost of allowing multiple parallel lookups for huge directories probably beats out the extra searching slowdown for the uncommon many-links-to-inode case.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

For vfat at least its all broken.

cd foo
mv ../file .
more file

ESTALE.

Looks like a different issue, not dentry aliasing per se.

Yes, VFAT ends up encoding the parent directory in the FH, so renaming will invalidate the old file handle, and if you cache inodes (and thus filehandles) over a directory move, badness happens.

Arguably it's a NFS client problem - the path revalidate at open time should have caught the ESTALE and forced a new inode lookup. But I think you can also argue that VFAT over NFS is just non-unixy enough that it just isn't really "supported".

I think it's more of a "you can NFS-export strange filesystems for some limited file sharing, but if things break, you get to keep both pieces".

Linus

No no - this happens on LOCAL disk. No NFS needed at all.

How do you make a local filesystem return ESTALE? A quick grep shows it in fs/fat/inode.c, but it should only be in the "export" functions used to export it for NFS.

Curious. What am I missing?

Linus

fs/vfat - d_revalidate: vfat_revalidate

That still shouldn't cause ESTALE, it should just force a dropping of the dentry, and a re-lookup (and that, in turn, should either get the right thing, or should return ENOENT).

Or are you talking about 2.4.x and that is doing something strange these days?

[ You have entered the twilight zone: "Tee-dee tee-dee.." ]

Linus

.... and vfat lookup will pick what can be picked. We do not get hashed aliases there.

I've seen it on early 2.5 and on 2.4, current 2.5.x seems to be ok from a quick test.

> I've seen it on early 2.5 and on 2.4, current 2.5.x seems to be > ok from a quick test.

2.4 has the 'return ESTALE if current dir fails d_revalidate()' test. Looks like the vfat stuff has the same problem that

Sigh... Can we perhaps add FS_ALWAYS_REVAL in order to flag what kind of behaviour filesystems expect in link_path_walk()? NFS needs revalidation on all open() calls (including opendir(".")), so removing the ESTALE code is not an option.

Cheers,
Trond

> 2.4 has the 'return ESTALE if current dir fails d_revalidate()' > test. Looks like the vfat stuff has the same problem that

I should learn to complete my own sentences before sending... The above should read:

Looks like the vfat stuff has the same problem that Coda did. It is unintentionally triggering the ESTALE code, as it assumes that d_revalidate() is advisory only.

Cheers,
Trond

Coda still has the problem with 2.4. The only thing I have been telling people that hit the problem is to take the revalidate patch out.

btw. The sheer number of problem cases is already reduced significantly by the following patch which avoids calling revalidate on every name that happens to start with a '.'.

Jan

diff -urN --exclude-from=dontdiff linux-2.4.21-rc2/fs/namei.c linux-2.4.21-rc2-coda/fs/namei.c
@@ -627,6 +627,8 @@
 			nd->last_type = LAST_DOT;
 		else if (this.len == 2 && this.name[1] == '.')
 			nd->last_type = LAST_DOTDOT;
+		else
+			goto return_base;
 return_reval:
 		/*
 		 * We bypassed the ordinary revalidation routines.

     > On Wed, Jun 11, 2003 at 08:26:52PM +0200, Trond Myklebust
     > wrote:
    >> diff -u --recursive --new-file linux-2.4.21-rc6/fs/namei.c
    >> linux/fs/namei.c
    >> --- linux-2.4.21-rc6/fs/namei.c 2002-12-30 09:39:54.000000000
    >>     -0800
    >> +++ linux/fs/namei.c 2003-06-11 11:16:49.000000000 -0700
    >> @@ -633,7 +633,8 @@
    >> * Check the cached dentry for staleness.
    >> */ dentry = nd->dentry;
    >> - if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {

     > Doesn't the above simply always revalidate?

That's the whole problem in a nutshell. Read the thread...

Cheers,
Trond

Oh, I thought you were fixing an NFS problem with that patch.

/fc