Atomic write test fixes & refactoring

Add a generic helper function to get the sysfs queue path for block devices, properly handling partitions. Partitions don't have their own queue directory in sysfs - they inherit from their parent device.

This helper checks if a device is a partition and returns the parent device's queue path accordingly, making it easier to access queue attributes for both whole disks and partitions.

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 common/rc | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

common/rc +24 -0

index 5fe44e21..d7db5db1 100644
@@ -5172,6 +5172,30 @@ _sysfs_dev()
 	echo /sys/dev/block/$maj:$min
 }
 
+# Get the sysfs queue path for a block device, handling partitions correctly.
+_sysfs_queue_path()
+{
+	local dev parent
+	dev=$(_short_dev "$1")
+
+	# For partitions, queue details are in the parent device's sysfs dir
+	if [ -e "/sys/class/block/$dev/partition" ]; then
+		parent=$(basename "$(readlink -f /sys/class/block/$dev/..)")
+	else
+		parent="$dev"
+	fi
+
+	local queue_path="/sys/block/$parent/queue"
+
+	# Verify the path exists before returning
+	if [ -e "$queue_path" ]; then
+		echo "$queue_path"
+		return 0
+	else
+		return 1
+	fi
+}
+
 # Get the minimum block size of a file.  Usually this is the
 # minimum fs block size, but some filesystems (ocfs2) do block
 # mappings in larger units.

Signature · 1 line

2.53.0

This tests checks atomic write limits reported by statx() are same as the ones reported by sysfs, however nvme partitions don't have the /sys/block/nvme0n1p1 style entry and also use the namespace's queue limits for atomic writes. This causes the test to fail because it expects /sys/block/nvme0n*p* to be present.

Hence, use the new _sysfs_queue_path() helper to get the correct atomic write limits

Also, fix a small typo (s/awu_min/awu_max) when printing awu limits to seqres.full

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 tests/generic/765 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

tests/generic/765 +5 -4

index 8c4e0bd0..09be53db 100755
@@ -94,16 +94,17 @@ test_atomic_writes()
     _scratch_unmount
 }
 
-sys_min_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_min_bytes")
-sys_max_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_max_bytes")
+queue_path=$(_sysfs_queue_path $SCRATCH_DEV)
+sys_min_write=$(cat "$queue_path/atomic_write_unit_min_bytes")
+sys_max_write=$(cat "$queue_path/atomic_write_unit_max_bytes")
 
 bdev_min_write=$(_get_atomic_write_unit_min $SCRATCH_DEV)
 bdev_max_write=$(_get_atomic_write_unit_max $SCRATCH_DEV)
 
 echo "sysfs awu_min $sys_min_write" >> $seqres.full
-echo "sysfs awu_min $sys_max_write" >> $seqres.full
+echo "sysfs awu_max $sys_max_write" >> $seqres.full
 echo "bdev awu_min $bdev_min_write" >> $seqres.full
-echo "bdev awu_min $bdev_max_write" >> $seqres.full
+echo "bdev awu_max $bdev_max_write" >> $seqres.full
 
 # Test that statx atomic values are the same as sysfs values
 if [ "$sys_min_write" -ne "$bdev_min_write" ]; then

Signature · 1 line

2.53.0

We use i as the iteration variable in the main test loop as well as some internal loops. Due to this clash, $i variable of main test loops was getting modified by the following loop in prep_mixed_mapping().

	for ((i=0; i<num_blocks; i++)); do

If num_blocks is less than 10 (example ext4 with blocksize 4k and cluster size 8k) i would always be set as 3 and the main loop would never exit. Use local variable to fix this and while we are at it, rename it for clarity.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 tests/generic/775 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/775 +2 -1

index 2a4287bb..6dc005a0 100755
@@ -41,7 +41,8 @@ prep_mixed_mapping() {
 
 	local operations=("W" "H" "U")
 	local num_blocks=$((awu_max / blksz))
-	for ((i=0; i<num_blocks; i++)); do
+	local blkno
+	for ((blkno=0; blkno<num_blocks; blkno++)); do
 		local index=$((RANDOM % ${#operations[@]}))
 		local map="${operations[$index]}"
 		local mapping="${mapping}${map}"

Signature · 1 line

2.53.0

The sysfs queue dir for partitions is present in their parent device's sysfs dir. This requires extra checks when trying to reading queue data from sysfs. Currently, fstests open codes this handling or straight up ignores it in some places.

Avoid this by converting all remaining direct sysfs queue accesses to use the _sysfs_queue_path helper function. This ensures proper handling of partitions across the entire codebase and provides a consistent interface for accessing queue attributes.

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 common/encrypt  |  7 ++-----
 common/rc       | 12 ++++++++----
 common/report   | 20 +++++++++++---------
 common/zoned    |  5 +++--
 tests/btrfs/076 | 11 +++++++----
 tests/btrfs/237 |  5 +++--
 tests/btrfs/273 |  4 +++-
 tests/btrfs/283 |  4 +++-
 8 files changed, 40 insertions(+), 28 deletions(-)

common/encrypt +2 -5

index f2687631..c9e0853d 100644
@@ -173,11 +173,8 @@ _require_hw_wrapped_key_support()
 	local dev=$1
 
 	echo "Checking for HW-wrapped key support on $dev" >> $seqres.full
-	local sysfs_dir=$(_sysfs_dev $dev)
-	if [ ! -e $sysfs_dir/queue ]; then
-		sysfs_dir=$sysfs_dir/..
-	fi
-	if [ ! -e $sysfs_dir/queue/crypto/hw_wrapped_keys ]; then
+	local queue_path=$(_sysfs_queue_path $dev)
+	if [ $? -ne 0 ] || [ ! -e $queue_path/crypto/hw_wrapped_keys ]; then
 		_notrun "$dev doesn't support hardware-wrapped inline encryption keys"
 	fi
 

common/rc +8 -4

index d7db5db1..9632b211 100644
@@ -2502,7 +2502,11 @@ __scratch_dev_has_dax()
 {
 	local sysfs="/sys/block/$(_short_dev $SCRATCH_DEV)"
 	test -e "${sysfs}/dax" && return 0
-	test "$(cat "${sysfs}/queue/dax" 2>/dev/null)" = "1" && return 0
+
+	local queue_path=$(_sysfs_queue_path $SCRATCH_DEV)
+	if [ $? -eq 0 ]; then
+		test "$(cat "${queue_path}/dax" 2>/dev/null)" = "1" && return 0
+	fi
 
 	return 1
 }

@@ -2562,10 +2566,10 @@ _zone_type()
 		echo "Usage: _zone_type <device>"
 		_exit 1
 	fi
-	local sdev=`_short_dev $target`
 
-	if [ -e /sys/block/${sdev}/queue/zoned ]; then
-		cat /sys/block/${sdev}/queue/zoned
+	local queue_path=$(_sysfs_queue_path $target)
+	if [ $? -eq 0 ] && [ -e "$queue_path/zoned" ]; then
+		cat "$queue_path/zoned"
 	else
 		echo none
 	fi

common/report +11 -9

index 7128bbeb..0d9ad78c 100644
@@ -37,16 +37,18 @@ __generate_blockdev_report_vars() {
 
 	test -z "$bdev" && return
 	test -b "$bdev" || return
-	local sysfs_bdev="$(_sysfs_dev "$bdev")"
-
 	REPORT_VARS["${bdev_var}_SIZE_KB"]="$(( "$(blockdev --getsz "$bdev")" / 2 ))"
-	REPORT_VARS["${bdev_var}_ROTATIONAL"]="$(cat "$sysfs_bdev/queue/rotational" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_DAX"]="$(cat "$sysfs_bdev/queue/dax" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_DISCARD"]="$(sed -e 's/[1-9][0-9]*/1/g' "$sysfs_bdev/queue/discard_max_bytes" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_WRITE_ZEROES"]="$(sed -e 's/[1-9][0-9]*/1/g' "$sysfs_bdev/queue/write_zeroes_max_bytes" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_PHYS_BLOCK_BYTES"]="$(cat "$sysfs_bdev/queue/physical_block_size" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_LBA_BYTES"]="$(cat "$sysfs_bdev/queue/logical_block_size" 2>/dev/null)"
-	REPORT_VARS["${bdev_var}_ZONES"]="$(cat "$sysfs_bdev/queue/nr_zones" 2>/dev/null)"
+
+	local queue_path="$(_sysfs_queue_path "$bdev")"
+	if [ $? -eq 0 ]; then
+		REPORT_VARS["${bdev_var}_ROTATIONAL"]="$(cat "$queue_path/rotational" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_DAX"]="$(cat "$queue_path/dax" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_DISCARD"]="$(sed -e 's/[1-9][0-9]*/1/g' "$queue_path/discard_max_bytes" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_WRITE_ZEROES"]="$(sed -e 's/[1-9][0-9]*/1/g' "$queue_path/write_zeroes_max_bytes" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_PHYS_BLOCK_BYTES"]="$(cat "$queue_path/physical_block_size" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_LBA_BYTES"]="$(cat "$queue_path/logical_block_size" 2>/dev/null)"
+		REPORT_VARS["${bdev_var}_ZONES"]="$(cat "$queue_path/nr_zones" 2>/dev/null)"
+	fi
 }
 
 __import_report_vars() {

common/zoned +3 -2

index 88b81de5..89d0a061 100644
@@ -17,8 +17,9 @@ _filter_blkzone_report()
 
 _require_limited_active_zones() {
     local dev=$1
-    local sysfs=$(_sysfs_dev ${dev})
-    local attr="${sysfs}/queue/max_active_zones"
+    local queue_path=$(_sysfs_queue_path ${dev})
+    [ $? -ne 0 ] && _notrun "cannot find queue path: $queue_path"
+    local attr="${queue_path}/max_active_zones"
 
     [ -e "${attr}" ] || _notrun "cannot find queue/max_active_zones. Maybe non-zoned device?"
     if [ $(cat "${attr}") == 0 ]; then

tests/btrfs/076 +7 -4

index c148406f..0a1e660d 100755
@@ -28,10 +28,13 @@ _fixed_by_kernel_commit 4bcbb3325513 \
 # An extent size can be up to BTRFS_MAX_UNCOMPRESSED
 max_extent_size=$(( 128 * 1024 ))
 if _scratch_btrfs_is_zoned; then
-	zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes")
-	if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then
-		# Round down to PAGE_SIZE
-		max_extent_size=$(( $zone_append_max / 4096 * 4096 ))
+	queue_path=$(_sysfs_queue_path $SCRATCH_DEV)
+	if [ $? -eq 0 ]; then
+		zone_append_max=$(cat "$queue_path/zone_append_max_bytes")
+		if [[ $zone_append_max -gt 0 && $zone_append_max -lt $max_extent_size ]]; then
+			# Round down to PAGE_SIZE
+			max_extent_size=$(( $zone_append_max / 4096 * 4096 ))
+		fi
 	fi
 fi
 file_size=$(( 10 * 1024 * 1024 ))

tests/btrfs/237 +3 -2

index 675f4c42..07a56336 100755
@@ -44,8 +44,9 @@ get_data_bg_physical()
 $BLKZONE_PROG report $SCRATCH_DEV | grep -q -e "nw" && \
 	_notrun "test is unreliable on devices with conventional zones"
 
-sdev="$(_short_dev $SCRATCH_DEV)"
-zone_size=$(($(cat /sys/block/${sdev}/queue/chunk_sectors) << 9))
+queue_path=$(_sysfs_queue_path $SCRATCH_DEV) || \
+    _notrun "Failed to get queue path: $queue_path"
+zone_size=$(($(cat "$queue_path/chunk_sectors") << 9))
 fssize=$((zone_size * 16))
 devsize=$(($(_get_device_size $SCRATCH_DEV) * 1024))
 # Create a minimal FS to kick the reclaim process

tests/btrfs/273 +3 -1

index 06ae5d53..60c3967b 100755
@@ -38,7 +38,9 @@ _require_btrfs_command inspect-internal dump-tree
 # enabled.
 _require_no_compress
 
-max_active=$(cat $(_sysfs_dev ${SCRATCH_DEV})/queue/max_active_zones)
+queue_path=$(_sysfs_queue_path ${SCRATCH_DEV}) || \
+    _notrun "Failed to get queue path: $queue_path"
+max_active=$(cat "$queue_path/max_active_zones")
 
 # Fill the zones leaving the last 1MB
 fill_active_zones() {

tests/btrfs/283 +3 -1

index f7a8290a..9f89fafb 100755
@@ -26,7 +26,9 @@ _wants_kernel_commit c7499a64dcf6 \
 
 extent_size=$(( 128 * 1024 ))
 if _scratch_btrfs_is_zoned; then
-	zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes")
+	queue_path=$(_sysfs_queue_path $SCRATCH_DEV) ||
+	    _notrun "Couldn't find queue path for zoned device"
+	zone_append_max=$(cat "$queue_path/zone_append_max_bytes")
 	if [[ $zone_append_max -gt 0 && $zone_append_max -lt $extent_size ]]; then
 		_notrun "zone append max $zone_append_max is smaller than wanted extent size $extent_size"
 	fi

Signature · 1 line

2.53.0

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +2 -1

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 
     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"
     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 12 lines · Ojaswin Mujoo

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Quoted 2 lines

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Seems fine, though I think I'd rather mke2fs get patched to fix this.

I'd let the maintainer decide if he wants this, but ... I'm not sure if he's actually going to receive this message. :/

--D

Quoted 22 lines

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +2 -1

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 
     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"
     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 12 lines · Ojaswin Mujoo

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Quoted 2 lines

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Seems fine, though I think I'd rather mke2fs get patched to fix this.

I'd let the maintainer decide if he wants this, but ... I'm not sure if he's actually going to receive this message. :/

--D

Quoted 11 lines

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +0 -0

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 

tests/generic/765 test_atomic_writes() · line 68

Quoted 4 lines

tests/generic/765 +2 -1

     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"

Quoted 7 lines

tests/generic/765 +0 -0

     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 16 lines · Darrick J. Wong

Quoted 14 lines · Ojaswin Mujoo

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Quoted 4 lines

Seems fine, though I think I'd rather mke2fs get patched to fix this.

I'd let the maintainer decide if he wants this, but ... I'm not sure if he's actually going to receive this message. :/

Hi Darrick, yes I'm preparing a quick patch for this. We might still need this so people on older distro who wont have the e2fsprogs fix dont see the failure.

Regards,
ojaswin

Quoted 25 lines

--D

Quoted 22 lines

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +2 -1

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 
     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"
     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 13 lines · Darrick J. Wong

Quoted 12 lines · Ojaswin Mujoo

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Quoted 7 lines

Quoted 2 lines

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Seems fine, though I think I'd rather mke2fs get patched to fix this.

I'd let the maintainer decide if he wants this, but ... I'm not sure if he's actually going to receive this message. :/

Hi Darrick and folks,

I'm back.

I apologize for the silence these days. I had planned to do a release last week, but a sudden big regional layoff and immediate cutoff of all connections caught me completely off guard. Beyond losing my account, I lost everything on the company servers, all my documents (including a book I was halfway through writing), my code, and my entire environment for handling emails, patches, and system workflows. (This is a painful reminder to never leave your things on company server..)

Amidst the shock and grief, I am now struggling to rebuild and reconfigure my local environment from scratch. I have redirected my [email protected] alias to my personal email -- [email protected]. *Please do not send any more emails to my old [email protected] address*.

Starting tomorrow, I will begin reviewing and merging last week's patches bit by bit. I still need long time to get my environment and workflows back in order, in the meantime, I will also be focusing my energy on finding a new job. So I appreciate everyone's patience and understanding.

Quoted 15 lines

--D

Quoted 11 lines

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +0 -0

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 

Quoted 4 lines

tests/generic/765 test_atomic_writes() · line 68

Quoted 4 lines

tests/generic/765 +2 -1

     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"

Now for this patch, I think we can have it for unfixed e2fsprogs testing, as this test case isn't related with this e2fsprogs "bug", except you hope to uncover it by this test case :)

Reviewed-by Zorro Lang <[email protected]>

Quoted 9 lines

Quoted 8 lines

tests/generic/765 +0 -0

     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 17 lines · Ojaswin Mujoo

Quoted 16 lines · Darrick J. Wong

Quoted 14 lines · Ojaswin Mujoo

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

Quoted 8 lines

Quoted 4 lines

Seems fine, though I think I'd rather mke2fs get patched to fix this.

I'd let the maintainer decide if he wants this, but ... I'm not sure if he's actually going to receive this message. :/

Hi Darrick, yes I'm preparing a quick patch for this. We might still need this so people on older distro who wont have the e2fsprogs fix dont see the failure.

Hi Darrick,

Looking at it again, I am just wondering if its really an issue that mke2fs is printing this warning to stderr. Isn't stderr the right place for errors and warning (even non critical ones)?

Futher, mke2fs has multiple instances where we print warnings to stderr, should we go and fix all of them as well?

Regards,
ojaswin

Quoted 29 lines

Regards,
ojaswin

Quoted 25 lines

--D

Quoted 22 lines

---
 tests/generic/765 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +2 -1

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
     local bsize=$1
 
     get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"
     _scratch_mount
 
     test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

Signature · 1 line

2.53.0

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Quoted 2 lines

Futher, mke2fs has multiple instances where we print warnings to stderr, should we go and fix all of them as well?

"stderr" meaning "standard error", I'd say that errors are anything that prohibits the format from completing, and only errors should go there.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

You could also suppress the warnings using the -q option, for example:

mke2fs -Fq -t ext4 -O bigalloc,quota -b 4096 -C 131072 /tmp/foo.img 4G

Quoted 3 lines

Quoted 2 lines

Futher, mke2fs has multiple instances where we print warnings to stderr, should we go and fix all of them as well?

Quoted 2 lines

"stderr" meaning "standard error", I'd say that errors are anything that prohibits the format from completing, and only errors should go there.

Sure, I'll accept those changes. But adding -q will allow the test to pass using older versions of e2fsprogs, while still allowing stderr to go out the expected output.

Fix 2 issues in the tests:

1. Use fs atomic write limits instead of bdev's

The tests use block device's limits instead of limits advertised by filesystem. This can cause failures because block device might advertise a higher maximum than the FS. Fix the tests to use filesystem's limits instead.

2. Fix the test loop range

Fix the test loop start and end so that our calculations for blocksize, clustersize and iosize are correct.

Reported-by Disha Goel <[email protected]>

Reviewed-by "Darrick J. Wong" <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  tests/ext4/061 | 28 ++++++++++++++++++++--------
  tests/ext4/062 | 26 +++++++++++++++++++-------
  2 files changed, 39 insertions(+), 15 deletions(-)

tests/ext4/061 +20 -8

index 1d61c8b0..69ddeaca 100755
@@ -31,10 +31,19 @@ _require_aiodio
  FIO_LOAD=$(($(nproc) * 2 * LOAD_FACTOR))
  SIZE=$((100*1024*1024))
 
-# Calculate fsblocksize as per bdev atomic write units.
-bdev_awu_min=$(_get_atomic_write_unit_min $SCRATCH_DEV)
-bdev_awu_max=$(_get_atomic_write_unit_max $SCRATCH_DEV)
-bs=$(_max 4096 "$bdev_awu_min")
+_scratch_mkfs > /dev/null 2>&1 || \
+	_notrun "mkfs failed"
+_try_scratch_mount || \
+	_notrun "mount failed"
+
+touch $SCRATCH_MNT/f
+
+fs_awu_min=$(_get_atomic_write_unit_min $SCRATCH_MNT/f)
+fs_awu_max=$(_get_atomic_write_unit_max $SCRATCH_MNT/f)
+bs=$(_max 4096 "$fs_awu_min")
+ps=$(_get_page_size)
+
+_scratch_unmount
 
  function create_fio_configs()
  {

@@ -97,10 +106,11 @@ run_test_one() {
  	local cs=$2
  	local iosize=$3
 
+	echo "== Testing: bs=$bs cs=$cs iosize=$iosize ==" >> $seqres.full
+
  	MKFS_OPTIONS="-O bigalloc -b $bs -C $cs"
  	_scratch_mkfs_ext4  >> $seqres.full 2>&1 || return
  	if _try_scratch_mount >> $seqres.full 2>&1; then
-		echo "== Testing: bs=$bs cs=$cs iosize=$iosize ==" >> $seqres.full
 
  		touch $SCRATCH_MNT/f1
  		create_fio_configs $iosize

@@ -127,7 +137,7 @@ run_test() {
  	# cluster sizes above 16 x blocksize are experimental so avoid them
  	# Also, cap cluster size at 128kb to keep it reasonable for large
  	# blocks size
-	max_cs=$(_min $((16 * bs)) "$bdev_awu_max" $((128 * 1024)))
+	max_cs=$(_min $((16 * bs)) "$fs_awu_max" $((128 * 1024)))
 
  	# Fuzz for combinations of blocksize, clustersize and
  	# iosize that cover most of the cases

@@ -145,8 +155,10 @@ fio_out=$tmp.fio.out
  create_fio_configs $bs
  _require_fio $fio_aw_config
 
-for ((bs=$bs; bs <= $(_get_page_size); bs = $bs << 1)); do
-	run_test $bs
+echo "Awu min: $fs_awu_min Awu max: $fs_awu_max" >> $seqres.full
+
+for ((bs=$fs_awu_min; bs <= $(_min ps fs_awu_max); bs = $bs << 1)); do
+	run_test $bs $cs $iosize
  done
 
  # success, all done

tests/ext4/062 +19 -7

index 05cce696..56bf080c 100755
@@ -33,10 +33,19 @@ _require_aiodio
  FSSIZE=$((360*1024*1024))
  FIO_LOAD=$(($(nproc) * LOAD_FACTOR))
 
-# Calculate bs as per bdev atomic write units.
-bdev_awu_min=$(_get_atomic_write_unit_min $SCRATCH_DEV)
-bdev_awu_max=$(_get_atomic_write_unit_max $SCRATCH_DEV)
-bs=$(_max 4096 "$bdev_awu_min")
+_scratch_mkfs > /dev/null 2>&1 || \
+	_notrun "mkfs failed"
+_try_scratch_mount || \
+	_notrun "mount failed"
+
+touch $SCRATCH_MNT/f
+
+fs_awu_min=$(_get_atomic_write_unit_min $SCRATCH_MNT/f)
+fs_awu_max=$(_get_atomic_write_unit_max $SCRATCH_MNT/f)
+bs=$(_max 4096 "$fs_awu_min")
+ps=$(_get_page_size)
+
+_scratch_unmount
 
  function create_fio_configs()
  {

@@ -146,10 +155,11 @@ run_test_one() {
  	local cs=$2
  	local iosize=$3
 
+	echo "Testing: bs=$bs cs=$cs iosize=$iosize" >> $seqres.full
+
  	MKFS_OPTIONS="-O bigalloc -b $bs -C $cs"
  	_scratch_mkfs_sized "$FSSIZE" >> $seqres.full 2>&1 || return
  	if _try_scratch_mount >> $seqres.full 2>&1; then
-		echo "Testing: bs=$bs cs=$cs iosize=$iosize" >> $seqres.full
 
  		touch $SCRATCH_MNT/f1
  		create_fio_configs $iosize

@@ -175,7 +185,7 @@ run_test() {
  	# cluster sizes above 16 x blocksize are experimental so avoid them
  	# Also, cap cluster size at 128kb to keep it reasonable for large
  	# blocks size
-	max_cs=$(_min $((16 * bs)) "$bdev_awu_max" $((128 * 1024)))
+	max_cs=$(_min $((16 * bs)) "$fs_awu_max" $((128 * 1024)))
 
  	# Fuzz for combinations of blocksize, clustersize and
  	# iosize that cover most of the cases

@@ -193,7 +203,9 @@ fio_out=$tmp.fio.out
  create_fio_configs $bs
  _require_fio $fio_aw_config
 
-for ((bs=$bs; bs <= $(_get_page_size); bs = $bs << 1)); do
+echo "Awu min: $fs_awu_min Awu max: $fs_awu_max" >> $seqres.full
+
+for ((bs=$fs_awu_min; bs <= $(_min ps fs_awu_max); bs = $bs << 1)); do
  	run_test $bs $cs $iosize
  done
 

This tests checks atomic write limits reported by statx() are same as the ones reported by sysfs, however nvme partitions don't have the /sys/block/nvme0n1p1 style entry and also use the namespace's queue limits for atomic writes. This causes the test to fail because it expects /sys/block/nvme0n*p* to be present.

Hence, use the new _sysfs_queue_path() helper to get the correct atomic write limits

Also, fix a small typo (s/awu_min/awu_max) when printing awu limits to seqres.full

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  tests/generic/765 | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

tests/generic/765 +5 -4

index 8c4e0bd0..09be53db 100755
@@ -94,16 +94,17 @@ test_atomic_writes()
      _scratch_unmount
  }
 
-sys_min_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_min_bytes")
-sys_max_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_max_bytes")
+queue_path=$(_sysfs_queue_path $SCRATCH_DEV)
+sys_min_write=$(cat "$queue_path/atomic_write_unit_min_bytes")
+sys_max_write=$(cat "$queue_path/atomic_write_unit_max_bytes")
 
  bdev_min_write=$(_get_atomic_write_unit_min $SCRATCH_DEV)
  bdev_max_write=$(_get_atomic_write_unit_max $SCRATCH_DEV)
 
  echo "sysfs awu_min $sys_min_write" >> $seqres.full
-echo "sysfs awu_min $sys_max_write" >> $seqres.full
+echo "sysfs awu_max $sys_max_write" >> $seqres.full
  echo "bdev awu_min $bdev_min_write" >> $seqres.full
-echo "bdev awu_min $bdev_max_write" >> $seqres.full
+echo "bdev awu_max $bdev_max_write" >> $seqres.full
 
  # Test that statx atomic values are the same as sysfs values
  if [ "$sys_min_write" -ne "$bdev_min_write" ]; then

This test validates atomic writes for all possible block sizes. In ext4, for smaller block sizes with configurations like:

export MKFS_OPTIONS="-O bigalloc,quota -b 65536 -C 131072"

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  tests/generic/765 | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/765 +2 -1

index 09be53db..b7cc70f1 100755
@@ -64,7 +64,8 @@ test_atomic_writes()
      local bsize=$1
 
      get_mkfs_opts $bsize
-    _scratch_mkfs $mkfs_opts >> $seqres.full
+    _scratch_mkfs $mkfs_opts &>> $seqres.full || \
+        echo "mkfs $mkfs_opts failed"
      _scratch_mount
 
      test "$FSTYP" = "xfs" && _xfs_force_bdev data $SCRATCH_MNT

We use i as the iteration variable in the main test loop as well as some internal loops. Due to this clash, $i variable of main test loops was getting modified by the following loop in prep_mixed_mapping().

	for ((i=0; i<num_blocks; i++)); do

If num_blocks is less than 10 (example ext4 with blocksize 4k and cluster size 8k) i would always be set as 3 and the main loop would never exit. Use local variable to fix this and while we are at it, rename it for clarity.

Reported-by Disha Goel <[email protected]>

Signed-off-by Ojaswin Mujoo <[email protected]>

---
  tests/generic/775 | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

tests/generic/775 +2 -1

index 2a4287bb..6dc005a0 100755
@@ -41,7 +41,8 @@ prep_mixed_mapping() {
 
  	local operations=("W" "H" "U")
  	local num_blocks=$((awu_max / blksz))
-	for ((i=0; i<num_blocks; i++)); do
+	local blkno
+	for ((blkno=0; blkno<num_blocks; blkno++)); do
  		local index=$((RANDOM % ${#operations[@]}))
  		local map="${operations[$index]}"
  		local mapping="${mapping}${map}"

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

You could also suppress the warnings using the -q option, for example:

mke2fs -Fq -t ext4 -O bigalloc,quota -b 4096 -C 131072 /tmp/foo.img 4G

Quoted 3 lines

Quoted 2 lines

Futher, mke2fs has multiple instances where we print warnings to stderr, should we go and fix all of them as well?

Quoted 2 lines

"stderr" meaning "standard error", I'd say that errors are anything that prohibits the format from completing, and only errors should go there.

Sure, I'll accept those changes. But adding -q will allow the test to pass using older versions of e2fsprogs, while still allowing stderr to go out the expected output.

- Ted

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The output can get corrupted with warnings like below because clustersize more than 16xbs is experimental:

+ 16 times the block size is considered experimental

Hence pipe these to seqres.full to avoid false negatives.

You could also suppress the warnings using the -q option, for example:

mke2fs -Fq -t ext4 -O bigalloc,quota -b 4096 -C 131072 /tmp/foo.img 4G

Quoted 3 lines

Quoted 2 lines

Futher, mke2fs has multiple instances where we print warnings to stderr, should we go and fix all of them as well?

Quoted 2 lines

"stderr" meaning "standard error", I'd say that errors are anything that prohibits the format from completing, and only errors should go there.

Sure, I'll accept those changes. But adding -q will allow the test to pass using older versions of e2fsprogs, while still allowing stderr to go out the expected output.

- Ted