linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHSET 0/5] fstests: support external logs when caching prepopulated images
@ 2022-10-12  1:45 Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

Hi all,

This patchset fixes the code that manages prepopulated fs images so that
it deals with external log devices properly.  The cached image generator
requires a clean filesystem, so it's safe to reformat the external
journal whenever we restore a cached image; this fixes various
regressions when external journals are in use.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=cached-image-external-log
---
 common/config   |    1 +
 common/fuzzy    |    2 +-
 common/populate |   49 +++++++++++++++++++++++++++++++++++++++++--------
 tests/xfs/129   |    3 ++-
 tests/xfs/234   |    3 ++-
 tests/xfs/253   |    3 ++-
 tests/xfs/284   |    3 ++-
 tests/xfs/291   |    3 ++-
 tests/xfs/336   |    3 ++-
 tests/xfs/432   |    3 ++-
 tests/xfs/503   |    9 +++++----
 11 files changed, 62 insertions(+), 20 deletions(-)


^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 1/5] populate: export the metadump description name
  2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
@ 2022-10-12  1:45 ` Darrick J. Wong
  2022-10-13 14:55   ` Zorro Lang
  2022-10-14 18:21   ` [PATCH v1.1 " Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image Darrick J. Wong
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Not sure why this hasn't been broken all along, but we should be
exporting this variable so that it shows up in subshells....

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/common/populate b/common/populate
index cfdaf766f0..b501c2fe45 100644
--- a/common/populate
+++ b/common/populate
@@ -868,9 +868,9 @@ _scratch_populate_cached() {
 	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
 	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
 
-	# These variables are shared outside this function
-	POPULATE_METADUMP="${metadump_stem}.metadump"
-	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
+	# This variable is shared outside this function
+	export POPULATE_METADUMP="${metadump_stem}.metadump"
+	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
 
 	# Don't keep metadata images cached for more 48 hours...
 	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image
  2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
@ 2022-10-12  1:45 ` Darrick J. Wong
  2022-10-13 15:10   ` Zorro Lang
  2022-10-12  1:45 ` [PATCH 3/5] populate: reformat external ext[34] journal " Darrick J. Wong
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

The fs population code has the ability to save cached metadumps of
filesystems to save time when running fstests.  The cached images should
be unmounted cleanly, so we never save the contents of external log
devices.

Unfortunately, the cache restore code fails to wipe the external log
when restoring a clean image, so we end up with strange test failures
because the log doesn't match the filesystem:

* ERROR: mismatched uuid in log
*            SB : 5ffec625-d3bb-4f4e-a181-1f9efe543d9c
*            log: 607bd75a-a63d-400c-8779-2139f0a3d384

Worse yet, xfs_repair will overwrite a filesystem's uuid with the log
uuid, which leads to corruption messages later on:

Metadata corruption detected at 0x561f69a9a2a8, xfs_agf block 0x8/0x1000
xfs_db: cannot init perag data (117). Continuing anyway.

Solve this by wiping the log device when restoring.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)


diff --git a/common/populate b/common/populate
index b501c2fe45..0bd78e0a0a 100644
--- a/common/populate
+++ b/common/populate
@@ -11,7 +11,12 @@ _require_populate_commands() {
 	_require_xfs_io_command "falloc"
 	_require_xfs_io_command "fpunch"
 	_require_test_program "punch-alternating"
-	_require_command "$XFS_DB_PROG" "xfs_db"
+	case "${FSTYP}" in
+	"xfs")
+		_require_command "$XFS_DB_PROG" "xfs_db"
+		_require_command "$WIPEFS_PROG" "wipefs"
+		;;
+	esac
 }
 
 _require_xfs_db_blocktrash_z_command() {
@@ -851,7 +856,19 @@ _scratch_populate_restore_cached() {
 
 	case "${FSTYP}" in
 	"xfs")
-		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}" && return 0
+		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}"
+		res=$?
+		test $res -ne 0 && return $res
+
+		# Cached images should have been unmounted cleanly, so if
+		# there's an external log we need to wipe it and run repair to
+		# format it to match this filesystem.
+		if [ -n "${SCRATCH_LOGDEV}" ]; then
+			$WIPEFS_PROG -a "${SCRATCH_LOGDEV}"
+			_scratch_xfs_repair
+			res=$?
+		fi
+		return $res
 		;;
 	"ext2"|"ext3"|"ext4")
 		# ext4 cannot e2image external logs, so we cannot restore


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/5] populate: reformat external ext[34] journal devices when restoring a cached image
  2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image Darrick J. Wong
@ 2022-10-12  1:45 ` Darrick J. Wong
  2022-10-13 15:47   ` Zorro Lang
  2022-10-12  1:45 ` [PATCH 4/5] populate: require e2image before populating Darrick J. Wong
  2022-10-12  1:45 ` [PATCH 5/5] fstests: refactor xfs_mdrestore calls Darrick J. Wong
  4 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

The fs population code has the ability to save cached metadumps of
filesystems to save time when running fstests.  The cached images should
be unmounted cleanly, so we never save the contents of external journal
devices.

Unfortunately, the cache restore code fails to reset the external
journal when restoring a clean image, so we ignore cached images because
the journal doesn't match the filesystem.  This makes test runtimes
longer than they need to be.

Solve this by reformatting the external journal to match the filesystem.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)


diff --git a/common/populate b/common/populate
index 0bd78e0a0a..66c55b682f 100644
--- a/common/populate
+++ b/common/populate
@@ -16,6 +16,9 @@ _require_populate_commands() {
 		_require_command "$XFS_DB_PROG" "xfs_db"
 		_require_command "$WIPEFS_PROG" "wipefs"
 		;;
+	ext*)
+		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
+		;;
 	esac
 }
 
@@ -871,9 +874,20 @@ _scratch_populate_restore_cached() {
 		return $res
 		;;
 	"ext2"|"ext3"|"ext4")
-		# ext4 cannot e2image external logs, so we cannot restore
-		test -n "${SCRATCH_LOGDEV}" && return 1
-		e2image -r "${metadump}" "${SCRATCH_DEV}" && return 0
+		e2image -r "${metadump}" "${SCRATCH_DEV}"
+		ret=$?
+		test $ret -ne 0 && return $ret
+
+		# ext4 cannot e2image external logs, so we have to reformat
+		# the scratch device to match the restored fs
+		if [ -n "${SCRATCH_LOGDEV}" ]; then
+			local fsuuid="$($DUMPE2FS_PROG -h "${SCRATCH_DEV}" 2>/dev/null | \
+					grep 'Journal UUID:' | \
+					sed -e 's/Journal UUID:[[:space:]]*//g')"
+			$MKFS_EXT4_PROG -O journal_dev "${SCRATCH_LOGDEV}" \
+					-F -U "${fsuuid}"
+		fi
+		return 0
 		;;
 	esac
 	return 1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 4/5] populate: require e2image before populating
  2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
                   ` (2 preceding siblings ...)
  2022-10-12  1:45 ` [PATCH 3/5] populate: reformat external ext[34] journal " Darrick J. Wong
@ 2022-10-12  1:45 ` Darrick J. Wong
  2022-10-13 15:48   ` Zorro Lang
  2022-10-12  1:45 ` [PATCH 5/5] fstests: refactor xfs_mdrestore calls Darrick J. Wong
  4 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Use $E2IMAGE_PROG, not e2image, and check that it exists before
proceeding.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/common/populate b/common/populate
index 66c55b682f..05bdfe33c5 100644
--- a/common/populate
+++ b/common/populate
@@ -18,6 +18,7 @@ _require_populate_commands() {
 		;;
 	ext*)
 		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
+		_require_command "$E2IMAGE_PROG" "e2image"
 		;;
 	esac
 }
@@ -874,7 +875,7 @@ _scratch_populate_restore_cached() {
 		return $res
 		;;
 	"ext2"|"ext3"|"ext4")
-		e2image -r "${metadump}" "${SCRATCH_DEV}"
+		$E2IMAGE_PROG -r "${metadump}" "${SCRATCH_DEV}"
 		ret=$?
 		test $ret -ne 0 && return $ret
 


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5/5] fstests: refactor xfs_mdrestore calls
  2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
                   ` (3 preceding siblings ...)
  2022-10-12  1:45 ` [PATCH 4/5] populate: require e2image before populating Darrick J. Wong
@ 2022-10-12  1:45 ` Darrick J. Wong
  2022-10-13 16:06   ` Zorro Lang
  4 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-12  1:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Wrap calls to xfs_mdrestore in the usual $XFS_MDRESTORE_PROG variable.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/config   |    1 +
 common/fuzzy    |    2 +-
 common/populate |    3 ++-
 tests/xfs/129   |    3 ++-
 tests/xfs/234   |    3 ++-
 tests/xfs/253   |    3 ++-
 tests/xfs/284   |    3 ++-
 tests/xfs/291   |    3 ++-
 tests/xfs/336   |    3 ++-
 tests/xfs/432   |    3 ++-
 tests/xfs/503   |    9 +++++----
 11 files changed, 23 insertions(+), 13 deletions(-)


diff --git a/common/config b/common/config
index 5eaae4471d..0c3813dc23 100644
--- a/common/config
+++ b/common/config
@@ -156,6 +156,7 @@ export XFS_LOGPRINT_PROG="$(type -P xfs_logprint)"
 export XFS_REPAIR_PROG="$(type -P xfs_repair)"
 export XFS_DB_PROG="$(type -P xfs_db)"
 export XFS_METADUMP_PROG="$(type -P xfs_metadump)"
+export XFS_MDRESTORE_PROG="$(type -P xfs_mdrestore)"
 export XFS_ADMIN_PROG="$(type -P xfs_admin)"
 export XFS_GROWFS_PROG=$(type -P xfs_growfs)
 export XFS_SPACEMAN_PROG="$(type -P xfs_spaceman)"
diff --git a/common/fuzzy b/common/fuzzy
index b4fda6f533..2d688fd27b 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -159,7 +159,7 @@ __scratch_xfs_fuzz_mdrestore()
 	test -e "${POPULATE_METADUMP}" || _fail "Need to set POPULATE_METADUMP"
 
 	__scratch_xfs_fuzz_unmount
-	xfs_mdrestore "${POPULATE_METADUMP}" "${SCRATCH_DEV}"
+	$XFS_MDRESTORE_PROG "${POPULATE_METADUMP}" "${SCRATCH_DEV}"
 }
 
 __fuzz_notify() {
diff --git a/common/populate b/common/populate
index 05bdfe33c5..0c0538554a 100644
--- a/common/populate
+++ b/common/populate
@@ -15,6 +15,7 @@ _require_populate_commands() {
 	"xfs")
 		_require_command "$XFS_DB_PROG" "xfs_db"
 		_require_command "$WIPEFS_PROG" "wipefs"
+		_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 		;;
 	ext*)
 		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
@@ -860,7 +861,7 @@ _scratch_populate_restore_cached() {
 
 	case "${FSTYP}" in
 	"xfs")
-		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}"
+		$XFS_MDRESTORE_PROG "${metadump}" "${SCRATCH_DEV}"
 		res=$?
 		test $res -ne 0 && return $res
 
diff --git a/tests/xfs/129 b/tests/xfs/129
index 90887d5476..09d40630d0 100755
--- a/tests/xfs/129
+++ b/tests/xfs/129
@@ -25,6 +25,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_loop
 _require_scratch_reflink
 
@@ -52,7 +53,7 @@ _scratch_xfs_metadump $metadump_file
 
 # Now restore the obfuscated one back and take a look around
 echo "Restore metadump"
-xfs_mdrestore $metadump_file $TEST_DIR/image
+$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
 SCRATCH_DEV=$TEST_DIR/image _scratch_mount
 SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
 
diff --git a/tests/xfs/234 b/tests/xfs/234
index 6ff2f228e8..cc1ee9a8ca 100755
--- a/tests/xfs/234
+++ b/tests/xfs/234
@@ -24,6 +24,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_loop
 _require_xfs_scratch_rmapbt
 _require_xfs_io_command "fpunch"
@@ -52,7 +53,7 @@ _scratch_xfs_metadump $metadump_file
 
 # Now restore the obfuscated one back and take a look around
 echo "Restore metadump"
-xfs_mdrestore $metadump_file $TEST_DIR/image
+$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
 SCRATCH_DEV=$TEST_DIR/image _scratch_mount
 SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
 
diff --git a/tests/xfs/253 b/tests/xfs/253
index 1899ce5226..1cfc218088 100755
--- a/tests/xfs/253
+++ b/tests/xfs/253
@@ -32,6 +32,7 @@ _cleanup()
 # Import common functions.
 . ./common/filter
 
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_test
 _require_scratch
 
@@ -151,7 +152,7 @@ _scratch_unmount
 _scratch_xfs_metadump $METADUMP_FILE
 
 # Now restore the obfuscated one back and take a look around
-xfs_mdrestore "${METADUMP_FILE}" "${SCRATCH_DEV}"
+$XFS_MDRESTORE_PROG "${METADUMP_FILE}" "${SCRATCH_DEV}"
 
 _scratch_mount
 
diff --git a/tests/xfs/284 b/tests/xfs/284
index fa7bfdd0f0..e2bd05d4c7 100755
--- a/tests/xfs/284
+++ b/tests/xfs/284
@@ -24,6 +24,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_xfs_copy
 _require_test
 _require_scratch
@@ -48,7 +49,7 @@ _scratch_unmount
 # xfs_mdrestore should refuse to restore to a mounted device
 _scratch_xfs_metadump $METADUMP_FILE
 _scratch_mount
-xfs_mdrestore $METADUMP_FILE $SCRATCH_DEV 2>&1 | filter_mounted
+$XFS_MDRESTORE_PROG $METADUMP_FILE $SCRATCH_DEV 2>&1 | filter_mounted
 _scratch_unmount
 
 # Test xfs_copy to a mounted device
diff --git a/tests/xfs/291 b/tests/xfs/291
index a2425e472d..f5fea7f9a5 100755
--- a/tests/xfs/291
+++ b/tests/xfs/291
@@ -13,6 +13,7 @@ _begin_fstest auto repair metadump
 . ./common/filter
 
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 
 # real QA test starts here
 _require_scratch
@@ -92,7 +93,7 @@ _scratch_xfs_check >> $seqres.full 2>&1 || _fail "xfs_check failed"
 # Yes they can!  Now...
 # Can xfs_metadump cope with this monster?
 _scratch_xfs_metadump $tmp.metadump || _fail "xfs_metadump failed"
-xfs_mdrestore $tmp.metadump $tmp.img || _fail "xfs_mdrestore failed"
+$XFS_MDRESTORE_PROG $tmp.metadump $tmp.img || _fail "xfs_mdrestore failed"
 SCRATCH_DEV=$tmp.img _scratch_xfs_repair -f &>> $seqres.full || \
 	_fail "xfs_repair of metadump failed"
 
diff --git a/tests/xfs/336 b/tests/xfs/336
index 6592419d03..b1de8e5fc1 100755
--- a/tests/xfs/336
+++ b/tests/xfs/336
@@ -21,6 +21,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_realtime
 _require_xfs_scratch_rmapbt
 _require_test_program "punch-alternating"
@@ -64,7 +65,7 @@ _scratch_xfs_metadump $metadump_file
 
 # Now restore the obfuscated one back and take a look around
 echo "Restore metadump"
-xfs_mdrestore $metadump_file $TEST_DIR/image
+$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
 SCRATCH_DEV=$TEST_DIR/image _scratch_mount
 SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
 
diff --git a/tests/xfs/432 b/tests/xfs/432
index e1e610d054..676be9bd8a 100755
--- a/tests/xfs/432
+++ b/tests/xfs/432
@@ -28,6 +28,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs xfs
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_scratch
 
 rm -f "$seqres.full"
@@ -86,7 +87,7 @@ test -n "$extlen" || _notrun "could not create dir extent > 1000 blocks"
 
 echo "Try to metadump"
 _scratch_xfs_metadump $metadump_file -w
-xfs_mdrestore $metadump_file $metadump_img
+$XFS_MDRESTORE_PROG $metadump_file $metadump_img
 
 echo "Check restored metadump image"
 SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
diff --git a/tests/xfs/503 b/tests/xfs/503
index 6c4bfd1c45..18bd8694c8 100755
--- a/tests/xfs/503
+++ b/tests/xfs/503
@@ -28,6 +28,7 @@ testdir=$TEST_DIR/test-$seq
 # real QA test starts here
 _supported_fs xfs
 
+_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
 _require_xfs_copy
 _require_scratch_nocheck
 _require_populate_commands
@@ -65,25 +66,25 @@ _check_scratch_fs
 _scratch_unmount
 
 echo mdrestore
-xfs_mdrestore $metadump_file $SCRATCH_DEV
+$XFS_MDRESTORE_PROG $metadump_file $SCRATCH_DEV
 _scratch_mount
 _check_scratch_fs
 _scratch_unmount
 
 echo mdrestore a
-xfs_mdrestore $metadump_file_a $SCRATCH_DEV
+$XFS_MDRESTORE_PROG $metadump_file_a $SCRATCH_DEV
 _scratch_mount
 _check_scratch_fs
 _scratch_unmount
 
 echo mdrestore g
-xfs_mdrestore $metadump_file_g $SCRATCH_DEV
+$XFS_MDRESTORE_PROG $metadump_file_g $SCRATCH_DEV
 _scratch_mount
 _check_scratch_fs
 _scratch_unmount
 
 echo mdrestore ag
-xfs_mdrestore $metadump_file_ag $SCRATCH_DEV
+$XFS_MDRESTORE_PROG $metadump_file_ag $SCRATCH_DEV
 _scratch_mount
 _check_scratch_fs
 _scratch_unmount


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] populate: export the metadump description name
  2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
@ 2022-10-13 14:55   ` Zorro Lang
  2022-10-13 15:54     ` Darrick J. Wong
  2022-10-14 18:21   ` [PATCH v1.1 " Darrick J. Wong
  1 sibling, 1 reply; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 14:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Tue, Oct 11, 2022 at 06:45:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Not sure why this hasn't been broken all along, but we should be
> exporting this variable so that it shows up in subshells....

May I ask where's the subshell which uses $POPULATE_METADUMP?

> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index cfdaf766f0..b501c2fe45 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -868,9 +868,9 @@ _scratch_populate_cached() {
>  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
>  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
>  
> -	# These variables are shared outside this function
> -	POPULATE_METADUMP="${metadump_stem}.metadump"
> -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> +	# This variable is shared outside this function
> +	export POPULATE_METADUMP="${metadump_stem}.metadump"
> +	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"

If the POPULATE_METADUMP_DESCR is not shared outside anymore, how about change
it to lower-case?

>  
>  	# Don't keep metadata images cached for more 48 hours...
>  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image
  2022-10-12  1:45 ` [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image Darrick J. Wong
@ 2022-10-13 15:10   ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 15:10 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Tue, Oct 11, 2022 at 06:45:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The fs population code has the ability to save cached metadumps of
> filesystems to save time when running fstests.  The cached images should
> be unmounted cleanly, so we never save the contents of external log
> devices.
> 
> Unfortunately, the cache restore code fails to wipe the external log
> when restoring a clean image, so we end up with strange test failures
> because the log doesn't match the filesystem:
> 
> * ERROR: mismatched uuid in log
> *            SB : 5ffec625-d3bb-4f4e-a181-1f9efe543d9c
> *            log: 607bd75a-a63d-400c-8779-2139f0a3d384
> 
> Worse yet, xfs_repair will overwrite a filesystem's uuid with the log
> uuid, which leads to corruption messages later on:
> 
> Metadata corruption detected at 0x561f69a9a2a8, xfs_agf block 0x8/0x1000
> xfs_db: cannot init perag data (117). Continuing anyway.
> 
> Solve this by wiping the log device when restoring.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Make sense,

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/populate |   21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index b501c2fe45..0bd78e0a0a 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -11,7 +11,12 @@ _require_populate_commands() {
>  	_require_xfs_io_command "falloc"
>  	_require_xfs_io_command "fpunch"
>  	_require_test_program "punch-alternating"
> -	_require_command "$XFS_DB_PROG" "xfs_db"
> +	case "${FSTYP}" in
> +	"xfs")
> +		_require_command "$XFS_DB_PROG" "xfs_db"
> +		_require_command "$WIPEFS_PROG" "wipefs"
> +		;;
> +	esac
>  }
>  
>  _require_xfs_db_blocktrash_z_command() {
> @@ -851,7 +856,19 @@ _scratch_populate_restore_cached() {
>  
>  	case "${FSTYP}" in
>  	"xfs")
> -		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}" && return 0
> +		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}"
> +		res=$?
> +		test $res -ne 0 && return $res
> +
> +		# Cached images should have been unmounted cleanly, so if
> +		# there's an external log we need to wipe it and run repair to
> +		# format it to match this filesystem.
> +		if [ -n "${SCRATCH_LOGDEV}" ]; then
> +			$WIPEFS_PROG -a "${SCRATCH_LOGDEV}"
> +			_scratch_xfs_repair
> +			res=$?
> +		fi
> +		return $res
>  		;;
>  	"ext2"|"ext3"|"ext4")
>  		# ext4 cannot e2image external logs, so we cannot restore
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] populate: reformat external ext[34] journal devices when restoring a cached image
  2022-10-12  1:45 ` [PATCH 3/5] populate: reformat external ext[34] journal " Darrick J. Wong
@ 2022-10-13 15:47   ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 15:47 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Tue, Oct 11, 2022 at 06:45:39PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The fs population code has the ability to save cached metadumps of
> filesystems to save time when running fstests.  The cached images should
> be unmounted cleanly, so we never save the contents of external journal
> devices.
> 
> Unfortunately, the cache restore code fails to reset the external
> journal when restoring a clean image, so we ignore cached images because
> the journal doesn't match the filesystem.  This makes test runtimes
> longer than they need to be.
> 
> Solve this by reformatting the external journal to match the filesystem.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |   20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 0bd78e0a0a..66c55b682f 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -16,6 +16,9 @@ _require_populate_commands() {
>  		_require_command "$XFS_DB_PROG" "xfs_db"
>  		_require_command "$WIPEFS_PROG" "wipefs"
>  		;;
> +	ext*)
> +		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
> +		;;
>  	esac
>  }
>  
> @@ -871,9 +874,20 @@ _scratch_populate_restore_cached() {
>  		return $res
>  		;;
>  	"ext2"|"ext3"|"ext4")
> -		# ext4 cannot e2image external logs, so we cannot restore
> -		test -n "${SCRATCH_LOGDEV}" && return 1
> -		e2image -r "${metadump}" "${SCRATCH_DEV}" && return 0
> +		e2image -r "${metadump}" "${SCRATCH_DEV}"
> +		ret=$?
> +		test $ret -ne 0 && return $ret
> +
> +		# ext4 cannot e2image external logs, so we have to reformat
> +		# the scratch device to match the restored fs
> +		if [ -n "${SCRATCH_LOGDEV}" ]; then
> +			local fsuuid="$($DUMPE2FS_PROG -h "${SCRATCH_DEV}" 2>/dev/null | \
> +					grep 'Journal UUID:' | \
> +					sed -e 's/Journal UUID:[[:space:]]*//g')"

This patch looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

Just ask, how about combine that grep and sed lines to:

  sed -n '/Journal UUID:/s/Journal UUID:[[:space:]]*//p'

That's not a big deal, both are OK to me.

Thanks,
Zorro

> +			$MKFS_EXT4_PROG -O journal_dev "${SCRATCH_LOGDEV}" \
> +					-F -U "${fsuuid}"
> +		fi
> +		return 0
>  		;;
>  	esac
>  	return 1
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] populate: require e2image before populating
  2022-10-12  1:45 ` [PATCH 4/5] populate: require e2image before populating Darrick J. Wong
@ 2022-10-13 15:48   ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 15:48 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Tue, Oct 11, 2022 at 06:45:44PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Use $E2IMAGE_PROG, not e2image, and check that it exists before
> proceeding.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Make sense,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/populate |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 66c55b682f..05bdfe33c5 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -18,6 +18,7 @@ _require_populate_commands() {
>  		;;
>  	ext*)
>  		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
> +		_require_command "$E2IMAGE_PROG" "e2image"
>  		;;
>  	esac
>  }
> @@ -874,7 +875,7 @@ _scratch_populate_restore_cached() {
>  		return $res
>  		;;
>  	"ext2"|"ext3"|"ext4")
> -		e2image -r "${metadump}" "${SCRATCH_DEV}"
> +		$E2IMAGE_PROG -r "${metadump}" "${SCRATCH_DEV}"
>  		ret=$?
>  		test $ret -ne 0 && return $ret
>  
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] populate: export the metadump description name
  2022-10-13 14:55   ` Zorro Lang
@ 2022-10-13 15:54     ` Darrick J. Wong
  2022-10-13 16:28       ` Zorro Lang
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-13 15:54 UTC (permalink / raw)
  To: Zorro Lang; +Cc: linux-xfs, fstests

On Thu, Oct 13, 2022 at 10:55:15PM +0800, Zorro Lang wrote:
> On Tue, Oct 11, 2022 at 06:45:27PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Not sure why this hasn't been broken all along, but we should be
> > exporting this variable so that it shows up in subshells....
> 
> May I ask where's the subshell which uses $POPULATE_METADUMP?

_scratch_xfs_fuzz_metadata does this:

	echo "${fields}" | while read field; do
		echo "${verbs}" | while read fuzzverb; do
			__scratch_xfs_fuzz_mdrestore
				_xfs_mdrestore "${POPULATE_METADUMP}"

The (nested) echo piped to while starts subshells.

> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |    6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index cfdaf766f0..b501c2fe45 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -868,9 +868,9 @@ _scratch_populate_cached() {
> >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> >  
> > -	# These variables are shared outside this function
> > -	POPULATE_METADUMP="${metadump_stem}.metadump"
> > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > +	# This variable is shared outside this function
> > +	export POPULATE_METADUMP="${metadump_stem}.metadump"
> > +	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> 
> If the POPULATE_METADUMP_DESCR is not shared outside anymore, how about change
> it to lower-case?

Ok.

--D

> >  
> >  	# Don't keep metadata images cached for more 48 hours...
> >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] fstests: refactor xfs_mdrestore calls
  2022-10-12  1:45 ` [PATCH 5/5] fstests: refactor xfs_mdrestore calls Darrick J. Wong
@ 2022-10-13 16:06   ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 16:06 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Tue, Oct 11, 2022 at 06:45:50PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Wrap calls to xfs_mdrestore in the usual $XFS_MDRESTORE_PROG variable.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Make sense,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/config   |    1 +
>  common/fuzzy    |    2 +-
>  common/populate |    3 ++-
>  tests/xfs/129   |    3 ++-
>  tests/xfs/234   |    3 ++-
>  tests/xfs/253   |    3 ++-
>  tests/xfs/284   |    3 ++-
>  tests/xfs/291   |    3 ++-
>  tests/xfs/336   |    3 ++-
>  tests/xfs/432   |    3 ++-
>  tests/xfs/503   |    9 +++++----
>  11 files changed, 23 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/common/config b/common/config
> index 5eaae4471d..0c3813dc23 100644
> --- a/common/config
> +++ b/common/config
> @@ -156,6 +156,7 @@ export XFS_LOGPRINT_PROG="$(type -P xfs_logprint)"
>  export XFS_REPAIR_PROG="$(type -P xfs_repair)"
>  export XFS_DB_PROG="$(type -P xfs_db)"
>  export XFS_METADUMP_PROG="$(type -P xfs_metadump)"
> +export XFS_MDRESTORE_PROG="$(type -P xfs_mdrestore)"
>  export XFS_ADMIN_PROG="$(type -P xfs_admin)"
>  export XFS_GROWFS_PROG=$(type -P xfs_growfs)
>  export XFS_SPACEMAN_PROG="$(type -P xfs_spaceman)"
> diff --git a/common/fuzzy b/common/fuzzy
> index b4fda6f533..2d688fd27b 100644
> --- a/common/fuzzy
> +++ b/common/fuzzy
> @@ -159,7 +159,7 @@ __scratch_xfs_fuzz_mdrestore()
>  	test -e "${POPULATE_METADUMP}" || _fail "Need to set POPULATE_METADUMP"
>  
>  	__scratch_xfs_fuzz_unmount
> -	xfs_mdrestore "${POPULATE_METADUMP}" "${SCRATCH_DEV}"
> +	$XFS_MDRESTORE_PROG "${POPULATE_METADUMP}" "${SCRATCH_DEV}"
>  }
>  
>  __fuzz_notify() {
> diff --git a/common/populate b/common/populate
> index 05bdfe33c5..0c0538554a 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -15,6 +15,7 @@ _require_populate_commands() {
>  	"xfs")
>  		_require_command "$XFS_DB_PROG" "xfs_db"
>  		_require_command "$WIPEFS_PROG" "wipefs"
> +		_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  		;;
>  	ext*)
>  		_require_command "$DUMPE2FS_PROG" "dumpe2fs"
> @@ -860,7 +861,7 @@ _scratch_populate_restore_cached() {
>  
>  	case "${FSTYP}" in
>  	"xfs")
> -		xfs_mdrestore "${metadump}" "${SCRATCH_DEV}"
> +		$XFS_MDRESTORE_PROG "${metadump}" "${SCRATCH_DEV}"
>  		res=$?
>  		test $res -ne 0 && return $res
>  
> diff --git a/tests/xfs/129 b/tests/xfs/129
> index 90887d5476..09d40630d0 100755
> --- a/tests/xfs/129
> +++ b/tests/xfs/129
> @@ -25,6 +25,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_loop
>  _require_scratch_reflink
>  
> @@ -52,7 +53,7 @@ _scratch_xfs_metadump $metadump_file
>  
>  # Now restore the obfuscated one back and take a look around
>  echo "Restore metadump"
> -xfs_mdrestore $metadump_file $TEST_DIR/image
> +$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
>  SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>  SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
>  
> diff --git a/tests/xfs/234 b/tests/xfs/234
> index 6ff2f228e8..cc1ee9a8ca 100755
> --- a/tests/xfs/234
> +++ b/tests/xfs/234
> @@ -24,6 +24,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_loop
>  _require_xfs_scratch_rmapbt
>  _require_xfs_io_command "fpunch"
> @@ -52,7 +53,7 @@ _scratch_xfs_metadump $metadump_file
>  
>  # Now restore the obfuscated one back and take a look around
>  echo "Restore metadump"
> -xfs_mdrestore $metadump_file $TEST_DIR/image
> +$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
>  SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>  SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
>  
> diff --git a/tests/xfs/253 b/tests/xfs/253
> index 1899ce5226..1cfc218088 100755
> --- a/tests/xfs/253
> +++ b/tests/xfs/253
> @@ -32,6 +32,7 @@ _cleanup()
>  # Import common functions.
>  . ./common/filter
>  
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_test
>  _require_scratch
>  
> @@ -151,7 +152,7 @@ _scratch_unmount
>  _scratch_xfs_metadump $METADUMP_FILE
>  
>  # Now restore the obfuscated one back and take a look around
> -xfs_mdrestore "${METADUMP_FILE}" "${SCRATCH_DEV}"
> +$XFS_MDRESTORE_PROG "${METADUMP_FILE}" "${SCRATCH_DEV}"
>  
>  _scratch_mount
>  
> diff --git a/tests/xfs/284 b/tests/xfs/284
> index fa7bfdd0f0..e2bd05d4c7 100755
> --- a/tests/xfs/284
> +++ b/tests/xfs/284
> @@ -24,6 +24,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_xfs_copy
>  _require_test
>  _require_scratch
> @@ -48,7 +49,7 @@ _scratch_unmount
>  # xfs_mdrestore should refuse to restore to a mounted device
>  _scratch_xfs_metadump $METADUMP_FILE
>  _scratch_mount
> -xfs_mdrestore $METADUMP_FILE $SCRATCH_DEV 2>&1 | filter_mounted
> +$XFS_MDRESTORE_PROG $METADUMP_FILE $SCRATCH_DEV 2>&1 | filter_mounted
>  _scratch_unmount
>  
>  # Test xfs_copy to a mounted device
> diff --git a/tests/xfs/291 b/tests/xfs/291
> index a2425e472d..f5fea7f9a5 100755
> --- a/tests/xfs/291
> +++ b/tests/xfs/291
> @@ -13,6 +13,7 @@ _begin_fstest auto repair metadump
>  . ./common/filter
>  
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  
>  # real QA test starts here
>  _require_scratch
> @@ -92,7 +93,7 @@ _scratch_xfs_check >> $seqres.full 2>&1 || _fail "xfs_check failed"
>  # Yes they can!  Now...
>  # Can xfs_metadump cope with this monster?
>  _scratch_xfs_metadump $tmp.metadump || _fail "xfs_metadump failed"
> -xfs_mdrestore $tmp.metadump $tmp.img || _fail "xfs_mdrestore failed"
> +$XFS_MDRESTORE_PROG $tmp.metadump $tmp.img || _fail "xfs_mdrestore failed"
>  SCRATCH_DEV=$tmp.img _scratch_xfs_repair -f &>> $seqres.full || \
>  	_fail "xfs_repair of metadump failed"
>  
> diff --git a/tests/xfs/336 b/tests/xfs/336
> index 6592419d03..b1de8e5fc1 100755
> --- a/tests/xfs/336
> +++ b/tests/xfs/336
> @@ -21,6 +21,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_realtime
>  _require_xfs_scratch_rmapbt
>  _require_test_program "punch-alternating"
> @@ -64,7 +65,7 @@ _scratch_xfs_metadump $metadump_file
>  
>  # Now restore the obfuscated one back and take a look around
>  echo "Restore metadump"
> -xfs_mdrestore $metadump_file $TEST_DIR/image
> +$XFS_MDRESTORE_PROG $metadump_file $TEST_DIR/image
>  SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>  SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
>  
> diff --git a/tests/xfs/432 b/tests/xfs/432
> index e1e610d054..676be9bd8a 100755
> --- a/tests/xfs/432
> +++ b/tests/xfs/432
> @@ -28,6 +28,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs xfs
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_scratch
>  
>  rm -f "$seqres.full"
> @@ -86,7 +87,7 @@ test -n "$extlen" || _notrun "could not create dir extent > 1000 blocks"
>  
>  echo "Try to metadump"
>  _scratch_xfs_metadump $metadump_file -w
> -xfs_mdrestore $metadump_file $metadump_img
> +$XFS_MDRESTORE_PROG $metadump_file $metadump_img
>  
>  echo "Check restored metadump image"
>  SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
> diff --git a/tests/xfs/503 b/tests/xfs/503
> index 6c4bfd1c45..18bd8694c8 100755
> --- a/tests/xfs/503
> +++ b/tests/xfs/503
> @@ -28,6 +28,7 @@ testdir=$TEST_DIR/test-$seq
>  # real QA test starts here
>  _supported_fs xfs
>  
> +_require_command "$XFS_MDRESTORE_PROG" "xfs_mdrestore"
>  _require_xfs_copy
>  _require_scratch_nocheck
>  _require_populate_commands
> @@ -65,25 +66,25 @@ _check_scratch_fs
>  _scratch_unmount
>  
>  echo mdrestore
> -xfs_mdrestore $metadump_file $SCRATCH_DEV
> +$XFS_MDRESTORE_PROG $metadump_file $SCRATCH_DEV
>  _scratch_mount
>  _check_scratch_fs
>  _scratch_unmount
>  
>  echo mdrestore a
> -xfs_mdrestore $metadump_file_a $SCRATCH_DEV
> +$XFS_MDRESTORE_PROG $metadump_file_a $SCRATCH_DEV
>  _scratch_mount
>  _check_scratch_fs
>  _scratch_unmount
>  
>  echo mdrestore g
> -xfs_mdrestore $metadump_file_g $SCRATCH_DEV
> +$XFS_MDRESTORE_PROG $metadump_file_g $SCRATCH_DEV
>  _scratch_mount
>  _check_scratch_fs
>  _scratch_unmount
>  
>  echo mdrestore ag
> -xfs_mdrestore $metadump_file_ag $SCRATCH_DEV
> +$XFS_MDRESTORE_PROG $metadump_file_ag $SCRATCH_DEV
>  _scratch_mount
>  _check_scratch_fs
>  _scratch_unmount
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] populate: export the metadump description name
  2022-10-13 15:54     ` Darrick J. Wong
@ 2022-10-13 16:28       ` Zorro Lang
  2022-10-13 19:12         ` Darrick J. Wong
  0 siblings, 1 reply; 21+ messages in thread
From: Zorro Lang @ 2022-10-13 16:28 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Thu, Oct 13, 2022 at 08:54:35AM -0700, Darrick J. Wong wrote:
> On Thu, Oct 13, 2022 at 10:55:15PM +0800, Zorro Lang wrote:
> > On Tue, Oct 11, 2022 at 06:45:27PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Not sure why this hasn't been broken all along, but we should be
> > > exporting this variable so that it shows up in subshells....
> > 
> > May I ask where's the subshell which uses $POPULATE_METADUMP?
> 
> _scratch_xfs_fuzz_metadata does this:
> 
> 	echo "${fields}" | while read field; do
> 		echo "${verbs}" | while read fuzzverb; do
> 			__scratch_xfs_fuzz_mdrestore
> 				_xfs_mdrestore "${POPULATE_METADUMP}"
> 
> The (nested) echo piped to while starts subshells.

I'm not so familar with this part, so I didn't a simple test[1], and looks like
the PARAM can be seen, even it's not exported. Do I misunderstand something?

Thanks,
Zorro

[1]
$ echo "$list"
a
b
cc
$ PARAM="This's a test"
$ echo "$list"|while read c1;do echo "$list"|while read c2;do echo $PARAM;done; done
This's a test
This's a test
This's a test
This's a test
This's a test
This's a test
This's a test
This's a test
This's a test

> 
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  common/populate |    6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > diff --git a/common/populate b/common/populate
> > > index cfdaf766f0..b501c2fe45 100644
> > > --- a/common/populate
> > > +++ b/common/populate
> > > @@ -868,9 +868,9 @@ _scratch_populate_cached() {
> > >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> > >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> > >  
> > > -	# These variables are shared outside this function
> > > -	POPULATE_METADUMP="${metadump_stem}.metadump"
> > > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > +	# This variable is shared outside this function
> > > +	export POPULATE_METADUMP="${metadump_stem}.metadump"
> > > +	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > 
> > If the POPULATE_METADUMP_DESCR is not shared outside anymore, how about change
> > it to lower-case?
> 
> Ok.
> 
> --D
> 
> > >  
> > >  	# Don't keep metadata images cached for more 48 hours...
> > >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > > 
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] populate: export the metadump description name
  2022-10-13 16:28       ` Zorro Lang
@ 2022-10-13 19:12         ` Darrick J. Wong
  2022-10-14  1:17           ` Zorro Lang
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-13 19:12 UTC (permalink / raw)
  To: Zorro Lang; +Cc: linux-xfs, fstests

On Fri, Oct 14, 2022 at 12:28:26AM +0800, Zorro Lang wrote:
> On Thu, Oct 13, 2022 at 08:54:35AM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 13, 2022 at 10:55:15PM +0800, Zorro Lang wrote:
> > > On Tue, Oct 11, 2022 at 06:45:27PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Not sure why this hasn't been broken all along, but we should be
> > > > exporting this variable so that it shows up in subshells....
> > > 
> > > May I ask where's the subshell which uses $POPULATE_METADUMP?
> > 
> > _scratch_xfs_fuzz_metadata does this:
> > 
> > 	echo "${fields}" | while read field; do
> > 		echo "${verbs}" | while read fuzzverb; do
> > 			__scratch_xfs_fuzz_mdrestore
> > 				_xfs_mdrestore "${POPULATE_METADUMP}"
> > 
> > The (nested) echo piped to while starts subshells.
> 
> I'm not so familar with this part, so I didn't a simple test[1], and looks like
> the PARAM can be seen, even it's not exported. Do I misunderstand something?
> 
> Thanks,
> Zorro
> 
> [1]
> $ echo "$list"
> a
> b
> cc
> $ PARAM="This's a test"
> $ echo "$list"|while read c1;do echo "$list"|while read c2;do echo $PARAM;done; done
> This's a test
> This's a test
> This's a test
> This's a test
> This's a test
> This's a test
> This's a test
> This's a test
> This's a test

Hmm.  I can't figure out why I needed the export here.  It was late one
night, something was broken, and exporting the variable made it work.
Now I can't recall exactly what that was and it seems fine without
it...?

I guess I'll put it back and rerun the entire fuzz suite to see what
pops out...

--D

> > 
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  common/populate |    6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/common/populate b/common/populate
> > > > index cfdaf766f0..b501c2fe45 100644
> > > > --- a/common/populate
> > > > +++ b/common/populate
> > > > @@ -868,9 +868,9 @@ _scratch_populate_cached() {
> > > >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> > > >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> > > >  
> > > > -	# These variables are shared outside this function
> > > > -	POPULATE_METADUMP="${metadump_stem}.metadump"
> > > > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > > +	# This variable is shared outside this function
> > > > +	export POPULATE_METADUMP="${metadump_stem}.metadump"
> > > > +	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > 
> > > If the POPULATE_METADUMP_DESCR is not shared outside anymore, how about change
> > > it to lower-case?
> > 
> > Ok.
> > 
> > --D
> > 
> > > >  
> > > >  	# Don't keep metadata images cached for more 48 hours...
> > > >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > > > 
> > > 
> > 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] populate: export the metadump description name
  2022-10-13 19:12         ` Darrick J. Wong
@ 2022-10-14  1:17           ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-14  1:17 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Thu, Oct 13, 2022 at 12:12:19PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 14, 2022 at 12:28:26AM +0800, Zorro Lang wrote:
> > On Thu, Oct 13, 2022 at 08:54:35AM -0700, Darrick J. Wong wrote:
> > > On Thu, Oct 13, 2022 at 10:55:15PM +0800, Zorro Lang wrote:
> > > > On Tue, Oct 11, 2022 at 06:45:27PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > Not sure why this hasn't been broken all along, but we should be
> > > > > exporting this variable so that it shows up in subshells....
> > > > 
> > > > May I ask where's the subshell which uses $POPULATE_METADUMP?
> > > 
> > > _scratch_xfs_fuzz_metadata does this:
> > > 
> > > 	echo "${fields}" | while read field; do
> > > 		echo "${verbs}" | while read fuzzverb; do
> > > 			__scratch_xfs_fuzz_mdrestore
> > > 				_xfs_mdrestore "${POPULATE_METADUMP}"
> > > 
> > > The (nested) echo piped to while starts subshells.
> > 
> > I'm not so familar with this part, so I didn't a simple test[1], and looks like
> > the PARAM can be seen, even it's not exported. Do I misunderstand something?
> > 
> > Thanks,
> > Zorro
> > 
> > [1]
> > $ echo "$list"
> > a
> > b
> > cc
> > $ PARAM="This's a test"
> > $ echo "$list"|while read c1;do echo "$list"|while read c2;do echo $PARAM;done; done
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> > This's a test
> 
> Hmm.  I can't figure out why I needed the export here.  It was late one
> night, something was broken, and exporting the variable made it work.
> Now I can't recall exactly what that was and it seems fine without
> it...?
> 
> I guess I'll put it back and rerun the entire fuzz suite to see what
> pops out...

Sure, I don't have objection on this patch, so you can have the RVB when
you change "local POPULATE_METADUMP_DESCR" to lower-case, and show at least
one example about why POPULATE_METADUMP need to be exported in next version.
No push:)

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

> 
> --D
> 
> > > 
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  common/populate |    6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > 
> > > > > diff --git a/common/populate b/common/populate
> > > > > index cfdaf766f0..b501c2fe45 100644
> > > > > --- a/common/populate
> > > > > +++ b/common/populate
> > > > > @@ -868,9 +868,9 @@ _scratch_populate_cached() {
> > > > >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> > > > >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> > > > >  
> > > > > -	# These variables are shared outside this function
> > > > > -	POPULATE_METADUMP="${metadump_stem}.metadump"
> > > > > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > > > +	# This variable is shared outside this function
> > > > > +	export POPULATE_METADUMP="${metadump_stem}.metadump"
> > > > > +	local POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > > 
> > > > If the POPULATE_METADUMP_DESCR is not shared outside anymore, how about change
> > > > it to lower-case?
> > > 
> > > Ok.
> > > 
> > > --D
> > > 
> > > > >  
> > > > >  	# Don't keep metadata images cached for more 48 hours...
> > > > >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > > > > 
> > > > 
> > > 
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
  2022-10-13 14:55   ` Zorro Lang
@ 2022-10-14 18:21   ` Darrick J. Wong
  2022-10-15  5:01     ` Zorro Lang
  1 sibling, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-14 18:21 UTC (permalink / raw)
  To: guaneryu, zlang; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Make the variable that holds the contents of the metadump description
file a local variable since we don't need it outside of that function.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
v1.1: dont export POPULATE_METADUMP; change the description a bit
---
 common/populate |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/populate b/common/populate
index cfdaf766f0..ba34ca5844 100644
--- a/common/populate
+++ b/common/populate
@@ -868,15 +868,15 @@ _scratch_populate_cached() {
 	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
 	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
 
-	# These variables are shared outside this function
+	# This variable is shared outside this function
 	POPULATE_METADUMP="${metadump_stem}.metadump"
-	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
+	local populate_metadump_descr="${metadump_stem}.txt"
 
 	# Don't keep metadata images cached for more 48 hours...
 	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
 
 	# Throw away cached image if it doesn't match our spec.
-	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
+	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
 		rm -rf "${POPULATE_METADUMP}"
 
 	# Try to restore from the metadump
@@ -885,7 +885,7 @@ _scratch_populate_cached() {
 
 	# Oh well, just create one from scratch
 	_scratch_mkfs
-	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
+	echo "${meta_descr}" > "${populate_metadump_descr}"
 	case "${FSTYP}" in
 	"xfs")
 		_scratch_xfs_populate $@

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-14 18:21   ` [PATCH v1.1 " Darrick J. Wong
@ 2022-10-15  5:01     ` Zorro Lang
  2022-10-15  5:10       ` Zorro Lang
  2022-10-15  7:23       ` Darrick J. Wong
  0 siblings, 2 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-15  5:01 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, linux-xfs, fstests

On Fri, Oct 14, 2022 at 11:21:55AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make the variable that holds the contents of the metadump description
> file a local variable since we don't need it outside of that function.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> ---
> v1.1: dont export POPULATE_METADUMP; change the description a bit
> ---

So you don't need to export the POPULATE_METADUMP anymore? I remembered you
said something broken and "exporting the variable made it work". Before I
merge this patch, hope to double check with you.

Thanks,
Zorro

>  common/populate |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/populate b/common/populate
> index cfdaf766f0..ba34ca5844 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -868,15 +868,15 @@ _scratch_populate_cached() {
>  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
>  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
>  
> -	# These variables are shared outside this function
> +	# This variable is shared outside this function
>  	POPULATE_METADUMP="${metadump_stem}.metadump"
> -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> +	local populate_metadump_descr="${metadump_stem}.txt"
>  
>  	# Don't keep metadata images cached for more 48 hours...
>  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
>  
>  	# Throw away cached image if it doesn't match our spec.
> -	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
> +	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
>  		rm -rf "${POPULATE_METADUMP}"
>  
>  	# Try to restore from the metadump
> @@ -885,7 +885,7 @@ _scratch_populate_cached() {
>  
>  	# Oh well, just create one from scratch
>  	_scratch_mkfs
> -	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
> +	echo "${meta_descr}" > "${populate_metadump_descr}"
>  	case "${FSTYP}" in
>  	"xfs")
>  		_scratch_xfs_populate $@
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-15  5:01     ` Zorro Lang
@ 2022-10-15  5:10       ` Zorro Lang
  2022-10-15  7:28         ` Darrick J. Wong
  2022-10-15  7:23       ` Darrick J. Wong
  1 sibling, 1 reply; 21+ messages in thread
From: Zorro Lang @ 2022-10-15  5:10 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Sat, Oct 15, 2022 at 01:01:44PM +0800, Zorro Lang wrote:
> On Fri, Oct 14, 2022 at 11:21:55AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make the variable that holds the contents of the metadump description
> > file a local variable since we don't need it outside of that function.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > ---
> > v1.1: dont export POPULATE_METADUMP; change the description a bit
> > ---
> 
> So you don't need to export the POPULATE_METADUMP anymore? I remembered you
> said something broken and "exporting the variable made it work". Before I
> merge this patch, hope to double check with you.

And the subject is still "populate: export the metadump description name", I
think it's not suit for this change. Anyway, as this patch is not depended by
others, I can merge other 4 patches at first. To give you enough time to think
about what do you really like to change :)

Thanks,
Zorro

> 
> Thanks,
> Zorro
> 
> >  common/populate |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/populate b/common/populate
> > index cfdaf766f0..ba34ca5844 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -868,15 +868,15 @@ _scratch_populate_cached() {
> >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> >  
> > -	# These variables are shared outside this function
> > +	# This variable is shared outside this function
> >  	POPULATE_METADUMP="${metadump_stem}.metadump"
> > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > +	local populate_metadump_descr="${metadump_stem}.txt"
> >  
> >  	# Don't keep metadata images cached for more 48 hours...
> >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> >  
> >  	# Throw away cached image if it doesn't match our spec.
> > -	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
> > +	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
> >  		rm -rf "${POPULATE_METADUMP}"
> >  
> >  	# Try to restore from the metadump
> > @@ -885,7 +885,7 @@ _scratch_populate_cached() {
> >  
> >  	# Oh well, just create one from scratch
> >  	_scratch_mkfs
> > -	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
> > +	echo "${meta_descr}" > "${populate_metadump_descr}"
> >  	case "${FSTYP}" in
> >  	"xfs")
> >  		_scratch_xfs_populate $@
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-15  5:01     ` Zorro Lang
  2022-10-15  5:10       ` Zorro Lang
@ 2022-10-15  7:23       ` Darrick J. Wong
  1 sibling, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-15  7:23 UTC (permalink / raw)
  To: Zorro Lang; +Cc: zlang, linux-xfs, fstests

On Sat, Oct 15, 2022 at 01:01:44PM +0800, Zorro Lang wrote:
> On Fri, Oct 14, 2022 at 11:21:55AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make the variable that holds the contents of the metadump description
> > file a local variable since we don't need it outside of that function.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > ---
> > v1.1: dont export POPULATE_METADUMP; change the description a bit
> > ---
> 
> So you don't need to export the POPULATE_METADUMP anymore? I remembered you
> said something broken and "exporting the variable made it work". Before I
> merge this patch, hope to double check with you.

Yeah, I can't figure out why I ever decided that it had to be exported.
I probably did it in desperation late one night because I have too many
things on my plate and bash scripts suck. :/

IOWs it was a classic "BILD DAMMIT".  But we could lowercase the shoutly
local var :)

--D

> Thanks,
> Zorro
> 
> >  common/populate |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/populate b/common/populate
> > index cfdaf766f0..ba34ca5844 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -868,15 +868,15 @@ _scratch_populate_cached() {
> >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> >  
> > -	# These variables are shared outside this function
> > +	# This variable is shared outside this function
> >  	POPULATE_METADUMP="${metadump_stem}.metadump"
> > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > +	local populate_metadump_descr="${metadump_stem}.txt"
> >  
> >  	# Don't keep metadata images cached for more 48 hours...
> >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> >  
> >  	# Throw away cached image if it doesn't match our spec.
> > -	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
> > +	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
> >  		rm -rf "${POPULATE_METADUMP}"
> >  
> >  	# Try to restore from the metadump
> > @@ -885,7 +885,7 @@ _scratch_populate_cached() {
> >  
> >  	# Oh well, just create one from scratch
> >  	_scratch_mkfs
> > -	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
> > +	echo "${meta_descr}" > "${populate_metadump_descr}"
> >  	case "${FSTYP}" in
> >  	"xfs")
> >  		_scratch_xfs_populate $@
> > 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-15  5:10       ` Zorro Lang
@ 2022-10-15  7:28         ` Darrick J. Wong
  2022-10-15  8:21           ` Zorro Lang
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2022-10-15  7:28 UTC (permalink / raw)
  To: Zorro Lang; +Cc: linux-xfs, fstests

On Sat, Oct 15, 2022 at 01:10:09PM +0800, Zorro Lang wrote:
> On Sat, Oct 15, 2022 at 01:01:44PM +0800, Zorro Lang wrote:
> > On Fri, Oct 14, 2022 at 11:21:55AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Make the variable that holds the contents of the metadump description
> > > file a local variable since we don't need it outside of that function.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > ---
> > > v1.1: dont export POPULATE_METADUMP; change the description a bit
> > > ---
> > 
> > So you don't need to export the POPULATE_METADUMP anymore? I remembered you
> > said something broken and "exporting the variable made it work". Before I
> > merge this patch, hope to double check with you.
> 
> And the subject is still "populate: export the metadump description name", I
> think it's not suit for this change. Anyway, as this patch is not depended by
> others, I can merge other 4 patches at first. To give you enough time to think
> about what do you really like to change :)

<sigh> I updated the commit message in git and forgot to copy-paste the
new subject into the email.  Sorry about all this stupid crap, bash is a
terrible language and email is a godawful process and both should be
smote off the planet.

--D

> Thanks,
> Zorro
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  common/populate |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/common/populate b/common/populate
> > > index cfdaf766f0..ba34ca5844 100644
> > > --- a/common/populate
> > > +++ b/common/populate
> > > @@ -868,15 +868,15 @@ _scratch_populate_cached() {
> > >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> > >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> > >  
> > > -	# These variables are shared outside this function
> > > +	# This variable is shared outside this function
> > >  	POPULATE_METADUMP="${metadump_stem}.metadump"
> > > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > +	local populate_metadump_descr="${metadump_stem}.txt"
> > >  
> > >  	# Don't keep metadata images cached for more 48 hours...
> > >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > >  
> > >  	# Throw away cached image if it doesn't match our spec.
> > > -	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
> > > +	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
> > >  		rm -rf "${POPULATE_METADUMP}"
> > >  
> > >  	# Try to restore from the metadump
> > > @@ -885,7 +885,7 @@ _scratch_populate_cached() {
> > >  
> > >  	# Oh well, just create one from scratch
> > >  	_scratch_mkfs
> > > -	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
> > > +	echo "${meta_descr}" > "${populate_metadump_descr}"
> > >  	case "${FSTYP}" in
> > >  	"xfs")
> > >  		_scratch_xfs_populate $@
> > > 
> > 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH v1.1 1/5] populate: export the metadump description name
  2022-10-15  7:28         ` Darrick J. Wong
@ 2022-10-15  8:21           ` Zorro Lang
  0 siblings, 0 replies; 21+ messages in thread
From: Zorro Lang @ 2022-10-15  8:21 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Sat, Oct 15, 2022 at 12:28:09AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 15, 2022 at 01:10:09PM +0800, Zorro Lang wrote:
> > On Sat, Oct 15, 2022 at 01:01:44PM +0800, Zorro Lang wrote:
> > > On Fri, Oct 14, 2022 at 11:21:55AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Make the variable that holds the contents of the metadump description
> > > > file a local variable since we don't need it outside of that function.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > > ---
> > > > v1.1: dont export POPULATE_METADUMP; change the description a bit
> > > > ---
> > > 
> > > So you don't need to export the POPULATE_METADUMP anymore? I remembered you
> > > said something broken and "exporting the variable made it work". Before I
> > > merge this patch, hope to double check with you.
> > 
> > And the subject is still "populate: export the metadump description name", I
> > think it's not suit for this change. Anyway, as this patch is not depended by
> > others, I can merge other 4 patches at first. To give you enough time to think
> > about what do you really like to change :)
> 
> <sigh> I updated the commit message in git and forgot to copy-paste the
> new subject into the email.  Sorry about all this stupid crap, bash is a
> terrible language and email is a godawful process and both should be
> smote off the planet.

I can help to change this subject if you're sure this change is fine for you.

BTW, if the POPULATE_METADUMP isn't affect your test anymore, I think we're
not hurry to have this patch which only make a variable to be local. How about
I merge other 4 patches at first, then you can have more time to watch your
later testing results, and think about if you need more changes?

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > >  common/populate |    8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/common/populate b/common/populate
> > > > index cfdaf766f0..ba34ca5844 100644
> > > > --- a/common/populate
> > > > +++ b/common/populate
> > > > @@ -868,15 +868,15 @@ _scratch_populate_cached() {
> > > >  	local meta_tag="$(echo "${meta_descr}" | md5sum - | cut -d ' ' -f 1)"
> > > >  	local metadump_stem="${TEST_DIR}/__populate.${FSTYP}.${meta_tag}"
> > > >  
> > > > -	# These variables are shared outside this function
> > > > +	# This variable is shared outside this function
> > > >  	POPULATE_METADUMP="${metadump_stem}.metadump"
> > > > -	POPULATE_METADUMP_DESCR="${metadump_stem}.txt"
> > > > +	local populate_metadump_descr="${metadump_stem}.txt"
> > > >  
> > > >  	# Don't keep metadata images cached for more 48 hours...
> > > >  	rm -rf "$(find "${POPULATE_METADUMP}" -mtime +2 2>/dev/null)"
> > > >  
> > > >  	# Throw away cached image if it doesn't match our spec.
> > > > -	cmp -s "${POPULATE_METADUMP_DESCR}" <(echo "${meta_descr}") || \
> > > > +	cmp -s "${populate_metadump_descr}" <(echo "${meta_descr}") || \
> > > >  		rm -rf "${POPULATE_METADUMP}"
> > > >  
> > > >  	# Try to restore from the metadump
> > > > @@ -885,7 +885,7 @@ _scratch_populate_cached() {
> > > >  
> > > >  	# Oh well, just create one from scratch
> > > >  	_scratch_mkfs
> > > > -	echo "${meta_descr}" > "${POPULATE_METADUMP_DESCR}"
> > > > +	echo "${meta_descr}" > "${populate_metadump_descr}"
> > > >  	case "${FSTYP}" in
> > > >  	"xfs")
> > > >  		_scratch_xfs_populate $@
> > > > 
> > > 
> > 
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-10-15  8:22 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-12  1:45 [PATCHSET 0/5] fstests: support external logs when caching prepopulated images Darrick J. Wong
2022-10-12  1:45 ` [PATCH 1/5] populate: export the metadump description name Darrick J. Wong
2022-10-13 14:55   ` Zorro Lang
2022-10-13 15:54     ` Darrick J. Wong
2022-10-13 16:28       ` Zorro Lang
2022-10-13 19:12         ` Darrick J. Wong
2022-10-14  1:17           ` Zorro Lang
2022-10-14 18:21   ` [PATCH v1.1 " Darrick J. Wong
2022-10-15  5:01     ` Zorro Lang
2022-10-15  5:10       ` Zorro Lang
2022-10-15  7:28         ` Darrick J. Wong
2022-10-15  8:21           ` Zorro Lang
2022-10-15  7:23       ` Darrick J. Wong
2022-10-12  1:45 ` [PATCH 2/5] populate: wipe external xfs log devices when restoring a cached image Darrick J. Wong
2022-10-13 15:10   ` Zorro Lang
2022-10-12  1:45 ` [PATCH 3/5] populate: reformat external ext[34] journal " Darrick J. Wong
2022-10-13 15:47   ` Zorro Lang
2022-10-12  1:45 ` [PATCH 4/5] populate: require e2image before populating Darrick J. Wong
2022-10-13 15:48   ` Zorro Lang
2022-10-12  1:45 ` [PATCH 5/5] fstests: refactor xfs_mdrestore calls Darrick J. Wong
2022-10-13 16:06   ` Zorro Lang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).