Re: [PATCH] ovl: require xwhiteout feature flag on layer roots

[Alexander Larsson <alexl@redhat.com>]

Resending with plan text.

On Thu, 2024-01-18 at 12:39 +0100, Miklos Szeredi wrote:
> On Thu, 18 Jan 2024 at 12:22, Amir Goldstein <amir73il@gmail.com>
> wrote:
> > 
> > On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi
> > <mszeredi@redhat.com> wrote:
> > > 
> > > Add a check on each layer for the xwhiteout feature.  This
> > > prevents
> > > unnecessary checking the overlay.whiteouts xattr when reading a
> > > directory if this feature is not enabled, i.e. most of the time.
> > 
> > Does it really have a significant cost or do you just not like the
> > unneeded check?
> 
> It's probably insignificant.   But I don't know and it would be hard
> to prove.
> 
> > IIRC, we anyway check for ORIGIN xattr and IMPURE xattr on
> > readdir.
> 
> We check those on lookup, not at readdir.  Might make sense to check
> XWHITEOUTS at lookup regardless of this patch, just for consistency.
> 
> > > --- a/fs/overlayfs/overlayfs.h
> > > +++ b/fs/overlayfs/overlayfs.h
> > > @@ -51,6 +51,7 @@ enum ovl_xattr {
> > >         OVL_XATTR_PROTATTR,
> > >         OVL_XATTR_XWHITEOUT,
> > >         OVL_XATTR_XWHITEOUTS,
> > > +       OVL_XATTR_FEATURE_XWHITEOUT,
> > 
> > Can we not add a new OVL_XATTR_FEATURE_XWHITEOUT xattr.
> > 
> > Setting OVL_XATTR_XWHITEOUTS on directories with xwhiteouts is
> > anyway the responsibility of the layer composer.
> > 
> > Let's just require the layer composer to set OVL_XATTR_XWHITEOUTS
> > on the layer root even if it does not have any immediate xwhiteout
> > children as "layer may have xwhiteouts" indication. ok?
> 
> Okay.
> > 

This will cause readdir() on the root dir to always look for whiteouts
even though there are none, but that is probably fine.

It does mean we don't have to change xfstests, but I still have to
change mkcomposefs.


> > > @@ -1414,6 +1414,17 @@ int ovl_fill_super(struct super_block *sb,
> > > struct fs_context *fc)
> > >         if (err)
> > >                 goto out_free_oe;
> > > 
> > > +       for (i = 0; i < ofs->numlayer; i++) {
> > > +               struct path path = { .mnt = layers[i].mnt };
> > > +
> > > +               if (path.mnt) {
> > > +                       path.dentry = path.mnt->mnt_root;
> > > +                       err = ovl_path_getxattr(ofs, &path,
> > > OVL_XATTR_FEATURE_XWHITEOUT, NULL, 0);
> > > +                       if (err >= 0)
> > > +                               layers[i].feature_xwhiteout =
> > > true;
> > 
> > 
> > Any reason not to do this in ovl_get_layers() when adding the
> > layer?
> 
> Well, ovl_get_layers() is called form ovl_get_lowerstack() implying
> that it's part of the lower layer setup.
> 
> Otherwise I don't see why it could not be in ovl_get_layers().  
> Maybe
> some renaming can help.
> 

In the version I was preparing 
(https://github.com/alexlarsson/linux/tree/ovl-xattr-whiteouts-feature)
it does the setup in ovl_get_layers(). The one difference this makes is
that it doesn't apply feature_xwhiteout on the upperdir layer. I don't
think we want to do xwhiteouts on the upperdir, but if we do it needs
to be initialized in ovl_get_upper() too.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a fast talking alcoholic grifter searching for his wife's true 
killer. She's a wealthy communist Hell's Angel in the witness
protection 
program. They fight crime!