Re: [PATCH] overlayfs, doc: Do not allow lower layer recreation with redirect_dir enabled

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Jul 09, 2020 at 05:49:05PM +0300, Amir Goldstein wrote:
> On Thu, Jul 9, 2020 at 5:14 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Thu, Jul 09, 2020 at 10:02:20AM -0400, Vivek Goyal wrote:
> > > Currently we seem to support lower layer recreation and re-use with existing
> > > upper until and unless "index" or "metadata only copy up" feature is
> > > enabled.
> > >
> > > If redirect_dir feature is enabled then re-creating/modifying lower layers
> > > will break things. For example.
> > >
> > > - mkdir lower lower/foo upper work merged
> > > - touch lower/foo/foo-child
> > > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > > - mv merged/foo merged/bar
> > > - ls merged/bar/ (this should list foo-child)
> > >
> > > - umount merged
> > > - mv lower/foo lower/baz
> > > - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,redirect_dir=on none merged
> > > - ls merged/bar/  (Now foo-child has disappeared)
> > >
> > > IOW, modifying lower layers did not crash overlay but it resulted in
> > > directory contents being lost and that can be unexpected. So don't
> > > support lower layer recreation/modification when redirect_dir is enabled
> > > at any point of time.
> 
> I don't understand why this has to do with redirect_dir.
> The same text holds also if you do not do mv merged/foo merged/bar

It does not. "mv foo bar" gets -EXDEV and then mv copies up whole directory
tree. So even if lower layer is modified, we still see "bar/foo-child".

So behavior is little different in two cases. In one case, we lose
directory tree (as lower renamed it) and in another case we still
see it(because we copied it up).

Now one can argue that probably user will expect it (as long as they
know what redirect_dir does and all the semantics of copy up).

If our goal is that we need to allow this (as long as system does
not crash and does not throw errors) and all the resulting behaviors
can be explained away, then we probably don't need this patch.

I am not sure why do we need to allow updating lower and reuse with
same upper. It is hard to support these configurations and resulting
behavior. So with this patch I am trying to cut down the number
of options which allow this configuraiton.

In general, if any overlay features stores metadata in upper about lower
layer, then with lower layer change, that metadata can be broken. And
we probably should not allow such configuraiton. I am approaching it
from this angle. (until and unless there is a strong use case that
why we need to continue to support it with newer overlay features).

Thanks
Vivek


> 
> 
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  Documentation/filesystems/overlayfs.rst | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > > index 660dbaf0b9b8..1d1a8da7fdbc 100644
> > > --- a/Documentation/filesystems/overlayfs.rst
> > > +++ b/Documentation/filesystems/overlayfs.rst
> > > @@ -371,8 +371,8 @@ conflict with metacopy=on, and will result in an error.
> > >  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
> > >  given.
> > >
> > > -Sharing and copying layers
> > > ---------------------------
> > > +Sharing, copying and recreating lower layers
> > > +--------------------------------------------
> > >
> > >  Lower layers may be shared among several overlay mounts and that is indeed
> > >  a very common practice.  An overlay mount may use the same lower layer
> > > @@ -388,8 +388,12 @@ though it will not result in a crash or deadlock.
> > >
> > >  Mounting an overlay using an upper layer path, where the upper layer path
> > >  was previously used by another mounted overlay in combination with a
> > > -different lower layer path, is allowed, unless the "inodes index" feature
> > > -or "metadata only copy up" feature is enabled.
> > > +different lower layer path, is allowed, unless any of the following features
> > > +is enabled at any point of time.
> > > +
> > > +- inode index
> > > +- metadata only copy up
> > > +- redirect_dir
> >
> > I probably should add "nfs_export" to the list as well. Though it is
> > implicitly there as enabling nfs export requires to enable index. But
> > saying it explicitly is even better.
> >
> 
> Ok you can mention it but xino is also relevant.
> I wonder if we should define "legacy mode" where no options expect for
> lowerdir,upperdir,workdir,default_permissions are provided
> 
> Thanks,
> Amir.
>