From: Steve Dickson <SteveD@redhat.com> Subject: [RHEL5.1] [PATCH] NFS hang due to multiple dentries pointing to same directory inode Date: Mon, 16 Apr 2007 12:04:57 -0400 Bugzilla: 208862 Message-Id: <46239EA9.5020006@RedHat.com> Changelog: [nfs] fix multiple dentries pointing to same directory inode This upstream patch detects dentry aliases (i.e. an dentry that is already instantiated but under a different name) and tries to reused the alised dentry. The patch stops the following command sequence from causing deadlock on Client1. Client1: mkdir foo foo/bar foo/baz cd foo/bar Client2: mv foo/bar . && mv foo bar Client1: mv ../baz foo/bag The bz is: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=208862 steved. commit 9eaef27b36a6b716384948da94b8fc5bfba7b712 Author: Trond Myklebust <Trond.Myklebust@netapp.com> [PATCH] VFS: Make d_materialise_unique() enforce directory uniqueness If the caller tries to instantiate a directory using an inode that already has a dentry alias, then we attempt to rename the existing dentry instead of instantiating a new one. Fail with an ELOOP error if the rename would affect one of our parent directories. This behaviour is needed in order to avoid issues such as http://bugzilla.kernel.org/show_bug.cgi?id=7178 Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> Cc: Miklos Szeredi <miklos@szeredi.hu> Cc: Maneesh Soni <maneesh@in.ibm.com> Cc: Dipankar Sarma <dipankar@in.ibm.com> Cc: Neil Brown <neilb@cse.unsw.edu.au> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org> --- linux-2.6.18.i686/fs/dcache.c.orig 2007-04-13 08:31:46.000000000 -0700 +++ linux-2.6.18.i686/fs/dcache.c 2007-04-16 07:42:36.000000000 -0700 @@ -1472,23 +1472,21 @@ static void switch_names(struct dentry * * deleted it. */ -/** - * d_move - move a dentry +/* + * d_move_locked - move a dentry * @dentry: entry to move * @target: new dentry * * Update the dcache to reflect the move of a file name. Negative * dcache entries should not be moved in this way. */ - -void d_move(struct dentry * dentry, struct dentry * target) +static void d_move_locked(struct dentry * dentry, struct dentry * target) { struct hlist_head *list; if (!dentry->d_inode) printk(KERN_WARNING "VFS: moving negative dcache entry\n"); - spin_lock(&dcache_lock); write_seqlock(&rename_lock); /* * XXXX: do we really need to take target->d_lock? @@ -1549,7 +1547,81 @@ already_unhashed: fsnotify_d_move(dentry); spin_unlock(&dentry->d_lock); write_sequnlock(&rename_lock); +} + +/** + * d_move - move a dentry + * @dentry: entry to move + * @target: new dentry + * + * Update the dcache to reflect the move of a file name. Negative + * dcache entries should not be moved in this way. + */ + +void d_move(struct dentry * dentry, struct dentry * target) +{ + spin_lock(&dcache_lock); + d_move_locked(dentry, target); + spin_unlock(&dcache_lock); +} + +/* + * Helper that returns 1 if p1 is a parent of p2, else 0 + */ +static int d_isparent(struct dentry *p1, struct dentry *p2) +{ + struct dentry *p; + + for (p = p2; p->d_parent != p; p = p->d_parent) { + if (p->d_parent == p1) + return 1; + } + return 0; +} + +/* + * This helper attempts to cope with remotely renamed directories + * + * It assumes that the caller is already holding + * dentry->d_parent->d_inode->i_mutex and the dcache_lock + * + * Note: If ever the locking in lock_rename() changes, then please + * remember to update this too... + * + * On return, dcache_lock will have been unlocked. + */ +static struct dentry *__d_unalias(struct dentry *dentry, struct dentry *alias) +{ + struct mutex *m1 = NULL, *m2 = NULL; + struct dentry *ret; + + /* If alias and dentry share a parent, then no extra locks required */ + if (alias->d_parent == dentry->d_parent) + goto out_unalias; + + /* Check for loops */ + ret = ERR_PTR(-ELOOP); + if (d_isparent(alias, dentry)) + goto out_err; + + /* See lock_rename() */ + ret = ERR_PTR(-EBUSY); + if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex)) + goto out_err; + m1 = &dentry->d_sb->s_vfs_rename_mutex; + if (!mutex_trylock(&alias->d_parent->d_inode->i_mutex)) + goto out_err; + m2 = &alias->d_parent->d_inode->i_mutex; +out_unalias: + d_move_locked(alias, dentry); + ret = alias; +out_err: spin_unlock(&dcache_lock); + if (m2) + mutex_unlock(m2); + if (m1) + mutex_unlock(m1); + return ret; } /* @@ -1594,7 +1666,7 @@ static void __d_materialise_dentry(struc */ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode) { - struct dentry *alias, *actual; + struct dentry *actual; BUG_ON(!d_unhashed(dentry)); @@ -1606,26 +1678,27 @@ struct dentry *d_materialise_unique(stru goto found_lock; } - /* See if a disconnected directory already exists as an anonymous root - * that we should splice into the tree instead */ - if (S_ISDIR(inode->i_mode) && (alias = __d_find_alias(inode, 1))) { - spin_lock(&alias->d_lock); - - /* Is this a mountpoint that we could splice into our tree? */ - if (IS_ROOT(alias)) - goto connect_mountpoint; - - if (alias->d_name.len == dentry->d_name.len && - alias->d_parent == dentry->d_parent && - memcmp(alias->d_name.name, - dentry->d_name.name, - dentry->d_name.len) == 0) - goto replace_with_alias; + if (S_ISDIR(inode->i_mode)) { + struct dentry *alias; - spin_unlock(&alias->d_lock); - - /* Doh! Seem to be aliasing directories for some reason... */ - dput(alias); + /* Does an aliased dentry already exist? */ + alias = __d_find_alias(inode, 0); + if (alias) { + actual = alias; + /* Is this an anonymous mountpoint that we could splice + * into our tree? */ + if (IS_ROOT(alias)) { + spin_lock(&alias->d_lock); + __d_materialise_dentry(dentry, alias); + __d_drop(alias); + goto found; + } + /* Nope, but we must(!) avoid directory aliasing */ + actual = __d_unalias(dentry, alias); + if (IS_ERR(actual)) + dput(alias); + goto out_nolock; + } } /* Add a unique reference */ @@ -1641,7 +1714,7 @@ found: _d_rehash(actual); spin_unlock(&actual->d_lock); spin_unlock(&dcache_lock); - +out_nolock: if (actual == dentry) { security_d_instantiate(dentry, inode); return NULL; @@ -1650,16 +1723,6 @@ found: iput(inode); return actual; - /* Convert the anonymous/root alias into an ordinary dentry */ -connect_mountpoint: - __d_materialise_dentry(dentry, alias); - - /* Replace the candidate dentry with the alias in the tree */ -replace_with_alias: - __d_drop(alias); - actual = alias; - goto found; - shouldnt_be_hashed: spin_unlock(&dcache_lock); BUG(); --- linux-2.6.18.i686/fs/nfs/dir.c.orig 2007-04-13 08:31:45.000000000 -0700 +++ linux-2.6.18.i686/fs/nfs/dir.c 2007-04-16 07:42:36.000000000 -0700 @@ -932,8 +932,11 @@ static struct dentry *nfs_lookup(struct no_entry: res = d_materialise_unique(dentry, inode); - if (res != NULL) + if (res != NULL) { + if (IS_ERR(res)) + goto out_unlock; dentry = res; + } nfs_renew_times(dentry); nfs_set_verifier(dentry, nfs_save_change_attribute(dir)); out_unlock: @@ -1129,6 +1132,8 @@ static struct dentry *nfs_readdir_lookup alias = d_materialise_unique(dentry, inode); if (alias != NULL) { dput(dentry); + if (IS_ERR(alias)) + return NULL; dentry = alias; }