Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH v3 0/9] Suppress negative dentry
@ 2020-05-15  7:20 Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 1/9] fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE Chengguang Xu
                   ` (10 more replies)
  0 siblings, 11 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
to indicate to drop negative dentry in slow path of lookup.

In overlayfs, negative dentries in upper/lower layers are useless
after construction of overlayfs' own dentry, so in order to
effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
flag when doing lookup in upper/lower layers.

Patch 1 adds flag LOOKUP_DONTCACHE_NEGATIVE and related logic in vfs layer.
Patch 2 does lookup optimazation for overlayfs.
Patch 3-9 just adjusts function argument when calling
lookup_positive_unlocked() and lookup_one_len_unlocked().

v1->v2:
- Only drop negative dentry in slow path of lookup.

v2->v3:
- Drop negative dentry in vfs layer.
- Rebase on latest linus-tree(5.7.0-rc5).

Chengguang Xu (9):
  fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
  ovl: Suppress negative dentry in lookup
  cifs: Adjust argument for lookup_positive_unlocked()
  debugfs: Adjust argument for lookup_positive_unlocked()
  ecryptfs: Adjust argument for lookup_one_len_unlocked()
  exportfs: Adjust argument for lookup_one_len_unlocked()
  kernfs: Adjust argument for lookup_positive_unlocked()
  nfsd: Adjust argument for lookup_positive_unlocked()
  quota: Adjust argument for lookup_positive_unlocked()

 fs/cifs/cifsfs.c      |  2 +-
 fs/debugfs/inode.c    |  2 +-
 fs/ecryptfs/inode.c   |  2 +-
 fs/exportfs/expfs.c   |  2 +-
 fs/kernfs/mount.c     |  2 +-
 fs/namei.c            | 14 ++++++++++----
 fs/nfsd/nfs3xdr.c     |  2 +-
 fs/nfsd/nfs4xdr.c     |  3 ++-
 fs/overlayfs/namei.c  |  9 +++++----
 fs/quota/dquot.c      |  3 ++-
 include/linux/namei.h |  9 +++++++--
 11 files changed, 32 insertions(+), 18 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 1/9] fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 2/9] ovl: Suppress negative dentry in lookup Chengguang Xu
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE to skip
caching negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/namei.c            | 14 ++++++++++----
 include/linux/namei.h |  9 +++++++--
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a320371899cf..a258f0a1d5d6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1532,6 +1532,9 @@ static struct dentry *__lookup_slow(const struct qstr *name,
 		if (unlikely(old)) {
 			dput(dentry);
 			dentry = old;
+		} else if ((flags & LOOKUP_DONTCACHE_NEGATIVE) &&
+			   d_is_negative(dentry)) {
+			d_drop(dentry);
 		}
 	}
 	return dentry;
@@ -2554,6 +2557,7 @@ EXPORT_SYMBOL(lookup_one_len);
  * @name:	pathname component to lookup
  * @base:	base directory to lookup from
  * @len:	maximum length @len should be interpreted to
+ * @flags:	lookup flags
  *
  * Note that this routine is purely a helper for filesystem usage and should
  * not be called by generic code.
@@ -2562,7 +2566,8 @@ EXPORT_SYMBOL(lookup_one_len);
  * i_mutex held, and will take the i_mutex itself if necessary.
  */
 struct dentry *lookup_one_len_unlocked(const char *name,
-				       struct dentry *base, int len)
+				       struct dentry *base, int len,
+				       unsigned int flags)
 {
 	struct qstr this;
 	int err;
@@ -2574,7 +2579,7 @@ struct dentry *lookup_one_len_unlocked(const char *name,
 
 	ret = lookup_dcache(&this, base, 0);
 	if (!ret)
-		ret = lookup_slow(&this, base, 0);
+		ret = lookup_slow(&this, base, flags);
 	return ret;
 }
 EXPORT_SYMBOL(lookup_one_len_unlocked);
@@ -2588,9 +2593,10 @@ EXPORT_SYMBOL(lookup_one_len_unlocked);
  * this one avoids such problems.
  */
 struct dentry *lookup_positive_unlocked(const char *name,
-				       struct dentry *base, int len)
+				       struct dentry *base, int len,
+				       unsigned int flags)
 {
-	struct dentry *ret = lookup_one_len_unlocked(name, base, len);
+	struct dentry *ret = lookup_one_len_unlocked(name, base, len, flags);
 	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
 		dput(ret);
 		ret = ERR_PTR(-ENOENT);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a4bb992623c4..1d43fc481e47 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -49,6 +49,9 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
 /* LOOKUP_* flags which do scope-related checks based on the dirfd. */
 #define LOOKUP_IS_SCOPED (LOOKUP_BENEATH | LOOKUP_IN_ROOT)
 
+/* Hint: don't cache negative. */
+#define LOOKUP_DONTCACHE_NEGATIVE	0x200000
+
 extern int path_pts(struct path *path);
 
 extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
@@ -68,8 +71,10 @@ extern struct dentry *kern_path_locked(const char *, struct path *);
 
 extern struct dentry *try_lookup_one_len(const char *, struct dentry *, int);
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
-extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
-extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
+extern struct dentry *lookup_one_len_unlocked(const char *name,
+			struct dentry *base, int len, unsigned int flags);
+extern struct dentry *lookup_positive_unlocked(const char *name,
+			struct dentry *base, int len, unsigned int flags);
 
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 2/9] ovl: Suppress negative dentry in lookup
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 1/9] fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 3/9] cifs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

In order to reduce useless negative dentries in upper/lower
layers, specify LOOKUP_DONTCACHE_NEGATIVE flag in lookup
so that we can drop negative dentries which generated in
slow path of lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/namei.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 0db23baf98e7..a400a7d126bd 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -200,7 +200,8 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 	int err;
 	bool last_element = !post[0];
 
-	this = lookup_positive_unlocked(name, base, namelen);
+	this = lookup_positive_unlocked(name, base, namelen,
+					LOOKUP_DONTCACHE_NEGATIVE);
 	if (IS_ERR(this)) {
 		err = PTR_ERR(this);
 		this = NULL;
@@ -657,7 +658,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh)
 	if (err)
 		return ERR_PTR(err);
 
-	index = lookup_positive_unlocked(name.name, ofs->indexdir, name.len);
+	index = lookup_positive_unlocked(name.name, ofs->indexdir, name.len, 0);
 	kfree(name.name);
 	if (IS_ERR(index)) {
 		if (PTR_ERR(index) == -ENOENT)
@@ -689,7 +690,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 	if (err)
 		return ERR_PTR(err);
 
-	index = lookup_positive_unlocked(name.name, ofs->indexdir, name.len);
+	index = lookup_positive_unlocked(name.name, ofs->indexdir, name.len, 0);
 	if (IS_ERR(index)) {
 		err = PTR_ERR(index);
 		if (err == -ENOENT) {
@@ -1137,7 +1138,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 		struct dentry *lowerdir = poe->lowerstack[i].dentry;
 
 		this = lookup_positive_unlocked(name->name, lowerdir,
-					       name->len);
+					       name->len, 0);
 		if (IS_ERR(this)) {
 			switch (PTR_ERR(this)) {
 			case -ENOENT:
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 3/9] cifs: Adjust argument for lookup_positive_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 1/9] fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 2/9] ovl: Suppress negative dentry in lookup Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 4/9] debugfs: " Chengguang Xu
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_positive_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/cifs/cifsfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c31f362fa098..e52a3b35ebac 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -752,7 +752,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		while (*s && *s != sep)
 			s++;
 
-		child = lookup_positive_unlocked(p, dentry, s - p);
+		child = lookup_positive_unlocked(p, dentry, s - p, 0);
 		dput(dentry);
 		dentry = child;
 	} while (!IS_ERR(dentry));
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 4/9] debugfs: Adjust argument for lookup_positive_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (2 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 3/9] cifs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 5/9] ecryptfs: Adjust argument for lookup_one_len_unlocked() Chengguang Xu
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_positive_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/debugfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index b7f2e971ecbc..df4f37a6a9ab 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -299,7 +299,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
 	if (!parent)
 		parent = debugfs_mount->mnt_root;
 
-	dentry = lookup_positive_unlocked(name, parent, strlen(name));
+	dentry = lookup_positive_unlocked(name, parent, strlen(name), 0);
 	if (IS_ERR(dentry))
 		return NULL;
 	return dentry;
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 5/9] ecryptfs: Adjust argument for lookup_one_len_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (3 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 4/9] debugfs: " Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 6/9] exportfs: " Chengguang Xu
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_one_len_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ecryptfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index e23752d9a79f..e39af6313ad9 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -407,7 +407,7 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode,
 		name = encrypted_and_encoded_name;
 	}
 
-	lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len);
+	lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len, 0);
 	if (IS_ERR(lower_dentry)) {
 		ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned "
 				"[%ld] on lower_dentry = [%s]\n", __func__,
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 6/9] exportfs: Adjust argument for lookup_one_len_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (4 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 5/9] ecryptfs: Adjust argument for lookup_one_len_unlocked() Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 7/9] kernfs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_one_len_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/exportfs/expfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 2dd55b172d57..a4276d14aebb 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -145,7 +145,7 @@ static struct dentry *reconnect_one(struct vfsmount *mnt,
 	if (err)
 		goto out_err;
 	dprintk("%s: found name: %s\n", __func__, nbuf);
-	tmp = lookup_one_len_unlocked(nbuf, parent, strlen(nbuf));
+	tmp = lookup_one_len_unlocked(nbuf, parent, strlen(nbuf), 0);
 	if (IS_ERR(tmp)) {
 		dprintk("%s: lookup failed: %d\n", __func__, PTR_ERR(tmp));
 		err = PTR_ERR(tmp);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 7/9] kernfs: Adjust argument for lookup_positive_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (5 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 6/9] exportfs: " Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 8/9] nfsd: " Chengguang Xu
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_positive_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/kernfs/mount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index 9dc7e7a64e10..3162d2bf39b3 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -224,7 +224,7 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
 			return ERR_PTR(-EINVAL);
 		}
 		dtmp = lookup_positive_unlocked(kntmp->name, dentry,
-					       strlen(kntmp->name));
+					       strlen(kntmp->name), 0);
 		dput(dentry);
 		if (IS_ERR(dtmp))
 			return dtmp;
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 8/9] nfsd: Adjust argument for lookup_positive_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (6 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 7/9] kernfs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:20 ` [RFC PATCH v3 9/9] quota: " Chengguang Xu
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_positive_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/nfsd/nfs3xdr.c | 2 +-
 fs/nfsd/nfs4xdr.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index aae514d40b64..19628922969c 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -855,7 +855,7 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp,
 		} else
 			dchild = dget(dparent);
 	} else
-		dchild = lookup_positive_unlocked(name, dparent, namlen);
+		dchild = lookup_positive_unlocked(name, dparent, namlen, 0);
 	if (IS_ERR(dchild))
 		return rv;
 	if (d_mountpoint(dchild))
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 996ac01ee977..1a69e60b8d59 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3066,7 +3066,8 @@ nfsd4_encode_dirent_fattr(struct xdr_stream *xdr, struct nfsd4_readdir *cd,
 	__be32 nfserr;
 	int ignore_crossmnt = 0;
 
-	dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry, namlen);
+	dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry,
+					  namlen, 0);
 	if (IS_ERR(dentry))
 		return nfserrno(PTR_ERR(dentry));
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v3 9/9] quota: Adjust argument for lookup_positive_unlocked()
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (7 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 8/9] nfsd: " Chengguang Xu
@ 2020-05-15  7:20 ` Chengguang Xu
  2020-05-15  7:30 ` [RFC PATCH v3 0/9] Suppress negative dentry Amir Goldstein
  2020-05-18  0:53 ` Ian Kent
  10 siblings, 0 replies; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  7:20 UTC (permalink / raw)
  To: miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs, Chengguang Xu

Set 0 as lookup flag argument when calling lookup_positive_unlocked(),
because we don't hope to drop negative dentry in lookup.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/quota/dquot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index b6a4f692d345..8d6c32f75a4d 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2488,7 +2488,8 @@ int dquot_quota_on_mount(struct super_block *sb, char *qf_name,
 	struct dentry *dentry;
 	int error;
 
-	dentry = lookup_positive_unlocked(qf_name, sb->s_root, strlen(qf_name));
+	dentry = lookup_positive_unlocked(qf_name, sb->s_root,
+					  strlen(qf_name), 0);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (8 preceding siblings ...)
  2020-05-15  7:20 ` [RFC PATCH v3 9/9] quota: " Chengguang Xu
@ 2020-05-15  7:30 ` Amir Goldstein
  2020-05-15  8:25   ` Chengguang Xu
  2020-05-18  0:53 ` Ian Kent
  10 siblings, 1 reply; 26+ messages in thread
From: Amir Goldstein @ 2020-05-15  7:30 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Miklos Szeredi, Al Viro, linux-fsdevel, overlayfs

On Fri, May 15, 2020 at 10:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
> to indicate to drop negative dentry in slow path of lookup.
>
> In overlayfs, negative dentries in upper/lower layers are useless
> after construction of overlayfs' own dentry, so in order to
> effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
> flag when doing lookup in upper/lower layers.
>
> Patch 1 adds flag LOOKUP_DONTCACHE_NEGATIVE and related logic in vfs layer.
> Patch 2 does lookup optimazation for overlayfs.
> Patch 3-9 just adjusts function argument when calling
> lookup_positive_unlocked() and lookup_one_len_unlocked().

Hmm you cannot do that, build must not be broken mid series.
When Miklos said split he meant to patch 1 and 2.
Patch 1 must convert all callers to the new argument list,
at which point all overlayfs calls are with 0 flags.

Once that's done, you may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-15  7:30 ` [RFC PATCH v3 0/9] Suppress negative dentry Amir Goldstein
@ 2020-05-15  8:25   ` Chengguang Xu
  2020-05-15  8:42     ` Miklos Szeredi
  0 siblings, 1 reply; 26+ messages in thread
From: Chengguang Xu @ 2020-05-15  8:25 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, Al Viro, linux-fsdevel, overlayfs

 ---- 在 星期五, 2020-05-15 15:30:27 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Fri, May 15, 2020 at 10:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
 > > to indicate to drop negative dentry in slow path of lookup.
 > >
 > > In overlayfs, negative dentries in upper/lower layers are useless
 > > after construction of overlayfs' own dentry, so in order to
 > > effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
 > > flag when doing lookup in upper/lower layers.
 > >
 > > Patch 1 adds flag LOOKUP_DONTCACHE_NEGATIVE and related logic in vfs layer.
 > > Patch 2 does lookup optimazation for overlayfs.
 > > Patch 3-9 just adjusts function argument when calling
 > > lookup_positive_unlocked() and lookup_one_len_unlocked().
 > 
 > Hmm you cannot do that, build must not be broken mid series.
 > When Miklos said split he meant to patch 1 and 2.
 > Patch 1 must convert all callers to the new argument list,
 > at which point all overlayfs calls are with 0 flags.
 > 
 > Once that's done, you may add:
 > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
 > 

OK, I got it, I'll still wait for a while in case of other feedbacks.

Miklos, AI

I'm not sure this series will go into whose tree in the end, 
so I just rebased on current linus-tree, any suggestion for the code base?

Thanks,
cgxu




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-15  8:25   ` Chengguang Xu
@ 2020-05-15  8:42     ` Miklos Szeredi
  0 siblings, 0 replies; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-15  8:42 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Amir Goldstein, Al Viro, linux-fsdevel, overlayfs

On Fri, May 15, 2020 at 10:26 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期五, 2020-05-15 15:30:27 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > On Fri, May 15, 2020 at 10:21 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >
>  > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
>  > > to indicate to drop negative dentry in slow path of lookup.
>  > >
>  > > In overlayfs, negative dentries in upper/lower layers are useless
>  > > after construction of overlayfs' own dentry, so in order to
>  > > effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
>  > > flag when doing lookup in upper/lower layers.
>  > >
>  > > Patch 1 adds flag LOOKUP_DONTCACHE_NEGATIVE and related logic in vfs layer.
>  > > Patch 2 does lookup optimazation for overlayfs.
>  > > Patch 3-9 just adjusts function argument when calling
>  > > lookup_positive_unlocked() and lookup_one_len_unlocked().
>  >
>  > Hmm you cannot do that, build must not be broken mid series.
>  > When Miklos said split he meant to patch 1 and 2.
>  > Patch 1 must convert all callers to the new argument list,
>  > at which point all overlayfs calls are with 0 flags.
>  >
>  > Once that's done, you may add:
>  > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>  >
>
> OK, I got it, I'll still wait for a while in case of other feedbacks.
>
> Miklos, AI
>
> I'm not sure this series will go into whose tree in the end,
> so I just rebased on current linus-tree, any suggestion for the code base?

Linus' tree is a good base in this case.  I'm happier if VFS changes
go through Al's tree, but simple stuff can go through overlayfs tree
as well.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
                   ` (9 preceding siblings ...)
  2020-05-15  7:30 ` [RFC PATCH v3 0/9] Suppress negative dentry Amir Goldstein
@ 2020-05-18  0:53 ` Ian Kent
  2020-05-18  5:27   ` Amir Goldstein
  10 siblings, 1 reply; 26+ messages in thread
From: Ian Kent @ 2020-05-18  0:53 UTC (permalink / raw)
  To: Chengguang Xu, miklos, viro, amir73il; +Cc: linux-fsdevel, linux-unionfs

On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote:
> This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
> to indicate to drop negative dentry in slow path of lookup.
> 
> In overlayfs, negative dentries in upper/lower layers are useless
> after construction of overlayfs' own dentry, so in order to
> effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
> flag when doing lookup in upper/lower layers.

I've looked at this a couple of times now.

I'm not at all sure of the wisdom of adding a flag to a VFS function
that allows circumventing what a file system chooses to do.

I also do really see the need for it because only hashed negative
dentrys will be retained by the VFS so, if you see a hashed negative
dentry then you can cause it to be discarded on release of the last
reference by dropping it.

So what's different here, why is adding an argument to do that drop
in the VFS itself needed instead of just doing it in overlayfs?

> 
> Patch 1 adds flag LOOKUP_DONTCACHE_NEGATIVE and related logic in vfs
> layer.
> Patch 2 does lookup optimazation for overlayfs.
> Patch 3-9 just adjusts function argument when calling
> lookup_positive_unlocked() and lookup_one_len_unlocked().
> 
> v1->v2:
> - Only drop negative dentry in slow path of lookup.
> 
> v2->v3:
> - Drop negative dentry in vfs layer.
> - Rebase on latest linus-tree(5.7.0-rc5).
> 
> Chengguang Xu (9):
>   fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
>   ovl: Suppress negative dentry in lookup
>   cifs: Adjust argument for lookup_positive_unlocked()
>   debugfs: Adjust argument for lookup_positive_unlocked()
>   ecryptfs: Adjust argument for lookup_one_len_unlocked()
>   exportfs: Adjust argument for lookup_one_len_unlocked()
>   kernfs: Adjust argument for lookup_positive_unlocked()
>   nfsd: Adjust argument for lookup_positive_unlocked()
>   quota: Adjust argument for lookup_positive_unlocked()
> 
>  fs/cifs/cifsfs.c      |  2 +-
>  fs/debugfs/inode.c    |  2 +-
>  fs/ecryptfs/inode.c   |  2 +-
>  fs/exportfs/expfs.c   |  2 +-
>  fs/kernfs/mount.c     |  2 +-
>  fs/namei.c            | 14 ++++++++++----
>  fs/nfsd/nfs3xdr.c     |  2 +-
>  fs/nfsd/nfs4xdr.c     |  3 ++-
>  fs/overlayfs/namei.c  |  9 +++++----
>  fs/quota/dquot.c      |  3 ++-
>  include/linux/namei.h |  9 +++++++--
>  11 files changed, 32 insertions(+), 18 deletions(-)
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  0:53 ` Ian Kent
@ 2020-05-18  5:27   ` Amir Goldstein
  2020-05-18  7:52     ` Miklos Szeredi
  2020-05-18 10:26     ` Ian Kent
  0 siblings, 2 replies; 26+ messages in thread
From: Amir Goldstein @ 2020-05-18  5:27 UTC (permalink / raw)
  To: Ian Kent; +Cc: Chengguang Xu, Miklos Szeredi, Al Viro, linux-fsdevel, overlayfs

On Mon, May 18, 2020 at 3:53 AM Ian Kent <raven@themaw.net> wrote:
>
> On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote:
> > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
> > to indicate to drop negative dentry in slow path of lookup.
> >
> > In overlayfs, negative dentries in upper/lower layers are useless
> > after construction of overlayfs' own dentry, so in order to
> > effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
> > flag when doing lookup in upper/lower layers.
>
> I've looked at this a couple of times now.
>
> I'm not at all sure of the wisdom of adding a flag to a VFS function
> that allows circumventing what a file system chooses to do.

But it is not really a conscious choice is it?
How exactly does a filesystem express its desire to cache a negative
dentry? The documentation of lookup() in vfs.rst makes it clear that
it is not up to the filesystem to make that decision.
The VFS needs to cache the negative dentry on lookup(), so
it can turn it positive on create().
Low level kernel modules that call the VFS lookup() might know
that caching the negative dentry is counter productive.

>
> I also do really see the need for it because only hashed negative
> dentrys will be retained by the VFS so, if you see a hashed negative
> dentry then you can cause it to be discarded on release of the last
> reference by dropping it.
>
> So what's different here, why is adding an argument to do that drop
> in the VFS itself needed instead of just doing it in overlayfs?

That was v1 patch. It was dealing with the possible race of
returned negative dentry becoming positive before dropping it
in an intrusive manner.

In retrospect, I think this race doesn't matter and there is no
harm in dropping a positive dentry in a race obviously caused by
accessing the underlying layer, which as documented results in
"undefined behavior".

Miklos, am I missing something?

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  5:27   ` Amir Goldstein
@ 2020-05-18  7:52     ` Miklos Szeredi
  2020-05-18  8:51       ` Amir Goldstein
  2020-05-19  5:01       ` cgxu
  2020-05-18 10:26     ` Ian Kent
  1 sibling, 2 replies; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-18  7:52 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Ian Kent, Chengguang Xu, Al Viro, linux-fsdevel, overlayfs

On Mon, May 18, 2020 at 7:27 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, May 18, 2020 at 3:53 AM Ian Kent <raven@themaw.net> wrote:
> >
> > On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote:
> > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
> > > to indicate to drop negative dentry in slow path of lookup.
> > >
> > > In overlayfs, negative dentries in upper/lower layers are useless
> > > after construction of overlayfs' own dentry, so in order to
> > > effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE
> > > flag when doing lookup in upper/lower layers.
> >
> > I've looked at this a couple of times now.
> >
> > I'm not at all sure of the wisdom of adding a flag to a VFS function
> > that allows circumventing what a file system chooses to do.
>
> But it is not really a conscious choice is it?
> How exactly does a filesystem express its desire to cache a negative
> dentry? The documentation of lookup() in vfs.rst makes it clear that
> it is not up to the filesystem to make that decision.
> The VFS needs to cache the negative dentry on lookup(), so
> it can turn it positive on create().
> Low level kernel modules that call the VFS lookup() might know
> that caching the negative dentry is counter productive.
>
> >
> > I also do really see the need for it because only hashed negative
> > dentrys will be retained by the VFS so, if you see a hashed negative
> > dentry then you can cause it to be discarded on release of the last
> > reference by dropping it.
> >
> > So what's different here, why is adding an argument to do that drop
> > in the VFS itself needed instead of just doing it in overlayfs?
>
> That was v1 patch. It was dealing with the possible race of
> returned negative dentry becoming positive before dropping it
> in an intrusive manner.
>
> In retrospect, I think this race doesn't matter and there is no
> harm in dropping a positive dentry in a race obviously caused by
> accessing the underlying layer, which as documented results in
> "undefined behavior".
>
> Miklos, am I missing something?

Dropping a positive dentry is harmful in case there's a long term
reference to the dentry (e.g. an open file) since it will look as if
the file was deleted, when in fact it wasn't.

It's possible to unhash a negative dentry in a safe way if we make
sure it cannot become positive.  One way is to grab d_lock and remove
it from the hash table only if count is one.

So yes, we could have a helper to do that instead of the lookup flag.
The disadvantage being that we'd also be dropping negatives that did
not enter the cache because of our lookup.

I don't really care, both are probably good enough for the overlayfs case.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  7:52     ` Miklos Szeredi
@ 2020-05-18  8:51       ` Amir Goldstein
  2020-05-18  9:17         ` Miklos Szeredi
  2020-05-19  5:01       ` cgxu
  1 sibling, 1 reply; 26+ messages in thread
From: Amir Goldstein @ 2020-05-18  8:51 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Ian Kent, Chengguang Xu, Al Viro, linux-fsdevel, overlayfs

> > > I also do really see the need for it because only hashed negative
> > > dentrys will be retained by the VFS so, if you see a hashed negative
> > > dentry then you can cause it to be discarded on release of the last
> > > reference by dropping it.
> > >
> > > So what's different here, why is adding an argument to do that drop
> > > in the VFS itself needed instead of just doing it in overlayfs?
> >
> > That was v1 patch. It was dealing with the possible race of
> > returned negative dentry becoming positive before dropping it
> > in an intrusive manner.
> >
> > In retrospect, I think this race doesn't matter and there is no
> > harm in dropping a positive dentry in a race obviously caused by
> > accessing the underlying layer, which as documented results in
> > "undefined behavior".
> >
> > Miklos, am I missing something?
>
> Dropping a positive dentry is harmful in case there's a long term
> reference to the dentry (e.g. an open file) since it will look as if
> the file was deleted, when in fact it wasn't.
>

I see. My point was that the negative->positive transition cannot
happen on underlying layers without user modifying underlying
layers underneath overlay, so it is fine to be in the "undefined" behavior
zone.

> It's possible to unhash a negative dentry in a safe way if we make
> sure it cannot become positive.  One way is to grab d_lock and remove
> it from the hash table only if count is one.
>
> So yes, we could have a helper to do that instead of the lookup flag.
> The disadvantage being that we'd also be dropping negatives that did
> not enter the cache because of our lookup.
>
> I don't really care, both are probably good enough for the overlayfs case.
>

There is another point to consider.
A negative underlying fs dentry may be useless for *this* overlayfs instance,
but since lower layers can be shared among many overlayfs instances,
for example, thousands of containers all testing for existence of file /etc/FOO
on startup.

It sounds like if we want to go through with DONTCACHE_NEGATIVE, that
it should be opt-in behavior for overlayfs.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  8:51       ` Amir Goldstein
@ 2020-05-18  9:17         ` Miklos Szeredi
  0 siblings, 0 replies; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-18  9:17 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Ian Kent, Chengguang Xu, Al Viro, linux-fsdevel, overlayfs

On Mon, May 18, 2020 at 10:52 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > > I also do really see the need for it because only hashed negative
> > > > dentrys will be retained by the VFS so, if you see a hashed negative
> > > > dentry then you can cause it to be discarded on release of the last
> > > > reference by dropping it.
> > > >
> > > > So what's different here, why is adding an argument to do that drop
> > > > in the VFS itself needed instead of just doing it in overlayfs?
> > >
> > > That was v1 patch. It was dealing with the possible race of
> > > returned negative dentry becoming positive before dropping it
> > > in an intrusive manner.
> > >
> > > In retrospect, I think this race doesn't matter and there is no
> > > harm in dropping a positive dentry in a race obviously caused by
> > > accessing the underlying layer, which as documented results in
> > > "undefined behavior".
> > >
> > > Miklos, am I missing something?
> >
> > Dropping a positive dentry is harmful in case there's a long term
> > reference to the dentry (e.g. an open file) since it will look as if
> > the file was deleted, when in fact it wasn't.
> >
>
> I see. My point was that the negative->positive transition cannot
> happen on underlying layers without user modifying underlying
> layers underneath overlay, so it is fine to be in the "undefined" behavior
> zone.

Right, I don't think you can actually crash a filesystem by unhashing
a positive dentry in the middle of a create op, but it would
definitely be prudent to avoid that.

>
> > It's possible to unhash a negative dentry in a safe way if we make
> > sure it cannot become positive.  One way is to grab d_lock and remove
> > it from the hash table only if count is one.
> >
> > So yes, we could have a helper to do that instead of the lookup flag.
> > The disadvantage being that we'd also be dropping negatives that did
> > not enter the cache because of our lookup.
> >
> > I don't really care, both are probably good enough for the overlayfs case.
> >
>
> There is another point to consider.
> A negative underlying fs dentry may be useless for *this* overlayfs instance,
> but since lower layers can be shared among many overlayfs instances,
> for example, thousands of containers all testing for existence of file /etc/FOO
> on startup.
>
> It sounds like if we want to go through with DONTCACHE_NEGATIVE, that
> it should be opt-in behavior for overlayfs.

Good point.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  5:27   ` Amir Goldstein
  2020-05-18  7:52     ` Miklos Szeredi
@ 2020-05-18 10:26     ` Ian Kent
  2020-05-18 10:39       ` Ian Kent
  1 sibling, 1 reply; 26+ messages in thread
From: Ian Kent @ 2020-05-18 10:26 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Chengguang Xu, Miklos Szeredi, Al Viro, linux-fsdevel, overlayfs

On Mon, 2020-05-18 at 08:27 +0300, Amir Goldstein wrote:
> On Mon, May 18, 2020 at 3:53 AM Ian Kent <raven@themaw.net> wrote:
> > On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote:
> > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
> > > to indicate to drop negative dentry in slow path of lookup.
> > > 
> > > In overlayfs, negative dentries in upper/lower layers are useless
> > > after construction of overlayfs' own dentry, so in order to
> > > effectively reclaim those dentries, specify
> > > LOOKUP_DONTCACHE_NEGATIVE
> > > flag when doing lookup in upper/lower layers.
> > 
> > I've looked at this a couple of times now.
> > 
> > I'm not at all sure of the wisdom of adding a flag to a VFS
> > function
> > that allows circumventing what a file system chooses to do.
> 
> But it is not really a conscious choice is it?

I thought that too until recently when I had the need to ask the
question "how do these negative hashed dentries get into the dcache.

> How exactly does a filesystem express its desire to cache a negative
> dentry? The documentation of lookup() in vfs.rst makes it clear that
> it is not up to the filesystem to make that decision.

That's the question I had too and, somewhat embarrassingly, got
a response that started with "Are you serious ..." for Al when
I made a claim that ext4 doesn't create negative hashed dentries.

What was so bad about that claim is it's really obvious that ext4
does do this in ext4/namei.c:ext4_lookup():

...
inode = NULL;
if (bh) {
...
}
...
return d_splice_alias(inode, dentry);

and inode can be negative here.

Now d_splice_alias() is pretty complicated but, if passed a NULL
dentry it amounts to calling __d_add(dentry, NULL) which produces
a negative hashed dentry, a decision made by ext4 ->lookup() (and
I must say typical of the very few ways such dentries get into
the dcache).

Now on final dput of the walk these dentries should end up with a
reference count of zero which triggers dput() to add them to the
lru list so they can be considered as prune targets and can be
found in subsequent lookups (they are hashed).

This is how using negative hashed detries helps to avoid the
expensive alloc/free (at least) that occurs when looking up paths
that don't exist.

Negative "unhashed" dentries are discarded on final dput() so
don't really come into the picture except that dropping a negative
hashed dentry will cause it to be discarded on final dput().

But nothing is ever quite as simple as a description of how it
appears to (is meant to) work so, by all means, take what I say
with a grain of salt, ;)

> The VFS needs to cache the negative dentry on lookup(), so
> it can turn it positive on create().
> Low level kernel modules that call the VFS lookup() might know
> that caching the negative dentry is counter productive.
> 
> > I also do really see the need for it because only hashed negative
> > dentrys will be retained by the VFS so, if you see a hashed
> > negative
> > dentry then you can cause it to be discarded on release of the last
> > reference by dropping it.
> > 
> > So what's different here, why is adding an argument to do that drop
> > in the VFS itself needed instead of just doing it in overlayfs?
> 
> That was v1 patch. It was dealing with the possible race of
> returned negative dentry becoming positive before dropping it
> in an intrusive manner.

Right, very much something that overlay fs must care about, file
systems not so much.

It might be possible to assume that if the underlying file system
->lookup() call has produced a negative hashed dentry that it will
stay negative. That assumption definitely can't be made for negative
unhashed dentries, of course, since they may still be in the process
of being filled in.

But, unfortunately, I see your point, it's a risky assumption.

> 
> In retrospect, I think this race doesn't matter and there is no
> harm in dropping a positive dentry in a race obviously caused by
> accessing the underlying layer, which as documented results in
> "undefined behavior".

Umm ... I thought we were talking about negative dentries ...

Ian


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18 10:26     ` Ian Kent
@ 2020-05-18 10:39       ` Ian Kent
  0 siblings, 0 replies; 26+ messages in thread
From: Ian Kent @ 2020-05-18 10:39 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Chengguang Xu, Miklos Szeredi, Al Viro, linux-fsdevel, overlayfs

On Mon, 2020-05-18 at 18:26 +0800, Ian Kent wrote:
> 
> Now d_splice_alias() is pretty complicated but, if passed a NULL
> dentry it amounts to calling __d_add(dentry, NULL) which produces
> a negative hashed dentry, a decision made by ext4 ->lookup() (and
> I must say typical of the very few ways such dentries get into
> the dcache).

Oh, rather that's a NULL inode not dentry.

Ian


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-18  7:52     ` Miklos Szeredi
  2020-05-18  8:51       ` Amir Goldstein
@ 2020-05-19  5:01       ` cgxu
  2020-05-19  8:21         ` Miklos Szeredi
  1 sibling, 1 reply; 26+ messages in thread
From: cgxu @ 2020-05-19  5:01 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

  ---- 在 星期一, 2020-05-18 15:52:48 Miklos Szeredi <miklos@szeredi.hu> 撰写 
----
  > On Mon, May 18, 2020 at 7:27 AM Amir Goldstein <amir73il@gmail.com> 
wrote:
  > >
  > > On Mon, May 18, 2020 at 3:53 AM Ian Kent <raven@themaw.net> wrote:
  > > >
  > > > On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote:
  > > > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE
  > > > > to indicate to drop negative dentry in slow path of lookup.
  > > > >
  > > > > In overlayfs, negative dentries in upper/lower layers are useless
  > > > > after construction of overlayfs' own dentry, so in order to
  > > > > effectively reclaim those dentries, specify 
LOOKUP_DONTCACHE_NEGATIVE
  > > > > flag when doing lookup in upper/lower layers.
  > > >
  > > > I've looked at this a couple of times now.
  > > >
  > > > I'm not at all sure of the wisdom of adding a flag to a VFS function
  > > > that allows circumventing what a file system chooses to do.
  > >
  > > But it is not really a conscious choice is it?
  > > How exactly does a filesystem express its desire to cache a negative
  > > dentry? The documentation of lookup() in vfs.rst makes it clear that
  > > it is not up to the filesystem to make that decision.
  > > The VFS needs to cache the negative dentry on lookup(), so
  > > it can turn it positive on create().
  > > Low level kernel modules that call the VFS lookup() might know
  > > that caching the negative dentry is counter productive.
  > >
  > > >
  > > > I also do really see the need for it because only hashed negative
  > > > dentrys will be retained by the VFS so, if you see a hashed negative
  > > > dentry then you can cause it to be discarded on release of the last
  > > > reference by dropping it.
  > > >
  > > > So what's different here, why is adding an argument to do that drop
  > > > in the VFS itself needed instead of just doing it in overlayfs?
  > >
  > > That was v1 patch. It was dealing with the possible race of
  > > returned negative dentry becoming positive before dropping it
  > > in an intrusive manner.
  > >
  > > In retrospect, I think this race doesn't matter and there is no
  > > harm in dropping a positive dentry in a race obviously caused by
  > > accessing the underlying layer, which as documented results in
  > > "undefined behavior".
  > >
  > > Miklos, am I missing something?
  >  > Dropping a positive dentry is harmful in case there's a long term
  > reference to the dentry (e.g. an open file) since it will look as if
  > the file was deleted, when in fact it wasn't.
  >  > It's possible to unhash a negative dentry in a safe way if we make
  > sure it cannot become positive.  One way is to grab d_lock and remove
  > it from the hash table only if count is one.
  >  > So yes, we could have a helper to do that instead of the lookup flag.
  > The disadvantage being that we'd also be dropping negatives that did
  > not enter the cache because of our lookup.
  >


If we don't consider that only drop negative dentry of our lookup,
it is possible to do like below, isn't it?



diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 723d17744758..fa339e23b0f8 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -200,7 +200,7 @@ static int ovl_lookup_single(struct dentry *base, 
struct ovl_lookup_data *d,
         int err;
         bool last_element = !post[0];

-       this = lookup_positive_unlocked(name, base, namelen);
+       this = lookup_one_len_unlocked(name, base, namelen);
         if (IS_ERR(this)) {
                 err = PTR_ERR(this);
                 this = NULL;
@@ -209,6 +209,18 @@ static int ovl_lookup_single(struct dentry *base, 
struct ovl_lookup_data *d,
                 goto out_err;
         }

+       if (d_flags_negative(this->d_flags)) {
+               inode_lock_shared(base->d_inode);
+               if (d_flags_negative(this->d_flags))
+                       d_drop(this);
+               inode_unlock_shared(base->d_inode);
+
+               dput(this);
+               this = NULL;
+               err = -ENOENT;
+               goto out;
+       }
+
         if (ovl_dentry_weird(this)) {
                 /* Don't support traversing automounts and other 
weirdness */
                 err = -EREMOTE;


Thanks,
cgxu


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-19  5:01       ` cgxu
@ 2020-05-19  8:21         ` Miklos Szeredi
  2020-05-19  9:23           ` cgxu
  0 siblings, 1 reply; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-19  8:21 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@mykernel.net> wrote:

> If we don't consider that only drop negative dentry of our lookup,
> it is possible to do like below, isn't it?

Yes, the code looks good, though I'd consider using d_lock on dentry
instead if i_lock on parent, something like this:

if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
    spin_lock(&dentry->d_lock);
    /* Recheck condition under lock */
    if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
        __d_drop(dentry)
    spin_unlock(&dentry->d_lock);
}

But as Amir noted, we do need to take into account the case where
lower layers are shared by multiple overlays, in which case dropping
the negative dentries could result in a performance regression.
Have you looked at that case, and the effect of this patch on negative
dentry lookup performance?

Upper layer negative dentries don't have this issue, since they are
never shared, so I think it would be safe to drop them
unconditionally.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-19  8:21         ` Miklos Szeredi
@ 2020-05-19  9:23           ` cgxu
  2020-05-20 14:44             ` Miklos Szeredi
  0 siblings, 1 reply; 26+ messages in thread
From: cgxu @ 2020-05-19  9:23 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

On 5/19/20 4:21 PM, Miklos Szeredi wrote:
> On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@mykernel.net> wrote:
>
>> If we don't consider that only drop negative dentry of our lookup,
>> it is possible to do like below, isn't it?
> Yes, the code looks good, though I'd consider using d_lock on dentry
> instead if i_lock on parent, something like this:
>
> if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
>      spin_lock(&dentry->d_lock);
>      /* Recheck condition under lock */
>      if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
>          __d_drop(dentry)
>      spin_unlock(&dentry->d_lock);

And after this we will still treat 'dentry' as negative dentry and dput it
regardless of the second check result of d_is_negative(dentry), right?


> }
>
> But as Amir noted, we do need to take into account the case where
> lower layers are shared by multiple overlays, in which case dropping
> the negative dentries could result in a performance regression.
> Have you looked at that case, and the effect of this patch on negative
> dentry lookup performance?

The container which is affected by this feature is just take advantage
of previous another container but we could not guarantee that always
happening. I think there no way for best of both worlds, consider that
some malicious containers continuously make negative dentries by
searching non-exist files, so that page cache of clean data, clean
inodes/dentries will be freed by memory reclaim. All of those
behaviors will impact the performance of other container instances.

On the other hand, if this feature significantly affects particular 
container,
doesn't that mean the container is noisy neighbor and should be restricted
in some way?

Thanks,
cgxu


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-19  9:23           ` cgxu
@ 2020-05-20 14:44             ` Miklos Szeredi
  2020-05-25 13:37               ` Chengguang Xu
  0 siblings, 1 reply; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-20 14:44 UTC (permalink / raw)
  To: cgxu; +Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

On Tue, May 19, 2020 at 11:24 AM cgxu <cgxu519@mykernel.net> wrote:
>
> On 5/19/20 4:21 PM, Miklos Szeredi wrote:
> > On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@mykernel.net> wrote:
> >
> >> If we don't consider that only drop negative dentry of our lookup,
> >> it is possible to do like below, isn't it?
> > Yes, the code looks good, though I'd consider using d_lock on dentry
> > instead if i_lock on parent, something like this:
> >
> > if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
> >      spin_lock(&dentry->d_lock);
> >      /* Recheck condition under lock */
> >      if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
> >          __d_drop(dentry)
> >      spin_unlock(&dentry->d_lock);
>
> And after this we will still treat 'dentry' as negative dentry and dput it
> regardless of the second check result of d_is_negative(dentry), right?

I'd restructure it in the same way as lookup_positive_unlocked()...

> > }
> >
> > But as Amir noted, we do need to take into account the case where
> > lower layers are shared by multiple overlays, in which case dropping
> > the negative dentries could result in a performance regression.
> > Have you looked at that case, and the effect of this patch on negative
> > dentry lookup performance?
>
> The container which is affected by this feature is just take advantage
> of previous another container but we could not guarantee that always
> happening. I think there no way for best of both worlds, consider that
> some malicious containers continuously make negative dentries by
> searching non-exist files, so that page cache of clean data, clean
> inodes/dentries will be freed by memory reclaim. All of those
> behaviors will impact the performance of other container instances.
>
> On the other hand, if this feature significantly affects particular
> container,
> doesn't that mean the container is noisy neighbor and should be restricted
> in some way?

Not necessarily.   Negative dentries can be useful and in case of
layers shared between two containers having negative dentries cached
in the lower layer can in theory positively affect performance.   I
don't have data to back this up, nor the opposite.  You should run
some numbers for container startup times with and without this patch.

Thanks,
Milklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-20 14:44             ` Miklos Szeredi
@ 2020-05-25 13:37               ` Chengguang Xu
  2020-05-25 13:50                 ` Miklos Szeredi
  0 siblings, 1 reply; 26+ messages in thread
From: Chengguang Xu @ 2020-05-25 13:37 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

在 5/20/2020 10:44 PM, Miklos Szeredi 写道:
> On Tue, May 19, 2020 at 11:24 AM cgxu <cgxu519@mykernel.net> wrote:
>> On 5/19/20 4:21 PM, Miklos Szeredi wrote:
>>> On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@mykernel.net> wrote:
>>>
>>>> If we don't consider that only drop negative dentry of our lookup,
>>>> it is possible to do like below, isn't it?
>>> Yes, the code looks good, though I'd consider using d_lock on dentry
>>> instead if i_lock on parent, something like this:
>>>
>>> if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
>>>       spin_lock(&dentry->d_lock);
>>>       /* Recheck condition under lock */
>>>       if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
>>>           __d_drop(dentry)
>>>       spin_unlock(&dentry->d_lock);
>> And after this we will still treat 'dentry' as negative dentry and dput it
>> regardless of the second check result of d_is_negative(dentry), right?
> I'd restructure it in the same way as lookup_positive_unlocked()...
>
>>> }
>>>
>>> But as Amir noted, we do need to take into account the case where
>>> lower layers are shared by multiple overlays, in which case dropping
>>> the negative dentries could result in a performance regression.
>>> Have you looked at that case, and the effect of this patch on negative
>>> dentry lookup performance?
>> The container which is affected by this feature is just take advantage
>> of previous another container but we could not guarantee that always
>> happening. I think there no way for best of both worlds, consider that
>> some malicious containers continuously make negative dentries by
>> searching non-exist files, so that page cache of clean data, clean
>> inodes/dentries will be freed by memory reclaim. All of those
>> behaviors will impact the performance of other container instances.
>>
>> On the other hand, if this feature significantly affects particular
>> container,
>> doesn't that mean the container is noisy neighbor and should be restricted
>> in some way?
> Not necessarily.   Negative dentries can be useful and in case of
> layers shared between two containers having negative dentries cached
> in the lower layer can in theory positively affect performance.   I
> don't have data to back this up, nor the opposite.  You should run
> some numbers for container startup times with and without this patch.

I did some simple tests  for it but the result seems not very steady, so 
I need to take time to do more detail tests later. Is it possible to 
apply the patch for upper layer first?

Thanks,
cgxu



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v3 0/9] Suppress negative dentry
  2020-05-25 13:37               ` Chengguang Xu
@ 2020-05-25 13:50                 ` Miklos Szeredi
  0 siblings, 0 replies; 26+ messages in thread
From: Miklos Szeredi @ 2020-05-25 13:50 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Amir Goldstein, Ian Kent, Al Viro, linux-fsdevel, overlayfs

On Mon, May 25, 2020 at 3:37 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> 在 5/20/2020 10:44 PM, Miklos Szeredi 写道:
> > On Tue, May 19, 2020 at 11:24 AM cgxu <cgxu519@mykernel.net> wrote:
> >> On 5/19/20 4:21 PM, Miklos Szeredi wrote:
> >>> On Tue, May 19, 2020 at 7:02 AM cgxu <cgxu519@mykernel.net> wrote:
> >>>
> >>>> If we don't consider that only drop negative dentry of our lookup,
> >>>> it is possible to do like below, isn't it?
> >>> Yes, the code looks good, though I'd consider using d_lock on dentry
> >>> instead if i_lock on parent, something like this:
> >>>
> >>> if (d_is_negative(dentry) && dentry->d_lockref.count == 1) {
> >>>       spin_lock(&dentry->d_lock);
> >>>       /* Recheck condition under lock */
> >>>       if (d_is_negative(dentry) && dentry->d_lockref.count == 1)
> >>>           __d_drop(dentry)
> >>>       spin_unlock(&dentry->d_lock);
> >> And after this we will still treat 'dentry' as negative dentry and dput it
> >> regardless of the second check result of d_is_negative(dentry), right?
> > I'd restructure it in the same way as lookup_positive_unlocked()...
> >
> >>> }
> >>>
> >>> But as Amir noted, we do need to take into account the case where
> >>> lower layers are shared by multiple overlays, in which case dropping
> >>> the negative dentries could result in a performance regression.
> >>> Have you looked at that case, and the effect of this patch on negative
> >>> dentry lookup performance?
> >> The container which is affected by this feature is just take advantage
> >> of previous another container but we could not guarantee that always
> >> happening. I think there no way for best of both worlds, consider that
> >> some malicious containers continuously make negative dentries by
> >> searching non-exist files, so that page cache of clean data, clean
> >> inodes/dentries will be freed by memory reclaim. All of those
> >> behaviors will impact the performance of other container instances.
> >>
> >> On the other hand, if this feature significantly affects particular
> >> container,
> >> doesn't that mean the container is noisy neighbor and should be restricted
> >> in some way?
> > Not necessarily.   Negative dentries can be useful and in case of
> > layers shared between two containers having negative dentries cached
> > in the lower layer can in theory positively affect performance.   I
> > don't have data to back this up, nor the opposite.  You should run
> > some numbers for container startup times with and without this patch.
>
> I did some simple tests  for it but the result seems not very steady, so
> I need to take time to do more detail tests later. Is it possible to
> apply the patch for upper layer first?

Sure, that's a good start.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15  7:20 [RFC PATCH v3 0/9] Suppress negative dentry Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 1/9] fs/dcache: Introduce a new lookup flag LOOKUP_DONTCACHE_NEGATIVE Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 2/9] ovl: Suppress negative dentry in lookup Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 3/9] cifs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 4/9] debugfs: " Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 5/9] ecryptfs: Adjust argument for lookup_one_len_unlocked() Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 6/9] exportfs: " Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 7/9] kernfs: Adjust argument for lookup_positive_unlocked() Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 8/9] nfsd: " Chengguang Xu
2020-05-15  7:20 ` [RFC PATCH v3 9/9] quota: " Chengguang Xu
2020-05-15  7:30 ` [RFC PATCH v3 0/9] Suppress negative dentry Amir Goldstein
2020-05-15  8:25   ` Chengguang Xu
2020-05-15  8:42     ` Miklos Szeredi
2020-05-18  0:53 ` Ian Kent
2020-05-18  5:27   ` Amir Goldstein
2020-05-18  7:52     ` Miklos Szeredi
2020-05-18  8:51       ` Amir Goldstein
2020-05-18  9:17         ` Miklos Szeredi
2020-05-19  5:01       ` cgxu
2020-05-19  8:21         ` Miklos Szeredi
2020-05-19  9:23           ` cgxu
2020-05-20 14:44             ` Miklos Szeredi
2020-05-25 13:37               ` Chengguang Xu
2020-05-25 13:50                 ` Miklos Szeredi
2020-05-18 10:26     ` Ian Kent
2020-05-18 10:39       ` Ian Kent

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git