[PATCH v2 0/2] d_unhashed fixes and cleanup

[PATCH v2 0/2] d_unhashed fixes and cleanup

[Alexey Lyashkov]

Re introduces a DCACHE_DENTRY_UNHASHED flag to solve a race with checking a
unhashed state vs rename. Bug found while a Cray Lustre testing but looks not a
FS specific, du comment in dcache.h 

> Any filesystem which supports nfsd_operations MUST have a lookup function which,
> if it finds a directory inode with a DCACHE_DISCONNECTED dentry, will d_move
> that dentry into place and return that dentry rather than the passed one, 
> typically using d_splice_alias. *

So potentially any FS will affected.

v2 removed to use a sequence lock from d_unhashed check, due large number
problems while d_unhashed checked with any spinlock hold.

---

Alexey Lyashkov (2):
      rehash process protected with d_seq and d_lock locks, but VFS have access to the
      cleanup of d_unhashed usage.


 fs/configfs/inode.c    |    2 +-
 fs/dcache.c            |   36 ++++++++++++++++++------------------
 fs/nfs/dir.c           |    2 +-
 include/linux/dcache.h |    6 ++++--
 4 files changed, 24 insertions(+), 22 deletions(-)

--

[PATCH v2 1/2] Re introduces a DCACHE_DENTRY_UNHASHED flag

[Alexey Lyashkov]

rehash process protected with d_seq and d_lock locks, but VFS have access to the
d_hashed field without any locks sometimes. It produce errors with get cwd
operations or fsnotify may report an unlink event sometimes. d_seq lock isn’t
used to protect due possibility to sleep with holding locks, and d_lock is too
hot to check a simple state, lets return a DCACHE_DENTRY_UNHASHED flag to
ability to check a unhashed state without locks held.

#include <pthread.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <assert.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>

void *thread_main(void *unused)
{
        int rc;
        for (;;) {
                rc = rename("/tmp/t-a", "/tmp/t-b");
                assert(rc == 0);
                rc = rename("/tmp/t-b", "/tmp/t-a");
                assert(rc == 0);
        }

        return NULL;
}

int main(void) {
        int rc, i;
        pthread_t ptt;

        rmdir("/tmp/t-a");
        rmdir("/tmp/t-b");

        rc = mkdir("/tmp/t-a", 0666);
        assert(rc == 0);

        rc = chdir("/tmp/t-a");
        assert(rc == 0);

        rc = pthread_create(&ptt, NULL, thread_main, NULL);
        assert(rc == 0);

        for (i=0; ;i++) {
                char buf[100], *b;
                b = getcwd(buf, sizeof(buf));
                if (b == NULL) {
                        printf("getcwd failed on iter %d with %d\n", i, errno);
                        break;
                }
        }

        return 0;
}

Reported-by: Andrew Perepechko <anserper@yandex.ru>
Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
---
 fs/configfs/inode.c    |    2 +-
 fs/dcache.c            |   24 ++++++++++++++++--------
 fs/nfs/dir.c           |    2 +-
 include/linux/dcache.h |    6 ++++--
 4 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index ad718e5..9f531f7 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -248,7 +248,7 @@ void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
 		spin_lock(&dentry->d_lock);
 		if (simple_positive(dentry)) {
 			dget_dlock(dentry);
-			__d_drop(dentry);
+			_d_drop(dentry);
 			spin_unlock(&dentry->d_lock);
 			simple_unlink(d_inode(parent), dentry);
 		} else
diff --git a/fs/dcache.c b/fs/dcache.c
index 5c7cc95..0863841 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -438,7 +438,7 @@ static void dentry_lru_add(struct dentry *dentry)
  */
 void __d_drop(struct dentry *dentry)
 {
-	if (!d_unhashed(dentry)) {
+	if (!hlist_bl_unhashed(&dentry->d_hash)) {
 		struct hlist_bl_head *b;
 		/*
 		 * Hashed dentries are normally on the dentry hashtable,
@@ -458,12 +458,18 @@ void __d_drop(struct dentry *dentry)
 		write_seqcount_invalidate(&dentry->d_seq);
 	}
 }
-EXPORT_SYMBOL(__d_drop);
+
+void _d_drop(struct dentry *dentry)
+{
+	dentry->d_flags |= DCACHE_DENTRY_UNHASHED;
+	__d_drop(dentry);
+}
+EXPORT_SYMBOL(_d_drop);
 
 void d_drop(struct dentry *dentry)
 {
 	spin_lock(&dentry->d_lock);
-	__d_drop(dentry);
+	_d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
 }
 EXPORT_SYMBOL(d_drop);
@@ -530,7 +536,7 @@ static void __dentry_kill(struct dentry *dentry)
 			d_lru_del(dentry);
 	}
 	/* if it was on the hash then remove it */
-	__d_drop(dentry);
+	_d_drop(dentry);
 	dentry_unlist(dentry, parent);
 	if (parent)
 		spin_unlock(&parent->d_lock);
@@ -1486,7 +1492,7 @@ static void check_and_drop(void *_data)
 	struct detach_data *data = _data;
 
 	if (!data->mountpoint && !data->select.found)
-		__d_drop(data->select.start);
+		_d_drop(data->select.start);
 }
 
 /**
@@ -1601,7 +1607,7 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	dentry->d_name.name = dname;
 
 	dentry->d_lockref.count = 1;
-	dentry->d_flags = 0;
+	dentry->d_flags = DCACHE_DENTRY_UNHASHED;
 	spin_lock_init(&dentry->d_lock);
 	seqcount_init(&dentry->d_seq);
 	dentry->d_inode = NULL;
@@ -1926,6 +1932,7 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 	spin_lock(&tmp->d_lock);
 	__d_set_inode_and_type(tmp, inode, add_flags);
 	hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry);
+	tmp->d_flags &= ~DCACHE_DENTRY_UNHASHED;
 	hlist_bl_lock(&tmp->d_sb->s_anon);
 	hlist_bl_add_head(&tmp->d_hash, &tmp->d_sb->s_anon);
 	hlist_bl_unlock(&tmp->d_sb->s_anon);
@@ -2331,7 +2338,7 @@ void d_delete(struct dentry * dentry)
 	}
 
 	if (!d_unhashed(dentry))
-		__d_drop(dentry);
+		_d_drop(dentry);
 
 	spin_unlock(&dentry->d_lock);
 
@@ -2342,7 +2349,8 @@ EXPORT_SYMBOL(d_delete);
 static void __d_rehash(struct dentry *entry)
 {
 	struct hlist_bl_head *b = d_hash(entry->d_name.hash);
-	BUG_ON(!d_unhashed(entry));
+	BUG_ON(!hlist_bl_unhashed(&entry->d_hash));
+	entry->d_flags &= ~DCACHE_DENTRY_UNHASHED;
 	hlist_bl_lock(b);
 	hlist_bl_add_head_rcu(&entry->d_hash, b);
 	hlist_bl_unlock(b);
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 5f1af4c..a1911bb 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1891,7 +1891,7 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry)
 		goto out;
 	}
 	if (!d_unhashed(dentry)) {
-		__d_drop(dentry);
+		_d_drop(dentry);
 		need_rehash = 1;
 	}
 	spin_unlock(&dentry->d_lock);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 5beed7b..6b0b3d7 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -213,6 +213,7 @@ struct dentry_operations {
 #define DCACHE_PAR_LOOKUP		0x10000000 /* being looked up (with parent locked shared) */
 #define DCACHE_DENTRY_CURSOR		0x20000000
 
+#define DCACHE_DENTRY_UNHASHED		0x40000000 /* dentry unhashed from tree with unlink */
 extern seqlock_t rename_lock;
 
 /*
@@ -221,7 +222,7 @@ extern seqlock_t rename_lock;
 extern void d_instantiate(struct dentry *, struct inode *);
 extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *);
 extern int d_instantiate_no_diralias(struct dentry *, struct inode *);
-extern void __d_drop(struct dentry *dentry);
+extern void _d_drop(struct dentry *dentry);
 extern void d_drop(struct dentry *dentry);
 extern void d_delete(struct dentry *);
 extern void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op);
@@ -326,11 +327,12 @@ extern struct dentry *dget_parent(struct dentry *dentry);
  *	@dentry: entry to check
  *
  *	Returns true if the dentry passed is not currently hashed.
+ *	hlist_bl_unhashed can't be used as race with d_move().
  */
  
 static inline int d_unhashed(const struct dentry *dentry)
 {
-	return hlist_bl_unhashed(&dentry->d_hash);
+	return dentry->d_flags & DCACHE_DENTRY_UNHASHED;
 }
 
 static inline int d_unlinked(const struct dentry *dentry)

[PATCH v2 2/2] cleanup of d_unhashed usage.

[Alexey Lyashkov]

We don't need to hold a d_lock / d_seq locks while d_unhashed checking.
Lets do these locks less hot.

Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
---
 fs/dcache.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 0863841..c23effb 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1327,12 +1327,8 @@ int d_set_mounted(struct dentry *dentry)
 	write_seqlock(&rename_lock);
 	for (p = dentry->d_parent; !IS_ROOT(p); p = p->d_parent) {
 		/* Need exclusion wrt. d_invalidate() */
-		spin_lock(&p->d_lock);
-		if (unlikely(d_unhashed(p))) {
-			spin_unlock(&p->d_lock);
+		if (unlikely(d_unhashed(p)))
 			goto out;
-		}
-		spin_unlock(&p->d_lock);
 	}
 	spin_lock(&dentry->d_lock);
 	if (!d_unlinked(dentry)) {
@@ -1510,12 +1506,8 @@ void d_invalidate(struct dentry *dentry)
 	/*
 	 * If it's already been dropped, return OK.
 	 */
-	spin_lock(&dentry->d_lock);
-	if (d_unhashed(dentry)) {
-		spin_unlock(&dentry->d_lock);
+	if (d_unhashed(dentry))
 		return;
-	}
-	spin_unlock(&dentry->d_lock);
 
 	/* Negative dentries can be dropped without further checks */
 	if (!dentry->d_inode) {

Re: [PATCH v2 1/2] Re introduces a DCACHE_DENTRY_UNHASHED flag

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

Hi Alexey,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Alexey-Lyashkov/d_unhashed-fixes-and-cleanup/20161101-230954
config: x86_64-randconfig-i0-201644 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/infiniband/hw/qib/qib_fs.c: In function 'remove_file':
>> drivers/infiniband/hw/qib/qib_fs.c:443:3: error: implicit declaration of function '__d_drop' [-Werror=implicit-function-declaration]
      __d_drop(tmp);
      ^
   cc1: some warnings being treated as errors

vim +/__d_drop +443 drivers/infiniband/hw/qib/qib_fs.c

f931551ba Ralph Campbell 2010-05-23  437  		ret = PTR_ERR(tmp);
f931551ba Ralph Campbell 2010-05-23  438  		goto bail;
f931551ba Ralph Campbell 2010-05-23  439  	}
f931551ba Ralph Campbell 2010-05-23  440  
f931551ba Ralph Campbell 2010-05-23  441  	spin_lock(&tmp->d_lock);
dc3f4198e Al Viro        2015-05-18  442  	if (simple_positive(tmp)) {
f931551ba Ralph Campbell 2010-05-23 @443  		__d_drop(tmp);
f931551ba Ralph Campbell 2010-05-23  444  		spin_unlock(&tmp->d_lock);
75c3cfa85 David Howells  2015-03-17  445  		simple_unlink(d_inode(parent), tmp);
f931551ba Ralph Campbell 2010-05-23  446  	} else {

:::::: The code at line 443 was first introduced by commit
:::::: f931551bafe1f10ded7f5282e2aa162c267a2e5d IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters

:::::: TO: Ralph Campbell <ralph.campbell@qlogic.com>
:::::: CC: Roland Dreier <rolandd@cisco.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24880 bytes --]

Re: [PATCH v2 1/2] Re introduces a DCACHE_DENTRY_UNHASHED flag

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

Hi Alexey,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Alexey-Lyashkov/d_unhashed-fixes-and-cleanup/20161101-230954
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/platforms/cell/spufs/inode.c: In function 'spufs_prune_dir':
>> arch/powerpc/platforms/cell/spufs/inode.c:171:4: error: implicit declaration of function '__d_drop' [-Werror=implicit-function-declaration]
       __d_drop(dentry);
       ^~~~~~~~
   cc1: some warnings being treated as errors

vim +/__d_drop +171 arch/powerpc/platforms/cell/spufs/inode.c

6263203ed Arnd Bergmann 2006-10-04  165  
5955102c9 Al Viro       2016-01-22  166  	inode_lock(d_inode(dir));
946e51f2b Al Viro       2014-10-26  167  	list_for_each_entry_safe(dentry, tmp, &dir->d_subdirs, d_child) {
67207b966 Arnd Bergmann 2005-11-15  168  		spin_lock(&dentry->d_lock);
dc3f4198e Al Viro       2015-05-18  169  		if (simple_positive(dentry)) {
dc0474be3 Nick Piggin   2011-01-07  170  			dget_dlock(dentry);
67207b966 Arnd Bergmann 2005-11-15 @171  			__d_drop(dentry);
67207b966 Arnd Bergmann 2005-11-15  172  			spin_unlock(&dentry->d_lock);
75c3cfa85 David Howells 2015-03-17  173  			simple_unlink(d_inode(dir), dentry);
b5c84bf6f Nick Piggin   2011-01-07  174  			/* XXX: what was dcache_lock protecting here? Other

:::::: The code at line 171 was first introduced by commit
:::::: 67207b9664a8d603138ef1556141e6d0a102bea7 [PATCH] spufs: The SPU file system, base

:::::: TO: Arnd Bergmann <arnd@arndb.de>
:::::: CC: Paul Mackerras <paulus@samba.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22609 bytes --]