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