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

[Alexander Larsson <alexl@redhat.com>]

On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
> On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi <mszeredi@redhat.com>
> 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@vger.kernel.org> # v6.7
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> > 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@redhat.com>

I'll have a look at doing the required changes in composefs.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
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!