On Thu, Jan 18, 2024 at 2:08 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > wrote:
> > >
> > > On Thu, Jan 18, 2024 at 12:41 PM Miklos Szeredi
> > > <mszeredi@xxxxxxxxxx> 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.
>
If there is no use case for xwhiteouts support in upper then
maybe we should not support it?
Anyway, I am fine with Miklos' version or with checking xwhiteouts in
ovl_get_upper() and ovl_get_layers() or with not supporting
xwhiteouts on upper.
Thanks,
Amir.
