Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Amir Goldstein <amir73il@gmail.com>]

> > > @@ -1023,7 +1020,7 @@ struct dentry *ovl_lookup(struct inode *
> > >          *
> > >          * Always lookup index of non-dir non-metacopy and non-upper.
> > >          */
> > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > +       if (ctr && (!upperdentry || (!d.is_dir && !uppermetacopy)))
> > >                 origin = stack[0].dentry;
> > >
> >
> > I think this should be:
> >
> >           * Always lookup index of non-dir and non-upper.
> >           */
> >           if (!origin && ctr && (!upperdentry || !d.is_dir))
> >                  origin = stack[0].dentry;
> >
> > uppermetacopy is guaranteed to either have origin already set or
> > exit with an an error for ovl_verify_origin().
>
> Only if index is enabled and upper had origin xattr.
>
> (!d.is_dir && ofs->config.index && origin_path)
>
> So if index is disabled or uppermetacopy did not have "origin" xattr,
> we will not have origin set by the time we come out of the loop.
>

True. But if index is disabled, setting origin is moot. origin is only
ever used here to lookup the index.

About "origin" xattr. If it is not set in upper that lower fs probably does
not have file handle support. In that case, index cannot be enabled
anyway.

> I see for non-metacopy regular files, if upper did not have origin
> xattr, that means origin_path will by NULL. That means ctr will be
> 0 and that means we will not set "origin" for non-metacopy regular
> files in such case. So question is, should we set "origin" for
> metacopy upper files in such a case.
>
> We did not have origin xattr, but we looked up lower layers for
> upper metacopy. In theory, stack[0].dentry is origin for upper
> metacopy files. Should we use it? Current logic does not and that's
> why this additiona check (!d.is_dir && !uppermetacopy).
>

I agree with your analysis, but this is a very theoretical discussion.
Unless I am missing something, I think we have written a very complex
condition for a corner case that doesn't seem to be valid or interesting.

Basically, for non-dir, if there is no "origin" xattr, then there should be no
index, because the metacopy feature was added way long after we
started storing "origin" on copy up. That's not the case for directories.

There is one corner case where it may be relevant -
overlay layers with metacopy that were created on fs with no file handle
support (or no uuid) that are migrated to a filesystem with file handle
support (and metacopy xattr are preserved in migration).
In that case, index may be enabled while upper metacopy exists
without "origin".

What happens if we do not set origin and do not lookup index in that case?
We can get two overlay inodes, both from different metacopy upper inodes
redirected to the same lower inode, that have the same st_ino, but differnt
metadata.

I suppose we should have a protection in place for not making broken
upper hardlinks that are metacopy, but I don't see it...

> >
> > HOWEVER, if we set origin to lower, which turns out to be a lower
> > metacopy, we then skip this layer to the next one, but origin remains
> > set on the skipped layer dentry, which we had already dput().
> > Ay ay ay!
>
> We only skip the intermediate metacopy entries in lower. So top most
> lower metacopy will still be retained. For example, if there are 3
> lower layers where top two are metacopy and one data, then we will
> only skip middle one. And middle one should not be origin for upper.
>
>                 /*
>                  * Do not store intermediate metacopy dentries in chain,
>                  * except top most lower metacopy dentry
>                  */
>                 if (d.metacopy && ctr) {
>                         dput(this);
>                         continue;
>                 }
>
> For the first lower, ctr will be 0 and we will always store it in
> stack. So if it is metacopy dentry, it will still be stored at
> stack[0].
>
> Do you still see the problem?

No. it's fine. My eyes missed the ctr condition.
I still think since you are changing this code.
It will be much easier to follow if both simple continue statement
are at the top of the loop.

Thanks,
Amir.