[PATCH blktests v2 0/4] enable bs > ps device testing

[PATCH blktests v2 0/4] enable bs > ps device testing

[Luis Chamberlain]

This v2 series addresses the feedback from the first series [0], namely:

  - uses less device specific names
  - checks for fio arguments --filename or --directory to extract the
    min io target or path
  - adds a new patch to verify the sector size will work before creating
    a filesystem
  - a diagram is provided to help easily disect why we use statx
    blocksize, although not included in the docs we could later if
    it helps

This goes tested against a 64k sector size NVMe drive, the patches for
which will be posted soon rebased on v6.14-rc1.

[0] https://lkml.kernel.org/r/20241218112153.3917518-1-mcgrof@kernel.org
[1] https://docs.google.com/drawings/d/e/2PACX-1vQeZaBq2a0dgg9RDyd_XAJBSH-wbuGCtm95sLp2oFj66oghHWmXunib7tYOTPr84AlQ791VGiaKWvKF/pub?w=1006&h=929                                                  
Luis Chamberlain (4):
  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: check for max supported sector size

 common/fio      | 23 +++++++++++++++++++++--
 common/rc       | 21 +++++++++++++++++++++
 common/xfs      | 15 ++++++++++++++-
 tests/block/003 |  4 +++-
 tests/block/007 |  3 ++-
 tests/nvme/049  |  8 ++++++--
 6 files changed, 67 insertions(+), 7 deletions(-)

-- 
2.45.2

[PATCH blktests v2 1/4] 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 | 23 +++++++++++++++++++++--
 common/rc  | 21 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/common/fio b/common/fio
index b9ea087fc6c5..557150656b29 100644
--- a/common/fio
+++ b/common/fio
@@ -189,15 +189,34 @@ _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=$(_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=$(_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 bcb215d35114..e12ecd025868 100644
--- a/common/rc
+++ b/common/rc
@@ -387,6 +387,27 @@ _test_dev_is_partition() {
 	[[ -n ${TEST_DEV_PART_SYSFS} ]]
 }
 
+_min_io() {
+	local path_or_dev=$1
+        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
+                stat --printf=%o "$path_or_dev"
+        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 v2 2/4] 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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/xfs b/common/xfs
index 569770fecd53..8b068837fa37 100644
--- a/common/xfs
+++ b/common/xfs
@@ -13,10 +13,16 @@ _have_xfs() {
 _xfs_mkfs_and_mount() {
 	local bdev=$1
 	local mount_dir=$2
+	local bs=$(_min_io $bdev)
+	local xfs_logsize="64m"
+
+	if [[ $bs -gt 4096 ]]; then
+		xfs_logsize="128m"
+	fi
 
 	mkdir -p "${mount_dir}"
 	umount "${mount_dir}"
-	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 v2 3/4] 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 | 4 +++-
 tests/block/007 | 3 ++-
 tests/nvme/049  | 8 ++++++--
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tests/block/003 b/tests/block/003
index 2af9b89ec3e5..d59f2eadd465 100755
--- a/tests/block/003
+++ b/tests/block/003
@@ -18,10 +18,12 @@ device_requires() {
 }
 
 test_device() {
+	local 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..8d4ee0758b12 100755
--- a/tests/block/007
+++ b/tests/block/007
@@ -31,13 +31,14 @@ cleanup_fallback_device() {
 }
 
 run_fio_job() {
+	local 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..77bb4daf5b08 100755
--- a/tests/nvme/049
+++ b/tests/nvme/049
@@ -19,10 +19,12 @@ test_device() {
 	echo "Running ${TEST_NAME}"
 
 	local ngdev=${TEST_DEV/nvme/ng}
+	local test_dev_bs=$(_min_io $ngdev)
+	local target_size=4096
 	local common_args=(
 		--size=1M
 		--filename="$ngdev"
-		--bs=4k
+		--bs=$test_dev_bs
 		--rw=randread
 		--numjobs=1
 		--iodepth=16
@@ -34,8 +36,10 @@ test_device() {
 	)
 	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 v2 4/4] common/xfs: check for max supported sector size

[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.

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

diff --git a/common/xfs b/common/xfs
index 8b068837fa37..dbae572e4390 100644
--- a/common/xfs
+++ b/common/xfs
@@ -15,11 +15,18 @@ _xfs_mkfs_and_mount() {
 	local mount_dir=$2
 	local bs=$(_min_io $bdev)
 	local xfs_logsize="64m"
+	local sysfs="/sys/block/${bdev#/dev/}"
+	local logical_block_size=$(cat $sysfs/queue/logical_block_size)
 
 	if [[ $bs -gt 4096 ]]; then
 		xfs_logsize="128m"
 	fi
 
+	if [[ $logical_block_size -gt 32768 ]]; then
+		SKIP_REASONS+=("max sector size for XFS is 32768 but device $bdev has a larger sector size $logical_block_size")
+		return 1
+	fi
+
 	mkdir -p "${mount_dir}"
 	umount "${mount_dir}"
 	mkfs.xfs -l size=$xfs_logsize -f "${bdev}" -b size=$bs || return $?
-- 
2.45.2

Re: [PATCH blktests v2 0/4] enable bs > ps device testing

[Shinichiro Kawasaki]

On Feb 04, 2025 / 14:57, Luis Chamberlain wrote:
> This v2 series addresses the feedback from the first series [0], namely:
> 
>   - uses less device specific names
>   - checks for fio arguments --filename or --directory to extract the
>     min io target or path
>   - adds a new patch to verify the sector size will work before creating
>     a filesystem
>   - a diagram is provided to help easily disect why we use statx
>     blocksize, although not included in the docs we could later if
>     it helps

Thanks for this v2 series. I ran the tests, and they look working. Good.
I also ran "make check" and saw shellcheck warnings. Could you address them?

$ make check
shellcheck -x -e SC2119 -f gcc check common/* \
        tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
common/fio:212:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
common/fio:213:16: note: Double quote to prevent globbing and word splitting. [SC2086]
common/fio:218:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
common/fio:219:74: note: Double quote to prevent globbing and word splitting. [SC2086]
common/xfs:16:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
common/xfs:16:21: note: Double quote to prevent globbing and word splitting. [SC2086]
common/xfs:19:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
common/xfs:19:33: note: Double quote to prevent globbing and word splitting. [SC2086]
common/xfs:32:53: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/003:21:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
tests/block/003:21:30: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/003:26:22: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/007:34:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
tests/block/007:34:30: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/007:41:17: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/049:22:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
tests/nvme/049:22:30: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/049:27:8: warning: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. [SC2206]
tests/nvme/049:42:42: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/049:42:62: note: Double quote to prevent globbing and word splitting. [SC2086]
make: *** [Makefile:21: check] Error 1


I will comment on the 1st and 4th patches. Other than the shellcheck
warnings, the 2nd and 3rd patches look good to me.

> 
> This goes tested against a 64k sector size NVMe drive, the patches for
> which will be posted soon rebased on v6.14-rc1.
> 
> [0] https://lkml.kernel.org/r/20241218112153.3917518-1-mcgrof@kernel.org
> [1] https://docs.google.com/drawings/d/e/2PACX-1vQeZaBq2a0dgg9RDyd_XAJBSH-wbuGCtm95sLp2oFj66oghHWmXunib7tYOTPr84AlQ791VGiaKWvKF/pub?w=1006&h=929

I'm interested in the link [1]. I guess it is the diagram noted, isn't it?
But it looks like I can not access it. I just see a blank page.

Re: [PATCH blktests v2 1/4] common: add and use min io for fio

[Shinichiro Kawasaki]

On Feb 04, 2025 / 14:57, 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 | 23 +++++++++++++++++++++--
>  common/rc  | 21 +++++++++++++++++++++
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/common/fio b/common/fio
> index b9ea087fc6c5..557150656b29 100644
> --- a/common/fio
> +++ b/common/fio
> @@ -189,15 +189,34 @@ _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"
> +}

Spaces are used for indent in the hunk above. Let's use tabs instead.

[...]

> diff --git a/common/rc b/common/rc
> index bcb215d35114..e12ecd025868 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -387,6 +387,27 @@ _test_dev_is_partition() {
>  	[[ -n ${TEST_DEV_PART_SYSFS} ]]
>  }
>  
> +_min_io() {
> +	local path_or_dev=$1
> +        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
> +                stat --printf=%o "$path_or_dev"
> +        else
> +                echo "Error: '$path_or_dev' does not exist or is not accessible"
> +                return 1
> +        fi
> +}

Same here, let's use tabs.

Re: [PATCH blktests v2 4/4] common/xfs: check for max supported sector size

[Shinichiro Kawasaki]

On Feb 04, 2025 / 14:57, 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.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  common/xfs | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 8b068837fa37..dbae572e4390 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -15,11 +15,18 @@ _xfs_mkfs_and_mount() {
>  	local mount_dir=$2
>  	local bs=$(_min_io $bdev)
>  	local xfs_logsize="64m"
> +	local sysfs="/sys/block/${bdev#/dev/}"
> +	local logical_block_size=$(cat $sysfs/queue/logical_block_size)
>  
>  	if [[ $bs -gt 4096 ]]; then
>  		xfs_logsize="128m"
>  	fi
>  
> +	if [[ $logical_block_size -gt 32768 ]]; then
> +		SKIP_REASONS+=("max sector size for XFS is 32768 but device $bdev has a larger sector size $logical_block_size")

Adding SKIP_REASONS here is not ideal, since this function is called from
test() or test_device(). It's the better to check the requirement in
requires() or device_requires(), before touching the test target devices.

If test() calls _xfs_mkfs_and_mount(), the test case should be able to
control the sector size smaller than 32k, so no need to check the
requirement. I think block/032 and nvme/012 fall in this category.

If test_device() calls _xfs_mkfs_and_mount(), it's the better to check the
requirement in device_requires(). Maybe we can add a helper function
_test_dev_suits_xfs() like below (untested) to common/xfs and call it from
device_requires(). I hope this will work for nvme/035.

_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
}

> +		return 1
> +	fi
> +
>  	mkdir -p "${mount_dir}"
>  	umount "${mount_dir}"
>  	mkfs.xfs -l size=$xfs_logsize -f "${bdev}" -b size=$bs || return $?
> -- 
> 2.45.2
> 

Re: [PATCH blktests v2 4/4] common/xfs: check for max supported sector size

[Luis Chamberlain]

On Fri, Feb 07, 2025 at 11:58:29AM +0000, Shinichiro Kawasaki wrote:
> On Feb 04, 2025 / 14:57, 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.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  common/xfs | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/common/xfs b/common/xfs
> > index 8b068837fa37..dbae572e4390 100644
> > --- a/common/xfs
> > +++ b/common/xfs
> > @@ -15,11 +15,18 @@ _xfs_mkfs_and_mount() {
> >  	local mount_dir=$2
> >  	local bs=$(_min_io $bdev)
> >  	local xfs_logsize="64m"
> > +	local sysfs="/sys/block/${bdev#/dev/}"
> > +	local logical_block_size=$(cat $sysfs/queue/logical_block_size)
> >  
> >  	if [[ $bs -gt 4096 ]]; then
> >  		xfs_logsize="128m"
> >  	fi
> >  
> > +	if [[ $logical_block_size -gt 32768 ]]; then
> > +		SKIP_REASONS+=("max sector size for XFS is 32768 but device $bdev has a larger sector size $logical_block_size")
> 
> Adding SKIP_REASONS here is not ideal, since this function is called from
> test() or test_device(). It's the better to check the requirement in
> requires() or device_requires(), before touching the test target devices.
> 
> If test() calls _xfs_mkfs_and_mount(), the test case should be able to
> control the sector size smaller than 32k, so no need to check the
> requirement.

I see, I'll fix.

> I think block/032 and nvme/012 fall in this category.

Indeed. I also noted _min_io() needs to return 4096 by default too as we
want to use it as default in case we get a lower value to retain
backward compatbility. That fixed some block/032 and nvme/012 issues
with 512 min-io values.

  Luis

Re: [PATCH blktests v2 1/4] common: add and use min io for fio

[Luis Chamberlain]

On Fri, Feb 07, 2025 at 11:26:17AM +0000, Shinichiro Kawasaki wrote:
> On Feb 04, 2025 / 14:57, Luis Chamberlain wrote:
> Spaces are used for indent in the hunk above. Let's use tabs instead.
> Same here, let's use tabs.

Fixed thanks.

 Luis

Re: [PATCH blktests v2 0/4] enable bs > ps device testing

[Luis Chamberlain]

On Fri, Feb 07, 2025 at 11:24:10AM +0000, Shinichiro Kawasaki wrote:
> On Feb 04, 2025 / 14:57, Luis Chamberlain wrote:
> > This v2 series addresses the feedback from the first series [0], namely:
> > 
> >   - uses less device specific names
> >   - checks for fio arguments --filename or --directory to extract the
> >     min io target or path
> >   - adds a new patch to verify the sector size will work before creating
> >     a filesystem
> >   - a diagram is provided to help easily disect why we use statx
> >     blocksize, although not included in the docs we could later if
> >     it helps
> 
> Thanks for this v2 series. I ran the tests, and they look working. Good.
> I also ran "make check" and saw shellcheck warnings. Could you address them?
> 
> $ make check
> shellcheck -x -e SC2119 -f gcc check common/* \

I fixed them, will submit shortly.

> > This goes tested against a 64k sector size NVMe drive, the patches for
> > which will be posted soon rebased on v6.14-rc1.
> > 
> > [0] https://lkml.kernel.org/r/20241218112153.3917518-1-mcgrof@kernel.org
> > [1] https://docs.google.com/drawings/d/e/2PACX-1vQeZaBq2a0dgg9RDyd_XAJBSH-wbuGCtm95sLp2oFj66oghHWmXunib7tYOTPr84AlQ791VGiaKWvKF/pub?w=1006&h=929
> 
> I'm interested in the link [1]. I guess it is the diagram noted, isn't it?
> But it looks like I can not access it. I just see a blank page.

That's so odd, I embedded this document here too:

https://kernelnewbies.org/KernelProjects/large-block-size

It is below the "stat --print=%o" example:

Can you see that image?

  Luis