Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Mar 08, 2018 at 10:43:09AM +0200, Amir Goldstein wrote:
> On Wed, Mar 7, 2018 at 10:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:
> [...]
> 
> >> >  static bool ovl_is_opaquedir(struct dentry *dentry)
> >> >  {
> >> >         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
> >> > @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >> >                 goto put_and_out;
> >> >         }
> >> >         if (!d_can_lookup(this)) {
> >> > -               d->stop = true;
> >> >                 if (d->is_dir)
> >>
> >> You need to set d->stop here because this is a non-dir below upper dir.
> >
> > With metacopy dentry on regular files, we want to do d->stop=true only
> > if this is not a metacopy. Otherwise we want to continue to do path
> > based lookup in lower layers. That's the whole point of this patch
> > series.
> >
> 
> Yes we do, but the point that yo missed about (d->is_dir) check is that it means
> that upper layer was a directory (e.g. upperdir/foo) and underneath it we find
> a non-dir (e.g. lowerdir/foo). This is a case were lookup needs to stop and
> result in a non-merge pure upper dir.

But d->is_dir does not necessarily reflect the state of upper. It
reflects the state of last found dentry. And in case of redirect, last
found dentry can very well be parent of current dentry.

For example, say you have upperdir/foo and it has redirect set to 
lowerdir/bar/bar-child.txt. Now, by the time we find bar-child.txt
d->is_dir is always true because it reflects the state of parent "bar"
dentry which is always directory. I mean it does not reflect state
of upperdir/foo. And that state is already lost.

If we really want to make sure that we don't merge upper dir with lower
non-dir, then we need to do this check in ovl_lookup() and not in
ovl_lookup_single(), IMHO.

> 
> >>
> >> >                         goto put_and_out;
> >> > +               err = ovl_check_metacopy_xattr(this);
> >> > +               if (err < 0)
> >> > +                       goto out_err;
> >> > +               if (!err) {
> >> > +                       d->stop = true;
> >> > +                       d->metacopy = false;
> >> > +               } else
> >> > +                       d->metacopy = true;
> >>
> >> Need to have {} in both if and else, but better not use if at all:
> >> d->stop = !err;
> >> d->metacopy = !!err;
> >
> > I think I will put {}. Its much easier to read the code that way.
> 
> Well, execution branch is less efficient and I don't know if compiler
> will optimize it away, but in any case I think d->metacopy = err is
> nicer because  ovl_check_metacopy_xattr() returns an error or
> an answer to is_metacopy.
> 
> [...]
> 
> >> > +               /*
> >> > +                * For non-dir dentry, make sure dentry found by lookup
> >> > +                * matches the origin stored in upper
> >> > +                */
> >> > +               if (!d.is_dir && upperdentry && !ctr && origin_path) {
> >> > +                       err = ovl_verify_origin(upperdentry, this, false);
> >> > +                       if (err) {
> >> > +                               dput(this);
> >> > +                               goto out_put;
> >> > +                       }
> >> > +               }
> >> > +
> >>
> >> Why is this code duplicated from d.is_dir case?
> >
> > Primarily because for the case of dir, you are verifying origin only
> > if nfs_export is enabled. While I doing this verification even when
> > nfs_export is not enabled.
> 
> (d.is_dir && ovl_verify_lower(dentry->d_sb)) || (!d.is_dir && origin_path)
> 
> The case of verifying dir found by path and file found by found is exactly
> the same case. You should just include the new case in the comment above
> current code. The idea is to make metacopy lookup as similar as possible
> to merge dir lookup, not to have a different code path for it.

Ok, will do.

> 
> >
> > So far by default we always installed origin in lowerstack[0] for non-dir.
> > Now we are doing lookup for non-dir files and looked up file might
> > be different from origin. So we need to make sure file found by lookup
> > is same as pointed by origin, otherwise something is not right?
> >
> 
> [...]
> 
> >> > +       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> >> > +               if (WARN_ON(stack != NULL)) {
> >> > +                       err = -EIO;
> >> > +                       goto out_put;
> >> > +               }
> >> > +               stack = origin_path;
> >> > +               ctr = 1;
> >> > +               origin_path = NULL;
> >>
> >> I am having a hard time understanding why origin_path is needed at all
> >> and why not lookup only by path and then ovl_verify_origin().
> >
> > Right now we look for non-dir file in lower only if upper is metacopy. So
> > if upper is not metacopy, then origin has to be installed in
> > lowerstack[0]. That's why I save origin in origin_path, and install it
> > in lowerstack[0] when there is no lower.
> 
> OK. you don't need to allocate origin_path, because it is limited to stack
> size of 1. See how ovl_verify_index() initializes stock and origin vars.

Ok, I can allocate origin_path on stack in caller instead of relying on
ovl_check_origin_fh() doing kmalloc.

> 
> >
> > Now other way could be that we lookup for dentry in lower even for
> > non-metacopy non-dir files and do ovl_verify_origin(). I think that's
> > what you are referring to. But that seems like unnecessary lookup in
> > lower. We don't have to do it.
> >
> 
> Nah, that will require also doing redirect on non-dir without metacopy.

Thanks
Vivek