Re: [PATCH 2/3] overlayfs: ovl_lookup(): Use only uppermetacopy state

[Vivek Goyal <vgoyal@redhat.com>]

On Sat, May 30, 2020 at 02:01:28PM +0300, Amir Goldstein wrote:
[..]
> > @@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 }
> >         }
> >
> > -       if (metacopy) {
> > -               /*
> > -                * Found a metacopy dentry but did not find corresponding
> > -                * data dentry
> > -                */
> > -               if (d.metacopy) {
> > -                       err = -EIO;
> > -                       goto out_put;
> > -               }
> > +       /* Found a metacopy dentry but did not find corresponding data dentry */
> > +       if (d.metacopy) {
> 
> I suggested this change and I think it is correct, but it is correct for a bit
> of a subtle reason.
> It is correct because ovl_lookup_layer() (currently) cannot return NULL
> and set d.metacopy to false.
> So I suggest to be a bit more defensive and write this condition as:
> 
>        if (d.metacopy || (uppermetacopy && !ctr)) {

Ok, will do.

> 
> > +               err = -EIO;
> > +               goto out_put;
> > +       }
> >
> > -               err = -EPERM;
> > -               if (!ofs->config.metacopy) {
> > -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> > -                                           dentry);
> > -                       goto out_put;
> > -               }
> > -       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> > +       /* For regular non-metacopy upper dentries, there is no lower
> > +        * path based lookup, hence ctr will be zero. dentry found using
> > +        * ORIGIN xattr on upper, install it in stack.
> > +        */
> > +       if (!d.is_dir && upperdentry && !ctr && origin_path) {
> 
> I don't like this comment style for multi line comment and I don't
> like that you detached this if statement from else if.
> I think it made more sense with the else because this is (as you write)
> the non-metacopy case.

Will do in V2.

Thanks
Vivek