question of copying stack in ovl_lookup()

question of copying stack in ovl_lookup()

[cgxu519]

Hi guys,

I锟斤拷m a little curious for below code in ovl_lookup, is there any background for
copy stack instead of directly using it?

memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);


Thanks,
Chengguang.

Re: question of copying stack in ovl_lookup()

[Amir Goldstein]

On Tue, Apr 10, 2018 at 4:29 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
> Hi guys,
>
> I’m a little curious for below code in ovl_lookup, is there any background for
> copy stack instead of directly using it?
>
> memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>

Memory compaction.

stack has ofs->numlower entries, which could be very large,
but after lookup, oe->numlower can end up being much much
smaller. Also, ovl_alloc_entry() packs lowerstack together with
ovl_entry, so the overall memory usage of the long lived overlay
dentry cache entries is lower.

Cheers,
Amir.

Re: question of copying stack in ovl_lookup()

[cgxu519]

Hi Amir,

Thanks for explanation!


> 锟斤拷 2018锟斤拷4锟斤拷10锟秸ｏ拷锟斤拷锟斤拷1:38锟斤拷Amir Goldstein <amir73il@gmail.com> 写锟斤拷锟斤拷
> 
> On Tue, Apr 10, 2018 at 4:29 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>> Hi guys,
>> 
>> I锟斤拷m a little curious for below code in ovl_lookup, is there any background for
>> copy stack instead of directly using it?
>> 
>> memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>> 
> 
> Memory compaction.
> 
> stack has ofs->numlower entries, which could be very large,
> but after lookup, oe->numlower can end up being much much
> smaller. Also, ovl_alloc_entry() packs lowerstack together with
> ovl_entry, so the overall memory usage of the long lived overlay
> dentry cache entries is lower.
> 
> Cheers,
> Amir.

Re: question of copying stack in ovl_lookup()

[cgxu519]

> 锟斤拷 2018锟斤拷4锟斤拷10锟秸ｏ拷锟斤拷锟斤拷1:38锟斤拷Amir Goldstein <amir73il@gmail.com> 写锟斤拷锟斤拷
> 
> On Tue, Apr 10, 2018 at 4:29 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>> Hi guys,
>> 
>> I锟斤拷m a little curious for below code in ovl_lookup, is there any background for
>> copy stack instead of directly using it?
>> 
>> memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>> 
> 
> Memory compaction.
> 
> stack has ofs->numlower entries, which could be very large,
> but after lookup, oe->numlower can end up being much much
> smaller. Also, ovl_alloc_entry() packs lowerstack together with
> ovl_entry, so the overall memory usage of the long lived overlay
> dentry cache entries is lower.

For regular file the maximum numlower is 1, so for optimizing memory
allocation/free maybe we can put one ovl_path entry inside ovl_entry
and allocate/deallocate ovl_entry via cache pool and for dir we can
link to ovl_path array from ovl_entry. Does it make sense?

Thanks,
Chengguang.

Re: question of copying stack in ovl_lookup()

[Miklos Szeredi]

On Wed, Apr 11, 2018 at 7:41 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>
>> 在 2018年4月10日，下午1:38，Amir Goldstein <amir73il@gmail.com> 写道：
>>
>> On Tue, Apr 10, 2018 at 4:29 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>>> Hi guys,
>>>
>>> I’m a little curious for below code in ovl_lookup, is there any background for
>>> copy stack instead of directly using it?
>>>
>>> memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>>>
>>
>> Memory compaction.
>>
>> stack has ofs->numlower entries, which could be very large,
>> but after lookup, oe->numlower can end up being much much
>> smaller. Also, ovl_alloc_entry() packs lowerstack together with
>> ovl_entry, so the overall memory usage of the long lived overlay
>> dentry cache entries is lower.
>
> For regular file the maximum numlower is 1, so for optimizing memory
> allocation/free maybe we can put one ovl_path entry inside ovl_entry
> and allocate/deallocate ovl_entry via cache pool and for dir we can
> link to ovl_path array from ovl_entry. Does it make sense?

Yes.  We can do it for dir as well if there's only one lower layer
(it's going to be the common case).

I suggest you look at  dentry->d_name/d_iname for the implementation.

Thanks,
Miklos

Re: question of copying stack in ovl_lookup()

[Amir Goldstein]

On Wed, Apr 11, 2018 at 10:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Apr 11, 2018 at 7:41 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>>
>>> 在 2018年4月10日，下午1:38，Amir Goldstein <amir73il@gmail.com> 写道：
>>>
>>> On Tue, Apr 10, 2018 at 4:29 AM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
>>>> Hi guys,
>>>>
>>>> I’m a little curious for below code in ovl_lookup, is there any background for
>>>> copy stack instead of directly using it?
>>>>
>>>> memcpy(oe->lowerstack, stack, sizeof(struct ovl_path) * ctr);
>>>>
>>>
>>> Memory compaction.
>>>
>>> stack has ofs->numlower entries, which could be very large,
>>> but after lookup, oe->numlower can end up being much much
>>> smaller. Also, ovl_alloc_entry() packs lowerstack together with
>>> ovl_entry, so the overall memory usage of the long lived overlay
>>> dentry cache entries is lower.
>>
>> For regular file the maximum numlower is 1, so for optimizing memory
>> allocation/free maybe we can put one ovl_path entry inside ovl_entry
>> and allocate/deallocate ovl_entry via cache pool and for dir we can
>> link to ovl_path array from ovl_entry. Does it make sense?
>
> Yes.  We can do it for dir as well if there's only one lower layer
> (it's going to be the common case).
>

You can do this for dir not only if ofs->numlower == 1, but also if
oe->numlower == 1 and decide how to free oe depending on oe->numlower.

Pre-allocating one ovl_path is waste for pure upper entries, but I guess
that is the less common case(?).

The size of allocated ovl_entry for non-dir or merge dir with single lower
layer on 64bit machine is 40 bytes, which means quite a big waste with
kmalloc (24 bytes), so cache pool makes sense.

We could pack the ovl_entry further to 32 bytes with or without cache
pool. Not sure if it is worse it though(?):

struct ovl_path {
        unsigned short reserved;
        unsigned short layer_idx;
        struct dentry *dentry;
};

struct ovl_entry {
        union {
                struct {
                        unsigned long flags;
                };
                struct rcu_head rcu;
        };
        union {
                unsigned short numlower;
                struct ovl_path lowerstack[1];
        };
};

And for example:

static inline struct ovl_layer *ovl_dentry_layer(struct dentry *dentry, int idx)
{
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *oe = dentry->d_fsdata;

        return idx ? &ofs->layer_layers[oe->lowerstack[idx -
1]->layer_idx - 1] : NULL;
}

Thanks,
Amir.

Re: question of copying stack in ovl_lookup()

[Miklos Szeredi]

On Wed, Apr 11, 2018 at 10:35 AM, Amir Goldstein <amir73il@gmail.com> wrote:

> We could pack the ovl_entry further to 32 bytes with or without cache
> pool. Not sure if it is worse it though(?):
>
> struct ovl_path {
>         unsigned short reserved;
>         unsigned short layer_idx;
>         struct dentry *dentry;
> };
>
> struct ovl_entry {
>         union {
>                 struct {
>                         unsigned long flags;

Heh, this anonymous struct is no longer needed.

>                 };
>                 struct rcu_head rcu;

Not sure what are we protecting in overlay_entry with rcu.  A comment
somewhere would be good.  Will need to investigate...

>         };
>         union {
>                 unsigned short numlower;
>                 struct ovl_path lowerstack[1];
>         };
> };

Ugly.

Thanks,
Miklos