Hi Luis, thanks for this patch. Please find my comments in line.
On Dec 18, 2024 / 03:21, 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@xxxxxxxxxx>
> ---
> common/fio | 6 ++++--
> common/rc | 10 ++++++++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/common/fio b/common/fio
> index b9ea087fc6c5..5676338d3c97 100644
> --- a/common/fio
> +++ b/common/fio
> @@ -192,12 +192,14 @@ _run_fio() {
> # 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 test_dev_bs=$(_test_dev_min_io $TEST_DEV)
Some of the test cases that calls _run_fio_rand_io can not refer to $TEST_DEV
e.g., nvme/040. Instead of $TEST_DEV, the device should be extracted from the
--filename=X option in the arguments. I suggest to introduce a helper function
as follows (_min_io is the function I suggest to rename from _test_dev_min_io).
# If --filename=file option exists in the given options and if the
# specified file is a block device or a character device, try to get its
# minimum IO size. Otherwise return 4096 as the default minimum IO size.
_fio_opts_to_min_io() {
local arg dev
local -i min_io=4096
for arg in "$@"; do
[[ "$arg" =~ ^--filename= ]] || continue
dev="${arg##*=}"
if [[ -b "$dev" || -c "$dev" ]] &&
! min_io=$(_min_io "$dev"); then
echo "Failed to get min_io from fio opts" >> "$FULL"
return 1
fi
# Keep 4K minimum IO size for historical consistency
((min_io < 4096)) && min_io=4096
break
done
echo "$min_io"
}
With this, the desired block size 'bs' can be obtained like this:
bs=$(_fio_opts_to_min_io "$@") || return 1
> + _run_fio --bs=$test_dev_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 test_dev_bs=$(_test_dev_min_io $TEST_DEV)
> + _run_fio --name=verify --rw=randwrite --direct=1 --ioengine=libaio --bs=$test_dev_bs \
> --iodepth=16 --verify=crc32c --verify_state_save=0 "$@"
> }
>
> diff --git a/common/rc b/common/rc
> index 0c8b51f64291..884677149c9e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -387,6 +387,16 @@ _test_dev_is_partition() {
> [[ -n ${TEST_DEV_PART_SYSFS} ]]
> }
>
> +_test_dev_min_io() {
I guess the word "test_dev" in this function name implies $TEST_DEV, but
this helper function is called not only for $TEST_DEV, but also for other
devices. I suggest simpler name _min_IO for this function.
> + local any_dev=$1
> + if [ -c $any_dev ]; then
> + if [[ "$any_dev" == /dev/ng* ]]; then
> + any_dev="${any_dev/ng/nvme}"
> + fi
> + fi
> + stat --printf=%o $any_dev
According to "man stat", "%o" prints "optimal I/O transfer size hint". Then it
is not super clear for me that it returns minimum I/O size. Instead, how about
to refer to the sysfs queue/minimum_io_size attribute? The following script will
do that. It relies on the patch attached, which introduces another new helper
function _get_sysfs_path.
_min_io() {
local any_dev sysfs_path
any_dev=$(realpath "$1")
if [[ -c "$any_dev" && "$any_dev" =~ ^/dev/ng ]]; then
any_dev="${any_dev/ng/nvme}"
fi
sysfs_path="$(_get_sysfs_path "$any_dev")"
cat "${sysfs_path}/queue/minimum_io_size"
}
From 9d91c0528f65d01e20b4d326aab2cc63f3304557 Mon Sep 17 00:00:00 2001
From: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
Date: Mon, 23 Dec 2024 15:27:34 +0900
Subject: [PATCH] check: factor out _get_sysfs_path()
The following change requires the code to get sysfs path from the block
device. Such code already exists in _find_sysfs_dirs(). Factor out it
to the new function _get_sysfs_path().
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
---
check | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/check b/check
index dad5e70..6f8010f 100755
--- a/check
+++ b/check
@@ -644,17 +644,28 @@ _run_group() {
return $ret
}
-_find_sysfs_dirs() {
- local test_dev="$1"
+_get_sysfs_path() {
+ local dev="$1"
local sysfs_path
- local major=$((0x$(stat -L -c '%t' "$test_dev")))
- local minor=$((0x$(stat -L -c '%T' "$test_dev")))
+ local major=$((0x$(stat -L -c '%t' "$dev")))
+ local minor=$((0x$(stat -L -c '%T' "$dev")))
# Get the canonical sysfs path
if ! sysfs_path="$(realpath "/sys/dev/block/${major}:${minor}")"; then
return 1
fi
+ echo "$sysfs_path"
+}
+
+_find_sysfs_dirs() {
+ local test_dev="$1"
+ local sysfs_path
+
+ if ! sysfs_path=$(_get_sysfs_path "$test_dev"); then
+ return 1
+ fi
+
if [[ -r "${sysfs_path}"/partition ]]; then
# If the device is a partition device, cut the last device name
# of the canonical sysfs path to access to the sysfs of its
--
2.46.2
