[PATCH blktests v3 0/6] enable bs > ps device testing

[PATCH blktests v3 0/6] enable bs > ps device testing

[Luis Chamberlain]

This v3 series addresses the feedback from the v2 series [0] and
makes some minor new changes, the change are:

  - Fixes all shellcheck complaints
  - Addresses spacing / tabs fixes
  - Adds _test_dev_suits_xfs() as suggested by Shinichiro and makes
    tests which require this depend on it
  - Clamps _min_io() to 4k as well for backward compatibility
  - Few minor enhancements to help capture up error messages from
    mkfs from block/032

This goes tested against a 64k sector size NVMe drive, and also
using ./check so regular loopback devices are used. This helps
test 64k sector devices, patches for which have been posted [1].
                                                                                                                                                                                              
[0] https://lkml.kernel.org/r/20250204225729.422949-1-mcgrof@kernel.org
[1] https://lkml.kernel.org/r/20250204231209.429356-1-mcgrof@kernel.org

Luis Chamberlain (6):
  common/xfs: ignore first umount error on _xfs_mkfs_and_mount()
  block/032: make error messages clearer if mkfs or mount fails
  common: add and use min io for fio
  common/xfs: use min io for fs blocksize
  tests: use test device min io to support bs > ps
  common/xfs: add _test_dev_suits_xfs() to verify logical block size
    will work

 common/fio      | 26 ++++++++++++++++++++++++--
 common/rc       | 24 ++++++++++++++++++++++++
 common/xfs      | 23 +++++++++++++++++++++--
 tests/block/003 |  6 +++++-
 tests/block/007 |  5 ++++-
 tests/block/032 |  7 ++++---
 tests/nvme/012  |  1 +
 tests/nvme/035  |  1 +
 tests/nvme/049  | 15 +++++++++++----
 9 files changed, 95 insertions(+), 13 deletions(-)

-- 
2.45.2

[PATCH blktests v3 1/6] common/xfs: ignore first umount error on _xfs_mkfs_and_mount()

[Luis Chamberlain]

We want to help capture error messages with _xfs_mkfs_and_mount() on
$FULL, to do that we should avoid spamming error messages for things
which we know are not fatal. Such is the case of when we try to
mkfs a filesystem but before that try to umount the target path.
The first umount is just for sanity, so ignore the error messages from
it.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 common/xfs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/xfs b/common/xfs
index 569770fecd53..67a3b8a97391 100644
--- a/common/xfs
+++ b/common/xfs
@@ -15,7 +15,7 @@ _xfs_mkfs_and_mount() {
 	local mount_dir=$2
 
 	mkdir -p "${mount_dir}"
-	umount "${mount_dir}"
+	umount "${mount_dir}" >/dev/null 2>&1
 	mkfs.xfs -l size=64m -f "${bdev}" || return $?
 	mount "${bdev}" "${mount_dir}"
 }
-- 
2.45.2

[PATCH blktests v3 2/6] block/032: make error messages clearer if mkfs or mount fails

[Luis Chamberlain]

If block/032 fails at mkfs we want to know why, so propagate
error messages. While at it, enhance the test to also propagate
the error return from mount and remove the odd sleep for a udevadm
settle as that's the only thing I can think of we need to wait for
here.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/block/032 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/block/032 b/tests/block/032
index 8975879660d7..fc6d1a51dcad 100755
--- a/tests/block/032
+++ b/tests/block/032
@@ -25,10 +25,10 @@ test() {
 	fi
 
 	mkdir -p "${TMPDIR}/mnt"
-	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" > /dev/null 2>&1
+	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> $FULL || return $?
 	echo 1 > "/sys/block/${SCSI_DEBUG_DEVICES[0]}/device/delete"
-	sleep 2
-	umount "${TMPDIR}/mnt"
+	udevadm settle
+	umount "${TMPDIR}/mnt" || return $?
 
 	_exit_scsi_debug
 
-- 
2.45.2

[PATCH blktests v3 3/6] common: add and use min io for fio

[Luis Chamberlain]

When using fio we should not issue IOs smaller than the device supports.
Today a lot of places have in place 4k, but soon we will have devices
which support bs > ps. For those devices we should check the minimum
supported IO.

However, since we also have a min optimal IO, we might as well use that
as well. By using this we can also leverage the same lookup with stat
whether or not the target file is a block device or a file.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 common/fio | 26 ++++++++++++++++++++++++--
 common/rc  | 24 ++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/common/fio b/common/fio
index b9ea087fc6c5..d147214f3d16 100644
--- a/common/fio
+++ b/common/fio
@@ -189,15 +189,37 @@ _run_fio() {
 	return $rc
 }
 
+_fio_opts_to_min_io() {
+	local arg path
+	local -i min_io=4096
+
+        for arg in "$@"; do
+		[[ "$arg" =~ ^--filename= || "$arg" =~ --directory= ]] || continue
+		path="${arg##*=}"
+		min_io=$(_min_io "$path")
+		# Keep 4K minimum IO size for historical consistency
+		((min_io < 4096)) && min_io=4096
+		break
+	done
+
+	echo "$min_io"
+}
+
 # Wrapper around _run_fio used if you need some I/O but don't really care much
 # about the details
 _run_fio_rand_io() {
-	_run_fio --bs=4k --rw=randread --norandommap --numjobs="$(nproc)" \
+	local bs
+
+	bs=$(_fio_opts_to_min_io "$@") || return 1
+	_run_fio --bs="$bs" --rw=randread --norandommap --numjobs="$(nproc)" \
 		--name=reads --direct=1 "$@"
 }
 
 _run_fio_verify_io() {
-	_run_fio --name=verify --rw=randwrite --direct=1 --ioengine=libaio --bs=4k \
+	local bs
+
+	bs=$(_fio_opts_to_min_io "$@") || return 1
+	_run_fio --name=verify --rw=randwrite --direct=1 --ioengine=libaio --bs="$bs" \
 		--iodepth=16 --verify=crc32c --verify_state_save=0 "$@"
 }
 
diff --git a/common/rc b/common/rc
index a7e899cfb419..6e7bddc844bf 100644
--- a/common/rc
+++ b/common/rc
@@ -400,6 +400,30 @@ _test_dev_is_partition() {
 	[[ -n ${TEST_DEV_PART_SYSFS} ]]
 }
 
+_min_io() {
+	local path_or_dev=$1
+	local min_io=4096
+	if [ -z "$path_or_dev" ]; then
+		echo "path for min_io does not exist"
+		return 1
+	fi
+
+	if [ -c "$path_or_dev" ]; then
+		if [[ "$path_or_dev" == /dev/ng* ]]; then
+			path_or_dev="${path_or_dev/ng/nvme}"
+		fi
+	fi
+
+	if [ -e "$path_or_dev" ]; then
+		min_io=$(stat --printf=%o "$path_or_dev")
+		((min_io < 4096)) && min_io=4096
+		echo "$min_io"
+	else
+		echo "Error: '$path_or_dev' does not exist or is not accessible"
+		return 1
+	fi
+}
+
 # Return max open zones or max active zones of the test target device.
 # If the device has both, return smaller value.
 _test_dev_max_open_active_zones() {
-- 
2.45.2

[PATCH blktests v3 4/6] common/xfs: use min io for fs blocksize

[Luis Chamberlain]

Use the min io for the target block size. Likewise we need to increase
the log size if using a bs > 4096.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 common/xfs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/common/xfs b/common/xfs
index 67a3b8a97391..226fdbd1c83f 100644
--- a/common/xfs
+++ b/common/xfs
@@ -13,10 +13,18 @@ _have_xfs() {
 _xfs_mkfs_and_mount() {
 	local bdev=$1
 	local mount_dir=$2
+	local bs
+	local xfs_logsize="64m"
+
+	bs=$(_min_io "$bdev")
+
+	if [[ $bs -gt 4096 ]]; then
+		xfs_logsize="128m"
+	fi
 
 	mkdir -p "${mount_dir}"
 	umount "${mount_dir}" >/dev/null 2>&1
-	mkfs.xfs -l size=64m -f "${bdev}" || return $?
+	mkfs.xfs -l size=$xfs_logsize -f "${bdev}" -b size="$bs" || return $?
 	mount "${bdev}" "${mount_dir}"
 }
 
-- 
2.45.2

[PATCH blktests v3 5/6] tests: use test device min io to support bs > ps

[Luis Chamberlain]

When a block device supports a minimum block size > ps we must
ensure we don't issue IOs below what is supported. Just leverage
the min optimal IO to also ensure we use the optimal IO as well.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/block/003 |  6 +++++-
 tests/block/007 |  5 ++++-
 tests/nvme/049  | 15 +++++++++++----
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/tests/block/003 b/tests/block/003
index 2af9b89ec3e5..b0dd1d1fd62b 100755
--- a/tests/block/003
+++ b/tests/block/003
@@ -18,10 +18,14 @@ device_requires() {
 }
 
 test_device() {
+	local test_dev_bs
+
+	test_dev_bs=$(_min_io "$TEST_DEV")
+
 	echo "Running ${TEST_NAME}"
 
 	FIO_PERF_FIELDS=("trim iops")
-	_fio_perf --bsrange=4k-4g --rw=randtrim --norandommap --name=discards \
+	_fio_perf --bsrange="${test_dev_bs}"-4g --rw=randtrim --norandommap --name=discards \
 		--filename="$TEST_DEV" --number_ios=200k
 
 	echo "Test complete"
diff --git a/tests/block/007 b/tests/block/007
index 3b68d0deec35..8043a83a565b 100755
--- a/tests/block/007
+++ b/tests/block/007
@@ -31,13 +31,16 @@ cleanup_fallback_device() {
 }
 
 run_fio_job() {
+	local test_dev_bs
+
+	test_dev_bs=$(_min_io "$TEST_DEV")
 	if _test_dev_is_rotational; then
 		size="32m"
 	else
 		size="1g"
 	fi
 
-	_fio_perf --bs=4k --rw=randread --norandommap --name=reads \
+	_fio_perf --bs="$test_dev_bs" --rw=randread --norandommap --name=reads \
 		--filename="$TEST_DEV" --size="$size" --direct=1 \
 		--ioengine=pvsync2 --hipri="$1"
 }
diff --git a/tests/nvme/049 b/tests/nvme/049
index 88d4fb122988..7304d6604d8f 100755
--- a/tests/nvme/049
+++ b/tests/nvme/049
@@ -19,10 +19,16 @@ test_device() {
 	echo "Running ${TEST_NAME}"
 
 	local ngdev=${TEST_DEV/nvme/ng}
-	local common_args=(
+	local test_dev_bs
+	local target_size=4096
+	local common_args=()
+	local fio_output
+
+	test_dev_bs=$(_min_io "$ngdev")
+	common_args=(
 		--size=1M
 		--filename="$ngdev"
-		--bs=4k
+		--bs="$test_dev_bs"
 		--rw=randread
 		--numjobs=1
 		--iodepth=16
@@ -32,10 +38,11 @@ test_device() {
 		--time_based
 		--runtime=2
 	)
-	local fio_output
+
+	((test_dev_bs > target_size)) && target_size=$test_dev_bs
 
 	# check security permission
-	if ! fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
+	if ! fio_output=$(fio --name=check --bs="$test_dev_bs" --size="$target_size" --filename="$ngdev" \
 			    --rw=read --ioengine=io_uring_cmd 2>&1) &&
 			grep -q -e "Operation not permitted" \
 				-e "Permission denied" <<< "$fio_output"; then
-- 
2.45.2

[PATCH blktests v3 6/6] common/xfs: add _test_dev_suits_xfs() to verify logical block size will work

[Luis Chamberlain]

mkfs.xfs will use the sector size exposed by the device, if this
is larger than 32k this will fail as the largest sector size on XFS
is 32k. Provide a sanity check to ensure we skip creating a filesystem
if the sector size is larger than what XFS supports.

Suggested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 common/xfs      | 11 +++++++++++
 tests/block/032 |  3 ++-
 tests/nvme/012  |  1 +
 tests/nvme/035  |  1 +
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/common/xfs b/common/xfs
index 226fdbd1c83f..1342a8e61f0b 100644
--- a/common/xfs
+++ b/common/xfs
@@ -10,6 +10,17 @@ _have_xfs() {
 	_have_fs xfs && _have_program mkfs.xfs
 }
 
+_test_dev_suits_xfs() {
+	local logical_block_size
+
+	logical_block_size=$(_test_dev_queue_get logical_block_size)
+	if ((logical_block_size > 32768 )); then
+		SKIP_REASONS+=("sector size ${logical_block_size} is larger than max XFS sector size 32768")
+		return 1
+	fi
+	return 0
+}
+
 _xfs_mkfs_and_mount() {
 	local bdev=$1
 	local mount_dir=$2
diff --git a/tests/block/032 b/tests/block/032
index fc6d1a51dcad..74688f7fca6e 100755
--- a/tests/block/032
+++ b/tests/block/032
@@ -15,6 +15,7 @@ QUICK=1
 requires() {
 	_have_xfs
 	_have_module scsi_debug
+	_test_dev_suits_xfs
 }
 
 test() {
@@ -25,7 +26,7 @@ test() {
 	fi
 
 	mkdir -p "${TMPDIR}/mnt"
-	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> $FULL || return $?
+	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> "$FULL" || return $?
 	echo 1 > "/sys/block/${SCSI_DEBUG_DEVICES[0]}/device/delete"
 	udevadm settle
 	umount "${TMPDIR}/mnt" || return $?
diff --git a/tests/nvme/012 b/tests/nvme/012
index f9bbdca911c0..f2727c06c893 100755
--- a/tests/nvme/012
+++ b/tests/nvme/012
@@ -17,6 +17,7 @@ requires() {
 	_have_loop
 	_require_nvme_trtype_is_fabrics
 	_require_nvme_test_img_size 350m
+	_test_dev_suits_xfs
 }
 
 set_conditions() {
diff --git a/tests/nvme/035 b/tests/nvme/035
index 9f84ced53ce6..14aa8c22956b 100755
--- a/tests/nvme/035
+++ b/tests/nvme/035
@@ -14,6 +14,7 @@ requires() {
 	_have_kernel_option NVME_TARGET_PASSTHRU
 	_have_xfs
 	_have_fio
+	_test_dev_suits_xfs
 }
 
 device_requires() {
-- 
2.45.2

Re: [PATCH blktests v3 1/6] common/xfs: ignore first umount error on _xfs_mkfs_and_mount()

[Bart Van Assche]

On 2/12/25 12:54 PM, Luis Chamberlain wrote:
> We want to help capture error messages with _xfs_mkfs_and_mount() on
> $FULL, to do that we should avoid spamming error messages for things
> which we know are not fatal. Such is the case of when we try to
> mkfs a filesystem but before that try to umount the target path.
> The first umount is just for sanity, so ignore the error messages from
> it.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   common/xfs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/xfs b/common/xfs
> index 569770fecd53..67a3b8a97391 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -15,7 +15,7 @@ _xfs_mkfs_and_mount() {
>   	local mount_dir=$2
>   
>   	mkdir -p "${mount_dir}"
> -	umount "${mount_dir}"
> +	umount "${mount_dir}" >/dev/null 2>&1
>   	mkfs.xfs -l size=64m -f "${bdev}" || return $?
>   	mount "${bdev}" "${mount_dir}"
>   }

Shouldn't ">/dev/null 2>&1" be added to the _xfs_mkfs_and_mount()
caller rather than inside the _xfs_mkfs_and_mount() implementation?
The above patch makes it impossible for any caller to capture the
stdout output of umount.

Thanks,

Bart.

Re: [PATCH blktests v3 1/6] common/xfs: ignore first umount error on _xfs_mkfs_and_mount()

[Luis Chamberlain]

On Wed, Feb 12, 2025 at 12:58:36PM -0800, Bart Van Assche wrote:
> On 2/12/25 12:54 PM, Luis Chamberlain wrote:
> > We want to help capture error messages with _xfs_mkfs_and_mount() on
> > $FULL, to do that we should avoid spamming error messages for things
> > which we know are not fatal. Such is the case of when we try to
> > mkfs a filesystem but before that try to umount the target path.
> > The first umount is just for sanity, so ignore the error messages from
> > it.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >   common/xfs | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/xfs b/common/xfs
> > index 569770fecd53..67a3b8a97391 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -15,7 +15,7 @@ _xfs_mkfs_and_mount() {
> >   	local mount_dir=$2
> >   	mkdir -p "${mount_dir}"
> > -	umount "${mount_dir}"
> > +	umount "${mount_dir}" >/dev/null 2>&1
> >   	mkfs.xfs -l size=64m -f "${bdev}" || return $?
> >   	mount "${bdev}" "${mount_dir}"
> >   }
> 
> Shouldn't ">/dev/null 2>&1" be added to the _xfs_mkfs_and_mount()
> caller rather than inside the _xfs_mkfs_and_mount() implementation?
> The above patch makes it impossible for any caller to capture the
> stdout output of umount.

That is the point. In this case the umount *can* fail, we do it for
sanity purposes. We don't care if umount failed.

  Luis

Re: [PATCH blktests v3 1/6] common/xfs: ignore first umount error on _xfs_mkfs_and_mount()

[Shinichiro Kawasaki]

On Feb 12, 2025 / 13:07, Luis Chamberlain wrote:
> On Wed, Feb 12, 2025 at 12:58:36PM -0800, Bart Van Assche wrote:
> > On 2/12/25 12:54 PM, Luis Chamberlain wrote:
> > > We want to help capture error messages with _xfs_mkfs_and_mount() on
> > > $FULL, to do that we should avoid spamming error messages for things
> > > which we know are not fatal. Such is the case of when we try to
> > > mkfs a filesystem but before that try to umount the target path.
> > > The first umount is just for sanity, so ignore the error messages from
> > > it.
> > > 
> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > ---
> > >   common/xfs | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/common/xfs b/common/xfs
> > > index 569770fecd53..67a3b8a97391 100644
> > > --- a/common/xfs
> > > +++ b/common/xfs
> > > @@ -15,7 +15,7 @@ _xfs_mkfs_and_mount() {
> > >   	local mount_dir=$2
> > >   	mkdir -p "${mount_dir}"
> > > -	umount "${mount_dir}"
> > > +	umount "${mount_dir}" >/dev/null 2>&1
> > >   	mkfs.xfs -l size=64m -f "${bdev}" || return $?
> > >   	mount "${bdev}" "${mount_dir}"
> > >   }
> > 
> > Shouldn't ">/dev/null 2>&1" be added to the _xfs_mkfs_and_mount()
> > caller rather than inside the _xfs_mkfs_and_mount() implementation?
> > The above patch makes it impossible for any caller to capture the
> > stdout output of umount.
> 
> That is the point. In this case the umount *can* fail, we do it for
> sanity purposes. We don't care if umount failed.
> 
>   Luis

IMO, umount --quiet option will help instead of redirect to /dev/null.

In most cases, the umount command just ensures that nothing is mounted at
${mount_dir}, and prints the "not mounted" error message. The --quiet option
will suppress it, and I guess it will address the problem Luis faces. With
this, still we can keep other error messages printed when anything unexpected
happens.

Re: [PATCH blktests v3 2/6] block/032: make error messages clearer if mkfs or mount fails

[Shinichiro Kawasaki]

On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> If block/032 fails at mkfs we want to know why, so propagate
> error messages. While at it, enhance the test to also propagate
> the error return from mount and remove the odd sleep for a udevadm
> settle as that's the only thing I can think of we need to wait for
> here.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  tests/block/032 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/block/032 b/tests/block/032
> index 8975879660d7..fc6d1a51dcad 100755
> --- a/tests/block/032
> +++ b/tests/block/032
> @@ -25,10 +25,10 @@ test() {
>  	fi
>  
>  	mkdir -p "${TMPDIR}/mnt"
> -	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" > /dev/null 2>&1
> +	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> $FULL || return $?

The $FULL needs double quotations to avoid a shellcheck warning. That change was
in the 6th patch, but let's fix it here.

When _xfs_mkfs_and_mount() fails, I think it is the better to call
_exit_scsi_debug() before returning from test(). It will clean up the scsi_debug
device, then will reduce the impact on following test cases.

>  	echo 1 > "/sys/block/${SCSI_DEBUG_DEVICES[0]}/device/delete"
> -	sleep 2
> -	umount "${TMPDIR}/mnt"
> +	udevadm settle
> +	umount "${TMPDIR}/mnt" || return $?
>  
>  	_exit_scsi_debug
>  
> -- 
> 2.45.2
> 

Re: [PATCH blktests v3 3/6] common: add and use min io for fio

[Shinichiro Kawasaki]

On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> When using fio we should not issue IOs smaller than the device supports.
> Today a lot of places have in place 4k, but soon we will have devices
> which support bs > ps. For those devices we should check the minimum
> supported IO.
> 
> However, since we also have a min optimal IO, we might as well use that
> as well. By using this we can also leverage the same lookup with stat
> whether or not the target file is a block device or a file.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  common/fio | 26 ++++++++++++++++++++++++--
>  common/rc  | 24 ++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/common/fio b/common/fio
> index b9ea087fc6c5..d147214f3d16 100644
> --- a/common/fio
> +++ b/common/fio
> @@ -189,15 +189,37 @@ _run_fio() {
>  	return $rc
>  }
>  
> +_fio_opts_to_min_io() {
> +	local arg path
> +	local -i min_io=4096
> +
> +        for arg in "$@"; do

Still spaces are left in the linve above.

> +		[[ "$arg" =~ ^--filename= || "$arg" =~ --directory= ]] || continue
> +		path="${arg##*=}"
> +		min_io=$(_min_io "$path")
> +		# Keep 4K minimum IO size for historical consistency
> +		((min_io < 4096)) && min_io=4096

I think the 4k comparison above is no longer required, since you added it in
_min_io().

> +		break
> +	done
> +
> +	echo "$min_io"
> +}
> +
>  # Wrapper around _run_fio used if you need some I/O but don't really care much
>  # about the details
>  _run_fio_rand_io() {
> -	_run_fio --bs=4k --rw=randread --norandommap --numjobs="$(nproc)" \
> +	local bs
> +
> +	bs=$(_fio_opts_to_min_io "$@") || return 1
> +	_run_fio --bs="$bs" --rw=randread --norandommap --numjobs="$(nproc)" \
>  		--name=reads --direct=1 "$@"
>  }
>  
>  _run_fio_verify_io() {
> -	_run_fio --name=verify --rw=randwrite --direct=1 --ioengine=libaio --bs=4k \
> +	local bs
> +
> +	bs=$(_fio_opts_to_min_io "$@") || return 1
> +	_run_fio --name=verify --rw=randwrite --direct=1 --ioengine=libaio --bs="$bs" \
>  		--iodepth=16 --verify=crc32c --verify_state_save=0 "$@"
>  }
>  
> diff --git a/common/rc b/common/rc
> index a7e899cfb419..6e7bddc844bf 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -400,6 +400,30 @@ _test_dev_is_partition() {
>  	[[ -n ${TEST_DEV_PART_SYSFS} ]]
>  }
>  
> +_min_io() {
> +	local path_or_dev=$1
> +	local min_io=4096
> +	if [ -z "$path_or_dev" ]; then
> +		echo "path for min_io does not exist"
> +		return 1
> +	fi
> +
> +	if [ -c "$path_or_dev" ]; then
> +		if [[ "$path_or_dev" == /dev/ng* ]]; then
> +			path_or_dev="${path_or_dev/ng/nvme}"
> +		fi
> +	fi
> +
> +	if [ -e "$path_or_dev" ]; then
> +		min_io=$(stat --printf=%o "$path_or_dev")
> +		((min_io < 4096)) && min_io=4096
> +		echo "$min_io"

Here's the added check, so _min_io() return value is always 4k or larger,
isn't it?

> +	else
> +		echo "Error: '$path_or_dev' does not exist or is not accessible"
> +		return 1
> +	fi
> +}
> +
>  # Return max open zones or max active zones of the test target device.
>  # If the device has both, return smaller value.
>  _test_dev_max_open_active_zones() {
> -- 
> 2.45.2
> 

Re: [PATCH blktests v3 6/6] common/xfs: add _test_dev_suits_xfs() to verify logical block size will work

[Shinichiro Kawasaki]

On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> mkfs.xfs will use the sector size exposed by the device, if this
> is larger than 32k this will fail as the largest sector size on XFS
> is 32k. Provide a sanity check to ensure we skip creating a filesystem
> if the sector size is larger than what XFS supports.
> 
> Suggested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  common/xfs      | 11 +++++++++++
>  tests/block/032 |  3 ++-
>  tests/nvme/012  |  1 +
>  tests/nvme/035  |  1 +
>  4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/common/xfs b/common/xfs
> index 226fdbd1c83f..1342a8e61f0b 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -10,6 +10,17 @@ _have_xfs() {
>  	_have_fs xfs && _have_program mkfs.xfs
>  }
>  
> +_test_dev_suits_xfs() {
> +	local logical_block_size
> +
> +	logical_block_size=$(_test_dev_queue_get logical_block_size)
> +	if ((logical_block_size > 32768 )); then
> +		SKIP_REASONS+=("sector size ${logical_block_size} is larger than max XFS sector size 32768")
> +		return 1
> +	fi
> +	return 0
> +}
> +
>  _xfs_mkfs_and_mount() {
>  	local bdev=$1
>  	local mount_dir=$2
> diff --git a/tests/block/032 b/tests/block/032
> index fc6d1a51dcad..74688f7fca6e 100755
> --- a/tests/block/032
> +++ b/tests/block/032
> @@ -15,6 +15,7 @@ QUICK=1
>  requires() {
>  	_have_xfs
>  	_have_module scsi_debug
> +	_test_dev_suits_xfs

I don't think this check is needed.

_test_dev_suits_xfs() calls _test_dev_queue_get(), which works only for test
cases with test_device(). Then, it works for nvme/035. But does not work for
either block/032 or nvme/012, which prepares test target device in test(),
so they do not need the check.

>  }
>  
>  test() {
> @@ -25,7 +26,7 @@ test() {
>  	fi
>  
>  	mkdir -p "${TMPDIR}/mnt"
> -	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> $FULL || return $?
> +	_xfs_mkfs_and_mount "/dev/${SCSI_DEBUG_DEVICES[0]}" "${TMPDIR}/mnt" >> "$FULL" || return $?
>  	echo 1 > "/sys/block/${SCSI_DEBUG_DEVICES[0]}/device/delete"
>  	udevadm settle
>  	umount "${TMPDIR}/mnt" || return $?
> diff --git a/tests/nvme/012 b/tests/nvme/012
> index f9bbdca911c0..f2727c06c893 100755
> --- a/tests/nvme/012
> +++ b/tests/nvme/012
> @@ -17,6 +17,7 @@ requires() {
>  	_have_loop
>  	_require_nvme_trtype_is_fabrics
>  	_require_nvme_test_img_size 350m
> +	_test_dev_suits_xfs

Same here.

>  }
>  
>  set_conditions() {
> diff --git a/tests/nvme/035 b/tests/nvme/035
> index 9f84ced53ce6..14aa8c22956b 100755
> --- a/tests/nvme/035
> +++ b/tests/nvme/035
> @@ -14,6 +14,7 @@ requires() {
>  	_have_kernel_option NVME_TARGET_PASSTHRU
>  	_have_xfs
>  	_have_fio
> +	_test_dev_suits_xfs

This is the requirement check for TEST_DEV, so I suggest to move it from
requires() to device_requires().

>  }
>  
>  device_requires() {
> -- 
> 2.45.2
> 

Re: [PATCH blktests v3 0/6] enable bs > ps device testing

[Shinichiro Kawasaki]

On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> This v3 series addresses the feedback from the v2 series [0] and
> makes some minor new changes, the change are:
> 
>   - Fixes all shellcheck complaints
>   - Addresses spacing / tabs fixes
>   - Adds _test_dev_suits_xfs() as suggested by Shinichiro and makes
>     tests which require this depend on it
>   - Clamps _min_io() to 4k as well for backward compatibility
>   - Few minor enhancements to help capture up error messages from
>     mkfs from block/032
> 
> This goes tested against a 64k sector size NVMe drive, and also
> using ./check so regular loopback devices are used. This helps
> test 64k sector devices, patches for which have been posted [1].
>                                                                                                                                                                                               
> [0] https://lkml.kernel.org/r/20250204225729.422949-1-mcgrof@kernel.org
> [1] https://lkml.kernel.org/r/20250204231209.429356-1-mcgrof@kernel.org

Luis, thank you very much for the improvements. I made comments on some of
the patches. FYI, I reflected my comments on your patches, and pushed them to a
github repo branch [2]. Please take a look in them and see if my comments make
sense or not.

[2] https://github.com/kawasaki/blktests/commits/bs_ps/

Re: [PATCH blktests v3 0/6] enable bs > ps device testing

[Luis Chamberlain]

On Fri, Feb 14, 2025 at 11:34:07AM +0000, Shinichiro Kawasaki wrote:
> On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> > This v3 series addresses the feedback from the v2 series [0] and
> > makes some minor new changes, the change are:
> > 
> >   - Fixes all shellcheck complaints
> >   - Addresses spacing / tabs fixes
> >   - Adds _test_dev_suits_xfs() as suggested by Shinichiro and makes
> >     tests which require this depend on it
> >   - Clamps _min_io() to 4k as well for backward compatibility
> >   - Few minor enhancements to help capture up error messages from
> >     mkfs from block/032
> > 
> > This goes tested against a 64k sector size NVMe drive, and also
> > using ./check so regular loopback devices are used. This helps
> > test 64k sector devices, patches for which have been posted [1].
> >                                                                                                                                                                                               
> > [0] https://lkml.kernel.org/r/20250204225729.422949-1-mcgrof@kernel.org
> > [1] https://lkml.kernel.org/r/20250204231209.429356-1-mcgrof@kernel.org
> 
> Luis, thank you very much for the improvements. I made comments on some of
> the patches. FYI, I reflected my comments on your patches, and pushed them to a
> github repo branch [2]. Please take a look in them and see if my comments make
> sense or not.
> 
> [2] https://github.com/kawasaki/blktests/commits/bs_ps/

Looks good, thanks for doing this, passes all my tests too, please feel
free to merge :)

  Luis

Re: [PATCH blktests v3 0/6] enable bs > ps device testing

[Shinichiro Kawasaki]

On Feb 14, 2025 / 10:54, Luis Chamberlain wrote:
> On Fri, Feb 14, 2025 at 11:34:07AM +0000, Shinichiro Kawasaki wrote:
> > On Feb 12, 2025 / 12:54, Luis Chamberlain wrote:
> > > This v3 series addresses the feedback from the v2 series [0] and
> > > makes some minor new changes, the change are:
> > > 
> > >   - Fixes all shellcheck complaints
> > >   - Addresses spacing / tabs fixes
> > >   - Adds _test_dev_suits_xfs() as suggested by Shinichiro and makes
> > >     tests which require this depend on it
> > >   - Clamps _min_io() to 4k as well for backward compatibility
> > >   - Few minor enhancements to help capture up error messages from
> > >     mkfs from block/032
> > > 
> > > This goes tested against a 64k sector size NVMe drive, and also
> > > using ./check so regular loopback devices are used. This helps
> > > test 64k sector devices, patches for which have been posted [1].
> > >                                                                                                                                                                                               
> > > [0] https://lkml.kernel.org/r/20250204225729.422949-1-mcgrof@kernel.org
> > > [1] https://lkml.kernel.org/r/20250204231209.429356-1-mcgrof@kernel.org
> > 
> > Luis, thank you very much for the improvements. I made comments on some of
> > the patches. FYI, I reflected my comments on your patches, and pushed them to a
> > github repo branch [2]. Please take a look in them and see if my comments make
> > sense or not.
> > 
> > [2] https://github.com/kawasaki/blktests/commits/bs_ps/
> 
> Looks good, thanks for doing this, passes all my tests too, please feel
> free to merge :)

Thanks for the confirmation.
FYI, I applied the series with the modifications.