[PATCH 1/3] ovl: refactor lowerstack related functions

[PATCH 1/3] ovl: refactor lowerstack related functions

[Chengguang Xu]

Introduce __ovl_path_lower() to handle lowerstack related things
and refactor ovl_dentry_lower()/ovl_layer_lower() based on it.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 35 +++++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e0b7de7..abb5cf4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -215,6 +215,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct vfsmount *ovl_mnt_lower(struct dentry *dentry);
 struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 6f10780..a3459e6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -157,8 +157,8 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 	struct ovl_entry *oe = dentry->d_fsdata;
 
 	if (oe->numlower) {
-		path->mnt = oe->lowerstack[0].layer->mnt;
-		path->dentry = oe->lowerstack[0].dentry;
+		path->mnt = ovl_mnt_lower(dentry);
+		path->dentry = ovl_dentry_lower(dentry);
 	} else {
 		*path = (struct path) { };
 	}
@@ -181,18 +181,45 @@ struct dentry *ovl_dentry_upper(struct dentry *dentry)
 	return ovl_upperdentry_dereference(OVL_I(d_inode(dentry)));
 }
 
+struct ovl_path *__ovl_path_lower(struct ovl_entry *oe, int layer)
+{
+	if (layer > oe->numlower)
+		return NULL;
+
+	return &oe->lowerstack[layer - 1];
+}
+
+struct vfsmount *ovl_mnt_lower(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_path *path = __ovl_path_lower(oe, 1);
+
+	if (path)
+		return path->layer->mnt;
+	else
+		return NULL;
+}
+
 struct dentry *ovl_dentry_lower(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_path *path = __ovl_path_lower(oe, 1);
 
-	return oe->numlower ? oe->lowerstack[0].dentry : NULL;
+	if (path)
+		return path->dentry;
+	else
+		return NULL;
 }
 
 struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_path *path = __ovl_path_lower(oe, 1);
 
-	return oe->numlower ? oe->lowerstack[0].layer : NULL;
+	if (path)
+		return path->layer;
+	else
+		return NULL;
 }
 
 struct dentry *ovl_dentry_real(struct dentry *dentry)
-- 
1.8.3.1

[PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation

[Chengguang Xu]

ovl_*_lower() can handle lowerstack requirement well and more importantly
we can hide detail lowerstack implementation for outside. so using these
functions to replace bare array operaion.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/overlayfs/export.c    |  4 ++--
 fs/overlayfs/namei.c     | 18 ++++++++++--------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     |  8 ++++----
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 425a946..f2ba5fb 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -94,7 +94,7 @@ static int ovl_connectable_layer(struct dentry *dentry)
 		return 0;
 
 	/* We can get upper/overlay path from indexed/lower dentry */
-	return oe->lowerstack[0].layer->idx;
+	return ovl_layer_lower(dentry)->idx;
 }
 
 /*
@@ -115,7 +115,7 @@ static int ovl_connect_layer(struct dentry *dentry)
 	    WARN_ON(!ovl_dentry_lower(dentry)))
 		return -EIO;
 
-	origin_layer = OVL_E(dentry)->lowerstack[0].layer->idx;
+	origin_layer = ovl_layer_lower(dentry)->idx;
 	if (ovl_dentry_test_flag(OVL_E_CONNECTED, dentry))
 		return origin_layer;
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 2dba29e..eb3ec6c 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -779,6 +779,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_path *op;
 
 	BUG_ON(idx < 0);
 	if (idx == 0) {
@@ -788,8 +789,9 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
 		idx++;
 	}
 	BUG_ON(idx > oe->numlower);
-	path->dentry = oe->lowerstack[idx - 1].dentry;
-	path->mnt = oe->lowerstack[idx - 1].layer->mnt;
+	op = __ovl_path_lower(oe, idx);
+	path->dentry = op->dentry;
+	path->mnt = op->layer->mnt;
 
 	return (idx < oe->numlower) ? idx + 1 : -1;
 }
@@ -895,14 +897,14 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	}
 
 	for (i = 0; !d.stop && i < poe->numlower; i++) {
-		struct ovl_path lower = poe->lowerstack[i];
+		struct ovl_path *lower = __ovl_path_lower(poe, i + 1);
 
 		if (!ofs->config.redirect_follow)
 			d.last = i == poe->numlower - 1;
 		else
-			d.last = lower.layer->idx == roe->numlower;
+			d.last = lower->layer->idx == roe->numlower;
 
-		err = ovl_lookup_layer(lower.dentry, &d, &this);
+		err = ovl_lookup_layer(lower->dentry, &d, &this);
 		if (err)
 			goto out_put;
 
@@ -938,7 +940,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 
 		stack[ctr].dentry = this;
-		stack[ctr].layer = lower.layer;
+		stack[ctr].layer = lower->layer;
 		ctr++;
 
 		/*
@@ -964,7 +966,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
 			poe = roe;
 			/* Find the current layer on the root dentry */
-			i = lower.layer->idx - 1;
+			i = lower->layer->idx - 1;
 		}
 	}
 
@@ -1065,7 +1067,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 	/* Positive upper -> have to look up lower to see whether it exists */
 	for (i = 0; !done && !positive && i < poe->numlower; i++) {
 		struct dentry *this;
-		struct dentry *lowerdir = poe->lowerstack[i].dentry;
+		struct dentry *lowerdir = __ovl_path_lower(poe, i + 1)->dentry;
 
 		this = lookup_one_len_unlocked(name->name, lowerdir,
 					       name->len);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index abb5cf4..8039602 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -214,6 +214,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
 void ovl_path_lower(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
+struct ovl_path *__ovl_path_lower(struct ovl_entry *oe, int layer);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
 struct vfsmount *ovl_mnt_lower(struct dentry *dentry);
 struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e8551c9..38c1dd1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -144,7 +144,7 @@ static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
 	int ret = 1;
 
 	for (i = 0; i < oe->numlower; i++) {
-		struct dentry *d = oe->lowerstack[i].dentry;
+		struct dentry *d = __ovl_path_lower(oe, i + 1)->dentry;
 
 		if (d->d_flags & DCACHE_OP_REVALIDATE) {
 			ret = d->d_op->d_revalidate(d, flags);
@@ -167,7 +167,7 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
 	int ret = 1;
 
 	for (i = 0; i < oe->numlower; i++) {
-		struct dentry *d = oe->lowerstack[i].dentry;
+		struct dentry *d = __ovl_path_lower(oe, i + 1)->dentry;
 
 		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
 			ret = d->d_op->d_weak_revalidate(d, flags);
@@ -1123,8 +1123,8 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
 		return err;
 
 	/* Verify lower root is upper root origin */
-	err = ovl_verify_origin(upperpath->dentry, oe->lowerstack[0].dentry,
-				true);
+	err = ovl_verify_origin(upperpath->dentry,
+				__ovl_path_lower(oe, 1)->dentry, true);
 	if (err) {
 		pr_err("overlayfs: failed to verify upper root origin\n");
 		goto out;
-- 
1.8.3.1

[PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry

[Chengguang Xu]

Introduce a dedicated cache pool for ovl_entry to optimize
memory allocation adn deallocation.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/overlayfs/export.c    | 11 +++++++----
 fs/overlayfs/namei.c     |  7 +++++--
 fs/overlayfs/overlayfs.h |  4 ++++
 fs/overlayfs/ovl_entry.h |  9 +++++----
 fs/overlayfs/super.c     | 38 +++++++++++++++++++++++++++++---------
 fs/overlayfs/util.c      | 28 ++++++++++++++++++++++++----
 6 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index f2ba5fb..b1047ae 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -321,8 +321,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 			goto nomem;
 
 		if (lower) {
-			oe->lowerstack->dentry = dget(lower);
-			oe->lowerstack->layer = lowerpath->layer;
+			oe->lowerstack.dentry = dget(lower);
+			oe->lowerstack.layer = lowerpath->layer;
 		}
 		dentry->d_fsdata = oe;
 		if (upper_alias)
@@ -346,9 +346,12 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
 	if (!idx)
 		return ovl_dentry_upper(dentry);
 
+	if (oe->numlower == 1 && oe->lowerstack.layer->idx == idx)
+		return oe->lowerstack.dentry;
+
 	for (i = 0; i < oe->numlower; i++) {
-		if (oe->lowerstack[i].layer->idx == idx)
-			return oe->lowerstack[i].dentry;
+		if (oe->lowerstacks[i].layer->idx == idx)
+			return oe->lowerstacks[i].dentry;
 	}
 
 	return NULL;
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index eb3ec6c..51fd914 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -994,7 +994,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (!oe)
 		goto out_put;
 
-	memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
+	if (oe->numlower == 1)
+		memcpy(&oe->lowerstack, stack, sizeof(struct ovl_path));
+	else
+		memcpy(oe->lowerstacks, stack, sizeof(struct ovl_path) * ctr);
 	dentry->d_fsdata = oe;
 
 	if (upperopaque)
@@ -1028,7 +1031,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 out_free_oe:
 	dentry->d_fsdata = NULL;
-	kfree(oe);
+	ovl_free_entry(oe);
 out_put:
 	dput(index);
 	for (i = 0; i < ctr; i++)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 8039602..a104e02 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -207,6 +207,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
 bool ovl_index_all(struct super_block *sb);
 bool ovl_verify_lower(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
+void ovl_free_entry(struct ovl_entry *oe);
 bool ovl_dentry_remote(struct dentry *dentry);
 bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
@@ -378,3 +379,6 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 
 /* export.c */
 extern const struct export_operations ovl_export_operations;
+
+/* super.c */
+extern struct kmem_cache *ovl_entry_cachep;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 41655a7..3ed8ab4 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -71,13 +71,14 @@ struct ovl_fs {
 /* private information held for every overlayfs dentry */
 struct ovl_entry {
 	union {
-		struct {
-			unsigned long flags;
-		};
+		unsigned long flags;
 		struct rcu_head rcu;
 	};
 	unsigned numlower;
-	struct ovl_path lowerstack[];
+	union {
+		struct ovl_path lowerstack;
+		struct ovl_path *lowerstacks;
+	};
 };
 
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 38c1dd1..6327497 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -60,8 +60,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;
 
-	for (i = 0; i < oe->numlower; i++)
-		dput(oe->lowerstack[i].dentry);
+	if (oe->numlower == 1)
+		dput(oe->lowerstack.dentry);
+	else
+		for (i = 0; i < oe->numlower; i++)
+			dput(oe->lowerstacks[i].dentry);
 }
 
 static void ovl_dentry_release(struct dentry *dentry)
@@ -191,6 +194,7 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
 };
 
 static struct kmem_cache *ovl_inode_cachep;
+struct kmem_cache *ovl_entry_cachep;
 
 static struct inode *ovl_alloc_inode(struct super_block *sb)
 {
@@ -1332,9 +1336,14 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (!oe)
 		goto out_err;
 
-	for (i = 0; i < numlower; i++) {
-		oe->lowerstack[i].dentry = dget(stack[i].dentry);
-		oe->lowerstack[i].layer = &ofs->lower_layers[i];
+	if (numlower == 1) {
+		oe->lowerstack.dentry = stack[0].dentry;
+		oe->lowerstack.layer = &ofs->lower_layers[0];
+	} else {
+		for (i = 0; i < numlower; i++) {
+			oe->lowerstacks[i].dentry = dget(stack[i].dentry);
+			oe->lowerstacks[i].layer = &ofs->lower_layers[i];
+		}
 	}
 
 	if (remote)
@@ -1515,20 +1524,31 @@ static void ovl_inode_init_once(void *foo)
 
 static int __init ovl_init(void)
 {
-	int err;
+	int err = -ENOMEM;
 
 	ovl_inode_cachep = kmem_cache_create("ovl_inode",
 					     sizeof(struct ovl_inode), 0,
 					     (SLAB_RECLAIM_ACCOUNT|
 					      SLAB_MEM_SPREAD|SLAB_ACCOUNT),
 					     ovl_inode_init_once);
-	if (ovl_inode_cachep == NULL)
-		return -ENOMEM;
+	if (!ovl_inode_cachep)
+		return err;
+
+	ovl_entry_cachep = KMEM_CACHE(ovl_entry,
+				SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
+	if (!ovl_entry_cachep)
+		goto bad_entry_cachep;
 
 	err = register_filesystem(&ovl_fs_type);
 	if (err)
-		kmem_cache_destroy(ovl_inode_cachep);
+		goto err_out;
 
+	return 0;
+
+err_out:
+	kmem_cache_destroy(ovl_entry_cachep);
+bad_entry_cachep:
+	kmem_cache_destroy(ovl_inode_cachep);
 	return err;
 }
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index a3459e6..7d4ff3e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -95,13 +95,30 @@ bool ovl_verify_lower(struct super_block *sb)
 	return ofs->config.nfs_export && ofs->config.index;
 }
 
+void ovl_free_entry(struct ovl_entry *oe)
+{
+	if (oe->numlower > 1)
+		kfree(oe->lowerstacks);
+
+	kmem_cache_free(ovl_entry_cachep, oe);
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
-	size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
-	struct ovl_entry *oe = kzalloc(size, GFP_KERNEL);
+	size_t size = sizeof(struct ovl_path) * numlower;
+	struct ovl_entry *oe;
 
-	if (oe)
+	oe = kmem_cache_zalloc(ovl_entry_cachep, GFP_KERNEL);
+	if (oe) {
 		oe->numlower = numlower;
+		if (numlower > 1) {
+			oe->lowerstacks = kzalloc(size, GFP_KERNEL);
+			if (!oe->lowerstacks) {
+				kmem_cache_free(ovl_entry_cachep, oe);
+				oe = NULL;
+			}
+		}
+	}
 
 	return oe;
 }
@@ -186,7 +203,10 @@ struct ovl_path *__ovl_path_lower(struct ovl_entry *oe, int layer)
 	if (layer > oe->numlower)
 		return NULL;
 
-	return &oe->lowerstack[layer - 1];
+	if (layer == 1 && oe->numlower == 1)
+		return &oe->lowerstack;
+	else
+		return &oe->lowerstacks[layer - 1];
 }
 
 struct vfsmount *ovl_mnt_lower(struct dentry *dentry)
-- 
1.8.3.1

Re: [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry

[Amir Goldstein]

On Tue, Apr 17, 2018 at 5:42 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
> Introduce a dedicated cache pool for ovl_entry to optimize
> memory allocation adn deallocation.
>
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/overlayfs/export.c    | 11 +++++++----
>  fs/overlayfs/namei.c     |  7 +++++--
>  fs/overlayfs/overlayfs.h |  4 ++++
>  fs/overlayfs/ovl_entry.h |  9 +++++----
>  fs/overlayfs/super.c     | 38 +++++++++++++++++++++++++++++---------
>  fs/overlayfs/util.c      | 28 ++++++++++++++++++++++++----
>  6 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index f2ba5fb..b1047ae 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -321,8 +321,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>                         goto nomem;
>
>                 if (lower) {
> -                       oe->lowerstack->dentry = dget(lower);
> -                       oe->lowerstack->layer = lowerpath->layer;
> +                       oe->lowerstack.dentry = dget(lower);
> +                       oe->lowerstack.layer = lowerpath->layer;
>                 }
>                 dentry->d_fsdata = oe;
>                 if (upper_alias)
> @@ -346,9 +346,12 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
>         if (!idx)
>                 return ovl_dentry_upper(dentry);
>
> +       if (oe->numlower == 1 && oe->lowerstack.layer->idx == idx)
> +               return oe->lowerstack.dentry;
> +
>         for (i = 0; i < oe->numlower; i++) {
> -               if (oe->lowerstack[i].layer->idx == idx)
> -                       return oe->lowerstack[i].dentry;
> +               if (oe->lowerstacks[i].layer->idx == idx)
> +                       return oe->lowerstacks[i].dentry;
>         }
>
>         return NULL;
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index eb3ec6c..51fd914 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -994,7 +994,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         if (!oe)
>                 goto out_put;
>
> -       memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
> +       if (oe->numlower == 1)
> +               memcpy(&oe->lowerstack, stack, sizeof(struct ovl_path));
> +       else
> +               memcpy(oe->lowerstacks, stack, sizeof(struct ovl_path) * ctr);
>         dentry->d_fsdata = oe;
>
>         if (upperopaque)
> @@ -1028,7 +1031,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
>  out_free_oe:
>         dentry->d_fsdata = NULL;
> -       kfree(oe);
> +       ovl_free_entry(oe);
>  out_put:
>         dput(index);
>         for (i = 0; i < ctr; i++)
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8039602..a104e02 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -207,6 +207,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
>  bool ovl_index_all(struct super_block *sb);
>  bool ovl_verify_lower(struct super_block *sb);
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
> +void ovl_free_entry(struct ovl_entry *oe);
>  bool ovl_dentry_remote(struct dentry *dentry);
>  bool ovl_dentry_weird(struct dentry *dentry);
>  enum ovl_path_type ovl_path_type(struct dentry *dentry);
> @@ -378,3 +379,6 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>
>  /* export.c */
>  extern const struct export_operations ovl_export_operations;
> +
> +/* super.c */
> +extern struct kmem_cache *ovl_entry_cachep;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 41655a7..3ed8ab4 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -71,13 +71,14 @@ struct ovl_fs {
>  /* private information held for every overlayfs dentry */
>  struct ovl_entry {
>         union {
> -               struct {
> -                       unsigned long flags;
> -               };
> +               unsigned long flags;
>                 struct rcu_head rcu;
>         };
>         unsigned numlower;
> -       struct ovl_path lowerstack[];
> +       union {
> +               struct ovl_path lowerstack;
> +               struct ovl_path *lowerstacks;
> +       };

1. The names are not representative to what they stand for
'stack' is something containing many items already, so you
want 'lowerpath' (numlower == 1) and 'lowerstack' (numlower > 1).

2. I suggested a different approach, not sure if it is better.
I will repeat it in case you did not understand me:

       struct ovl_path lowerstack[1];

this approach requires no union and most of the code that does
if (numlower == 1) can be removed.
Only need special casing alloc and free (see below).

>  };
>
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 38c1dd1..6327497 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -60,8 +60,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
>  {
>         unsigned int i;
>
> -       for (i = 0; i < oe->numlower; i++)
> -               dput(oe->lowerstack[i].dentry);
> +       if (oe->numlower == 1)
> +               dput(oe->lowerstack.dentry);
> +       else
> +               for (i = 0; i < oe->numlower; i++)
> +                       dput(oe->lowerstacks[i].dentry);

No special casing here.

>  }
>
>  static void ovl_dentry_release(struct dentry *dentry)
> @@ -191,6 +194,7 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  };
>
>  static struct kmem_cache *ovl_inode_cachep;
> +struct kmem_cache *ovl_entry_cachep;
>
>  static struct inode *ovl_alloc_inode(struct super_block *sb)
>  {
> @@ -1332,9 +1336,14 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         if (!oe)
>                 goto out_err;
>
> -       for (i = 0; i < numlower; i++) {
> -               oe->lowerstack[i].dentry = dget(stack[i].dentry);
> -               oe->lowerstack[i].layer = &ofs->lower_layers[i];
> +       if (numlower == 1) {
> +               oe->lowerstack.dentry = stack[0].dentry;
> +               oe->lowerstack.layer = &ofs->lower_layers[0];
> +       } else {
> +               for (i = 0; i < numlower; i++) {
> +                       oe->lowerstacks[i].dentry = dget(stack[i].dentry);
> +                       oe->lowerstacks[i].layer = &ofs->lower_layers[i];
> +               }

No special casing here.

>         }
>
>         if (remote)
> @@ -1515,20 +1524,31 @@ static void ovl_inode_init_once(void *foo)
>
>  static int __init ovl_init(void)
>  {
> -       int err;
> +       int err = -ENOMEM;
>
>         ovl_inode_cachep = kmem_cache_create("ovl_inode",
>                                              sizeof(struct ovl_inode), 0,
>                                              (SLAB_RECLAIM_ACCOUNT|
>                                               SLAB_MEM_SPREAD|SLAB_ACCOUNT),
>                                              ovl_inode_init_once);
> -       if (ovl_inode_cachep == NULL)
> -               return -ENOMEM;
> +       if (!ovl_inode_cachep)
> +               return err;
> +
> +       ovl_entry_cachep = KMEM_CACHE(ovl_entry,
> +                               SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD);
> +       if (!ovl_entry_cachep)
> +               goto bad_entry_cachep;
>
>         err = register_filesystem(&ovl_fs_type);
>         if (err)
> -               kmem_cache_destroy(ovl_inode_cachep);
> +               goto err_out;
>
> +       return 0;
> +
> +err_out:
> +       kmem_cache_destroy(ovl_entry_cachep);
> +bad_entry_cachep:
> +       kmem_cache_destroy(ovl_inode_cachep);
>         return err;
>  }
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index a3459e6..7d4ff3e 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -95,13 +95,30 @@ bool ovl_verify_lower(struct super_block *sb)
>         return ofs->config.nfs_export && ofs->config.index;
>  }
>
> +void ovl_free_entry(struct ovl_entry *oe)
> +{
> +       if (oe->numlower > 1)
> +               kfree(oe->lowerstacks);
> +

      if (oe->numlower > 1)
              kfree(oe);
      else

> +       kmem_cache_free(ovl_entry_cachep, oe);
> +}
> +
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
>  {
> -       size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
> -       struct ovl_entry *oe = kzalloc(size, GFP_KERNEL);
> +       size_t size = sizeof(struct ovl_path) * numlower;
> +       struct ovl_entry *oe;
>
> -       if (oe)
> +       oe = kmem_cache_zalloc(ovl_entry_cachep, GFP_KERNEL);

    if (numlower > 1) {
       size = offsetof(struct ovl_entry, lowerstack[numlower]);
       oe = kzalloc(size, GFP_KERNEL);
   } else {
       oe = kmem_cache_zalloc(ovl_entry_cachep, GFP_KERNEL);
   }

> +       if (oe) {
>                 oe->numlower = numlower;
> +               if (numlower > 1) {
> +                       oe->lowerstacks = kzalloc(size, GFP_KERNEL);
> +                       if (!oe->lowerstacks) {
> +                               kmem_cache_free(ovl_entry_cachep, oe);
> +                               oe = NULL;
> +                       }
> +               }
> +       }
>
>         return oe;
>  }
> @@ -186,7 +203,10 @@ struct ovl_path *__ovl_path_lower(struct ovl_entry *oe, int layer)
>         if (layer > oe->numlower)
>                 return NULL;
>
> -       return &oe->lowerstack[layer - 1];
> +       if (layer == 1 && oe->numlower == 1)
> +               return &oe->lowerstack;
> +       else
> +               return &oe->lowerstacks[layer - 1];
>  }
>

No special casing here.

Thanks,
Amir.

Re: [PATCH 1/3] ovl: refactor lowerstack related functions

[Amir Goldstein]

On Tue, Apr 17, 2018 at 5:42 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
> Introduce __ovl_path_lower() to handle lowerstack related things
> and refactor ovl_dentry_lower()/ovl_layer_lower() based on it.
>
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/util.c      | 35 +++++++++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index e0b7de7..abb5cf4 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -215,6 +215,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> +struct vfsmount *ovl_mnt_lower(struct dentry *dentry);
>  struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
>  struct dentry *ovl_dentry_real(struct dentry *dentry);
>  struct dentry *ovl_i_dentry_upper(struct inode *inode);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 6f10780..a3459e6 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -157,8 +157,8 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
>         struct ovl_entry *oe = dentry->d_fsdata;
>
>         if (oe->numlower) {
> -               path->mnt = oe->lowerstack[0].layer->mnt;
> -               path->dentry = oe->lowerstack[0].dentry;
> +               path->mnt = ovl_mnt_lower(dentry);
> +               path->dentry = ovl_dentry_lower(dentry);
>         } else {
>                 *path = (struct path) { };
>         }
> @@ -181,18 +181,45 @@ struct dentry *ovl_dentry_upper(struct dentry *dentry)
>         return ovl_upperdentry_dereference(OVL_I(d_inode(dentry)));
>  }
>
> +struct ovl_path *__ovl_path_lower(struct ovl_entry *oe, int layer)

The name is going to be confused with ovl_path_lower, but
I don't have a better suggestion.
For the @layer arg I propose two options.
Either @idx for layer index, like other helpers
Or simply @i for lower layer index, because it doesn't make much sense
to request idx=0 (upper) from a helper called xxx_lower().
If you choose to stay with @idx convention, you should add
if (WARN_ON(idx < 1)) return NULL;

> +{
> +       if (layer > oe->numlower)
> +               return NULL;
> +
> +       return &oe->lowerstack[layer - 1];
> +}
> +
> +struct vfsmount *ovl_mnt_lower(struct dentry *dentry)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       struct ovl_path *path = __ovl_path_lower(oe, 1);
> +
> +       if (path)
> +               return path->layer->mnt;
> +       else
> +               return NULL;
> +}
> +
>  struct dentry *ovl_dentry_lower(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = dentry->d_fsdata;
> +       struct ovl_path *path = __ovl_path_lower(oe, 1);
>
> -       return oe->numlower ? oe->lowerstack[0].dentry : NULL;
> +       if (path)
> +               return path->dentry;
> +       else
> +               return NULL;
>  }
>
>  struct ovl_layer *ovl_layer_lower(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = dentry->d_fsdata;
> +       struct ovl_path *path = __ovl_path_lower(oe, 1);
>
> -       return oe->numlower ? oe->lowerstack[0].layer : NULL;
> +       if (path)
> +               return path->layer;
> +       else
> +               return NULL;
>  }
>


I am not yet convinced that this re-factoring is needed,
but if you keep it, please preserve the style:

      struct ovl_path *op = __ovl_path_lower(OVL_E(dentry), 1);

      return op ? op->layer : NULL;


Thanks,
Amir.

Re: [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation

[Amir Goldstein]

On Tue, Apr 17, 2018 at 5:42 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
> ovl_*_lower() can handle lowerstack requirement well and more importantly
> we can hide detail lowerstack implementation for outside. so using these
> functions to replace bare array operaion.
>

In not using bare array operation a worthy goal??
If you follow my suggestion in patch 3/3, you will be able to avoid this goal
and keep using array operations instead of __ovl_path_lower().

Thanks,
Amir.

Re: [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry

[cgxu519]

在 2018年4月17日，下午6:41，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Tue, Apr 17, 2018 at 5:42 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
>> Introduce a dedicated cache pool for ovl_entry to optimize
>> memory allocation adn deallocation.
>> 
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>> fs/overlayfs/export.c    | 11 +++++++----
>> fs/overlayfs/namei.c     |  7 +++++--
>> fs/overlayfs/overlayfs.h |  4 ++++
>> fs/overlayfs/ovl_entry.h |  9 +++++----
>> fs/overlayfs/super.c     | 38 +++++++++++++++++++++++++++++---------
>> fs/overlayfs/util.c      | 28 ++++++++++++++++++++++++----
>> 6 files changed, 74 insertions(+), 23 deletions(-)
>> 
>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>> index f2ba5fb..b1047ae 100644
>> --- a/fs/overlayfs/export.c
>> +++ b/fs/overlayfs/export.c
>> @@ -321,8 +321,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>>                        goto nomem;
>> 
>>                if (lower) {
>> -                       oe->lowerstack->dentry = dget(lower);
>> -                       oe->lowerstack->layer = lowerpath->layer;
>> +                       oe->lowerstack.dentry = dget(lower);
>> +                       oe->lowerstack.layer = lowerpath->layer;
>>                }
>>                dentry->d_fsdata = oe;
>>                if (upper_alias)
>> @@ -346,9 +346,12 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
>>        if (!idx)
>>                return ovl_dentry_upper(dentry);
>> 
>> +       if (oe->numlower == 1 && oe->lowerstack.layer->idx == idx)
>> +               return oe->lowerstack.dentry;
>> +
>>        for (i = 0; i < oe->numlower; i++) {
>> -               if (oe->lowerstack[i].layer->idx == idx)
>> -                       return oe->lowerstack[i].dentry;
>> +               if (oe->lowerstacks[i].layer->idx == idx)
>> +                       return oe->lowerstacks[i].dentry;
>>        }
>> 
>>        return NULL;
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index eb3ec6c..51fd914 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -994,7 +994,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>        if (!oe)
>>                goto out_put;
>> 
>> -       memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>> +       if (oe->numlower == 1)
>> +               memcpy(&oe->lowerstack, stack, sizeof(struct ovl_path));
>> +       else
>> +               memcpy(oe->lowerstacks, stack, sizeof(struct ovl_path) * ctr);
>>        dentry->d_fsdata = oe;
>> 
>>        if (upperopaque)
>> @@ -1028,7 +1031,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> 
>> out_free_oe:
>>        dentry->d_fsdata = NULL;
>> -       kfree(oe);
>> +       ovl_free_entry(oe);
>> out_put:
>>        dput(index);
>>        for (i = 0; i < ctr; i++)
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 8039602..a104e02 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -207,6 +207,7 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
>> bool ovl_index_all(struct super_block *sb);
>> bool ovl_verify_lower(struct super_block *sb);
>> struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
>> +void ovl_free_entry(struct ovl_entry *oe);
>> bool ovl_dentry_remote(struct dentry *dentry);
>> bool ovl_dentry_weird(struct dentry *dentry);
>> enum ovl_path_type ovl_path_type(struct dentry *dentry);
>> @@ -378,3 +379,6 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>> 
>> /* export.c */
>> extern const struct export_operations ovl_export_operations;
>> +
>> +/* super.c */
>> +extern struct kmem_cache *ovl_entry_cachep;
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index 41655a7..3ed8ab4 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -71,13 +71,14 @@ struct ovl_fs {
>> /* private information held for every overlayfs dentry */
>> struct ovl_entry {
>>        union {
>> -               struct {
>> -                       unsigned long flags;
>> -               };
>> +               unsigned long flags;
>>                struct rcu_head rcu;
>>        };
>>        unsigned numlower;
>> -       struct ovl_path lowerstack[];
>> +       union {
>> +               struct ovl_path lowerstack;
>> +               struct ovl_path *lowerstacks;
>> +       };
> 
> 1. The names are not representative to what they stand for
> 'stack' is something containing many items already, so you
> want 'lowerpath' (numlower == 1) and 'lowerstack' (numlower > 1).
> 
> 2. I suggested a different approach, not sure if it is better.
> I will repeat it in case you did not understand me:
> 
>       struct ovl_path lowerstack[1];
> 
> this approach requires no union and most of the code that does
> if (numlower == 1) can be removed.
> Only need special casing alloc and free (see below).

Sounds better, let's try this.

Thanks,
Chengguang.

Re: [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation

[cgxu519]

> 在 2018年4月17日，下午7:16，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Tue, Apr 17, 2018 at 5:42 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
>> ovl_*_lower() can handle lowerstack requirement well and more importantly
>> we can hide detail lowerstack implementation for outside. so using these
>> functions to replace bare array operaion.
>> 
> 
> In not using bare array operation a worthy goal??
> If you follow my suggestion in patch 3/3, you will be able to avoid this goal
> and keep using array operations instead of __ovl_path_lower().

In this patch series, hiding detail operation of array is necessary,
your suggestion makes the modification shorter and simpler but maybe
lose a bit of flexibility when trying to change lowerstack layers. 
However, we don’t have to worry about that too much on this stage.
I’ll repost modified patch.

Thanks,
Chengguang.