Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jun 9, 2021 at 10:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 9, 2021 at 10:28 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 9 Jun 2021 at 08:08, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Jun 8, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > > > > While you are here, do you think that will be sufficient for the on-disk format
> > > > > > > of overlay.xflags?
> > > > > > >
> > > > > > > struct ovl_xflags {
> > > > > > >         __le32 xflags;
> > > > > > >         __le32 xflags_mask;
> > > > > > > }
> > > > > >
> > > > > > I think I'd prefer a slightly more complex, but user friendlier
> > > > > > "+i,-a,..." format.
> > > > > >
> > > > >
> > > > > OK, but since this is not a merge, we'd only need:
> > > > > overlay.xflags = "ia..."
> > > > >
> > > > > Which is compatible with the format of:
> > > > > chattr =<xflags> <file>
> > > >
> > > > Fine.   Not sure what xflags_mask would be useful for in your proposal, though.
> > > >
> > >
> > > The idea was that in the context of fileattr_get(), any specific xflag
> > > value can be one of: SET, CLEAR, REAL.
> > >
> > > For most inodes all flags are REAL (no xflags xattr)
> > > All flags but the 4 in OVL_FS_XFLAGS_MASK are always REAL
> > > (i.e. taken from fileattr_get() on real inode).
> > >
> > > If we ever decide to extend OVL_FS_XFLAGS_MASK, say to include
> > > DIRSYNC, then an upper inode with DIRSYNC that was in state
> > > REAL before upgrade would become CLEAR after upgrade unless
> > > we kept the old xflags_mask in xattr.
> > >
> > > With the string format, this is not a concern.
> > > Therefore, I like the string format better.
> >
> > Hmm, so if the attribute letters would have fixed places in the string
> > and clear attributes would be represented by a space or a "-" then
> > that would be similarly extensible.   Just having a list of set
> > attribute letters would not allow having three states.
> >
>
> Right. Will do that.
>

Taking a step back.

The main problem this is trying to solve is losing persistent inode flags
on copy-up.

If this was just NOATIME and SYNC the solution would have been
simple - copy up the flags along with other metadata we copy up.

We wouldn't even need to limit ourselves to the 4 vfs inode flags
in ovl_copyflags(). We could add the the copied up flags more
fs specific flags that we know to be safe and rational to copy
such as NOCOW, NODUMP and DIRSYNC.

The secondary problem is that copying IMMUTABLE/APPEND
to upper inode on copy up is not an option, so the solution is to
store those properties in an xattr.

I think we should split the solution to the primary and secondary
problems and avoid an over-designed generic future extendable
xflags xattr feature.

So I am leaning towards a more focused solution for
IMMUTABLE/APPEND in the form of either two boolean
xattr overlay.{immutable,appendonly} or one single bytes
xattr overlay.protected.

Thanks,
Amir.