* [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation
2018-04-17 2:42 [PATCH 1/3] ovl: refactor lowerstack related functions Chengguang Xu
@ 2018-04-17 2:42 ` Chengguang Xu
2018-04-17 11:16 ` Amir Goldstein
2018-04-17 2:42 ` [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry Chengguang Xu
2018-04-17 11:04 ` [PATCH 1/3] ovl: refactor lowerstack related functions Amir Goldstein
2 siblings, 1 reply; 8+ messages in thread
From: Chengguang Xu @ 2018-04-17 2:42 UTC (permalink / raw)
To: linux-unionfs; +Cc: miklos, amir73il, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation
2018-04-17 2:42 ` [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation Chengguang Xu
@ 2018-04-17 11:16 ` Amir Goldstein
2018-04-18 3:42 ` cgxu519
0 siblings, 1 reply; 8+ messages in thread
From: Amir Goldstein @ 2018-04-17 11:16 UTC (permalink / raw)
To: Chengguang Xu; +Cc: overlayfs, Miklos Szeredi
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation
2018-04-17 11:16 ` Amir Goldstein
@ 2018-04-18 3:42 ` cgxu519
0 siblings, 0 replies; 8+ messages in thread
From: cgxu519 @ 2018-04-18 3:42 UTC (permalink / raw)
To: Amir Goldstein; +Cc: cgxu519, overlayfs, Miklos Szeredi
> 在 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry
2018-04-17 2:42 [PATCH 1/3] ovl: refactor lowerstack related functions Chengguang Xu
2018-04-17 2:42 ` [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation Chengguang Xu
@ 2018-04-17 2:42 ` Chengguang Xu
2018-04-17 10:41 ` Amir Goldstein
2018-04-17 11:04 ` [PATCH 1/3] ovl: refactor lowerstack related functions Amir Goldstein
2 siblings, 1 reply; 8+ messages in thread
From: Chengguang Xu @ 2018-04-17 2:42 UTC (permalink / raw)
To: linux-unionfs; +Cc: miklos, amir73il, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry
2018-04-17 2:42 ` [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry Chengguang Xu
@ 2018-04-17 10:41 ` Amir Goldstein
2018-04-18 0:59 ` cgxu519
0 siblings, 1 reply; 8+ messages in thread
From: Amir Goldstein @ 2018-04-17 10:41 UTC (permalink / raw)
To: Chengguang Xu; +Cc: overlayfs, Miklos Szeredi
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry
2018-04-17 10:41 ` Amir Goldstein
@ 2018-04-18 0:59 ` cgxu519
0 siblings, 0 replies; 8+ messages in thread
From: cgxu519 @ 2018-04-18 0:59 UTC (permalink / raw)
To: Amir Goldstein; +Cc: cgxu519, overlayfs, Miklos Szeredi
在 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] ovl: refactor lowerstack related functions
2018-04-17 2:42 [PATCH 1/3] ovl: refactor lowerstack related functions Chengguang Xu
2018-04-17 2:42 ` [PATCH 2/3] ovl: using lowestack handling functions to replace bare array operation Chengguang Xu
2018-04-17 2:42 ` [PATCH 3/3] ovl: introduce a dedicated cache pool for ovl_entry Chengguang Xu
@ 2018-04-17 11:04 ` Amir Goldstein
2 siblings, 0 replies; 8+ messages in thread
From: Amir Goldstein @ 2018-04-17 11:04 UTC (permalink / raw)
To: Chengguang Xu; +Cc: overlayfs, Miklos Szeredi
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.
^ permalink raw reply [flat|nested] 8+ messages in thread