Re: [PATCH 08/11] ovl: Set OVL_METACOPY flag during ovl_lookup()

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Oct 18, 2017 at 08:06:19AM +0300, Amir Goldstein wrote:
> On Wed, Oct 18, 2017 at 12:05 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > During lookup, check for presence of OVL_XATTR_METACOPY and if present,
> > set OVL_METACOPY bit in flags.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 88ff1daeb3d7..d32da041f4bf 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -26,6 +26,24 @@ struct ovl_lookup_data {
> >         char *redirect;
> >  };
> >
> > +/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> > +static int ovl_check_metacopy(struct dentry *dentry)
> > +{
> > +       int res;
> > +
> > +       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> > +       if (res < 0) {
> > +               if (res == -ENODATA || res == -EOPNOTSUPP)
> > +                       return 0;
> > +               goto out;
> > +       }
> > +
> > +       return 1;
> > +out:
> > +       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> > +       return res;
> > +}
> > +
> >  static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
> >                               size_t prelen, const char *post)
> >  {
> > @@ -594,6 +612,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         struct dentry *this;
> >         unsigned int i;
> >         int err;
> > +       bool metacopy = false;
> >         struct ovl_lookup_data d = {
> >                 .name = dentry->d_name,
> >                 .is_dir = false,
> > @@ -634,6 +653,12 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                                                roe->numlower, &stack, &ctr);
> >                         if (err)
> >                                 goto out;
> > +
> > +                       err = ovl_check_metacopy(upperdentry);
> > +                       if (err < 0)
> > +                               goto out;
> > +                       if (err == 1)
> > +                               metacopy = true;
> 
> Miklos seems to prefer
>   metacopy = err;
>   if (err < 0)
>     goto out;
> 
> Not sure if it is just coding style or for better branch prediction code.

Ok, will change.

> 
> 
> >                 }
> >
> >                 if (d.redirect) {
> > @@ -719,6 +744,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 OVL_I(inode)->redirect = upperredirect;
> >                 if (index)
> >                         ovl_set_flag(OVL_INDEX, inode);
> > +
> > +               if (metacopy) {
> > +                       /*
> > +                        * Found an upper with metacopy set but at the same
> > +                        * time there is no lower dentry. Something is not
> > +                        * right.
> > +                        */
> > +                       if (!ctr) {
> 
> Please move this test up right after ovl_check_metacopy()
> There is no reason to wait as ctr is not going to grow for non-dir after
> that point.

Makes sense. Will do.

> 
> > +                               err = -ESTALE;
> > +                               goto out_put_inode;
> > +                       }
> > +                       ovl_set_flag(OVL_METACOPY, inode);
> > +               }
> >         }
> >
> >         revert_creds(old_cred);
> > @@ -729,6 +767,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >
> >         return NULL;
> >
> > +out_put_inode:
> > +       iput(inode);
> 
> And than new goto label becomes obsolete

Sounds good.

Vivek

> 
> >  out_free_oe:
> >         dentry->d_fsdata = NULL;
> >         kfree(oe);
> > --
> > 2.13.5
> >