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

[Vivek Goyal <vgoyal@redhat.com>]

On Sat, May 30, 2020 at 01:37:37PM +0300, Amir Goldstein wrote:
> 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.

Hi Amir,

I ran above configurations and it passes with the patches.

Thanks for these suggestions. I used to run only "./run --ov" so far.
It will be nice to have some documentation about --meta, --verify in README.

> 
> > 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;

This looks better. What about the case of non existing dentry with ctr=0.
In that case we will set origin to NULL. It still works but it probably
will be nice if we do.

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

Just making it explicit that we try to use lower as origin only if
some lower dentry was found.

Thanks
Vivek