[PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation
@ 2022-10-18 22:45 Darrick J. Wong
  2022-10-18 22:45 ` [PATCH 1/3] xfs: refactor filesystem feature detection logic Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-18 22:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

Hi all,

There are numerous tests that do things based on the geometry of a
mounted filesystem.  Before we start adding more tests that do this
(e.g. online fsck stress tests), refactor them into common/xfs helpers.

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=refactor-xfs-geometry
---
 common/populate |   20 +++++------
 common/rc       |    9 ++++-
 common/xfs      |  103 +++++++++++++++++++++++++++++++++++++++++++++++++------
 tests/xfs/097   |    2 +
 tests/xfs/099   |    2 +
 tests/xfs/100   |    2 +
 tests/xfs/101   |    2 +
 tests/xfs/102   |    2 +
 tests/xfs/105   |    2 +
 tests/xfs/112   |    2 +
 tests/xfs/113   |    2 +
 tests/xfs/146   |    2 +
 tests/xfs/147   |    2 +
 tests/xfs/151   |    3 +-
 tests/xfs/271   |    2 +
 tests/xfs/307   |    2 +
 tests/xfs/308   |    2 +
 tests/xfs/348   |    2 +
 tests/xfs/530   |    3 +-
 19 files changed, 125 insertions(+), 41 deletions(-)


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] xfs: refactor filesystem feature detection logic
  2022-10-18 22:45 [PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation Darrick J. Wong
@ 2022-10-18 22:45 ` Darrick J. Wong
  2022-10-20 12:26   ` Zorro Lang
  2022-10-18 22:45 ` [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic Darrick J. Wong
  2022-10-18 22:45 ` [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic Darrick J. Wong
  2 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-18 22:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

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

There are a lot of places where we open-code detecting features of a
specific filesystem.  Refactor this into a couple of helpers in
preparation for adding stress tests for online repair and fuzzing.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |   14 ++++++-----
 common/rc       |    9 +++++++
 common/xfs      |   67 ++++++++++++++++++++++++++++++++++++++++++++++---------
 tests/xfs/097   |    2 +-
 tests/xfs/151   |    3 +-
 tests/xfs/271   |    2 +-
 tests/xfs/307   |    2 +-
 tests/xfs/308   |    2 +-
 tests/xfs/348   |    2 +-
 9 files changed, 77 insertions(+), 26 deletions(-)


diff --git a/common/populate b/common/populate
index 58b07e33be..9fa1a06798 100644
--- a/common/populate
+++ b/common/populate
@@ -131,7 +131,7 @@ _populate_xfs_qmount_option()
 	fi
 
 	# Turn on all the quotas
-	if $XFS_INFO_PROG "${TEST_DIR}" | grep -q 'crc=1'; then
+	if _xfs_has_feature "$TEST_DIR" crc; then
 		# v5 filesystems can have group & project quotas
 		quota="usrquota,grpquota,prjquota"
 	else
@@ -176,7 +176,7 @@ _scratch_xfs_populate() {
 
 	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
-	crc="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep crc= | sed -e 's/^.*crc=//g' -e 's/\([0-9]*\).*$/\1/g')"
+	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
 	if [ $crc -eq 1 ]; then
 		leaf_hdr_size=64
 	else
@@ -315,7 +315,7 @@ _scratch_xfs_populate() {
 	done
 
 	# Reverse-mapping btree
-	is_rmapbt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')"
+	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
 	if [ $is_rmapbt -gt 0 ]; then
 		echo "+ rmapbt btree"
 		nr="$((blksz * 2 / 24))"
@@ -332,7 +332,7 @@ _scratch_xfs_populate() {
 	fi
 
 	# Reference-count btree
-	is_reflink="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')"
+	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
 	if [ $is_reflink -gt 0 ]; then
 		echo "+ reflink btree"
 		nr="$((blksz * 2 / 12))"
@@ -597,9 +597,9 @@ _scratch_xfs_populate_check() {
 	leaf_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_LEAF")"
 	node_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_NODE")"
 	btree_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_BTREE")"
-	is_finobt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'finobt=1')
-	is_rmapbt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')
-	is_reflink=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')
+	is_finobt=$(_xfs_has_feature "$SCRATCH_MNT" finobt -v)
+	is_rmapbt=$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)
+	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
 
 	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
diff --git a/common/rc b/common/rc
index f4785c17ca..7944ef550e 100644
--- a/common/rc
+++ b/common/rc
@@ -247,7 +247,14 @@ _supports_filetype()
 	local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
 	case "$fstyp" in
 	xfs)
-		$XFS_INFO_PROG $dir | grep -q "ftype=1"
+		# If _xfs_has_feature is not present, grep xfs_info directly.
+		# This can happen if we're testing overlayfs atop XFS and have
+		# not sourced ./common/xfs.
+		if command -v "_xfs_has_feature" &>/dev/null; then
+			_xfs_has_feature $dir ftype
+		else
+			$XFS_INFO_PROG $dir | grep -q "ftype=1"
+		fi
 		;;
 	ext2|ext3|ext4)
 		local dev=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $1}'`
diff --git a/common/xfs b/common/xfs
index 2cd8254937..c7496bce3f 100644
--- a/common/xfs
+++ b/common/xfs
@@ -422,6 +422,56 @@ _require_xfs_crc()
 	_scratch_unmount
 }
 
+# If the xfs_info output for the given XFS filesystem mount mentions the given
+# feature.  If so, return 0 for success.  If not, return 1 for failure.  If the
+# third option is -v, echo 1 for success and 0 for not.
+#
+# Starting with xfsprogs 4.17, this also works for unmounted filesystems.
+_xfs_has_feature()
+{
+	local fs="$1"
+	local feat="$2"
+	local verbose="$3"
+
+	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
+	if [ "$answer" -ne 0 ]; then
+		test "$verbose" = "-v" && echo 1
+		return 0
+	fi
+
+	test "$verbose" = "-v" && echo 0
+	return 1
+}
+
+# Require that the xfs_info output for the given XFS filesystem mount mentions
+# the given feature flag.  If the third argument is -u (or is empty and the
+# second argument is $SCRATCH_MNT), unmount the fs on failure.  If a fourth
+# argument is supplied, it will be used as the _notrun message.
+_require_xfs_has_feature()
+{
+	local fs="$1"
+	local feat="$2"
+	local umount="$3"
+	local message="$4"
+
+	if [ -z "$umount" ] && [ "$fs" = "$SCRATCH_MNT" ]; then
+		umount="-u"
+	fi
+
+	_xfs_has_feature "$1" "$2" && return 0
+
+	test "$umount" = "-u" && umount "$fs" &>/dev/null
+
+	test -n "$message" && _notrun "$message"
+
+	case "$fs" in
+	"$TEST_DIR"|"$TEST_DEV")	fsname="test";;
+	"$SCRATCH_MNT"|"$SCRATCH_DEV")	fsname="scratch";;
+	*)				fsname="$fs";;
+	esac
+	_notrun "$2 not supported by $fsname filesystem type: $FSTYP"
+}
+
 # this test requires the xfs kernel support crc feature on scratch device
 #
 _require_scratch_xfs_crc()
@@ -429,7 +479,8 @@ _require_scratch_xfs_crc()
 	_scratch_mkfs_xfs >/dev/null 2>&1
 	_try_scratch_mount >/dev/null 2>&1 \
 	   || _notrun "Kernel doesn't support crc feature"
-	$XFS_INFO_PROG $SCRATCH_MNT | grep -q 'crc=1' || _notrun "crc feature not supported by this filesystem"
+	_require_xfs_has_feature $SCRATCH_MNT crc -u \
+		"crc feature not supported by this filesystem"
 	_scratch_unmount
 }
 
@@ -748,10 +799,7 @@ _check_xfs_test_fs()
 _require_xfs_test_rmapbt()
 {
 	_require_test
-
-	if [ "$($XFS_INFO_PROG "$TEST_DIR" | grep -c "rmapbt=1")" -ne 1 ]; then
-		_notrun "rmapbt not supported by test filesystem type: $FSTYP"
-	fi
+	_require_xfs_has_feature "$TEST_DIR" rmapbt
 }
 
 _require_xfs_scratch_rmapbt()
@@ -760,10 +808,7 @@ _require_xfs_scratch_rmapbt()
 
 	_scratch_mkfs > /dev/null
 	_scratch_mount
-	if [ "$($XFS_INFO_PROG "$SCRATCH_MNT" | grep -c "rmapbt=1")" -ne 1 ]; then
-		_scratch_unmount
-		_notrun "rmapbt not supported by scratch filesystem type: $FSTYP"
-	fi
+	_require_xfs_has_feature "$SCRATCH_MNT" rmapbt
 	_scratch_unmount
 }
 
@@ -1366,8 +1411,8 @@ _require_scratch_xfs_bigtime()
 		_notrun "mkfs.xfs doesn't support bigtime feature"
 	_try_scratch_mount || \
 		_notrun "kernel doesn't support xfs bigtime feature"
-	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "bigtime=1" || \
-		_notrun "bigtime feature not advertised on mount?"
+	_require_xfs_has_feature $SCRATCH_MNT bigtime -u \
+		"crc feature not supported by this filesystem"
 	_scratch_unmount
 }
 
diff --git a/tests/xfs/097 b/tests/xfs/097
index 4cad7216cd..1df34eeddc 100755
--- a/tests/xfs/097
+++ b/tests/xfs/097
@@ -42,7 +42,7 @@ _scratch_mkfs_xfs -m crc=1,finobt=1 > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-$XFS_INFO_PROG "${SCRATCH_MNT}" | grep -q "finobt=1" || _notrun "finobt not enabled"
+_require_xfs_has_feature "$SCRATCH_MNT" finobt
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 
 echo "+ make some files"
diff --git a/tests/xfs/151 b/tests/xfs/151
index 66425f6710..b2fe16aefb 100755
--- a/tests/xfs/151
+++ b/tests/xfs/151
@@ -24,8 +24,7 @@ echo "Format filesystem and populate"
 _scratch_mkfs > $seqres.full
 _scratch_mount >> $seqres.full
 
-$XFS_INFO_PROG $SCRATCH_MNT | grep -q ftype=1 || \
-	_notrun "filesystem does not support ftype"
+_require_xfs_has_feature "$SCRATCH_MNT" ftype
 
 filter_ls() {
 	awk '
diff --git a/tests/xfs/271 b/tests/xfs/271
index 14d64cd0e5..d67ac4d6c1 100755
--- a/tests/xfs/271
+++ b/tests/xfs/271
@@ -37,7 +37,7 @@ agcount=$(_xfs_mount_agcount $SCRATCH_MNT)
 # same owner (per-AG metadata) for rmap btree blocks and blocks on the AGFL and
 # the reverse mapping index merges records, the number of per-AG extents
 # reported will vary depending on whether the refcount btree is enabled.
-$XFS_INFO_PROG $SCRATCH_MNT | grep -q reflink=1
+_require_xfs_has_feature "$SCRATCH_MNT" reflink
 has_reflink=$(( 1 - $? ))
 perag_metadata_exts=2
 test $has_reflink -gt 0 && perag_metadata_exts=$((perag_metadata_exts + 1))
diff --git a/tests/xfs/307 b/tests/xfs/307
index ba7204dd00..f3c970fadf 100755
--- a/tests/xfs/307
+++ b/tests/xfs/307
@@ -22,7 +22,7 @@ _require_scratch_reflink
 echo "Format"
 _scratch_mkfs > $seqres.full 2>&1
 _scratch_mount >> $seqres.full
-is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
+is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
 _scratch_unmount
 
 _get_agf_data() {
diff --git a/tests/xfs/308 b/tests/xfs/308
index d0f47f5038..6da6622e14 100755
--- a/tests/xfs/308
+++ b/tests/xfs/308
@@ -22,7 +22,7 @@ _require_scratch_reflink
 echo "Format"
 _scratch_mkfs > $seqres.full 2>&1
 _scratch_mount >> $seqres.full
-is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
+is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
 _scratch_xfs_unmount_dirty
 
 _get_agf_data() {
diff --git a/tests/xfs/348 b/tests/xfs/348
index faf2dca50b..d1645d9462 100755
--- a/tests/xfs/348
+++ b/tests/xfs/348
@@ -39,7 +39,7 @@ mknod $testdir/CHRDEV c 1 1
 mknod $testdir/BLKDEV b 1 1
 mknod $testdir/FIFO p
 
-$XFS_INFO_PROG $SCRATCH_MNT | grep -q "ftype=1" && FTYPE_FEATURE=1
+_xfs_has_feature $SCRATCH_MNT ftype && FTYPE_FEATURE=1
 
 # Record test dir inode for xfs_repair filter
 inode_filter=$tmp.sed


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] xfs: refactor filesystem feature detection logic
  2022-10-18 22:45 ` [PATCH 1/3] xfs: refactor filesystem feature detection logic Darrick J. Wong
@ 2022-10-20 12:26   ` Zorro Lang
  2022-10-20 21:03     ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Zorro Lang @ 2022-10-20 12:26 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, linux-xfs, fstests

On Tue, Oct 18, 2022 at 03:45:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a lot of places where we open-code detecting features of a
> specific filesystem.  Refactor this into a couple of helpers in
> preparation for adding stress tests for online repair and fuzzing.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |   14 ++++++-----
>  common/rc       |    9 +++++++
>  common/xfs      |   67 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  tests/xfs/097   |    2 +-
>  tests/xfs/151   |    3 +-
>  tests/xfs/271   |    2 +-
>  tests/xfs/307   |    2 +-
>  tests/xfs/308   |    2 +-
>  tests/xfs/348   |    2 +-
>  9 files changed, 77 insertions(+), 26 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 58b07e33be..9fa1a06798 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -131,7 +131,7 @@ _populate_xfs_qmount_option()
>  	fi
>  
>  	# Turn on all the quotas
> -	if $XFS_INFO_PROG "${TEST_DIR}" | grep -q 'crc=1'; then
> +	if _xfs_has_feature "$TEST_DIR" crc; then
>  		# v5 filesystems can have group & project quotas
>  		quota="usrquota,grpquota,prjquota"
>  	else
> @@ -176,7 +176,7 @@ _scratch_xfs_populate() {
>  
>  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> -	crc="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep crc= | sed -e 's/^.*crc=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
>  	if [ $crc -eq 1 ]; then
>  		leaf_hdr_size=64
>  	else
> @@ -315,7 +315,7 @@ _scratch_xfs_populate() {
>  	done
>  
>  	# Reverse-mapping btree
> -	is_rmapbt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')"
> +	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
>  	if [ $is_rmapbt -gt 0 ]; then
>  		echo "+ rmapbt btree"
>  		nr="$((blksz * 2 / 24))"
> @@ -332,7 +332,7 @@ _scratch_xfs_populate() {
>  	fi
>  
>  	# Reference-count btree
> -	is_reflink="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')"
> +	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
>  	if [ $is_reflink -gt 0 ]; then
>  		echo "+ reflink btree"
>  		nr="$((blksz * 2 / 12))"
> @@ -597,9 +597,9 @@ _scratch_xfs_populate_check() {
>  	leaf_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_LEAF")"
>  	node_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_NODE")"
>  	btree_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_BTREE")"
> -	is_finobt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'finobt=1')
> -	is_rmapbt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')
> -	is_reflink=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')
> +	is_finobt=$(_xfs_has_feature "$SCRATCH_MNT" finobt -v)
> +	is_rmapbt=$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)
> +	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
>  
>  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> diff --git a/common/rc b/common/rc
> index f4785c17ca..7944ef550e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -247,7 +247,14 @@ _supports_filetype()
>  	local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
>  	case "$fstyp" in
>  	xfs)
> -		$XFS_INFO_PROG $dir | grep -q "ftype=1"
> +		# If _xfs_has_feature is not present, grep xfs_info directly.
> +		# This can happen if we're testing overlayfs atop XFS and have
> +		# not sourced ./common/xfs.

Hmm... I thought I've fixed this kind of problem in:
  61775fb9 common: source base fs specific common file

didn't I?

Others looks good to me,

Thanks,
Zorro


> +		if command -v "_xfs_has_feature" &>/dev/null; then
> +			_xfs_has_feature $dir ftype
> +		else
> +			$XFS_INFO_PROG $dir | grep -q "ftype=1"
> +		fi
>  		;;
>  	ext2|ext3|ext4)
>  		local dev=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $1}'`
> diff --git a/common/xfs b/common/xfs
> index 2cd8254937..c7496bce3f 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -422,6 +422,56 @@ _require_xfs_crc()
>  	_scratch_unmount
>  }
>  
> +# If the xfs_info output for the given XFS filesystem mount mentions the given
> +# feature.  If so, return 0 for success.  If not, return 1 for failure.  If the
> +# third option is -v, echo 1 for success and 0 for not.
> +#
> +# Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> +_xfs_has_feature()
> +{
> +	local fs="$1"
> +	local feat="$2"
> +	local verbose="$3"
> +
> +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> +	if [ "$answer" -ne 0 ]; then
> +		test "$verbose" = "-v" && echo 1
> +		return 0
> +	fi
> +
> +	test "$verbose" = "-v" && echo 0
> +	return 1
> +}
> +
> +# Require that the xfs_info output for the given XFS filesystem mount mentions
> +# the given feature flag.  If the third argument is -u (or is empty and the
> +# second argument is $SCRATCH_MNT), unmount the fs on failure.  If a fourth
> +# argument is supplied, it will be used as the _notrun message.
> +_require_xfs_has_feature()
> +{
> +	local fs="$1"
> +	local feat="$2"
> +	local umount="$3"
> +	local message="$4"
> +
> +	if [ -z "$umount" ] && [ "$fs" = "$SCRATCH_MNT" ]; then
> +		umount="-u"
> +	fi
> +
> +	_xfs_has_feature "$1" "$2" && return 0
> +
> +	test "$umount" = "-u" && umount "$fs" &>/dev/null
> +
> +	test -n "$message" && _notrun "$message"
> +
> +	case "$fs" in
> +	"$TEST_DIR"|"$TEST_DEV")	fsname="test";;
> +	"$SCRATCH_MNT"|"$SCRATCH_DEV")	fsname="scratch";;
> +	*)				fsname="$fs";;
> +	esac
> +	_notrun "$2 not supported by $fsname filesystem type: $FSTYP"
> +}
> +
>  # this test requires the xfs kernel support crc feature on scratch device
>  #
>  _require_scratch_xfs_crc()
> @@ -429,7 +479,8 @@ _require_scratch_xfs_crc()
>  	_scratch_mkfs_xfs >/dev/null 2>&1
>  	_try_scratch_mount >/dev/null 2>&1 \
>  	   || _notrun "Kernel doesn't support crc feature"
> -	$XFS_INFO_PROG $SCRATCH_MNT | grep -q 'crc=1' || _notrun "crc feature not supported by this filesystem"
> +	_require_xfs_has_feature $SCRATCH_MNT crc -u \
> +		"crc feature not supported by this filesystem"
>  	_scratch_unmount
>  }
>  
> @@ -748,10 +799,7 @@ _check_xfs_test_fs()
>  _require_xfs_test_rmapbt()
>  {
>  	_require_test
> -
> -	if [ "$($XFS_INFO_PROG "$TEST_DIR" | grep -c "rmapbt=1")" -ne 1 ]; then
> -		_notrun "rmapbt not supported by test filesystem type: $FSTYP"
> -	fi
> +	_require_xfs_has_feature "$TEST_DIR" rmapbt
>  }
>  
>  _require_xfs_scratch_rmapbt()
> @@ -760,10 +808,7 @@ _require_xfs_scratch_rmapbt()
>  
>  	_scratch_mkfs > /dev/null
>  	_scratch_mount
> -	if [ "$($XFS_INFO_PROG "$SCRATCH_MNT" | grep -c "rmapbt=1")" -ne 1 ]; then
> -		_scratch_unmount
> -		_notrun "rmapbt not supported by scratch filesystem type: $FSTYP"
> -	fi
> +	_require_xfs_has_feature "$SCRATCH_MNT" rmapbt
>  	_scratch_unmount
>  }
>  
> @@ -1366,8 +1411,8 @@ _require_scratch_xfs_bigtime()
>  		_notrun "mkfs.xfs doesn't support bigtime feature"
>  	_try_scratch_mount || \
>  		_notrun "kernel doesn't support xfs bigtime feature"
> -	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "bigtime=1" || \
> -		_notrun "bigtime feature not advertised on mount?"
> +	_require_xfs_has_feature $SCRATCH_MNT bigtime -u \
> +		"crc feature not supported by this filesystem"
>  	_scratch_unmount
>  }
>  
> diff --git a/tests/xfs/097 b/tests/xfs/097
> index 4cad7216cd..1df34eeddc 100755
> --- a/tests/xfs/097
> +++ b/tests/xfs/097
> @@ -42,7 +42,7 @@ _scratch_mkfs_xfs -m crc=1,finobt=1 > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -$XFS_INFO_PROG "${SCRATCH_MNT}" | grep -q "finobt=1" || _notrun "finobt not enabled"
> +_require_xfs_has_feature "$SCRATCH_MNT" finobt
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  
>  echo "+ make some files"
> diff --git a/tests/xfs/151 b/tests/xfs/151
> index 66425f6710..b2fe16aefb 100755
> --- a/tests/xfs/151
> +++ b/tests/xfs/151
> @@ -24,8 +24,7 @@ echo "Format filesystem and populate"
>  _scratch_mkfs > $seqres.full
>  _scratch_mount >> $seqres.full
>  
> -$XFS_INFO_PROG $SCRATCH_MNT | grep -q ftype=1 || \
> -	_notrun "filesystem does not support ftype"
> +_require_xfs_has_feature "$SCRATCH_MNT" ftype
>  
>  filter_ls() {
>  	awk '
> diff --git a/tests/xfs/271 b/tests/xfs/271
> index 14d64cd0e5..d67ac4d6c1 100755
> --- a/tests/xfs/271
> +++ b/tests/xfs/271
> @@ -37,7 +37,7 @@ agcount=$(_xfs_mount_agcount $SCRATCH_MNT)
>  # same owner (per-AG metadata) for rmap btree blocks and blocks on the AGFL and
>  # the reverse mapping index merges records, the number of per-AG extents
>  # reported will vary depending on whether the refcount btree is enabled.
> -$XFS_INFO_PROG $SCRATCH_MNT | grep -q reflink=1
> +_require_xfs_has_feature "$SCRATCH_MNT" reflink
>  has_reflink=$(( 1 - $? ))
>  perag_metadata_exts=2
>  test $has_reflink -gt 0 && perag_metadata_exts=$((perag_metadata_exts + 1))
> diff --git a/tests/xfs/307 b/tests/xfs/307
> index ba7204dd00..f3c970fadf 100755
> --- a/tests/xfs/307
> +++ b/tests/xfs/307
> @@ -22,7 +22,7 @@ _require_scratch_reflink
>  echo "Format"
>  _scratch_mkfs > $seqres.full 2>&1
>  _scratch_mount >> $seqres.full
> -is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
> +is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
>  _scratch_unmount
>  
>  _get_agf_data() {
> diff --git a/tests/xfs/308 b/tests/xfs/308
> index d0f47f5038..6da6622e14 100755
> --- a/tests/xfs/308
> +++ b/tests/xfs/308
> @@ -22,7 +22,7 @@ _require_scratch_reflink
>  echo "Format"
>  _scratch_mkfs > $seqres.full 2>&1
>  _scratch_mount >> $seqres.full
> -is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
> +is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
>  _scratch_xfs_unmount_dirty
>  
>  _get_agf_data() {
> diff --git a/tests/xfs/348 b/tests/xfs/348
> index faf2dca50b..d1645d9462 100755
> --- a/tests/xfs/348
> +++ b/tests/xfs/348
> @@ -39,7 +39,7 @@ mknod $testdir/CHRDEV c 1 1
>  mknod $testdir/BLKDEV b 1 1
>  mknod $testdir/FIFO p
>  
> -$XFS_INFO_PROG $SCRATCH_MNT | grep -q "ftype=1" && FTYPE_FEATURE=1
> +_xfs_has_feature $SCRATCH_MNT ftype && FTYPE_FEATURE=1
>  
>  # Record test dir inode for xfs_repair filter
>  inode_filter=$tmp.sed
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] xfs: refactor filesystem feature detection logic
  2022-10-20 12:26   ` Zorro Lang
@ 2022-10-20 21:03     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-20 21:03 UTC (permalink / raw)
  To: Zorro Lang; +Cc: zlang, linux-xfs, fstests

On Thu, Oct 20, 2022 at 08:26:22PM +0800, Zorro Lang wrote:
> On Tue, Oct 18, 2022 at 03:45:27PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a lot of places where we open-code detecting features of a
> > specific filesystem.  Refactor this into a couple of helpers in
> > preparation for adding stress tests for online repair and fuzzing.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |   14 ++++++-----
> >  common/rc       |    9 +++++++
> >  common/xfs      |   67 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >  tests/xfs/097   |    2 +-
> >  tests/xfs/151   |    3 +-
> >  tests/xfs/271   |    2 +-
> >  tests/xfs/307   |    2 +-
> >  tests/xfs/308   |    2 +-
> >  tests/xfs/348   |    2 +-
> >  9 files changed, 77 insertions(+), 26 deletions(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index 58b07e33be..9fa1a06798 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -131,7 +131,7 @@ _populate_xfs_qmount_option()
> >  	fi
> >  
> >  	# Turn on all the quotas
> > -	if $XFS_INFO_PROG "${TEST_DIR}" | grep -q 'crc=1'; then
> > +	if _xfs_has_feature "$TEST_DIR" crc; then
> >  		# v5 filesystems can have group & project quotas
> >  		quota="usrquota,grpquota,prjquota"
> >  	else
> > @@ -176,7 +176,7 @@ _scratch_xfs_populate() {
> >  
> >  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > -	crc="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep crc= | sed -e 's/^.*crc=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
> >  	if [ $crc -eq 1 ]; then
> >  		leaf_hdr_size=64
> >  	else
> > @@ -315,7 +315,7 @@ _scratch_xfs_populate() {
> >  	done
> >  
> >  	# Reverse-mapping btree
> > -	is_rmapbt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')"
> > +	is_rmapbt="$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)"
> >  	if [ $is_rmapbt -gt 0 ]; then
> >  		echo "+ rmapbt btree"
> >  		nr="$((blksz * 2 / 24))"
> > @@ -332,7 +332,7 @@ _scratch_xfs_populate() {
> >  	fi
> >  
> >  	# Reference-count btree
> > -	is_reflink="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')"
> > +	is_reflink="$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)"
> >  	if [ $is_reflink -gt 0 ]; then
> >  		echo "+ reflink btree"
> >  		nr="$((blksz * 2 / 12))"
> > @@ -597,9 +597,9 @@ _scratch_xfs_populate_check() {
> >  	leaf_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_LEAF")"
> >  	node_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_NODE")"
> >  	btree_attr="$(__populate_find_inode "${SCRATCH_MNT}/ATTR.FMT_BTREE")"
> > -	is_finobt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'finobt=1')
> > -	is_rmapbt=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rmapbt=1')
> > -	is_reflink=$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'reflink=1')
> > +	is_finobt=$(_xfs_has_feature "$SCRATCH_MNT" finobt -v)
> > +	is_rmapbt=$(_xfs_has_feature "$SCRATCH_MNT" rmapbt -v)
> > +	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
> >  
> >  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > diff --git a/common/rc b/common/rc
> > index f4785c17ca..7944ef550e 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -247,7 +247,14 @@ _supports_filetype()
> >  	local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
> >  	case "$fstyp" in
> >  	xfs)
> > -		$XFS_INFO_PROG $dir | grep -q "ftype=1"
> > +		# If _xfs_has_feature is not present, grep xfs_info directly.
> > +		# This can happen if we're testing overlayfs atop XFS and have
> > +		# not sourced ./common/xfs.
> 
> Hmm... I thought I've fixed this kind of problem in:
>   61775fb9 common: source base fs specific common file
> 
> didn't I?

Oh!  That /did/ fix it.  This hunk can go away.

--D

> Others looks good to me,
> 
> Thanks,
> Zorro
> 
> 
> > +		if command -v "_xfs_has_feature" &>/dev/null; then
> > +			_xfs_has_feature $dir ftype
> > +		else
> > +			$XFS_INFO_PROG $dir | grep -q "ftype=1"
> > +		fi
> >  		;;
> >  	ext2|ext3|ext4)
> >  		local dev=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $1}'`
> > diff --git a/common/xfs b/common/xfs
> > index 2cd8254937..c7496bce3f 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -422,6 +422,56 @@ _require_xfs_crc()
> >  	_scratch_unmount
> >  }
> >  
> > +# If the xfs_info output for the given XFS filesystem mount mentions the given
> > +# feature.  If so, return 0 for success.  If not, return 1 for failure.  If the
> > +# third option is -v, echo 1 for success and 0 for not.
> > +#
> > +# Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> > +_xfs_has_feature()
> > +{
> > +	local fs="$1"
> > +	local feat="$2"
> > +	local verbose="$3"
> > +
> > +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> > +	if [ "$answer" -ne 0 ]; then
> > +		test "$verbose" = "-v" && echo 1
> > +		return 0
> > +	fi
> > +
> > +	test "$verbose" = "-v" && echo 0
> > +	return 1
> > +}
> > +
> > +# Require that the xfs_info output for the given XFS filesystem mount mentions
> > +# the given feature flag.  If the third argument is -u (or is empty and the
> > +# second argument is $SCRATCH_MNT), unmount the fs on failure.  If a fourth
> > +# argument is supplied, it will be used as the _notrun message.
> > +_require_xfs_has_feature()
> > +{
> > +	local fs="$1"
> > +	local feat="$2"
> > +	local umount="$3"
> > +	local message="$4"
> > +
> > +	if [ -z "$umount" ] && [ "$fs" = "$SCRATCH_MNT" ]; then
> > +		umount="-u"
> > +	fi
> > +
> > +	_xfs_has_feature "$1" "$2" && return 0
> > +
> > +	test "$umount" = "-u" && umount "$fs" &>/dev/null
> > +
> > +	test -n "$message" && _notrun "$message"
> > +
> > +	case "$fs" in
> > +	"$TEST_DIR"|"$TEST_DEV")	fsname="test";;
> > +	"$SCRATCH_MNT"|"$SCRATCH_DEV")	fsname="scratch";;
> > +	*)				fsname="$fs";;
> > +	esac
> > +	_notrun "$2 not supported by $fsname filesystem type: $FSTYP"
> > +}
> > +
> >  # this test requires the xfs kernel support crc feature on scratch device
> >  #
> >  _require_scratch_xfs_crc()
> > @@ -429,7 +479,8 @@ _require_scratch_xfs_crc()
> >  	_scratch_mkfs_xfs >/dev/null 2>&1
> >  	_try_scratch_mount >/dev/null 2>&1 \
> >  	   || _notrun "Kernel doesn't support crc feature"
> > -	$XFS_INFO_PROG $SCRATCH_MNT | grep -q 'crc=1' || _notrun "crc feature not supported by this filesystem"
> > +	_require_xfs_has_feature $SCRATCH_MNT crc -u \
> > +		"crc feature not supported by this filesystem"
> >  	_scratch_unmount
> >  }
> >  
> > @@ -748,10 +799,7 @@ _check_xfs_test_fs()
> >  _require_xfs_test_rmapbt()
> >  {
> >  	_require_test
> > -
> > -	if [ "$($XFS_INFO_PROG "$TEST_DIR" | grep -c "rmapbt=1")" -ne 1 ]; then
> > -		_notrun "rmapbt not supported by test filesystem type: $FSTYP"
> > -	fi
> > +	_require_xfs_has_feature "$TEST_DIR" rmapbt
> >  }
> >  
> >  _require_xfs_scratch_rmapbt()
> > @@ -760,10 +808,7 @@ _require_xfs_scratch_rmapbt()
> >  
> >  	_scratch_mkfs > /dev/null
> >  	_scratch_mount
> > -	if [ "$($XFS_INFO_PROG "$SCRATCH_MNT" | grep -c "rmapbt=1")" -ne 1 ]; then
> > -		_scratch_unmount
> > -		_notrun "rmapbt not supported by scratch filesystem type: $FSTYP"
> > -	fi
> > +	_require_xfs_has_feature "$SCRATCH_MNT" rmapbt
> >  	_scratch_unmount
> >  }
> >  
> > @@ -1366,8 +1411,8 @@ _require_scratch_xfs_bigtime()
> >  		_notrun "mkfs.xfs doesn't support bigtime feature"
> >  	_try_scratch_mount || \
> >  		_notrun "kernel doesn't support xfs bigtime feature"
> > -	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "bigtime=1" || \
> > -		_notrun "bigtime feature not advertised on mount?"
> > +	_require_xfs_has_feature $SCRATCH_MNT bigtime -u \
> > +		"crc feature not supported by this filesystem"
> >  	_scratch_unmount
> >  }
> >  
> > diff --git a/tests/xfs/097 b/tests/xfs/097
> > index 4cad7216cd..1df34eeddc 100755
> > --- a/tests/xfs/097
> > +++ b/tests/xfs/097
> > @@ -42,7 +42,7 @@ _scratch_mkfs_xfs -m crc=1,finobt=1 > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -$XFS_INFO_PROG "${SCRATCH_MNT}" | grep -q "finobt=1" || _notrun "finobt not enabled"
> > +_require_xfs_has_feature "$SCRATCH_MNT" finobt
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  
> >  echo "+ make some files"
> > diff --git a/tests/xfs/151 b/tests/xfs/151
> > index 66425f6710..b2fe16aefb 100755
> > --- a/tests/xfs/151
> > +++ b/tests/xfs/151
> > @@ -24,8 +24,7 @@ echo "Format filesystem and populate"
> >  _scratch_mkfs > $seqres.full
> >  _scratch_mount >> $seqres.full
> >  
> > -$XFS_INFO_PROG $SCRATCH_MNT | grep -q ftype=1 || \
> > -	_notrun "filesystem does not support ftype"
> > +_require_xfs_has_feature "$SCRATCH_MNT" ftype
> >  
> >  filter_ls() {
> >  	awk '
> > diff --git a/tests/xfs/271 b/tests/xfs/271
> > index 14d64cd0e5..d67ac4d6c1 100755
> > --- a/tests/xfs/271
> > +++ b/tests/xfs/271
> > @@ -37,7 +37,7 @@ agcount=$(_xfs_mount_agcount $SCRATCH_MNT)
> >  # same owner (per-AG metadata) for rmap btree blocks and blocks on the AGFL and
> >  # the reverse mapping index merges records, the number of per-AG extents
> >  # reported will vary depending on whether the refcount btree is enabled.
> > -$XFS_INFO_PROG $SCRATCH_MNT | grep -q reflink=1
> > +_require_xfs_has_feature "$SCRATCH_MNT" reflink
> >  has_reflink=$(( 1 - $? ))
> >  perag_metadata_exts=2
> >  test $has_reflink -gt 0 && perag_metadata_exts=$((perag_metadata_exts + 1))
> > diff --git a/tests/xfs/307 b/tests/xfs/307
> > index ba7204dd00..f3c970fadf 100755
> > --- a/tests/xfs/307
> > +++ b/tests/xfs/307
> > @@ -22,7 +22,7 @@ _require_scratch_reflink
> >  echo "Format"
> >  _scratch_mkfs > $seqres.full 2>&1
> >  _scratch_mount >> $seqres.full
> > -is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
> > +is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
> >  _scratch_unmount
> >  
> >  _get_agf_data() {
> > diff --git a/tests/xfs/308 b/tests/xfs/308
> > index d0f47f5038..6da6622e14 100755
> > --- a/tests/xfs/308
> > +++ b/tests/xfs/308
> > @@ -22,7 +22,7 @@ _require_scratch_reflink
> >  echo "Format"
> >  _scratch_mkfs > $seqres.full 2>&1
> >  _scratch_mount >> $seqres.full
> > -is_rmap=$($XFS_INFO_PROG $SCRATCH_MNT | grep -c "rmapbt=1")
> > +is_rmap=$(_xfs_has_feature $SCRATCH_MNT rmapbt -v)
> >  _scratch_xfs_unmount_dirty
> >  
> >  _get_agf_data() {
> > diff --git a/tests/xfs/348 b/tests/xfs/348
> > index faf2dca50b..d1645d9462 100755
> > --- a/tests/xfs/348
> > +++ b/tests/xfs/348
> > @@ -39,7 +39,7 @@ mknod $testdir/CHRDEV c 1 1
> >  mknod $testdir/BLKDEV b 1 1
> >  mknod $testdir/FIFO p
> >  
> > -$XFS_INFO_PROG $SCRATCH_MNT | grep -q "ftype=1" && FTYPE_FEATURE=1
> > +_xfs_has_feature $SCRATCH_MNT ftype && FTYPE_FEATURE=1
> >  
> >  # Record test dir inode for xfs_repair filter
> >  inode_filter=$tmp.sed
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic
  2022-10-18 22:45 [PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation Darrick J. Wong
  2022-10-18 22:45 ` [PATCH 1/3] xfs: refactor filesystem feature detection logic Darrick J. Wong
@ 2022-10-18 22:45 ` Darrick J. Wong
  2022-10-20  6:07   ` Zorro Lang
  2022-10-18 22:45 ` [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic Darrick J. Wong
  2 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-18 22:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

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

There are a lot of places where we open-code determining the directory
block size for a specific filesystem.  Refactor this into a single
helper to clean up existing tests.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    4 ++--
 common/xfs      |    9 +++++++++
 tests/xfs/099   |    2 +-
 tests/xfs/100   |    2 +-
 tests/xfs/101   |    2 +-
 tests/xfs/102   |    2 +-
 tests/xfs/105   |    2 +-
 tests/xfs/112   |    2 +-
 tests/xfs/113   |    2 +-
 9 files changed, 18 insertions(+), 9 deletions(-)


diff --git a/common/populate b/common/populate
index 9fa1a06798..23b2fecf69 100644
--- a/common/populate
+++ b/common/populate
@@ -175,7 +175,7 @@ _scratch_xfs_populate() {
 	_xfs_force_bdev data $SCRATCH_MNT
 
 	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
-	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
 	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
 	if [ $crc -eq 1 ]; then
 		leaf_hdr_size=64
@@ -602,7 +602,7 @@ _scratch_xfs_populate_check() {
 	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
 
 	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
-	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
 	leaf_lblk="$((32 * 1073741824 / blksz))"
 	node_lblk="$((64 * 1073741824 / blksz))"
 	umount "${SCRATCH_MNT}"
diff --git a/common/xfs b/common/xfs
index c7496bce3f..6445bfd9db 100644
--- a/common/xfs
+++ b/common/xfs
@@ -203,6 +203,15 @@ _xfs_is_realtime_file()
 	$XFS_IO_PROG -c 'stat -v' "$1" | grep -q -w realtime
 }
 
+# Get the directory block size of a mounted filesystem.
+_xfs_get_dir_blocksize()
+{
+	local fs="$1"
+
+	$XFS_INFO_PROG "$fs" | grep 'naming.*bsize' | \
+		sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'
+}
+
 # Set or clear the realtime status of every supplied path.  The first argument
 # is either 'data' or 'realtime'.  All other arguments should be paths to
 # existing directories or empty regular files.
diff --git a/tests/xfs/099 b/tests/xfs/099
index a7eaff6e0c..82bef8ad26 100755
--- a/tests/xfs/099
+++ b/tests/xfs/099
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((dblksz / 40))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/100 b/tests/xfs/100
index 79da8cb02c..e638b4ba17 100755
--- a/tests/xfs/100
+++ b/tests/xfs/100
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((dblksz / 12))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/101 b/tests/xfs/101
index 64f4705aca..11ed329110 100755
--- a/tests/xfs/101
+++ b/tests/xfs/101
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((dblksz / 12))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/102 b/tests/xfs/102
index 24dce43058..43f4539181 100755
--- a/tests/xfs/102
+++ b/tests/xfs/102
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((16 * dblksz / 40))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/105 b/tests/xfs/105
index 22a8bf9fb0..002a712883 100755
--- a/tests/xfs/105
+++ b/tests/xfs/105
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((16 * dblksz / 40))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/112 b/tests/xfs/112
index bc1ab62895..e2d5932da6 100755
--- a/tests/xfs/112
+++ b/tests/xfs/112
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((16 * dblksz / 40))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"
diff --git a/tests/xfs/113 b/tests/xfs/113
index e820ed96da..9bb2cd304b 100755
--- a/tests/xfs/113
+++ b/tests/xfs/113
@@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
 
 echo "+ mount fs image"
 _scratch_mount
-dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
+dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
 nr="$((128 * dblksz / 40))"
 blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
 leaf_lblk="$((32 * 1073741824 / blksz))"


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic
  2022-10-18 22:45 ` [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic Darrick J. Wong
@ 2022-10-20  6:07   ` Zorro Lang
  2022-10-20 21:07     ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Zorro Lang @ 2022-10-20  6:07 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, linux-xfs, fstests

On Tue, Oct 18, 2022 at 03:45:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a lot of places where we open-code determining the directory
> block size for a specific filesystem.  Refactor this into a single
> helper to clean up existing tests.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |    4 ++--
>  common/xfs      |    9 +++++++++
>  tests/xfs/099   |    2 +-
>  tests/xfs/100   |    2 +-
>  tests/xfs/101   |    2 +-
>  tests/xfs/102   |    2 +-
>  tests/xfs/105   |    2 +-
>  tests/xfs/112   |    2 +-
>  tests/xfs/113   |    2 +-
>  9 files changed, 18 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 9fa1a06798..23b2fecf69 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -175,7 +175,7 @@ _scratch_xfs_populate() {
>  	_xfs_force_bdev data $SCRATCH_MNT
>  
>  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> -	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
>  	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
>  	if [ $crc -eq 1 ]; then
>  		leaf_hdr_size=64
> @@ -602,7 +602,7 @@ _scratch_xfs_populate_check() {
>  	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
>  
>  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> -	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
>  	leaf_lblk="$((32 * 1073741824 / blksz))"
>  	node_lblk="$((64 * 1073741824 / blksz))"
>  	umount "${SCRATCH_MNT}"
> diff --git a/common/xfs b/common/xfs
> index c7496bce3f..6445bfd9db 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -203,6 +203,15 @@ _xfs_is_realtime_file()
>  	$XFS_IO_PROG -c 'stat -v' "$1" | grep -q -w realtime
>  }
>  
> +# Get the directory block size of a mounted filesystem.
> +_xfs_get_dir_blocksize()
> +{
> +	local fs="$1"
> +
> +	$XFS_INFO_PROG "$fs" | grep 'naming.*bsize' | \
> +		sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'

As you've used escape char of sed, I think it doesn't need two pipe lines
and two sed commands, how about:

	$XFS_INFO_PROG "$fs" | sed -n "s/^naming.*bsize=\([[:digit:]]*\).*/\1/p"

Others looks good to me.

Thanks,
Zorro

> +}
> +
>  # Set or clear the realtime status of every supplied path.  The first argument
>  # is either 'data' or 'realtime'.  All other arguments should be paths to
>  # existing directories or empty regular files.
> diff --git a/tests/xfs/099 b/tests/xfs/099
> index a7eaff6e0c..82bef8ad26 100755
> --- a/tests/xfs/099
> +++ b/tests/xfs/099
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((dblksz / 40))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/100 b/tests/xfs/100
> index 79da8cb02c..e638b4ba17 100755
> --- a/tests/xfs/100
> +++ b/tests/xfs/100
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((dblksz / 12))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/101 b/tests/xfs/101
> index 64f4705aca..11ed329110 100755
> --- a/tests/xfs/101
> +++ b/tests/xfs/101
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((dblksz / 12))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/102 b/tests/xfs/102
> index 24dce43058..43f4539181 100755
> --- a/tests/xfs/102
> +++ b/tests/xfs/102
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((16 * dblksz / 40))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/105 b/tests/xfs/105
> index 22a8bf9fb0..002a712883 100755
> --- a/tests/xfs/105
> +++ b/tests/xfs/105
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((16 * dblksz / 40))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/112 b/tests/xfs/112
> index bc1ab62895..e2d5932da6 100755
> --- a/tests/xfs/112
> +++ b/tests/xfs/112
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((16 * dblksz / 40))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> diff --git a/tests/xfs/113 b/tests/xfs/113
> index e820ed96da..9bb2cd304b 100755
> --- a/tests/xfs/113
> +++ b/tests/xfs/113
> @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
>  
>  echo "+ mount fs image"
>  _scratch_mount
> -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
>  nr="$((128 * dblksz / 40))"
>  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
>  leaf_lblk="$((32 * 1073741824 / blksz))"
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic
  2022-10-20  6:07   ` Zorro Lang
@ 2022-10-20 21:07     ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-20 21:07 UTC (permalink / raw)
  To: Zorro Lang; +Cc: zlang, linux-xfs, fstests

On Thu, Oct 20, 2022 at 02:07:50PM +0800, Zorro Lang wrote:
> On Tue, Oct 18, 2022 at 03:45:33PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a lot of places where we open-code determining the directory
> > block size for a specific filesystem.  Refactor this into a single
> > helper to clean up existing tests.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |    4 ++--
> >  common/xfs      |    9 +++++++++
> >  tests/xfs/099   |    2 +-
> >  tests/xfs/100   |    2 +-
> >  tests/xfs/101   |    2 +-
> >  tests/xfs/102   |    2 +-
> >  tests/xfs/105   |    2 +-
> >  tests/xfs/112   |    2 +-
> >  tests/xfs/113   |    2 +-
> >  9 files changed, 18 insertions(+), 9 deletions(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index 9fa1a06798..23b2fecf69 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -175,7 +175,7 @@ _scratch_xfs_populate() {
> >  	_xfs_force_bdev data $SCRATCH_MNT
> >  
> >  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> > -	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
> >  	crc="$(_xfs_has_feature "$SCRATCH_MNT" crc -v)"
> >  	if [ $crc -eq 1 ]; then
> >  		leaf_hdr_size=64
> > @@ -602,7 +602,7 @@ _scratch_xfs_populate_check() {
> >  	is_reflink=$(_xfs_has_feature "$SCRATCH_MNT" reflink -v)
> >  
> >  	blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> > -	dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +	dblksz="$(_xfs_get_dir_blocksize "$SCRATCH_MNT")"
> >  	leaf_lblk="$((32 * 1073741824 / blksz))"
> >  	node_lblk="$((64 * 1073741824 / blksz))"
> >  	umount "${SCRATCH_MNT}"
> > diff --git a/common/xfs b/common/xfs
> > index c7496bce3f..6445bfd9db 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -203,6 +203,15 @@ _xfs_is_realtime_file()
> >  	$XFS_IO_PROG -c 'stat -v' "$1" | grep -q -w realtime
> >  }
> >  
> > +# Get the directory block size of a mounted filesystem.
> > +_xfs_get_dir_blocksize()
> > +{
> > +	local fs="$1"
> > +
> > +	$XFS_INFO_PROG "$fs" | grep 'naming.*bsize' | \
> > +		sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g'
> 
> As you've used escape char of sed, I think it doesn't need two pipe lines
> and two sed commands, how about:
> 
> 	$XFS_INFO_PROG "$fs" | sed -n "s/^naming.*bsize=\([[:digit:]]*\).*/\1/p"
> 
> Others looks good to me.

Oh!  I didn't know sed could do that.  I'll go make that change.

--D

> Thanks,
> Zorro
> 
> > +}
> > +
> >  # Set or clear the realtime status of every supplied path.  The first argument
> >  # is either 'data' or 'realtime'.  All other arguments should be paths to
> >  # existing directories or empty regular files.
> > diff --git a/tests/xfs/099 b/tests/xfs/099
> > index a7eaff6e0c..82bef8ad26 100755
> > --- a/tests/xfs/099
> > +++ b/tests/xfs/099
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((dblksz / 40))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/100 b/tests/xfs/100
> > index 79da8cb02c..e638b4ba17 100755
> > --- a/tests/xfs/100
> > +++ b/tests/xfs/100
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((dblksz / 12))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/101 b/tests/xfs/101
> > index 64f4705aca..11ed329110 100755
> > --- a/tests/xfs/101
> > +++ b/tests/xfs/101
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((dblksz / 12))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/102 b/tests/xfs/102
> > index 24dce43058..43f4539181 100755
> > --- a/tests/xfs/102
> > +++ b/tests/xfs/102
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((16 * dblksz / 40))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/105 b/tests/xfs/105
> > index 22a8bf9fb0..002a712883 100755
> > --- a/tests/xfs/105
> > +++ b/tests/xfs/105
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((16 * dblksz / 40))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/112 b/tests/xfs/112
> > index bc1ab62895..e2d5932da6 100755
> > --- a/tests/xfs/112
> > +++ b/tests/xfs/112
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((16 * dblksz / 40))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > diff --git a/tests/xfs/113 b/tests/xfs/113
> > index e820ed96da..9bb2cd304b 100755
> > --- a/tests/xfs/113
> > +++ b/tests/xfs/113
> > @@ -37,7 +37,7 @@ _scratch_mkfs_xfs > /dev/null
> >  
> >  echo "+ mount fs image"
> >  _scratch_mount
> > -dblksz="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')"
> > +dblksz=$(_xfs_get_dir_blocksize "$SCRATCH_MNT")
> >  nr="$((128 * dblksz / 40))"
> >  blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")"
> >  leaf_lblk="$((32 * 1073741824 / blksz))"
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic
  2022-10-18 22:45 [PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation Darrick J. Wong
  2022-10-18 22:45 ` [PATCH 1/3] xfs: refactor filesystem feature detection logic Darrick J. Wong
  2022-10-18 22:45 ` [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic Darrick J. Wong
@ 2022-10-18 22:45 ` Darrick J. Wong
  2022-10-20 12:15   ` Zorro Lang
  2 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-18 22:45 UTC (permalink / raw)
  To: djwong, guaneryu, zlang; +Cc: linux-xfs, fstests, guan

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

There are a lot of places where we open-code detecting the realtime
extent size and extent count of a specific filesystem.  Refactor this
into a couple of helpers to clean up the code.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    2 +-
 common/xfs      |   29 +++++++++++++++++++++++++++--
 tests/xfs/146   |    2 +-
 tests/xfs/147   |    2 +-
 tests/xfs/530   |    3 +--
 5 files changed, 31 insertions(+), 7 deletions(-)


diff --git a/common/populate b/common/populate
index 23b2fecf69..d9d4c6c300 100644
--- a/common/populate
+++ b/common/populate
@@ -323,7 +323,7 @@ _scratch_xfs_populate() {
 	fi
 
 	# Realtime Reverse-mapping btree
-	is_rt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rtextents=[1-9]')"
+	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
 	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
 		echo "+ rtrmapbt btree"
 		nr="$((blksz * 2 / 32))"
diff --git a/common/xfs b/common/xfs
index 6445bfd9db..20da537a9d 100644
--- a/common/xfs
+++ b/common/xfs
@@ -174,6 +174,24 @@ _scratch_mkfs_xfs()
 	return $mkfs_status
 }
 
+# Get the number of realtime extents of a mounted filesystem.
+_xfs_get_rtextents()
+{
+	local path="$1"
+
+	$XFS_INFO_PROG "$path" | grep 'rtextents' | \
+		sed -e 's/^.*rtextents=\([0-9]*\).*$/\1/g'
+}
+
+# Get the realtime extent size of a mounted filesystem.
+_xfs_get_rtextsize()
+{
+	local path="$1"
+
+	$XFS_INFO_PROG "$path" | grep 'realtime.*extsz' | \
+		sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
+}
+
 # Get the size of an allocation unit of a file.  Normally this is just the
 # block size of the file, but for realtime files, this is the realtime extent
 # size.
@@ -191,7 +209,7 @@ _xfs_get_file_block_size()
 	while ! $XFS_INFO_PROG "$path" &>/dev/null && [ "$path" != "/" ]; do
 		path="$(dirname "$path")"
 	done
-	$XFS_INFO_PROG "$path" | grep realtime | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
+	_xfs_get_rtextsize "$path"
 }
 
 # Decide if this path is a file on the realtime device
@@ -436,13 +454,20 @@ _require_xfs_crc()
 # third option is -v, echo 1 for success and 0 for not.
 #
 # Starting with xfsprogs 4.17, this also works for unmounted filesystems.
+# The feature 'realtime' looks for rtextents > 0.
 _xfs_has_feature()
 {
 	local fs="$1"
 	local feat="$2"
 	local verbose="$3"
+	local feat_regex="1"
 
-	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
+	if [ "$feat" = "realtime" ]; then
+		feat="rtextents"
+		feat_regex="[1-9][0-9]*"
+	fi
+
+	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -E -w -c "$feat=$feat_regex")"
 	if [ "$answer" -ne 0 ]; then
 		test "$verbose" = "-v" && echo 1
 		return 0
diff --git a/tests/xfs/146 b/tests/xfs/146
index 5516d396bf..123bdff59f 100755
--- a/tests/xfs/146
+++ b/tests/xfs/146
@@ -31,7 +31,7 @@ _scratch_mkfs > $seqres.full
 _scratch_mount >> $seqres.full
 
 blksz=$(_get_block_size $SCRATCH_MNT)
-rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
+rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
 rextblks=$((rextsize / blksz))
 
 echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
diff --git a/tests/xfs/147 b/tests/xfs/147
index e21fdd330c..33b3c99633 100755
--- a/tests/xfs/147
+++ b/tests/xfs/147
@@ -29,7 +29,7 @@ _scratch_mkfs -r extsize=256k > $seqres.full
 _scratch_mount >> $seqres.full
 
 blksz=$(_get_block_size $SCRATCH_MNT)
-rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
+rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
 rextblks=$((rextsize / blksz))
 
 echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
diff --git a/tests/xfs/530 b/tests/xfs/530
index c960738db7..56f5e7ebdb 100755
--- a/tests/xfs/530
+++ b/tests/xfs/530
@@ -73,8 +73,7 @@ _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
 formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
 test "$formatted_blksz" -ne "$dbsize" && \
 	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
-$XFS_INFO_PROG $SCRATCH_MNT | grep -E -q 'realtime.*blocks=0' && \
-	_notrun "Filesystem should have a realtime volume"
+_require_xfs_has_feature "$SCRATCH_MNT" realtime
 
 echo "Consume free space"
 fillerdir=$SCRATCH_MNT/fillerdir


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic
  2022-10-18 22:45 ` [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic Darrick J. Wong
@ 2022-10-20 12:15   ` Zorro Lang
  2022-10-20 21:21     ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Zorro Lang @ 2022-10-20 12:15 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: zlang, linux-xfs, fstests

On Tue, Oct 18, 2022 at 03:45:38PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a lot of places where we open-code detecting the realtime
> extent size and extent count of a specific filesystem.  Refactor this
> into a couple of helpers to clean up the code.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/populate |    2 +-
>  common/xfs      |   29 +++++++++++++++++++++++++++--
>  tests/xfs/146   |    2 +-
>  tests/xfs/147   |    2 +-
>  tests/xfs/530   |    3 +--
>  5 files changed, 31 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 23b2fecf69..d9d4c6c300 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -323,7 +323,7 @@ _scratch_xfs_populate() {
>  	fi
>  
>  	# Realtime Reverse-mapping btree
> -	is_rt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rtextents=[1-9]')"
> +	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
>  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
>  		echo "+ rtrmapbt btree"
>  		nr="$((blksz * 2 / 32))"
> diff --git a/common/xfs b/common/xfs
> index 6445bfd9db..20da537a9d 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -174,6 +174,24 @@ _scratch_mkfs_xfs()
>  	return $mkfs_status
>  }
>  
> +# Get the number of realtime extents of a mounted filesystem.
> +_xfs_get_rtextents()
> +{
> +	local path="$1"
> +
> +	$XFS_INFO_PROG "$path" | grep 'rtextents' | \
> +		sed -e 's/^.*rtextents=\([0-9]*\).*$/\1/g'

Same as patch 2/3, how about:

	$XFS_INFO_PROG "$path" | sed -n "s/^.*rtextents=\([[:digit:]]*\).*/\1/p"

and ...

> +}
> +
> +# Get the realtime extent size of a mounted filesystem.
> +_xfs_get_rtextsize()
> +{
> +	local path="$1"
> +
> +	$XFS_INFO_PROG "$path" | grep 'realtime.*extsz' | \
> +		sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'

...
	$XFS_INFO_PROG "$path" | sed -n "s/^realtime.*extsz=\([[:digit:]]*\).*/\1/p"

> +}
> +
>  # Get the size of an allocation unit of a file.  Normally this is just the
>  # block size of the file, but for realtime files, this is the realtime extent
>  # size.
> @@ -191,7 +209,7 @@ _xfs_get_file_block_size()
>  	while ! $XFS_INFO_PROG "$path" &>/dev/null && [ "$path" != "/" ]; do
>  		path="$(dirname "$path")"
>  	done
> -	$XFS_INFO_PROG "$path" | grep realtime | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> +	_xfs_get_rtextsize "$path"
>  }
>  
>  # Decide if this path is a file on the realtime device
> @@ -436,13 +454,20 @@ _require_xfs_crc()
>  # third option is -v, echo 1 for success and 0 for not.
>  #
>  # Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> +# The feature 'realtime' looks for rtextents > 0.
>  _xfs_has_feature()
>  {
>  	local fs="$1"
>  	local feat="$2"
>  	local verbose="$3"
> +	local feat_regex="1"
>  
> -	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> +	if [ "$feat" = "realtime" ]; then
> +		feat="rtextents"
> +		feat_regex="[1-9][0-9]*"
> +	fi

How about use this format:

case "$feat" in
realtime)
	feat="rtextents"
	feat_regex="[1-9][0-9]*"
	;;
...)
	feat="..."
	feat_regex="..."
	;;
*)
	feat="$2“
	feat_regex="1"
	;;
esac

due to I think you might add more "$feat" which not simply equal to 1/0 later :)

Others looks good to me.

Thanks,
Zorro

> +
> +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -E -w -c "$feat=$feat_regex")"
>  	if [ "$answer" -ne 0 ]; then
>  		test "$verbose" = "-v" && echo 1
>  		return 0
> diff --git a/tests/xfs/146 b/tests/xfs/146
> index 5516d396bf..123bdff59f 100755
> --- a/tests/xfs/146
> +++ b/tests/xfs/146
> @@ -31,7 +31,7 @@ _scratch_mkfs > $seqres.full
>  _scratch_mount >> $seqres.full
>  
>  blksz=$(_get_block_size $SCRATCH_MNT)
> -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
>  rextblks=$((rextsize / blksz))
>  
>  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> diff --git a/tests/xfs/147 b/tests/xfs/147
> index e21fdd330c..33b3c99633 100755
> --- a/tests/xfs/147
> +++ b/tests/xfs/147
> @@ -29,7 +29,7 @@ _scratch_mkfs -r extsize=256k > $seqres.full
>  _scratch_mount >> $seqres.full
>  
>  blksz=$(_get_block_size $SCRATCH_MNT)
> -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
>  rextblks=$((rextsize / blksz))
>  
>  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> diff --git a/tests/xfs/530 b/tests/xfs/530
> index c960738db7..56f5e7ebdb 100755
> --- a/tests/xfs/530
> +++ b/tests/xfs/530
> @@ -73,8 +73,7 @@ _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
>  formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
>  test "$formatted_blksz" -ne "$dbsize" && \
>  	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> -$XFS_INFO_PROG $SCRATCH_MNT | grep -E -q 'realtime.*blocks=0' && \
> -	_notrun "Filesystem should have a realtime volume"
> +_require_xfs_has_feature "$SCRATCH_MNT" realtime
>  
>  echo "Consume free space"
>  fillerdir=$SCRATCH_MNT/fillerdir
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic
  2022-10-20 12:15   ` Zorro Lang
@ 2022-10-20 21:21     ` Darrick J. Wong
  2022-10-21  3:13       ` Zorro Lang
  0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-20 21:21 UTC (permalink / raw)
  To: Zorro Lang; +Cc: zlang, linux-xfs, fstests

On Thu, Oct 20, 2022 at 08:15:34PM +0800, Zorro Lang wrote:
> On Tue, Oct 18, 2022 at 03:45:38PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > There are a lot of places where we open-code detecting the realtime
> > extent size and extent count of a specific filesystem.  Refactor this
> > into a couple of helpers to clean up the code.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/populate |    2 +-
> >  common/xfs      |   29 +++++++++++++++++++++++++++--
> >  tests/xfs/146   |    2 +-
> >  tests/xfs/147   |    2 +-
> >  tests/xfs/530   |    3 +--
> >  5 files changed, 31 insertions(+), 7 deletions(-)
> > 
> > 
> > diff --git a/common/populate b/common/populate
> > index 23b2fecf69..d9d4c6c300 100644
> > --- a/common/populate
> > +++ b/common/populate
> > @@ -323,7 +323,7 @@ _scratch_xfs_populate() {
> >  	fi
> >  
> >  	# Realtime Reverse-mapping btree
> > -	is_rt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rtextents=[1-9]')"
> > +	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
> >  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
> >  		echo "+ rtrmapbt btree"
> >  		nr="$((blksz * 2 / 32))"
> > diff --git a/common/xfs b/common/xfs
> > index 6445bfd9db..20da537a9d 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -174,6 +174,24 @@ _scratch_mkfs_xfs()
> >  	return $mkfs_status
> >  }
> >  
> > +# Get the number of realtime extents of a mounted filesystem.
> > +_xfs_get_rtextents()
> > +{
> > +	local path="$1"
> > +
> > +	$XFS_INFO_PROG "$path" | grep 'rtextents' | \
> > +		sed -e 's/^.*rtextents=\([0-9]*\).*$/\1/g'
> 
> Same as patch 2/3, how about:
> 
> 	$XFS_INFO_PROG "$path" | sed -n "s/^.*rtextents=\([[:digit:]]*\).*/\1/p"

Actually, if you don't mind, I'd like to retain the hoisted grep/sed
patterns in this patch (and patch 2) and add a new patch that simplifies
the grep|sed pattern throughout common/xfs.

> 
> and ...
> 
> > +}
> > +
> > +# Get the realtime extent size of a mounted filesystem.
> > +_xfs_get_rtextsize()
> > +{
> > +	local path="$1"
> > +
> > +	$XFS_INFO_PROG "$path" | grep 'realtime.*extsz' | \
> > +		sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> 
> ...
> 	$XFS_INFO_PROG "$path" | sed -n "s/^realtime.*extsz=\([[:digit:]]*\).*/\1/p"

But thanks for getting me most of the way there. :)

> > +}
> > +
> >  # Get the size of an allocation unit of a file.  Normally this is just the
> >  # block size of the file, but for realtime files, this is the realtime extent
> >  # size.
> > @@ -191,7 +209,7 @@ _xfs_get_file_block_size()
> >  	while ! $XFS_INFO_PROG "$path" &>/dev/null && [ "$path" != "/" ]; do
> >  		path="$(dirname "$path")"
> >  	done
> > -	$XFS_INFO_PROG "$path" | grep realtime | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> > +	_xfs_get_rtextsize "$path"
> >  }
> >  
> >  # Decide if this path is a file on the realtime device
> > @@ -436,13 +454,20 @@ _require_xfs_crc()
> >  # third option is -v, echo 1 for success and 0 for not.
> >  #
> >  # Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> > +# The feature 'realtime' looks for rtextents > 0.
> >  _xfs_has_feature()
> >  {
> >  	local fs="$1"
> >  	local feat="$2"
> >  	local verbose="$3"
> > +	local feat_regex="1"
> >  
> > -	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> > +	if [ "$feat" = "realtime" ]; then
> > +		feat="rtextents"
> > +		feat_regex="[1-9][0-9]*"
> > +	fi
> 
> How about use this format:
> 
> case "$feat" in
> realtime)
> 	feat="rtextents"
> 	feat_regex="[1-9][0-9]*"
> 	;;
> ...)
> 	feat="..."
> 	feat_regex="..."
> 	;;
> *)
> 	feat="$2“
> 	feat_regex="1"
> 	;;
> esac
> 
> due to I think you might add more "$feat" which not simply equal to 1/0 later :)

I haven't, but I don't mind setting things up for the future.

--D

> Others looks good to me.
> 
> Thanks,
> Zorro
> 
> > +
> > +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -E -w -c "$feat=$feat_regex")"
> >  	if [ "$answer" -ne 0 ]; then
> >  		test "$verbose" = "-v" && echo 1
> >  		return 0
> > diff --git a/tests/xfs/146 b/tests/xfs/146
> > index 5516d396bf..123bdff59f 100755
> > --- a/tests/xfs/146
> > +++ b/tests/xfs/146
> > @@ -31,7 +31,7 @@ _scratch_mkfs > $seqres.full
> >  _scratch_mount >> $seqres.full
> >  
> >  blksz=$(_get_block_size $SCRATCH_MNT)
> > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> >  rextblks=$((rextsize / blksz))
> >  
> >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > diff --git a/tests/xfs/147 b/tests/xfs/147
> > index e21fdd330c..33b3c99633 100755
> > --- a/tests/xfs/147
> > +++ b/tests/xfs/147
> > @@ -29,7 +29,7 @@ _scratch_mkfs -r extsize=256k > $seqres.full
> >  _scratch_mount >> $seqres.full
> >  
> >  blksz=$(_get_block_size $SCRATCH_MNT)
> > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> >  rextblks=$((rextsize / blksz))
> >  
> >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > diff --git a/tests/xfs/530 b/tests/xfs/530
> > index c960738db7..56f5e7ebdb 100755
> > --- a/tests/xfs/530
> > +++ b/tests/xfs/530
> > @@ -73,8 +73,7 @@ _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
> >  formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
> >  test "$formatted_blksz" -ne "$dbsize" && \
> >  	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> > -$XFS_INFO_PROG $SCRATCH_MNT | grep -E -q 'realtime.*blocks=0' && \
> > -	_notrun "Filesystem should have a realtime volume"
> > +_require_xfs_has_feature "$SCRATCH_MNT" realtime
> >  
> >  echo "Consume free space"
> >  fillerdir=$SCRATCH_MNT/fillerdir
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic
  2022-10-20 21:21     ` Darrick J. Wong
@ 2022-10-21  3:13       ` Zorro Lang
  2022-10-21 15:16         ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Zorro Lang @ 2022-10-21  3:13 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs, fstests

On Thu, Oct 20, 2022 at 02:21:08PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 20, 2022 at 08:15:34PM +0800, Zorro Lang wrote:
> > On Tue, Oct 18, 2022 at 03:45:38PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > There are a lot of places where we open-code detecting the realtime
> > > extent size and extent count of a specific filesystem.  Refactor this
> > > into a couple of helpers to clean up the code.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  common/populate |    2 +-
> > >  common/xfs      |   29 +++++++++++++++++++++++++++--
> > >  tests/xfs/146   |    2 +-
> > >  tests/xfs/147   |    2 +-
> > >  tests/xfs/530   |    3 +--
> > >  5 files changed, 31 insertions(+), 7 deletions(-)
> > > 
> > > 
> > > diff --git a/common/populate b/common/populate
> > > index 23b2fecf69..d9d4c6c300 100644
> > > --- a/common/populate
> > > +++ b/common/populate
> > > @@ -323,7 +323,7 @@ _scratch_xfs_populate() {
> > >  	fi
> > >  
> > >  	# Realtime Reverse-mapping btree
> > > -	is_rt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rtextents=[1-9]')"
> > > +	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
> > >  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
> > >  		echo "+ rtrmapbt btree"
> > >  		nr="$((blksz * 2 / 32))"
> > > diff --git a/common/xfs b/common/xfs
> > > index 6445bfd9db..20da537a9d 100644
> > > --- a/common/xfs
> > > +++ b/common/xfs
> > > @@ -174,6 +174,24 @@ _scratch_mkfs_xfs()
> > >  	return $mkfs_status
> > >  }
> > >  
> > > +# Get the number of realtime extents of a mounted filesystem.
> > > +_xfs_get_rtextents()
> > > +{
> > > +	local path="$1"
> > > +
> > > +	$XFS_INFO_PROG "$path" | grep 'rtextents' | \
> > > +		sed -e 's/^.*rtextents=\([0-9]*\).*$/\1/g'
> > 
> > Same as patch 2/3, how about:
> > 
> > 	$XFS_INFO_PROG "$path" | sed -n "s/^.*rtextents=\([[:digit:]]*\).*/\1/p"
> 
> Actually, if you don't mind, I'd like to retain the hoisted grep/sed
> patterns in this patch (and patch 2) and add a new patch that simplifies
> the grep|sed pattern throughout common/xfs.

Sure, that makes sense too, I'm OK to have one more patch. Actually that's
not a necessary change, I don't know if it's worth being a individual patch.
Anyway, I have no objection on that:)

Thanks,
Zorro

> 
> > 
> > and ...
> > 
> > > +}
> > > +
> > > +# Get the realtime extent size of a mounted filesystem.
> > > +_xfs_get_rtextsize()
> > > +{
> > > +	local path="$1"
> > > +
> > > +	$XFS_INFO_PROG "$path" | grep 'realtime.*extsz' | \
> > > +		sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> > 
> > ...
> > 	$XFS_INFO_PROG "$path" | sed -n "s/^realtime.*extsz=\([[:digit:]]*\).*/\1/p"
> 
> But thanks for getting me most of the way there. :)
> 
> > > +}
> > > +
> > >  # Get the size of an allocation unit of a file.  Normally this is just the
> > >  # block size of the file, but for realtime files, this is the realtime extent
> > >  # size.
> > > @@ -191,7 +209,7 @@ _xfs_get_file_block_size()
> > >  	while ! $XFS_INFO_PROG "$path" &>/dev/null && [ "$path" != "/" ]; do
> > >  		path="$(dirname "$path")"
> > >  	done
> > > -	$XFS_INFO_PROG "$path" | grep realtime | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> > > +	_xfs_get_rtextsize "$path"
> > >  }
> > >  
> > >  # Decide if this path is a file on the realtime device
> > > @@ -436,13 +454,20 @@ _require_xfs_crc()
> > >  # third option is -v, echo 1 for success and 0 for not.
> > >  #
> > >  # Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> > > +# The feature 'realtime' looks for rtextents > 0.
> > >  _xfs_has_feature()
> > >  {
> > >  	local fs="$1"
> > >  	local feat="$2"
> > >  	local verbose="$3"
> > > +	local feat_regex="1"
> > >  
> > > -	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> > > +	if [ "$feat" = "realtime" ]; then
> > > +		feat="rtextents"
> > > +		feat_regex="[1-9][0-9]*"
> > > +	fi
> > 
> > How about use this format:
> > 
> > case "$feat" in
> > realtime)
> > 	feat="rtextents"
> > 	feat_regex="[1-9][0-9]*"
> > 	;;
> > ...)
> > 	feat="..."
> > 	feat_regex="..."
> > 	;;
> > *)
> > 	feat="$2“
> > 	feat_regex="1"
> > 	;;
> > esac
> > 
> > due to I think you might add more "$feat" which not simply equal to 1/0 later :)
> 
> I haven't, but I don't mind setting things up for the future.
> 
> --D
> 
> > Others looks good to me.
> > 
> > Thanks,
> > Zorro
> > 
> > > +
> > > +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -E -w -c "$feat=$feat_regex")"
> > >  	if [ "$answer" -ne 0 ]; then
> > >  		test "$verbose" = "-v" && echo 1
> > >  		return 0
> > > diff --git a/tests/xfs/146 b/tests/xfs/146
> > > index 5516d396bf..123bdff59f 100755
> > > --- a/tests/xfs/146
> > > +++ b/tests/xfs/146
> > > @@ -31,7 +31,7 @@ _scratch_mkfs > $seqres.full
> > >  _scratch_mount >> $seqres.full
> > >  
> > >  blksz=$(_get_block_size $SCRATCH_MNT)
> > > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> > >  rextblks=$((rextsize / blksz))
> > >  
> > >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > > diff --git a/tests/xfs/147 b/tests/xfs/147
> > > index e21fdd330c..33b3c99633 100755
> > > --- a/tests/xfs/147
> > > +++ b/tests/xfs/147
> > > @@ -29,7 +29,7 @@ _scratch_mkfs -r extsize=256k > $seqres.full
> > >  _scratch_mount >> $seqres.full
> > >  
> > >  blksz=$(_get_block_size $SCRATCH_MNT)
> > > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> > >  rextblks=$((rextsize / blksz))
> > >  
> > >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > > diff --git a/tests/xfs/530 b/tests/xfs/530
> > > index c960738db7..56f5e7ebdb 100755
> > > --- a/tests/xfs/530
> > > +++ b/tests/xfs/530
> > > @@ -73,8 +73,7 @@ _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
> > >  formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
> > >  test "$formatted_blksz" -ne "$dbsize" && \
> > >  	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> > > -$XFS_INFO_PROG $SCRATCH_MNT | grep -E -q 'realtime.*blocks=0' && \
> > > -	_notrun "Filesystem should have a realtime volume"
> > > +_require_xfs_has_feature "$SCRATCH_MNT" realtime
> > >  
> > >  echo "Consume free space"
> > >  fillerdir=$SCRATCH_MNT/fillerdir
> > > 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic
  2022-10-21  3:13       ` Zorro Lang
@ 2022-10-21 15:16         ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-10-21 15:16 UTC (permalink / raw)
  To: Zorro Lang; +Cc: linux-xfs, fstests

On Fri, Oct 21, 2022 at 11:13:48AM +0800, Zorro Lang wrote:
> On Thu, Oct 20, 2022 at 02:21:08PM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 20, 2022 at 08:15:34PM +0800, Zorro Lang wrote:
> > > On Tue, Oct 18, 2022 at 03:45:38PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > There are a lot of places where we open-code detecting the realtime
> > > > extent size and extent count of a specific filesystem.  Refactor this
> > > > into a couple of helpers to clean up the code.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  common/populate |    2 +-
> > > >  common/xfs      |   29 +++++++++++++++++++++++++++--
> > > >  tests/xfs/146   |    2 +-
> > > >  tests/xfs/147   |    2 +-
> > > >  tests/xfs/530   |    3 +--
> > > >  5 files changed, 31 insertions(+), 7 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/common/populate b/common/populate
> > > > index 23b2fecf69..d9d4c6c300 100644
> > > > --- a/common/populate
> > > > +++ b/common/populate
> > > > @@ -323,7 +323,7 @@ _scratch_xfs_populate() {
> > > >  	fi
> > > >  
> > > >  	# Realtime Reverse-mapping btree
> > > > -	is_rt="$($XFS_INFO_PROG "${SCRATCH_MNT}" | grep -c 'rtextents=[1-9]')"
> > > > +	is_rt="$(_xfs_get_rtextents "$SCRATCH_MNT")"
> > > >  	if [ $is_rmapbt -gt 0 ] && [ $is_rt -gt 0 ]; then
> > > >  		echo "+ rtrmapbt btree"
> > > >  		nr="$((blksz * 2 / 32))"
> > > > diff --git a/common/xfs b/common/xfs
> > > > index 6445bfd9db..20da537a9d 100644
> > > > --- a/common/xfs
> > > > +++ b/common/xfs
> > > > @@ -174,6 +174,24 @@ _scratch_mkfs_xfs()
> > > >  	return $mkfs_status
> > > >  }
> > > >  
> > > > +# Get the number of realtime extents of a mounted filesystem.
> > > > +_xfs_get_rtextents()
> > > > +{
> > > > +	local path="$1"
> > > > +
> > > > +	$XFS_INFO_PROG "$path" | grep 'rtextents' | \
> > > > +		sed -e 's/^.*rtextents=\([0-9]*\).*$/\1/g'
> > > 
> > > Same as patch 2/3, how about:
> > > 
> > > 	$XFS_INFO_PROG "$path" | sed -n "s/^.*rtextents=\([[:digit:]]*\).*/\1/p"
> > 
> > Actually, if you don't mind, I'd like to retain the hoisted grep/sed
> > patterns in this patch (and patch 2) and add a new patch that simplifies
> > the grep|sed pattern throughout common/xfs.
> 
> Sure, that makes sense too, I'm OK to have one more patch. Actually that's
> not a necessary change, I don't know if it's worth being a individual patch.
> Anyway, I have no objection on that:)

It's worth a separate patch because (a) people can see the transition
from one style (grep|sed) to another and (b) I converted all the other
instances that I found under common/*.

--D

> Thanks,
> Zorro
> 
> > 
> > > 
> > > and ...
> > > 
> > > > +}
> > > > +
> > > > +# Get the realtime extent size of a mounted filesystem.
> > > > +_xfs_get_rtextsize()
> > > > +{
> > > > +	local path="$1"
> > > > +
> > > > +	$XFS_INFO_PROG "$path" | grep 'realtime.*extsz' | \
> > > > +		sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> > > 
> > > ...
> > > 	$XFS_INFO_PROG "$path" | sed -n "s/^realtime.*extsz=\([[:digit:]]*\).*/\1/p"
> > 
> > But thanks for getting me most of the way there. :)
> > 
> > > > +}
> > > > +
> > > >  # Get the size of an allocation unit of a file.  Normally this is just the
> > > >  # block size of the file, but for realtime files, this is the realtime extent
> > > >  # size.
> > > > @@ -191,7 +209,7 @@ _xfs_get_file_block_size()
> > > >  	while ! $XFS_INFO_PROG "$path" &>/dev/null && [ "$path" != "/" ]; do
> > > >  		path="$(dirname "$path")"
> > > >  	done
> > > > -	$XFS_INFO_PROG "$path" | grep realtime | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g'
> > > > +	_xfs_get_rtextsize "$path"
> > > >  }
> > > >  
> > > >  # Decide if this path is a file on the realtime device
> > > > @@ -436,13 +454,20 @@ _require_xfs_crc()
> > > >  # third option is -v, echo 1 for success and 0 for not.
> > > >  #
> > > >  # Starting with xfsprogs 4.17, this also works for unmounted filesystems.
> > > > +# The feature 'realtime' looks for rtextents > 0.
> > > >  _xfs_has_feature()
> > > >  {
> > > >  	local fs="$1"
> > > >  	local feat="$2"
> > > >  	local verbose="$3"
> > > > +	local feat_regex="1"
> > > >  
> > > > -	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -w -c "$feat=1")"
> > > > +	if [ "$feat" = "realtime" ]; then
> > > > +		feat="rtextents"
> > > > +		feat_regex="[1-9][0-9]*"
> > > > +	fi
> > > 
> > > How about use this format:
> > > 
> > > case "$feat" in
> > > realtime)
> > > 	feat="rtextents"
> > > 	feat_regex="[1-9][0-9]*"
> > > 	;;
> > > ...)
> > > 	feat="..."
> > > 	feat_regex="..."
> > > 	;;
> > > *)
> > > 	feat="$2“
> > > 	feat_regex="1"
> > > 	;;
> > > esac
> > > 
> > > due to I think you might add more "$feat" which not simply equal to 1/0 later :)
> > 
> > I haven't, but I don't mind setting things up for the future.
> > 
> > --D
> > 
> > > Others looks good to me.
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > > +
> > > > +	local answer="$($XFS_INFO_PROG "$fs" 2>&1 | grep -E -w -c "$feat=$feat_regex")"
> > > >  	if [ "$answer" -ne 0 ]; then
> > > >  		test "$verbose" = "-v" && echo 1
> > > >  		return 0
> > > > diff --git a/tests/xfs/146 b/tests/xfs/146
> > > > index 5516d396bf..123bdff59f 100755
> > > > --- a/tests/xfs/146
> > > > +++ b/tests/xfs/146
> > > > @@ -31,7 +31,7 @@ _scratch_mkfs > $seqres.full
> > > >  _scratch_mount >> $seqres.full
> > > >  
> > > >  blksz=$(_get_block_size $SCRATCH_MNT)
> > > > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > > > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> > > >  rextblks=$((rextsize / blksz))
> > > >  
> > > >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > > > diff --git a/tests/xfs/147 b/tests/xfs/147
> > > > index e21fdd330c..33b3c99633 100755
> > > > --- a/tests/xfs/147
> > > > +++ b/tests/xfs/147
> > > > @@ -29,7 +29,7 @@ _scratch_mkfs -r extsize=256k > $seqres.full
> > > >  _scratch_mount >> $seqres.full
> > > >  
> > > >  blksz=$(_get_block_size $SCRATCH_MNT)
> > > > -rextsize=$($XFS_INFO_PROG $SCRATCH_MNT | grep realtime.*extsz | sed -e 's/^.*extsz=\([0-9]*\).*$/\1/g')
> > > > +rextsize=$(_xfs_get_rtextsize "$SCRATCH_MNT")
> > > >  rextblks=$((rextsize / blksz))
> > > >  
> > > >  echo "blksz $blksz rextsize $rextsize rextblks $rextblks" >> $seqres.full
> > > > diff --git a/tests/xfs/530 b/tests/xfs/530
> > > > index c960738db7..56f5e7ebdb 100755
> > > > --- a/tests/xfs/530
> > > > +++ b/tests/xfs/530
> > > > @@ -73,8 +73,7 @@ _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
> > > >  formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
> > > >  test "$formatted_blksz" -ne "$dbsize" && \
> > > >  	_notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> > > > -$XFS_INFO_PROG $SCRATCH_MNT | grep -E -q 'realtime.*blocks=0' && \
> > > > -	_notrun "Filesystem should have a realtime volume"
> > > > +_require_xfs_has_feature "$SCRATCH_MNT" realtime
> > > >  
> > > >  echo "Consume free space"
> > > >  fillerdir=$SCRATCH_MNT/fillerdir
> > > > 
> > 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-10-21 15:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-18 22:45 [PATCHSET v23.1 0/3] fstests: refactor xfs geometry computation Darrick J. Wong
2022-10-18 22:45 ` [PATCH 1/3] xfs: refactor filesystem feature detection logic Darrick J. Wong
2022-10-20 12:26   ` Zorro Lang
2022-10-20 21:03     ` Darrick J. Wong
2022-10-18 22:45 ` [PATCH 2/3] xfs: refactor filesystem directory block size extraction logic Darrick J. Wong
2022-10-20  6:07   ` Zorro Lang
2022-10-20 21:07     ` Darrick J. Wong
2022-10-18 22:45 ` [PATCH 3/3] xfs: refactor filesystem realtime geometry detection logic Darrick J. Wong
2022-10-20 12:15   ` Zorro Lang
2022-10-20 21:21     ` Darrick J. Wong
2022-10-21  3:13       ` Zorro Lang
2022-10-21 15:16         ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.