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

["cgxu519@gmx.com" <cgxu519@gmx.com>]

在 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.