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

[Amir Goldstein <amir73il@gmail.com>]

On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> Currently we use a variable "metacopy" which signifies that dentry
> could be either uppermetacopy or lowermetacopy. Amir suggested that
> we can move code around and use d.metacopy in such a way that we
> don't need lowermetacopy and just can do away with uppermetacopy.
>
> So this patch replaces "metacopy" with "uppermetacopy".
>
> It also moves some code little higher to keep reading little simpler.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 57 ++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5d80d8cc0063..a1889a160708 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct dentry *this;
>         unsigned int i;
>         int err;
> -       bool metacopy = false;
> +       bool uppermetacopy = false;
>         struct ovl_lookup_data d = {
>                 .sb = dentry->d_sb,
>                 .name = dentry->d_name,
> @@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                 goto out_put_upper;
>
>                         if (d.metacopy)
> -                               metacopy = true;
> +                               uppermetacopy = true;
>                 }
>
>                 if (d.redirect) {
> @@ -906,6 +906,22 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 if (!this)
>                         continue;
>
> +               if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> +                       err = -EPERM;
> +                       pr_warn_ratelimited("refusing to follow metacopy origin"
> +                                           " for (%pd2)\n", dentry);
> +                       goto out_put;
> +               }
> +
> +               /*
> +                * Do not store intermediate metacopy dentries in chain,
> +                * except top most lower metacopy dentry
> +                */
> +               if (d.metacopy && ctr) {
> +                       dput(this);
> +                       continue;
> +               }
> +
>                 /*
>                  * If no origin fh is stored in upper of a merge dir, store fh
>                  * of lower dir and set upper parent "impure".
> @@ -940,17 +956,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         origin = this;
>                 }
>
> -               if (d.metacopy)
> -                       metacopy = true;
> -               /*
> -                * Do not store intermediate metacopy dentries in chain,
> -                * except top most lower metacopy dentry
> -                */
> -               if (d.metacopy && ctr) {
> -                       dput(this);
> -                       continue;
> -               }
> -
>                 stack[ctr].dentry = this;
>                 stack[ctr].layer = lower.layer;
>                 ctr++;
> @@ -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)) {

> +               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.

Thanks,
Amir.