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@xxxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > 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
