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

[Miklos Szeredi <miklos@szeredi.hu>]

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.

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

Thanks,
Miklos