On Fri, Jan 5, 2018 at 7:05 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
>>> Store the fs root layer index inside ovl_layer struct, so we can
>>> get the root fs layer index from merge dir lower layer instead of
>>> find it with ovl_find_layer() helper.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>>> ---
>>> fs/overlayfs/namei.c | 17 +----------------
>>> fs/overlayfs/ovl_entry.h | 2 ++
>>> fs/overlayfs/super.c | 1 +
>>> 3 files changed, 4 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>>> index 71db9a966d88..a48ee02c4524 100644
>>> --- a/fs/overlayfs/namei.c
>>> +++ b/fs/overlayfs/namei.c
>>> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
>>> return (idx < oe->numlower) ? idx + 1 : -1;
>>> }
>>>
>>> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
>>> -{
>>> - int i;
>>> -
>>> - for (i = 0; i < ofs->numlower; i++) {
>>> - if (ofs->lower_layers[i].mnt == path->layer->mnt)
>>> - break;
>>> - }
>>> -
>>> - return i;
>>> -}
>>> -
>>> /* Fix missing 'origin' xattr */
>>> static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
>>> struct dentry *upper)
>>> @@ -733,11 +721,8 @@ 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 = ovl_find_layer(ofs, &lower);
>>> - if (WARN_ON(i == ofs->numlower))
>>> - break;
>>> + i = lower.layer->idx - 1;
>>> }
>>> }
>>>
>>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>>> index 9d0bc03bf6e4..608e48755070 100644
>>> --- a/fs/overlayfs/ovl_entry.h
>>> +++ b/fs/overlayfs/ovl_entry.h
>>> @@ -22,6 +22,8 @@ struct ovl_config {
>>> struct ovl_layer {
>>> struct vfsmount *mnt;
>>> dev_t pseudo_dev;
>>> + /* Index of this layer in fs root (upper == 0) */
>>> + int idx;
>>
>> Just curious. If we are stroing idx, then do we have to store ovl_layer
>> and every ovl_entry. Just idx can give us the position and we should
>> be able to then accessovl_find_layer mnt and pseudo_dev directly from
>> ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
>> pseudo_dev per dentry.
>>
>
> But we are not storing vfsmount and pseudo_dev per dentry
> We are storing &ofs->lower_layers[idx] per dentry.
> Do we gain anything from storing the idx instead?
> Most of the times code needs to access ofs->lower_layers[idx]
> fields (mnt mostly).
> The cases where idx itself matter are rare, for example:
> if dentry has numlower == 1 and idx > 1, this is a quick indication
> that a parent of lower dir may be copied up but not be indexed
> (because a dir on top of it in layer 1 is indexed).
>
> The current series does not make use of the example above,
> but it can be used to relax copy up of dir on encode when
> lower idx == 1.
>
Evidently, not only that the series does not use layer idx info
for decoding lower dir file handle, it does not use layer idx at all,
except for the ovl_lookup() change in this patch.
The patch is in the series because of an early attempt on mine
to implement connectable file handles, which I abandoned for now.
However:
1. I think that getting rid of ovl_find_layer() alone is a good enough reason
to store layer idx
2. layer idx turned out to be useful in several cases I worked on, like ovl-xino
and ovl-redirect-origin, so it probably doesn't hurt to store it
for future sake
3. Based on the current code, I can post a trivial patch to relax copy up of dir
on encode when ofs->nulower > 1 in case lower dir layer->idx == 1.
Just wasn't sure if that case is worth optimizing, so decided to wait for
comments on the ofs->nulower > 1 behavior.
Cheers,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
