Re: [PATCH v1 2/3] overlay: Add the ability to remount volatile directories when safe

From: Amir Goldstein <amir73il@gmail.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Sargun Dhillon <sargun@sargun.me>,
	overlayfs <linux-unionfs@vger.kernel.org>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Daniel J Walsh <dwalsh@redhat.com>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	David Howells <dhowells@redhat.com>
Subject: Re: [PATCH v1 2/3] overlay: Add the ability to remount volatile directories when safe
Date: Wed, 25 Nov 2020 21:37:07 +0200	[thread overview]
Message-ID: <CAOQ4uxj9HYv5XXOx=Z2JhQCZauUey7V_T5_AA+=Wn=+hWLOy1g@mail.gmail.com> (raw)
In-Reply-To: <20201125185232.GF3095@redhat.com>

On Wed, Nov 25, 2020 at 8:52 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Nov 25, 2020 at 10:47:38AM -0800, Sargun Dhillon wrote:
> > On Wed, Nov 25, 2020 at 10:43 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Wed, Nov 25, 2020 at 10:31:36AM -0800, Sargun Dhillon wrote:
> > > > On Wed, Nov 25, 2020 at 10:17 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > >
> > > > > On Wed, Nov 25, 2020 at 02:46:20AM -0800, Sargun Dhillon wrote:
> > > > >
> > > > > [..]
> > > > > > @@ -1125,16 +1183,19 @@ static int ovl_workdir_cleanup_recurse(struct path *path, int level)
> > > > > >                       if (p->len == 2 && p->name[1] == '.')
> > > > > >                               continue;
> > > > > >               } else if (incompat) {
> > > > > > -                     pr_err("overlay with incompat feature '%s' cannot be mounted\n",
> > > > > > -                             p->name);
> > > > > > -                     err = -EINVAL;
> > > > > > -                     break;
> > > > > > +                     err = ovl_check_incompat(ofs, p, path);
> > > > > > +                     if (err < 0)
> > > > > > +                             break;
> > > > > > +                     /* Skip cleaning this */
> > > > > > +                     if (err == 1)
> > > > > > +                             continue;
> > > > > >               }
> > > > >
> > > > > Shouldn't we clean volatile/dirty on non-volatile mount. I did a
> > > > > volatile mount followed by a non-volatile remount and I still
> > > > > see work/incompat/volatile/dirty and "trusted.overlay.volatile" xattr
> > > > > on "volatile" dir. I would expect that this will be all cleaned up
> > > > > as soon as that upper/work is used for non-volatile mount.
> > > > >
> > > > >
> > > >
> > > > Amir pointed out this is incorrect behaviour earlier.
> > > > You should be able to go:
> > > > non-volatile -> volatile
> > > > volatile -> volatile
> > > >
> > > > But never
> > > > volatile -> non-volatile, since our mechanism is not bulletproof.
> > >
> > > Ok, so one needs to manually remove volatile/dirty to be able to
> > > go from volatile to non-volatile.
> > >
> > > I am wondering what does this change mean in terms of user visible
> > > behavior. So far, if somebody tried a remount of volatile overlay, it
> > > will fail. After this change, it will most likely succeed. I am
> > > hoping nobody relies on remount failure of volatile mount and
> > > complain that user visible behavior changed after kernel upgrade.
> > >
> > > Thanks
> > > Vivek
> > >
> > If I respin this shortly, can we get it in rc6, or do we want to wait
> > until 5.11?
>
> I think that trying to squeeze it in this late in cycle is probably
> not a good idea. If above is a valid concern, then this feature probably
> needs to be an opt-in.
>

Doesn't sound like a concern to me.

Thanks,
Amir.