Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature

[Amir Goldstein]

On Thu, Sep 24, 2020 at 4:18 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>
> On Thu, Sep 24, 2020 at 05:44:22AM +0300, Amir Goldstein wrote:
> > On Wed, Sep 23, 2020 at 10:47 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Sep 23, 2020 at 06:23:08PM +0300, Pavel Tikhomirov wrote:
> > > > This relaxes uuid checks for overlay index feature. It is only possible
> > > > in case there is only one filesystem for all the work/upper/lower
> > > > directories and bare file handles from this backing filesystem are uniq.
> > >
> > > Hi Pavel,
> > >
> > > Wondering why upper/work has to be on same filesystem as lower for this to
> > > work?
> > >
> >
> > I reckon that's because I asked for this constraint, so I will answer.
> >
> > You are right that the important thing is that all lower layers are
> > on the same fs, but because of
> >   a888db310195 ovl: fix regression with re-formatted lower squashfs
>
> Hi Amir,
>
> So with "upper on same as lower fs" contstraint we are just making it
> little harder so that people don't use recreated lower with existing
> upper? Is that the intention behind this constraint.
>
> On a side note, I have a question about above commit.
>
> So this is basically the issue of upper stored file handle resolving to
> a different file (in recreated lower). And we are considering this to
> be a corner case. But the very fact a user was running into it, it
> probably is not that hard to reproduce. So with the fix a888db310195,
> we avoided the problem for simple configurations (no-index, no-metacopy,
> and no xino). But if same user runs with index=on, with recreatd lower,
> they can still run into similar issues?
>
> >
> > I preferred to keep the rules simpler.
> >
> > Pavel's use case is clone of disk and change of its UUID.
> > This is a real use case and I don't think it is unique to Virtuozzo,
> > so I wanted index=nouuid to address that use case only and
> > I prefer that it is documented that way too.
>
> Sure. I understand that. I am only harping on this to make sure
> we tell people to not use this "recreated lower with existing upper".
> In Pavel's use case, it is more of a cloned use case and not
> re-created use case.
>
> Otherwise people will re-create lower layers with regular filesystems and
> use index=nouuid and then run into squashfs like issue one day.
>
> Or we could document what Miklos had said. Using existing upper
> with recreated lower will likely run into issues with advanced
> overlay features like (index, metacopy, xino etc).
>

I am perfectly fine with saying that
and with allowing the special case of cloning disk with index=nouuid.

There was a "patch" floating around for improving the doc,
I was assuming you will pick it up add your own proposed changes
and make it into a proper patch.

> >
> > Ironically, one of the justifications for index=nouuid is virtiofs -
> > because fuse is now allowed as upper (or as same fs),
> > one can already use fuse passthough (or one could use fuse
> > passthrough when nfs export works correctly) as a "uuid anonymizer"
> > for any fs, so in practice, index=nouuid cannot do any more harm
> > then one can already do when enabling index over virtiofs.
>
> Interesing. Using virtiofs or a fuse passthrough filesystem on top
> just to avoid uuid check will be lot of work.
>
> But keeping upper/ on same fs as lower fs constraint does not help with this.
>

No, it does not. I am only saying index=nouuid is "just" as bad
as what people can already do with virtiofs. Not much worse.

> >
> > That is why I prefer the interpretation that index=nouuid means
> > "use null uuid instead of s_uuid for ovl_fh" over the interpretation
> > "relax comparison of uuid in ovl_fh".
>
> So bottom line is that there are many ways where users can recreate
> lower layers and run into issues.
>
> - squashfs with index
> - use a fuse passthrough filesystem
> - use index=nouuid
>
> So to me documenting that don't use existig upper with recreated lower
> should help with all.
>
> And putting a constraint of "lower and upper being on same fs" seems fine
> for now but I am not sure it helps a lot. Anyway, I am fine with this
> constratint. Just wanted to understand the rationale behind it.
>

Only rational is - it is intended for cloned disk - don't make it easy
to use this for
anything else.

Thanks,
Amir.