On Fri, May 29, 2020 at 12:07:45AM +0300, Amir Goldstein wrote:
[..]
> > + /* Found a metacopy dentry but did not find corresponding data dentry */
> > + if (d.metacopy) {
> > + err = -EIO;
> > + goto out_put;
> > + }
> >
> > + if (lowermetacopy || uppermetacopy) {
> > err = -EPERM;
> > if (!ofs->config.metacopy) {
> > pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
>
> Move that test up to where setting metacopy = true for lower layers
> similar to "refusing to follow redirect" and make it:
> if (uppermetacopy || d.metacopy) {
>
> Then you got rid of lowermetacopy.
Agreed. Will change.
>
> > @@ -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.
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).
>
> 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?
>
> I think it would be best to move the check
> * Do not store intermediate metacopy dentries in chain,
> to right after ovl_lookup_layer(), before the ovl_fix_origin() and
> ovl_verify_origin() checks.
Thanks
Vivek
