Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Jan 05, 2018 at 01:22:34PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 7:05 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> 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@gmail.com>
> >>> ---
> >>>  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

Agreed. Just for the case of ovl_lookup(), storing idx in layer probably
is a good idea.

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

I must admit, I can't understand this. Please elaborate a bit more.

Vivek
> 
> Cheers,
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html