Re: [PATCH v3] ovl: mark xwhiteouts directory with overlay.opaque='x'

[Alexander Larsson <alexl@redhat.com>]

On Mon, 2024-01-22 at 13:09 +0200, Amir Goldstein wrote:
> On Mon, Jan 22, 2024 at 12:14 PM Alexander Larsson <alexl@redhat.com>
> wrote:
> > 
> > On Sun, 2024-01-21 at 17:05 +0200, Amir Goldstein wrote:
> > > An opaque directory cannot have xwhiteouts, so instead of marking
> > > an
> > > xwhiteouts directory with a new xattr, overload overlay.opaque
> > > xattr
> > > for marking both opaque dir ('y') and xwhiteouts dir ('x').
> > > 
> > > This is more efficient as the overlay.opaque xattr is checked
> > > during
> > > lookup of directory anyway.
> > > 
> > > This also prevents unnecessary checking the xattr when reading a
> > > directory without xwhiteouts, i.e. most of the time.
> > > 
> > > Note that the xwhiteouts marker is not checked on the upper layer
> > > and
> > > on the last layer in lowerstack, where xwhiteouts are not
> > > expected.
> > > 
> > > Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of whiteout")
> > > Cc: <stable@vger.kernel.org> # v6.7
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > > 
> > > Miklos,
> > > 
> > > Alex has reported a problem with your suggested approach of
> > > requiring
> > > xwhiteouts xattr on layers root dir [1].
> > > 
> > > Following counter proposal, amortizes the cost of checking opaque
> > > xattr
> > > on directories during lookup to also check for xwhiteouts.
> > > 
> > > This change requires the following change to test overlay/084:
> > > 
> > > --- a/tests/overlay/084
> > > +++ b/tests/overlay/084
> > > @@ -115,7 +115,8 @@ do_test_xwhiteout()
> > > 
> > >         mkdir -p $basedir/lower $basedir/upper $basedir/work
> > >         touch $basedir/lower/regular $basedir/lower/hidden
> > > $basedir/upper/hidden
> > > -       setfattr -n $prefix.overlay.whiteouts -v "y"
> > > $basedir/upper
> > > +       # overlay.opaque="x" means directory has xwhiteout
> > > children
> > > +       setfattr -n $prefix.overlay.opaque -v "x" $basedir/upper
> > >         setfattr -n $prefix.overlay.whiteout -v "y"
> > > $basedir/upper/hidden
> > > 
> > > 
> > > Alex,
> > > 
> > > Please let us know if this change is acceptable for composefs.
> > 
> > Yes, this looks very good to me. (Minor comments below)
> > I'll do some testing on this.
> > 
> 
> Excellent, I'll be expecting your RVB/Tested-by.
> > 

Yes
Reviewed-by: Alexander Larsson <alexl@redhat.com>
Tested-by: Alexander Larsson <alexl@redhat.com>

for the patch in the ovl-fixes branch.

I tested it manually, and with xfstest (with change), and also
with this composefs change:

https://github.com/alexlarsson/composefs/tree/new-format-version

I created a lowerdir with a regular whiteout in, and after running that
though the changed mkcomposefs I was able to mount the composefs image,
and then mount the lowerdirs from the composefs mount, and they
correctly handled the whiteout both when mounted normally and with
userxattr.


> > > Thanks,
> > > Amir.
> > > 
> > > [1]
> > > https://lore.kernel.org/linux-unionfs/5ee3a210f8f4fc89cb750b3d1a378a0ff0187c9f.camel@redhat.com/
> > > 
> > >  fs/overlayfs/namei.c     | 32 +++++++++++++++++++-------------
> > >  fs/overlayfs/overlayfs.h | 17 +++++++++++++----
> > >  fs/overlayfs/ovl_entry.h |  2 ++
> > >  fs/overlayfs/readdir.c   |  5 +++--
> > >  fs/overlayfs/super.c     |  9 +++++++++
> > >  fs/overlayfs/util.c      | 34 ++++++++++++++--------------------
> > >  6 files changed, 60 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index 984ffdaeed6c..caccf3803796 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -18,10 +18,11 @@
> > > 
> > >  struct ovl_lookup_data {
> > >       struct super_block *sb;
> > > -     struct vfsmount *mnt;
> > > +     const struct ovl_layer *layer;
> > >       struct qstr name;
> > >       bool is_dir;
> > >       bool opaque;
> > > +     bool xwhiteouts;
> > >       bool stop;
> > >       bool last;
> > >       char *redirect;
> > > @@ -201,17 +202,13 @@ struct dentry *ovl_decode_real_fh(struct
> > > ovl_fs
> > > *ofs, struct ovl_fh *fh,
> > >       return real;
> > >  }
> > > 
> > > -static bool ovl_is_opaquedir(struct ovl_fs *ofs, const struct
> > > path
> > > *path)
> > > -{
> > > -     return ovl_path_check_dir_xattr(ofs, path,
> > > OVL_XATTR_OPAQUE);
> > > -}
> > > -
> > >  static struct dentry *ovl_lookup_positive_unlocked(struct
> > > ovl_lookup_data *d,
> > >                                                  const char
> > > *name,
> > >                                                  struct dentry
> > > *base, int len,
> > >                                                  bool
> > > drop_negative)
> > >  {
> > > -     struct dentry *ret = lookup_one_unlocked(mnt_idmap(d->mnt),
> > > name, base, len);
> > > +     struct dentry *ret = lookup_one_unlocked(mnt_idmap(d-
> > > >layer-
> > > > mnt), name,
> > > +                                              base, len);
> > > 
> > >       if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret-
> > > > d_flags))) {
> > >               if (drop_negative && ret->d_lockref.count == 1) {
> > > @@ -232,10 +229,13 @@ static int ovl_lookup_single(struct dentry
> > > *base, struct ovl_lookup_data *d,
> > >                            size_t prelen, const char *post,
> > >                            struct dentry **ret, bool
> > > drop_negative)
> > >  {
> > > +     struct ovl_fs *ofs = OVL_FS(d->sb);
> > >       struct dentry *this;
> > >       struct path path;
> > >       int err;
> > >       bool last_element = !post[0];
> > > +     bool is_upper = d->layer->idx == 0;
> > > +     char val;
> > > 
> > >       this = ovl_lookup_positive_unlocked(d, name, base, namelen,
> > > drop_negative);
> > >       if (IS_ERR(this)) {
> > > @@ -253,8 +253,8 @@ static int ovl_lookup_single(struct dentry
> > > *base,
> > > struct ovl_lookup_data *d,
> > >       }
> > > 
> > >       path.dentry = this;
> > > -     path.mnt = d->mnt;
> > > -     if (ovl_path_is_whiteout(OVL_FS(d->sb), &path)) {
> > > +     path.mnt = d->layer->mnt;
> > > +     if (ovl_path_is_whiteout(ofs, &path)) {
> > >               d->stop = d->opaque = true;
> > >               goto put_and_out;
> > >       }
> > > @@ -272,7 +272,7 @@ static int ovl_lookup_single(struct dentry
> > > *base,
> > > struct ovl_lookup_data *d,
> > >                       d->stop = true;
> > >                       goto put_and_out;
> > >               }
> > > -             err = ovl_check_metacopy_xattr(OVL_FS(d->sb),
> > > &path,
> > > NULL);
> > > +             err = ovl_check_metacopy_xattr(ofs, &path, NULL);
> > >               if (err < 0)
> > >                       goto out_err;
> > > 
> > > @@ -292,7 +292,11 @@ static int ovl_lookup_single(struct dentry
> > > *base, struct ovl_lookup_data *d,
> > >               if (d->last)
> > >                       goto out;
> > > 
> > > -             if (ovl_is_opaquedir(OVL_FS(d->sb), &path)) {
> > > +             /* overlay.opaque=x means xwhiteouts directory */
> > > +             val = ovl_get_opaquedir_val(ofs, &path);
> > > +             if (last_element && !is_upper && val == 'x') {
> > > +                     d->xwhiteouts = true;
> > > +             } else if (val == 'y') {
> > >                       d->stop = true;
> > >                       if (last_element)
> > >                               d->opaque = true;
> > > @@ -1055,7 +1059,7 @@ struct dentry *ovl_lookup(struct inode
> > > *dir,
> > > struct dentry *dentry,
> > >       old_cred = ovl_override_creds(dentry->d_sb);
> > >       upperdir = ovl_dentry_upper(dentry->d_parent);
> > >       if (upperdir) {
> > > -             d.mnt = ovl_upper_mnt(ofs);
> > > +             d.layer = &ofs->layers[0];
> > >               err = ovl_lookup_layer(upperdir, &d, &upperdentry,
> > > true);
> > >               if (err)
> > >                       goto out;
> > > @@ -1111,7 +1115,7 @@ struct dentry *ovl_lookup(struct inode
> > > *dir,
> > > struct dentry *dentry,
> > >               else if (d.is_dir || !ofs->numdatalayer)
> > >                       d.last = lower.layer->idx ==
> > > ovl_numlower(roe);
> > > 
> > > -             d.mnt = lower.layer->mnt;
> > > +             d.layer = lower.layer;
> > >               err = ovl_lookup_layer(lower.dentry, &d, &this,
> > > false);
> > >               if (err)
> > >                       goto out_put;
> > > @@ -1278,6 +1282,8 @@ struct dentry *ovl_lookup(struct inode
> > > *dir,
> > > struct dentry *dentry,
> > > 
> > >       if (upperopaque)
> > >               ovl_dentry_set_opaque(dentry);
> > > +     if (d.xwhiteouts)
> > > +             ovl_dentry_set_xwhiteouts(dentry);
> > > 
> > >       if (upperdentry)
> > >               ovl_dentry_set_upper_alias(dentry);
> > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > > index 5ba11eb43767..410b3bfc3afc 100644
> > > --- a/fs/overlayfs/overlayfs.h
> > > +++ b/fs/overlayfs/overlayfs.h
> > > @@ -70,6 +70,8 @@ enum ovl_entry_flag {
> > >       OVL_E_UPPER_ALIAS,
> > >       OVL_E_OPAQUE,
> > >       OVL_E_CONNECTED,
> > > +     /* Lower stack may contain xwhiteout entries */
> > > +     OVL_E_XWHITEOUTS,
> > >  };
> > > 
> > >  enum {
> > > @@ -476,6 +478,8 @@ void ovl_dentry_clear_flag(unsigned long
> > > flag,
> > > struct dentry *dentry);
> > >  bool ovl_dentry_test_flag(unsigned long flag, struct dentry
> > > *dentry);
> > >  bool ovl_dentry_is_opaque(struct dentry *dentry);
> > >  bool ovl_dentry_is_whiteout(struct dentry *dentry);
> > > +bool ovl_dentry_is_xwhiteouts(struct dentry *dentry);
> > > +void ovl_dentry_set_xwhiteouts(struct dentry *dentry);
> > >  void ovl_dentry_set_opaque(struct dentry *dentry);
> > >  bool ovl_dentry_has_upper_alias(struct dentry *dentry);
> > >  void ovl_dentry_set_upper_alias(struct dentry *dentry);
> > > @@ -494,11 +498,10 @@ struct file *ovl_path_open(const struct
> > > path
> > > *path, int flags);
> > >  int ovl_copy_up_start(struct dentry *dentry, int flags);
> > >  void ovl_copy_up_end(struct dentry *dentry);
> > >  bool ovl_already_copied_up(struct dentry *dentry, int flags);
> > > -bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct
> > > path
> > > *path,
> > > -                           enum ovl_xattr ox);
> > > +char ovl_get_dir_xattr_val(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_init_uuid_xattr(struct super_block *sb, struct ovl_fs
> > > *ofs,
> > >                        const struct path *upperpath);
> > > 
> > > @@ -573,7 +576,13 @@ static inline bool ovl_is_impuredir(struct
> > > super_block *sb,
> > >               .mnt = ovl_upper_mnt(ofs),
> > >       };
> > > 
> > > -     return ovl_path_check_dir_xattr(ofs, &upperpath,
> > > OVL_XATTR_IMPURE);
> > > +     return ovl_get_dir_xattr_val(ofs, &upperpath,
> > > OVL_XATTR_IMPURE) == 'y';
> > > +}
> > > +
> > > +static inline char ovl_get_opaquedir_val(struct ovl_fs *ofs,
> > > +                                      const struct path *path)
> > > +{
> > > +     return ovl_get_dir_xattr_val(ofs, path, OVL_XATTR_OPAQUE);
> > >  }
> > > 
> > >  static inline bool ovl_redirect_follow(struct ovl_fs *ofs)
> > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > > index 5fa9c58af65f..0b7b21745ba3 100644
> > > --- a/fs/overlayfs/ovl_entry.h
> > > +++ b/fs/overlayfs/ovl_entry.h
> > > @@ -86,6 +86,8 @@ struct ovl_fs {
> > >       /* Shared whiteout cache */
> > >       struct dentry *whiteout;
> > >       bool no_shared_whiteout;
> > > +     /* xwhiteouts may exist in lower layers */
> > > +     bool xwhiteouts;
> > 
> > This comment is a bit off, this is now only used for the root dir.
> > 
> > >       /* r/o snapshot of upperdir sb's only taken on volatile
> > > mounts */
> > >       errseq_t errseq;
> > >  };
> > > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > > index e71156baa7bc..edef4e3401de 100644
> > > --- a/fs/overlayfs/readdir.c
> > > +++ b/fs/overlayfs/readdir.c
> > > @@ -165,7 +165,8 @@ static struct ovl_cache_entry
> > > *ovl_cache_entry_new(struct ovl_readdir_data *rdd,
> > >       p->is_upper = rdd->is_upper;
> > >       p->is_whiteout = false;
> > >       /* Defer check for overlay.whiteout to ovl_iterate() */
> > > -     p->check_xwhiteout = rdd->in_xwhiteouts_dir && d_type ==
> > > DT_REG;
> > > +     p->check_xwhiteout = rdd->in_xwhiteouts_dir &&
> > > +                         !rdd->is_upper && d_type == DT_REG;
> > > 
> > 
> > Maybe we can move the is_upper check to where we set
> > in_xwhiteouts_dir?
> > 
> > >       if (d_type == DT_CHR) {
> > >               p->next_maybe_whiteout = rdd->first_maybe_whiteout;
> > > @@ -306,7 +307,7 @@ static inline int ovl_dir_read(const struct
> > > path
> > > *realpath,
> > >               return PTR_ERR(realfile);
> > > 
> > >       rdd->in_xwhiteouts_dir = rdd->dentry &&
> > > -             ovl_path_check_xwhiteouts_xattr(OVL_FS(rdd->dentry-
> > > > d_sb), realpath);
> > > +             ovl_dentry_is_xwhiteouts(rdd->dentry);
> > 
> > Now that the xwhiteout flag is on the dentry, it will be set for
> > all
> > layers. Maybe we can avoid setting in_whiteouts_dir for the
> > lowermost
> > layer?
> > 
> 
> Applied this diff and pushed to the ovl-fixes branch.
> 
> Will wait for ACK from Miklos before sending PR.
> 
> Thanks,
> Amir.
> 
> 
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 0b7b21745ba3..c089e5ff37b5 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -86,7 +86,7 @@ struct ovl_fs {
>         /* Shared whiteout cache */
>         struct dentry *whiteout;
>         bool no_shared_whiteout;
> -       /* xwhiteouts may exist in lower layers */
> +       /* xwhiteouts may exist in lower layer root dirs */
>         bool xwhiteouts;
>         /* r/o snapshot of upperdir sb's only taken on volatile
> mounts */
>         errseq_t errseq;
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index edef4e3401de..3168e851ca1f 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -165,8 +165,7 @@ static struct ovl_cache_entry
> *ovl_cache_entry_new(struct ovl_readdir_data *rdd,
>         p->is_upper = rdd->is_upper;
>         p->is_whiteout = false;
>         /* Defer check for overlay.whiteout to ovl_iterate() */
> -       p->check_xwhiteout = rdd->in_xwhiteouts_dir &&
> -                           !rdd->is_upper && d_type == DT_REG;
> +       p->check_xwhiteout = rdd->in_xwhiteouts_dir && d_type ==
> DT_REG;
> 
>         if (d_type == DT_CHR) {
>                 p->next_maybe_whiteout = rdd->first_maybe_whiteout;
> @@ -306,8 +305,9 @@ 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_dentry_is_xwhiteouts(rdd->dentry);
> +       /* No need to check for xwhiteouts in upper and lowermost
> layers */
> +       rdd->in_xwhiteouts_dir = !rdd->is_upper && !rdd->is_lowest &&
> +               rdd->dentry && ovl_dentry_is_xwhiteouts(rdd->dentry);
>         rdd->first_maybe_whiteout = NULL;
>         rdd->ctx.pos = 0;
> 

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a suicidal devious gangster with a secret. She's a warm-hearted 
thirtysomething femme fatale in the witness protection program. They 
fight crime!