Re: [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Amir Goldstein <amir73il@gmail.com>]

On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> overlayfs can keep index of copied up files and directories and it
> seems to serve two primary puroposes. For regular files, it avoids
> breaking lower hardlinks over copy up. For directories it seems to
> be used for various error checks.
>
> During ovl_lookup(), we lookup for index using lower dentry in many
> a cases. That lower dentry is called "origin" and following is a summary
> of current logic.
>
> If there is no upperdentry, always lookup for index using lower dentry.
> For regular files it helps avoiding breaking hard links over copyup
> and for directories it seems to be just error checks.
>
> If there is an upperdentry, then there are 3 possible cases.
>
> - For directories, lower dentry is found using two ways. One is regular
>   path based lookup in lower layers and second is using ORIGIN xattr
>   on upper dentry. First verify that path based lookup lower dentry
>   matches the one pointed by upper ORIGIN xattr. If yes, use this
>   verified origin for index lookup.
>
> - For regular files (non-metacopy), there is no path based lookup in
>   lower layers as lookup stops once we find upper dentry. So there
>   is no origin verification. If there is ORIGIN xattr present on upper,
>   use that to lookup index otherwise don't.
>
> - For regular metacopy files, again lower dentry is found using
>   path based lookup as well as ORIGIN xattr on upper. Path based lookup
>   is continued in this case to find lower data dentry for metacopy
>   upper. So like directories we only use verified origin. If ORIGIN
>   xattr is not present (Either because lower did not support file
>   handles or because this is hardlink copied up with index=off), then
>   don't use path lookup based lower dentry as origin. This is same
>   as regular non-metacopy file case.
>

Very good summary.
You may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

But see one improvement below.
Also, please make sure to run unionmount setups:

./run --ov=10 --verify
./run --ov=10 --meta --verify

--verify will enable index and check st_dev;st_ino are not broken
on copy up. --ov=10 will cause lower hardlink copy up, because
after hardlink is creates by some test, upper is rotated to mid layer
and next modifying operation will trigger the hardlink copy up.

> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 0db23baf98e7..5d80d8cc0063 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1005,25 +1005,30 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>                 stack = origin_path;
>                 ctr = 1;
> +               origin = origin_path->dentry;
>                 origin_path = NULL;
>         }
>
[...]
> -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> +       if (!origin && ctr && !upperdentry)
>                 origin = stack[0].dentry;
>

No need to understand the long story to verify this change is correct.
This is true simply because the conditions to set stack = origin_path are:

       if (!metacopy && !d.is_dir && upperdentry && !ctr && origin_path)

And after getting there and setting ctr = 1, the complex conditions to
setting origin are met for certain:

        if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))

Therefore, it is logically equivalent (and makes much more sense)
to assign origin near stack = origin_path.

Further, thanks to Vivek's explanation, it is now clear to me that after
setting origin above, all that is left to do here is:

         /* Always lookup index of non-upper */
        if (!upperdentry)
                origin = stack[0].dentry;

All the upperdentry cases described in the commit message have already
been dealt with by the time we get here.

Thanks,
Amir.