dcache: return -ESTALE not -EBUSY on distributed fs race

From: "J. Bruce Fields" <[email protected]>

On a distributed filesystem it's possible for lookup to discover that a directory it just found is already cached elsewhere in the directory heirarchy. The dcache won't let us keep the directory in both places, so we have to move the dentry to the new location from the place we previously had it cached.

If the parent has changed, then this requires all the same locks as we'd need to do a cross-directory rename. But we're already in lookup holding one parent's i_mutex, so it's too late to acquire those locks in the right order.

The (unreliable) solution in __d_unalias is to trylock() the required locks and return -EBUSY if it fails.

I see no particular reason for returning -EBUSY, and -ESTALE is already the result of some other lookup races on NFS. I think -ESTALE is the more helpful error return. It also allows us to take advantage of the logic Jeff Layton added in c6a9428401c0 "vfs: fix renameat to retry on ESTALE errors" and ancestors, which hopefully resolves some of these errors before they're returned to userspace.

I can reproduce these cases using NFS with:

ssh root@$client '
mount -olookupcache=pos '$server':'$export' /mnt/ mkdir /mnt/TO
mkdir /mnt/DIR
touch /mnt/DIR/test.txt
while true; do
strace -e open cat /mnt/DIR/test.txt 2>&1 | grep EBUSY
done
'
ssh root@$server '
while true; do
mv $export/DIR $export/TO/DIR
mv $export/TO/DIR $export/DIR
done
'

It also helps to add some other concurrent use of the directory on the client (e.g., "ls /mnt/TO"). And you can replace the server-side mv's by client-side mv's that are repeatedly killed. (If the client is interrupted while waiting for the RENAME response then it's left with a dentry that has to go under one parent or the other, but it doesn't yet know which.)

---
 fs/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5bc72b07fde2..b7de7f1ae38f 100644
@@ -2612,7 +2612,7 @@ static struct dentry *__d_unalias(struct inode *inode,
 		struct dentry *dentry, struct dentry *alias)
 {
 	struct mutex *m1 = NULL, *m2 = NULL;
-	struct dentry *ret = ERR_PTR(-EBUSY);
+	struct dentry *ret = ERR_PTR(-ESTALE);
 
 	/* If alias and dentry share a parent, then no extra locks required */
 	if (alias->d_parent == dentry->d_parent)

1.9.3

I keep looking back at this problem trying to think of a better idea. Most recently I remember Jeff's work to retry ESTALE's and wondered if we could take advantage of that.

It's still kind of a bandaid, but it's the only thing I've thought of that at least helps a little and isn't a huge pain. Any other ideas?

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

"J. Bruce Fields":

How about introducing a tiny inline function which returns either EBUSY or ESTALE?

static inline int busy_or_stale()

{
	if (!test_distributed_fs())
		return -EBUSY;
	return -ESTALE;
}

For nfs, the test function will be something like this.

int test_nfsd()

{
	int ret;
	struct task_struct *tsk = current;
	char comm[sizeof(tsk->comm)];

	ret = 0;
	if (tsk->flags & PF_KTHREAD) {
		get_task_comm(comm, tsk);
		ret = !strcmp(comm, "nfsd");
	}

	return ret;
}

I know this "by-name" test is not good.

J. R. Okajima

Why do you think -EBUSY's the right error in the local filesystem case?

This condition really shouldn't happen in the local filesystem case anyway. If anything maybe it should be -EIO, since it looks like a sign your filesystem's corrupted.

Also you'd want to be checking for nfs, not nfsd.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

"J. Bruce Fields":

This busy_or_stale() is another bandaid, based upon your patch, EBUSY --> ESTALE.
Because the msg string of ESTALE is "Stale NFS file handle" on many systems, I don't think it a good idea to return it for local fs. If you think EIO is better than EBUSY you can change it to eio_or_stale(). If you think it is surely not happen on every local fs, then this inline function is not necessary.

J. R. Okajima
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I think the only way this could happen on a local fs would be for it to allow hardlinked directories, which is (of course) forbidden. After all, the comment above __d_unalias specifically mentions that it's there to handle remotely renamed directories.

I think the patch is almost certainly fine for NFS. The only question I'd have is whether other distributed filesystems (p9? ceph? gfs2?) might have an issue here. It seems unlikely though, and most of them would also benefit from redoing the lookup with LOOKUP_REVAL set in this situation.

Still, it's probably worth letting this soak in -next for a bit to be sure we're not missing anything.

Bruce, you can add my:

We use file handles on local filesystems. They have exactly the same semantics as NFS file handles and so local filesystems have the same ESTALE exposure as "distributed" filesystems to this problem. i.e:

$ man open_by_handle_at
.....
	ESTALE	The specified handle is not valid.  This error will
		occur if, for example, the file has been deleted.

We've been doing this on XFS since before it was ported to linux 15 years ago... ;)

Cheers,

Dave.

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner:

It is reasonable.
But do you think ESTALE is good in case of __d_unalias()?

J. R. Okajima

I don't think you'll ever see __d_unalias called on a local fs, as the __d_find_any_alias call should always return NULL on a local fs.

With a local fs, we always know that the dcache is correct (which is why local fs' generally don't supply a d_revalidate method). The only way to get to __d_unalias is for all of the following to be true:

- you're splicing a new dentry into the tree
- that new dentry is a directory
- the inode for it already has a dentry associated with it that's in a different place

...that can only happen in a remote filesystem. The only way for that to occur on a local fs is for that fs to allow a directory to be hardlinked, which should of course never happen.

As to whether ESTALE is as good as EBUSY there...I doubt it'll matter much. Remote filesystems generally have to be able to cope with ESTALE errors anyway, and I doubt a lot of userland code handles EBUSY especially for this situation.

Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton:

Ok, I understand.
Thank for explanation.

J. R. Okajima