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

[Luis Chamberlain <mcgrof@kernel.org>]

On Mon, Dec 23, 2024 at 09:37:48AM +0000, Shinichiro Kawasaki wrote:
> 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@kernel.org>
> > ---
> >  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.

Sure, I will fix all that, it will make it clearer its not always
TEST_DEV.

> 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

But here the file may be a regular file, and if you use 4k as the
default on a 16k XFS filesystem it won't work. This can be simplified
because using statx block size would be then be the correct thing to do
for a file. Then, it so turns out the the min-io can be obtained by
the same statx call to a block device as well.

Thoughts?

  Luis