Date:   Thu, 24 Sep 2020 09:18:53 -0400
From:   Vivek Goyal <vgoyal@...hat.com>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Miklos Szeredi <miklos@...redi.hu>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes
 index feature

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@...hat.com> 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).

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

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

Thanks
Vivek

Powered by blists - more mailing lists