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

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

[Amir Goldstein]

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.

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;
 	/* 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;
 
 	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);
 	rdd->first_maybe_whiteout = NULL;
 	rdd->ctx.pos = 0;
 	do {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4ab66e3d4cff..81f045025c96 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1026,6 +1026,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)
@@ -1068,6 +1069,12 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 		 */
 		mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
 
+		/* overlay.opaque=x means xwhiteouts directory */
+		root.mnt = mnt;
+		root.dentry = mnt->mnt_root;
+		if (ovl_get_opaquedir_val(ofs, &root) == 'x')
+			ofs->xwhiteouts = true;
+
 		layers[ofs->numlayer].trap = trap;
 		layers[ofs->numlayer].mnt = mnt;
 		layers[ofs->numlayer].idx = ofs->numlayer;
@@ -1272,6 +1279,8 @@ static struct dentry *ovl_get_root(struct super_block *sb,
 
 	/* Root is always merge -> can have whiteouts */
 	ovl_set_flag(OVL_WHITEOUTS, d_inode(root));
+	if (OVL_FS(sb)->xwhiteouts)
+		ovl_dentry_set_flag(OVL_E_XWHITEOUTS, root);
 	ovl_dentry_set_flag(OVL_E_CONNECTED, root);
 	ovl_set_upperdata(d_inode(root));
 	ovl_inode_init(d_inode(root), &oip, ino, fsid);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0217094c23ea..fb622995fb28 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -456,6 +456,16 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry)
 	return !dentry->d_inode && ovl_dentry_is_opaque(dentry);
 }
 
+bool ovl_dentry_is_xwhiteouts(struct dentry *dentry)
+{
+	return ovl_dentry_test_flag(OVL_E_XWHITEOUTS, dentry);
+}
+
+void ovl_dentry_set_xwhiteouts(struct dentry *dentry)
+{
+	ovl_dentry_set_flag(OVL_E_XWHITEOUTS, dentry);
+}
+
 void ovl_dentry_set_opaque(struct dentry *dentry)
 {
 	ovl_dentry_set_flag(OVL_E_OPAQUE, dentry);
@@ -739,19 +749,6 @@ 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)
-{
-	struct dentry *dentry = path->dentry;
-	int res;
-
-	/* xattr.whiteouts must be a directory */
-	if (!d_is_dir(dentry))
-		return false;
-
-	res = ovl_path_getxattr(ofs, path, OVL_XATTR_XWHITEOUTS, NULL, 0);
-	return res >= 0;
-}
-
 /*
  * Load persistent uuid from xattr into s_uuid if found, or store a new
  * random generated value in s_uuid and in xattr.
@@ -811,20 +808,17 @@ bool ovl_init_uuid_xattr(struct super_block *sb, struct ovl_fs *ofs,
 	return false;
 }
 
-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)
 {
 	int res;
 	char val;
 
 	if (!d_is_dir(path->dentry))
-		return false;
+		return 0;
 
 	res = ovl_path_getxattr(ofs, path, ox, &val, 1);
-	if (res == 1 && val == 'y')
-		return true;
-
-	return false;
+	return res == 1 ? val : 0;
 }
 
 #define OVL_XATTR_OPAQUE_POSTFIX	"opaque"
-- 
2.34.1

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

[Alexander Larsson]

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.

> 
> 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?

>  	rdd->first_maybe_whiteout = NULL;
>  	rdd->ctx.pos = 0;
>  	do {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4ab66e3d4cff..81f045025c96 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1026,6 +1026,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)
> @@ -1068,6 +1069,12 @@ static int ovl_get_layers(struct super_block
> *sb, struct ovl_fs *ofs,
>  		 */
>  		mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
>  
> +		/* overlay.opaque=x means xwhiteouts directory */
> +		root.mnt = mnt;
> +		root.dentry = mnt->mnt_root;
> +		if (ovl_get_opaquedir_val(ofs, &root) == 'x')
> +			ofs->xwhiteouts = true;
> +
>  		layers[ofs->numlayer].trap = trap;
>  		layers[ofs->numlayer].mnt = mnt;
>  		layers[ofs->numlayer].idx = ofs->numlayer;
> @@ -1272,6 +1279,8 @@ static struct dentry *ovl_get_root(struct
> super_block *sb,
>  
>  	/* Root is always merge -> can have whiteouts */
>  	ovl_set_flag(OVL_WHITEOUTS, d_inode(root));
> +	if (OVL_FS(sb)->xwhiteouts)
> +		ovl_dentry_set_flag(OVL_E_XWHITEOUTS, root);
>  	ovl_dentry_set_flag(OVL_E_CONNECTED, root);
>  	ovl_set_upperdata(d_inode(root));
>  	ovl_inode_init(d_inode(root), &oip, ino, fsid);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0217094c23ea..fb622995fb28 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -456,6 +456,16 @@ bool ovl_dentry_is_whiteout(struct dentry
> *dentry)
>  	return !dentry->d_inode && ovl_dentry_is_opaque(dentry);
>  }
>  
> +bool ovl_dentry_is_xwhiteouts(struct dentry *dentry)
> +{
> +	return ovl_dentry_test_flag(OVL_E_XWHITEOUTS, dentry);
> +}
> +
> +void ovl_dentry_set_xwhiteouts(struct dentry *dentry)
> +{
> +	ovl_dentry_set_flag(OVL_E_XWHITEOUTS, dentry);
> +}
> +
>  void ovl_dentry_set_opaque(struct dentry *dentry)
>  {
>  	ovl_dentry_set_flag(OVL_E_OPAQUE, dentry);
> @@ -739,19 +749,6 @@ 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)
> -{
> -	struct dentry *dentry = path->dentry;
> -	int res;
> -
> -	/* xattr.whiteouts must be a directory */
> -	if (!d_is_dir(dentry))
> -		return false;
> -
> -	res = ovl_path_getxattr(ofs, path, OVL_XATTR_XWHITEOUTS,
> NULL, 0);
> -	return res >= 0;
> -}
> -
>  /*
>   * Load persistent uuid from xattr into s_uuid if found, or store a
> new
>   * random generated value in s_uuid and in xattr.
> @@ -811,20 +808,17 @@ bool ovl_init_uuid_xattr(struct super_block
> *sb, struct ovl_fs *ofs,
>  	return false;
>  }
>  
> -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)
>  {
>  	int res;
>  	char val;
>  
>  	if (!d_is_dir(path->dentry))
> -		return false;
> +		return 0;
>  
>  	res = ovl_path_getxattr(ofs, path, ox, &val, 1);
> -	if (res == 1 && val == 'y')
> -		return true;
> -
> -	return false;
> +	return res == 1 ? val : 0;
>  }
>  
>  #define OVL_XATTR_OPAQUE_POSTFIX	"opaque"

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a deeply religious small-town paranormal investigator searching
for 
his wife's true killer. She's a strong-willed French-Canadian single 
mother operating on the wrong side of the law. They fight crime! 

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

[Amir Goldstein]

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.

> >
> > 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;

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

[Alexander Larsson]

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! 

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

[Miklos Szeredi]

On Sun, 21 Jan 2024 at 16:05, Amir Goldstein <amir73il@gmail.com> 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.

Concept looks good overall.

overlayfs.rst needs updating with the new format.   BTW the nesting
format should also be documented, but that's a separate patch.


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

Maybe I'm missing something, but can't we set the flag on the layer?

Thanks,
Miklos

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

[Amir Goldstein]

On Mon, Jan 22, 2024 at 1:52 PM Alexander Larsson <alexl@redhat.com> wrote:
>
> 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.

Thanks. pushed.

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

I noticed you comment in composefs:

 * 1 - Mark xwhitouts using the new opaque=x format as needed by Linux 6.8

Note that this "fix" is aimed to be backported to v6.7.y, so there is no kernel
version that is expected to retain support for the old format.

Thanks,
Amir.

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

[Alexander Larsson]

On Mon, 2024-01-22 at 14:52 +0200, Amir Goldstein wrote:
> On Mon, Jan 22, 2024 at 1:52 PM Alexander Larsson <alexl@redhat.com>
> wrote:
> > 
> > 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.
> 
> Thanks. pushed.
> 
> > 
> > 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.
> > 
> 
> I noticed you comment in composefs:
> 
>  * 1 - Mark xwhitouts using the new opaque=x format as needed by
> Linux 6.8
> 
> Note that this "fix" is aimed to be backported to v6.7.y, so there is
> no kernel
> version that is expected to retain support for the old format.

Yes, but the composefs format needs to be bitwise reproducible, and
this change in the image will cause a different digest for the produced
image, so we can't just change what we generate, it has to be opt in to
users and able to reproduce previous versions.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an uncontrollable zombie senator gone bad. She's a cosmopolitan 
hypochondriac doctor from Mars. They fight crime! 

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

[Amir Goldstein]

On Mon, Jan 22, 2024 at 2:50 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 21 Jan 2024 at 16:05, Amir Goldstein <amir73il@gmail.com> 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.
>
> Concept looks good overall.
>
> overlayfs.rst needs updating with the new format.
>

Something like this looks ok?

--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -145,7 +145,9 @@ filesystem, an overlay filesystem needs to record
in the upper filesystem
 that files have been removed.  This is done using whiteouts and opaque
 directories (non-directories are always opaque).

-A whiteout is created as a character device with 0/0 device number.
+A whiteout is created as a character device with 0/0 device number or
+as a regular file with the xattr "trusted.overlay.whiteout".
+
 When a whiteout is found in the upper level of a merged directory, any
 matching name in the lower level is ignored, and the whiteout itself
 is also hidden.
@@ -154,6 +156,11 @@ A directory is made opaque by setting the xattr
"trusted.overlay.opaque"
 to "y".  Where the upper filesystem contains an opaque directory, any
 directory in the lower filesystem with the same name is ignored.

+An opaque directory should not conntain any whiteouts, because they do not
+serve any purpose.  A merge directory containing regular files with the xattr
+"trusted.overlay.whiteout", should be additionally marked by setting the xattr
+"trusted.overlay.opaque" to "x" on the merge directory itself.
+
 readdir
 -------

> BTW the nesting
> format should also be documented, but that's a separate patch.
>

Alex already did that:

https://docs.kernel.org/filesystems/overlayfs.html#nesting-overlayfs-mounts

> > @@ -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;
>
> Maybe I'm missing something, but can't we set the flag on the layer?
>

We do not currently have per-directory-per-layer flags in ovl_lowerstack().

Your patch was optimizing only per-layer check_xwhiteout.
My patch is optimizing only per-directory check_xwhiteout.

The important thing is that for the common case (no xwhiteouts)
xwhiteout will never be checked.

Are you concerned about optimizing check_xwhiteout in a multi lower
overlayfs nested over a composefs overlay mount for the case that
one of the merge dirs in the stack have xwhiteouts and the other do not??

I guess we can use a combination of your patch (v2) and my patch (v3)
and do something like this:

              if (last_element && !is_upper && val == 'x') {
                       d->xwhiteouts = d->layer->xwhiteouts = true;

...

to mark the dentry as OVL_E_XWHITEOUTS
AND mark the layer as having xwhiteouts
and then in readdir we check that
BOTH dentry has xwhiteouts (in some layer)
AND the layer has xwhiteouts (in some directory).

Is that what you meant?

Would you like me to make this change?

Thanks,
Amir.

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

[Amir Goldstein]

On Mon, Jan 22, 2024 at 3:17 PM Alexander Larsson <alexl@redhat.com> wrote:
>
...
> > I noticed you comment in composefs:
> >
> >  * 1 - Mark xwhitouts using the new opaque=x format as needed by
> > Linux 6.8
> >
> > Note that this "fix" is aimed to be backported to v6.7.y, so there is
> > no kernel
> > version that is expected to retain support for the old format.
>
> Yes, but the composefs format needs to be bitwise reproducible, and
> this change in the image will cause a different digest for the produced
> image, so we can't just change what we generate, it has to be opt in to
> users and able to reproduce previous versions.
>

I understand. I just meant that the comment about Linux 6.8 is a bit
confusing.

I guess that composefs version 0 is a pre-release version, if we are
not counting v6.7 as a release that any distro is actually using...

Thanks,
Amir.

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

[Miklos Szeredi]

On Mon, 22 Jan 2024 at 14:18, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Something like this looks ok?
>
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -145,7 +145,9 @@ filesystem, an overlay filesystem needs to record
> in the upper filesystem
>  that files have been removed.  This is done using whiteouts and opaque
>  directories (non-directories are always opaque).
>
> -A whiteout is created as a character device with 0/0 device number.
> +A whiteout is created as a character device with 0/0 device number or
> +as a regular file with the xattr "trusted.overlay.whiteout".

It should also refer to the "whiteouts and opaque directories" section.

> +
>  When a whiteout is found in the upper level of a merged directory, any
>  matching name in the lower level is ignored, and the whiteout itself
>  is also hidden.
> @@ -154,6 +156,11 @@ A directory is made opaque by setting the xattr
> "trusted.overlay.opaque"
>  to "y".  Where the upper filesystem contains an opaque directory, any
>  directory in the lower filesystem with the same name is ignored.
>
> +An opaque directory should not conntain any whiteouts, because they do not
> +serve any purpose.  A merge directory containing regular files with the xattr
> +"trusted.overlay.whiteout", should be additionally marked by setting the xattr
> +"trusted.overlay.opaque" to "x" on the merge directory itself.

I think it's worth noting that this can have a performance impact on
readdir, so people don't think xwhiteouts are a drop in replacement.

> Alex already did that:
>
> https://docs.kernel.org/filesystems/overlayfs.html#nesting-overlayfs-mounts

Indeed, thanks.

> We do not currently have per-directory-per-layer flags in ovl_lowerstack().
>
> Your patch was optimizing only per-layer check_xwhiteout.
> My patch is optimizing only per-directory check_xwhiteout.
>
> The important thing is that for the common case (no xwhiteouts)
> xwhiteout will never be checked.
>
> Are you concerned about optimizing check_xwhiteout in a multi lower
> overlayfs nested over a composefs overlay mount for the case that
> one of the merge dirs in the stack have xwhiteouts and the other do not??
>
> I guess we can use a combination of your patch (v2) and my patch (v3)
> and do something like this:
>
>               if (last_element && !is_upper && val == 'x') {
>                        d->xwhiteouts = d->layer->xwhiteouts = true;
>
> ...
>
> to mark the dentry as OVL_E_XWHITEOUTS
> AND mark the layer as having xwhiteouts
> and then in readdir we check that
> BOTH dentry has xwhiteouts (in some layer)
> AND the layer has xwhiteouts (in some directory).
>
> Is that what you meant?

I didn't think it through, but yeah, this sounds good.

This way we can also remove the checks against layer not being upper
and lowest, right?

Thanks,
Miklos

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

[Amir Goldstein]

On Mon, Jan 22, 2024 at 3:35 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 22 Jan 2024 at 14:18, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Something like this looks ok?
> >
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -145,7 +145,9 @@ filesystem, an overlay filesystem needs to record
> > in the upper filesystem
> >  that files have been removed.  This is done using whiteouts and opaque
> >  directories (non-directories are always opaque).
> >
> > -A whiteout is created as a character device with 0/0 device number.
> > +A whiteout is created as a character device with 0/0 device number or
> > +as a regular file with the xattr "trusted.overlay.whiteout".
>
> It should also refer to the "whiteouts and opaque directories" section.

ok.

>
> > +
> >  When a whiteout is found in the upper level of a merged directory, any
> >  matching name in the lower level is ignored, and the whiteout itself
> >  is also hidden.
> > @@ -154,6 +156,11 @@ A directory is made opaque by setting the xattr
> > "trusted.overlay.opaque"
> >  to "y".  Where the upper filesystem contains an opaque directory, any
> >  directory in the lower filesystem with the same name is ignored.
> >
> > +An opaque directory should not conntain any whiteouts, because they do not
> > +serve any purpose.  A merge directory containing regular files with the xattr
> > +"trusted.overlay.whiteout", should be additionally marked by setting the xattr
> > +"trusted.overlay.opaque" to "x" on the merge directory itself.
>
> I think it's worth noting that this can have a performance impact on
> readdir, so people don't think xwhiteouts are a drop in replacement.
>

Ok, I will write something for v4.

> > Alex already did that:
> >
> > https://docs.kernel.org/filesystems/overlayfs.html#nesting-overlayfs-mounts
>
> Indeed, thanks.
>
> > We do not currently have per-directory-per-layer flags in ovl_lowerstack().
> >
> > Your patch was optimizing only per-layer check_xwhiteout.
> > My patch is optimizing only per-directory check_xwhiteout.
> >
> > The important thing is that for the common case (no xwhiteouts)
> > xwhiteout will never be checked.
> >
> > Are you concerned about optimizing check_xwhiteout in a multi lower
> > overlayfs nested over a composefs overlay mount for the case that
> > one of the merge dirs in the stack have xwhiteouts and the other do not??
> >
> > I guess we can use a combination of your patch (v2) and my patch (v3)
> > and do something like this:
> >
> >               if (last_element && !is_upper && val == 'x') {
> >                        d->xwhiteouts = d->layer->xwhiteouts = true;
> >
> > ...
> >
> > to mark the dentry as OVL_E_XWHITEOUTS
> > AND mark the layer as having xwhiteouts
> > and then in readdir we check that
> > BOTH dentry has xwhiteouts (in some layer)
> > AND the layer has xwhiteouts (in some directory).
> >
> > Is that what you meant?
>
> I didn't think it through, but yeah, this sounds good.
>
> This way we can also remove the checks against layer not being upper
> and lowest, right?

I think we can.
Let me try this out.

Thanks,
Amir.