On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
> On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi <mszeredi@xxxxxxxxxx>
> wrote:
> >
> > Add a check on each lower 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.
> >
> > Share the same xattr for the per-directory and the per-layer flag,
> > which
> > has the effect that if this is enabled for a layer, then the
> > optimization
> > to bypass checking of individual entries does not work on the root
> > of the
> > layer. This was deemed better, than having a separate xattr for
> > the layer
> > and the directory.
> >
> > Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout")
> > Cc: <stable@xxxxxxxxxxxxxxx> # v6.7
> > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > ---
> > v2:
> > - use overlay.whiteouts instead of overlay.feature_xwhiteout
> > - move initialization to ovl_get_layers()
> > - xwhiteouts can only be enabled on lower layer
> >
> > fs/overlayfs/namei.c | 10 +++++++---
> > fs/overlayfs/overlayfs.h | 7 +++++--
> > fs/overlayfs/ovl_entry.h | 2 ++
> > fs/overlayfs/readdir.c | 11 ++++++++---
> > fs/overlayfs/super.c | 13 +++++++++++++
> > fs/overlayfs/util.c | 7 ++++++-
> > 6 files changed, 41 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 03bc8d5dfa31..583cf56df66e 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -863,7 +863,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs
> > *ofs, struct dentry *upper,
> > * Returns next layer in stack starting from top.
> > * Returns -1 if this is the last layer.
> > */
> > -int ovl_path_next(int idx, struct dentry *dentry, struct path
> > *path)
> > +int ovl_path_next(int idx, struct dentry *dentry, struct path
> > *path,
> > + const struct ovl_layer **layer)
> > {
> > struct ovl_entry *oe = OVL_E(dentry);
> > struct ovl_path *lowerstack = ovl_lowerstack(oe);
> > @@ -871,13 +872,16 @@ int ovl_path_next(int idx, struct dentry
> > *dentry, struct path *path)
> > BUG_ON(idx < 0);
> > if (idx == 0) {
> > ovl_path_upper(dentry, path);
> > - if (path->dentry)
> > + if (path->dentry) {
> > + *layer = &OVL_FS(dentry->d_sb)->layers[0];
> > return ovl_numlower(oe) ? 1 : -1;
> > + }
> > idx++;
> > }
> > BUG_ON(idx > ovl_numlower(oe));
> > path->dentry = lowerstack[idx - 1].dentry;
> > - path->mnt = lowerstack[idx - 1].layer->mnt;
> > + *layer = lowerstack[idx - 1].layer;
> > + path->mnt = (*layer)->mnt;
> >
> > return (idx < ovl_numlower(oe)) ? idx + 1 : -1;
> > }
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 05c3dd597fa8..6359cf5c66ff 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -492,7 +492,9 @@ bool ovl_path_check_dir_xattr(struct ovl_fs
> > *ofs, const struct path *path,
> > enum ovl_xattr ox);
> > bool ovl_path_check_origin_xattr(struct ovl_fs *ofs, const struct
> > path *path);
> > bool ovl_path_check_xwhiteout_xattr(struct ovl_fs *ofs, const
> > struct path *path);
> > -bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const
> > struct path *path);
> > +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs,
> > + const struct ovl_layer *layer,
> > + const struct path *path);
> > bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs
> > *ofs,
> > const struct path *upperpath);
> >
> > @@ -674,7 +676,8 @@ int ovl_get_index_name(struct ovl_fs *ofs,
> > struct dentry *origin,
> > struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh
> > *fh);
> > struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry
> > *upper,
> > struct dentry *origin, bool
> > verify);
> > -int ovl_path_next(int idx, struct dentry *dentry, struct path
> > *path);
> > +int ovl_path_next(int idx, struct dentry *dentry, struct path
> > *path,
> > + const struct ovl_layer **layer);
> > int ovl_verify_lowerdata(struct dentry *dentry);
> > struct dentry *ovl_lookup(struct inode *dir, struct dentry
> > *dentry,
> > unsigned int flags);
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index d82d2a043da2..33fcd3d3af30 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -40,6 +40,8 @@ struct ovl_layer {
> > int idx;
> > /* One fsid per unique underlying sb (upper fsid == 0) */
> > int fsid;
> > + /* xwhiteouts are enabled on this layer*/
> > + bool xwhiteouts;
> > };
> >
> > struct ovl_path {
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index a490fc47c3e7..c2597075e3f8 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -305,8 +305,6 @@ static inline int ovl_dir_read(const struct
> > path *realpath,
> > if (IS_ERR(realfile))
> > return PTR_ERR(realfile);
> >
> > - rdd->in_xwhiteouts_dir = rdd->dentry &&
> > - ovl_path_check_xwhiteouts_xattr(OVL_FS(rdd->dentry-
> > >d_sb), realpath);
> > rdd->first_maybe_whiteout = NULL;
> > rdd->ctx.pos = 0;
> > do {
> > @@ -359,10 +357,14 @@ static int ovl_dir_read_merged(struct dentry
> > *dentry, struct list_head *list,
> > .is_lowest = false,
> > };
> > int idx, next;
> > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > + const struct ovl_layer *layer;
> >
> > for (idx = 0; idx != -1; idx = next) {
> > - next = ovl_path_next(idx, dentry, &realpath);
> > + next = ovl_path_next(idx, dentry, &realpath,
> > &layer);
> > rdd.is_upper = ovl_dentry_upper(dentry) ==
> > realpath.dentry;
> > + if (ovl_path_check_xwhiteouts_xattr(ofs, layer,
> > &realpath))
> > + rdd.in_xwhiteouts_dir = true;
> >
> > if (next != -1) {
> > err = ovl_dir_read(&realpath, &rdd);
> > @@ -568,6 +570,7 @@ static int ovl_dir_read_impure(const struct
> > path *path, struct list_head *list,
> > int err;
> > struct path realpath;
> > struct ovl_cache_entry *p, *n;
> > + struct ovl_fs *ofs = OVL_FS(path->dentry->d_sb);
> > struct ovl_readdir_data rdd = {
> > .ctx.actor = ovl_fill_plain,
> > .list = list,
> > @@ -577,6 +580,8 @@ static int ovl_dir_read_impure(const struct
> > path *path, struct list_head *list,
> > INIT_LIST_HEAD(list);
> > *root = RB_ROOT;
> > ovl_path_upper(path->dentry, &realpath);
> > + if (ovl_path_check_xwhiteouts_xattr(ofs, &ofs->layers[0],
> > &realpath))
> > + rdd.in_xwhiteouts_dir = true;
>
> Not needed since we do not support xwhiteouts on upper.
>
> >
> > err = ovl_dir_read(&realpath, &rdd);
> > if (err)
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index a0967bb25003..04588721eb2a 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1027,6 +1027,7 @@ static int ovl_get_layers(struct super_block
> > *sb, struct ovl_fs *ofs,
> > struct ovl_fs_context_layer *l = &ctx->lower[i];
> > struct vfsmount *mnt;
> > struct inode *trap;
> > + struct path root;
> > int fsid;
> >
> > if (i < nr_merged_lower)
> > @@ -1069,6 +1070,16 @@ static int ovl_get_layers(struct super_block
> > *sb, struct ovl_fs *ofs,
> > */
> > mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
> >
> > + /*
> > + * Check if xwhiteout (xattr whiteout) support is
> > enabled on
> > + * this layer.
> > + */
> > + root.mnt = mnt;
> > + root.dentry = mnt->mnt_root;
> > + err = ovl_path_getxattr(ofs, &root,
> > OVL_XATTR_XWHITEOUTS, NULL, 0);
> > + if (err >= 0)
> > + layers[ofs->numlayer].xwhiteouts = true;
> > +
> > layers[ofs->numlayer].trap = trap;
> > layers[ofs->numlayer].mnt = mnt;
> > layers[ofs->numlayer].idx = ofs->numlayer;
> > @@ -1079,6 +1090,8 @@ static int ovl_get_layers(struct super_block
> > *sb, struct ovl_fs *ofs,
> > l->name = NULL;
> > ofs->numlayer++;
> > ofs->fs[fsid].is_lower = true;
> > +
> > +
>
> extra spaces.
>
> > }
> >
> > /*
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index c3f020ca13a8..6c6e6f5893ea 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -739,11 +739,16 @@ bool ovl_path_check_xwhiteout_xattr(struct
> > ovl_fs *ofs, const struct path *path)
> > return res >= 0;
> > }
> >
> > -bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs, const
> > struct path *path)
> > +bool ovl_path_check_xwhiteouts_xattr(struct ovl_fs *ofs,
> > + const struct ovl_layer *layer,
> > + const struct path *path)
> > {
> > struct dentry *dentry = path->dentry;
> > int res;
> >
> > + if (!layer->xwhiteouts)
> > + return false;
> > +
> > /* xattr.whiteouts must be a directory */
> > if (!d_is_dir(dentry))
> > return false;
> > --
> > 2.43.0
> >
>
> Do you want me to fix/test and send this to Linus?
>
> Alex, can we add your RVB to v2?
>
Yeah, other that your comments this looks good to me (and I tested it
here too).
Reviewed-By: Alexander Larsson <alex@xxxxxxxxxx>
I'll have a look at doing the required changes in composefs.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a Nobel prize-winning voodoo matador haunted by memories of 'Nam.
She's an orphaned gypsy bodyguard on the trail of a serial killer. They
fight crime!
