Re: [PATCH 2/2] erofs-utils: mkfs: fix origin xattrs when merging layers

[Gao Xiang <xiang@kernel.org>]

Hi Wenyuan,

On Sat, Nov 07, 2026 at 04:27:03AM +0000, Wenyuan Xu wrote:
> Only supplement empty trusted.overlay.origin xattrs on whiteout parents
> during incremental builds or multi-source rebuilds. Ordinary local
> directory builds and single-source rebuilds should not synthesize this
> merge marker.
> 
> Preserve any existing origin xattr instead of appending a duplicate
> empty entry.
> 
> Fixes: 8680f3cf94ef ("erofs-utils: mkfs: enable incremental builds for local files")
> Assisted-by: LLM
> Signed-off-by: Wenyuan Xu <sereno.xu@deepseek.com>

Can the following diff resolve the issue as well? (untested)
If possible, could you ask LLM to refine this way?

Thanks,
Gao Xiang


diff --git a/include/erofs/internal.h b/include/erofs/internal.h
index 19e38fb..1eeb78f 100644
--- a/include/erofs/internal.h
+++ b/include/erofs/internal.h
@@ -269,6 +269,7 @@ struct erofs_inode {
 	char idata_type;
 	bool lazy_tailblock;
 	bool opaque;
+	bool origin;
 	/* OVL: non-merge dir that may contain whiteout entries */
 	bool whiteouts;
 	bool dot_omitted;
diff --git a/lib/tar.c b/lib/tar.c
index 72d867c..2f3e189 100644
--- a/lib/tar.c
+++ b/lib/tar.c
@@ -1080,7 +1080,6 @@ out_eot:
 		 * needed if the tar tree is used soon, thus we have no chance
 		 * to generate it from xattrs.  No impact to mergefs.
 		 */
-		d->inode->opaque = true;
 		ret = erofs_set_opaque_xattr(d->inode);
 		goto out;
 	} else if (th->typeflag == '1') {	/* hard link cases */
diff --git a/lib/xattr.c b/lib/xattr.c
index c670b12..59281d3 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -452,7 +452,7 @@ static bool erofs_is_skipped_xattr(const char *key)
 }
 
 static int read_xattrs_from_file(struct erofs_sb_info *sbi, const char *path,
-				 mode_t mode, struct list_head *ixattrs)
+				 mode_t mode, struct erofs_inode *inode)
 {
 	ssize_t kllen = erofs_sys_llistxattr(path, NULL, 0);
 	char *keylst, *key, *klend;
@@ -507,10 +507,16 @@ static int read_xattrs_from_file(struct erofs_sb_info *sbi, const char *path,
 			goto err;
 		}
 
-		if (ixattrs) {
-			ret = erofs_inode_xattr_add(ixattrs, item);
+		if (inode) {
+			ret = erofs_inode_xattr_add(&inode->i_xattrs, item);
 			if (ret < 0)
 				goto err;
+			if (!strcmp(key, OVL_XATTR_OPAQUE))
+				inode->opaque = S_ISDIR(inode->i_mode);
+			if (!strcmp(key, OVL_XATTR_ORIGIN)) {
+				inode->origin = true;
+				inode->whiteouts |= S_ISDIR(inode->i_mode);
+			}
 		}
 	}
 	free(keylst);
@@ -520,8 +526,8 @@ out:
 	item = erofs_get_selabel_xattr(sbi, path, mode);
 	if (IS_ERR(item))
 		return PTR_ERR(item);
-	if (item && ixattrs)
-		ret = erofs_inode_xattr_add(ixattrs, item);
+	if (item && inode)
+		ret = erofs_inode_xattr_add(&inode->i_xattrs, item);
 	return ret;
 
 err:
@@ -595,12 +601,18 @@ static void erofs_removexattr(struct erofs_inode *inode, const char *key)
 
 int erofs_set_opaque_xattr(struct erofs_inode *inode)
 {
+	if (inode->opaque)
+		return 0;
+	inode->opaque = true;
 	return erofs_vfs_setxattr(inode, OVL_XATTR_OPAQUE, "y", 1);
 }
 
 void erofs_clear_opaque_xattr(struct erofs_inode *inode)
 {
+	if (!inode->opaque)
+		return;
 	erofs_removexattr(inode, OVL_XATTR_OPAQUE);
+	inode->opaque = false;
 }
 
 bool erofs_get_opaque_from_disk(struct erofs_inode *inode)
@@ -610,6 +622,9 @@ bool erofs_get_opaque_from_disk(struct erofs_inode *inode)
 
 int erofs_set_origin_xattr(struct erofs_inode *inode)
 {
+	if (inode->origin)
+		return 0;
+	inode->origin = true;
 	return erofs_vfs_setxattr(inode, OVL_XATTR_ORIGIN, NULL, 0);
 }
 
@@ -659,10 +674,9 @@ static int erofs_droid_xattr_set_caps(struct erofs_inode *inode)
 int erofs_scan_file_xattrs(struct erofs_inode *inode)
 {
 	int ret;
-	struct list_head *ixattrs = &inode->i_xattrs;
 
 	ret = read_xattrs_from_file(inode->sbi, inode->i_srcpath,
-				    inode->i_mode, ixattrs);
+				    inode->i_mode, inode);
 	if (ret < 0)
 		return ret;
 
@@ -721,11 +735,9 @@ int erofs_read_xattrs_from_disk(struct erofs_inode *inode)
 				goto out;
 			}
 			DBG_BUGON(ret != size);
-		} else if (S_ISDIR(inode->i_mode) &&
-			   !strcmp(key, OVL_XATTR_ORIGIN)) {
-			ret = 0;
-			inode->whiteouts = true;
-			continue;
+		} else if (!strcmp(key, OVL_XATTR_ORIGIN)) {
+			inode->origin = true;
+			inode->whiteouts |= S_ISDIR(inode->i_mode);
 		}
 
 		ret = erofs_vfs_setxattr(inode, key, value, size);
-- 
2.50.1 (Apple Git-155)


> ---
>  include/erofs/importer.h |   1 +
>  lib/inode.c              |   2 +-
>  lib/xattr.c              |   2 +
>  mkfs/main.c              |   2 +
>  tests/Makefile.am        |   3 +
>  tests/erofs/033          | 122 +++++++++++++++++++++++++++++++++++++++
>  tests/erofs/033.out      |   2 +
>  7 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100755 tests/erofs/033
>  create mode 100644 tests/erofs/033.out
> 
> diff --git a/include/erofs/importer.h b/include/erofs/importer.h
> index 5141965..b8dcaf8 100644
> --- a/include/erofs/importer.h
> +++ b/include/erofs/importer.h
> @@ -54,6 +54,7 @@ struct erofs_importer_params {
>  	bool dirdata_in_metazone;
>  	bool hard_dereference;
>  	bool ovlfs_strip;
> +	bool ovlfs_merge;		/* build involves overlayfs-style merging */
>  	bool dot_omitted;
>  	bool no_xattrs;			/* don't store extended attributes */
>  	bool no_zcompact;
> diff --git a/lib/inode.c b/lib/inode.c
> index da29b5a..185bcfc 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -2134,7 +2134,7 @@ static int erofs_mkfs_handle_inode(const struct erofs_mkfs_btctx *ctx,
>  	/* strip all unnecessary overlayfs xattrs when ovlfs_strip is enabled */
>  	if (params->ovlfs_strip)
>  		erofs_clear_opaque_xattr(inode);
> -	else if (inode->whiteouts)
> +	else if (params->ovlfs_merge && inode->whiteouts)
>  		erofs_set_origin_xattr(inode);
>  
>  	if (!S_ISDIR(inode->i_mode)) {
> diff --git a/lib/xattr.c b/lib/xattr.c
> index a0ac76f..01a8547 100644
> --- a/lib/xattr.c
> +++ b/lib/xattr.c
> @@ -619,6 +619,8 @@ bool erofs_get_opaque_from_disk(struct erofs_inode *inode)
>  
>  int erofs_set_origin_xattr(struct erofs_inode *inode)
>  {
> +	if (erofs_inode_has_xattr(inode, OVL_XATTR_ORIGIN))
> +		return 0;
>  	return erofs_vfs_setxattr(inode, OVL_XATTR_ORIGIN, NULL, 0);
>  }
>  
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 93123c1..06d868a 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1899,6 +1899,8 @@ int main(int argc, char **argv)
>  	importer_params.source = cfg.c_src_path;
>  	importer_params.no_datainline = mkfs_no_datainline;
>  	importer_params.dot_omitted = mkfs_dot_omitted;
> +	importer_params.ovlfs_merge = incremental_mode ||
> +		(source_mode == EROFS_MKFS_SOURCE_REBUILD && rebuild_src_count > 1);
>  	if (importer_params.dedupe == EROFS_DEDUPE_FORCE_ON &&
>  	    !mkfscfg.chunkbits && !mkfscfg.total_zcfgs) {
>  		erofs_err("Compression is not enabled.  Turn on chunk-based data deduplication instead.");
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 363b93d..bfd1fc2 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -135,6 +135,9 @@ TESTS += erofs/031
>  # 032 - check that the xattr layout does not depend on listxattr(2) order
>  TESTS += erofs/032
>  
> +# 033 - preserve existing origin and only add empty origin when merging
> +TESTS += erofs/033
> +
>  # NEW TEST CASE HERE
>  # TESTS += erofs/999
>  
> diff --git a/tests/erofs/033 b/tests/erofs/033
> new file mode 100755
> index 0000000..1da7bca
> --- /dev/null
> +++ b/tests/erofs/033
> @@ -0,0 +1,122 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Only supplement missing origin xattrs on whiteout parents when merging.
> +# Existing origin values must survive without duplicate xattr names.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$(echo $0 | awk '{print $((NF-1))"/"$NF}' FS="/")
> +
> +. "${srcdir}/common/rc"
> +
> +cleanup()
> +{
> +	cd /
> +	rm -rf $tmp.*
> +}
> +
> +_require_root
> +_require_erofs
> +_require_xattr
> +command -v getfattr >/dev/null 2>&1 || _notrun "getfattr isn't installed"
> +
> +rm -f $seqres.full
> +
> +localdir=$tmp/$seq
> +mkdir -p "$localdir/source" "$localdir/base" || _fail "mkdir failed"
> +
> +# Probe capabilities as root alone does not guarantee CAP_MKNOD/SYS_ADMIN.
> +mknod "$localdir/probe" c 0 0 >> $seqres.full 2>&1 ||
> +	_notrun "cannot create whiteouts"
> +setfattr -n trusted.overlay.origin -v xxx "$localdir/source" \
> +	>> $seqres.full 2>&1 || _notrun "cannot set trusted xattrs"
> +getfattr -n trusted.overlay.origin --only-values "$localdir/source" \
> +	> "$localdir/probe-value" 2>> $seqres.full ||
> +	_notrun "cannot read trusted xattrs"
> +[ "$(cat "$localdir/probe-value")" = xxx ] ||
> +	_notrun "trusted xattrs are not visible"
> +setfattr -x trusted.overlay.origin "$localdir/source" \
> +	>> $seqres.full 2>&1 || _fail "cannot remove probe xattr"
> +
> +echo "QA output created by $seq"
> +
> +for state in absent empty value; do
> +	mkdir "$localdir/source/$state" || _fail "mkdir failed"
> +	mknod "$localdir/source/$state/whiteout" c 0 0 \
> +		>> $seqres.full 2>&1 || _fail "cannot create whiteout"
> +done
> +setfattr -n trusted.overlay.origin -v '' "$localdir/source/empty" \
> +	>> $seqres.full 2>&1 || _fail "cannot set empty origin"
> +setfattr -n trusted.overlay.origin -v xxx "$localdir/source/value" \
> +	>> $seqres.full 2>&1 || _fail "cannot set nonempty origin"
> +echo base > "$localdir/base/file"
> +
> +# Dumping all matching names also catches duplicate names: getxattr alone
> +# would retrieve the same first value for each occurrence of the name.
> +check_origin()
> +{
> +	getfattr --absolute-names -d -m '^trusted\.overlay\.origin$' \
> +		"$SCRATCH_MNT/$1" > "$localdir/attrs" 2>> $seqres.full ||
> +		_fail "cannot read $case_name/$1 xattrs"
> +	cat "$localdir/attrs" >> $seqres.full
> +	actual=$(sed -n '/^trusted\.overlay\.origin=/p' "$localdir/attrs")
> +	[ "$actual" = "$2" ] || _fail "unexpected origin in $case_name/$1"
> +	[ -c "$SCRATCH_MNT/$1/whiteout" ] ||
> +		_fail "whiteout lost in $case_name/$1"
> +}
> +
> +build_image()
> +{
> +	$MKFS_EROFS_PROG "$@" >> $seqres.full 2>&1 ||
> +		_fail "mkfs failed for $case_name"
> +}
> +
> +case_name=localdir
> +SCRATCH_DEV=$localdir/source.erofs
> +build_image "$SCRATCH_DEV" "$localdir/source"
> +_scratch_mount >> $seqres.full 2>&1
> +check_origin absent ''
> +check_origin empty 'trusted.overlay.origin=""'
> +check_origin value 'trusted.overlay.origin="xxx"'
> +_scratch_unmount >> $seqres.full 2>&1 || _fail "unmount failed"
> +
> +case_name=single-source
> +SCRATCH_DEV=$localdir/single.erofs
> +build_image --clean=data "$SCRATCH_DEV" "$localdir/source.erofs"
> +_scratch_mount >> $seqres.full 2>&1
> +check_origin absent ''
> +check_origin value 'trusted.overlay.origin="xxx"'
> +_scratch_unmount >> $seqres.full 2>&1 || _fail "unmount failed"
> +
> +case_name=base
> +build_image "$localdir/base.erofs" "$localdir/base"
> +
> +for case_name in multi-source incremental-localdir incremental-rebuild; do
> +	SCRATCH_DEV=$localdir/$case_name.erofs
> +	case $case_name in
> +	multi-source)
> +		build_image --clean=data "$SCRATCH_DEV" \
> +			"$localdir/base.erofs" "$localdir/source.erofs"
> +		;;
> +	incremental-*)
> +		cp "$localdir/base.erofs" "$SCRATCH_DEV" || _fail "copy failed"
> +		if [ "$case_name" = incremental-localdir ]; then
> +			source=$localdir/source
> +		else
> +			source=$localdir/source.erofs
> +		fi
> +		build_image --incremental=data "$SCRATCH_DEV" "$source"
> +		;;
> +	esac
> +	_scratch_mount >> $seqres.full 2>&1
> +	check_origin absent 'trusted.overlay.origin=""'
> +	check_origin empty 'trusted.overlay.origin=""'
> +	check_origin value 'trusted.overlay.origin="xxx"'
> +	[ "$(cat "$SCRATCH_MNT/file")" = base ] ||
> +		_fail "base file lost in $case_name"
> +	_scratch_unmount >> $seqres.full 2>&1 || _fail "unmount failed"
> +done
> +
> +echo Silence is golden
> +status=0
> +exit 0
> diff --git a/tests/erofs/033.out b/tests/erofs/033.out
> new file mode 100644
> index 0000000..c5d2914
> --- /dev/null
> +++ b/tests/erofs/033.out
> @@ -0,0 +1,2 @@
> +QA output created by 033
> +Silence is golden
> -- 
> 2.43.0
> 
>