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

[Luis Chamberlain <mcgrof@kernel.org>]

On Tue, Jan 07, 2025 at 05:19:01AM +0000, Shinichiro Kawasaki wrote:
> On Jan 03, 2025 / 20:10, Luis Chamberlain wrote:
> > 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.
> 
> I guessed that the 16k XFS filesystem will allow 4k writes to regular files
> (In case this is wrong, correct me). 

If the filesystem has a 4k sector size it will be allowed, but if both
the fs block size and the fs sector size is 16k then no 4k IOs will be
allowed.

> Do you mean that 16k writes will be more
> appropriate blktests condition on 16k XFS filesystem?

And yes this too. Even if you do allow 4k writes with a 16k fs block
size but with a 4k fs sector size, you still want 16k IO writes on this fs.
One reason for exampe to want this is to avoid RMW which can happen on
4k IO writes.

> If this is the case, I agree with it.

Great.

> > 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.
> 
> Ah, now I see why you used the "stat --printf=%o" command.
> 
> I find that _xfs_run_fio_verify_io() and zbd/009,010 call _run_fio_verify_io()
> with the --directory fio option. To cover this case, _fio_opts_to_min_io() above
> will need change to cover both --filename and --directory options. When the
> --directory option is specified, we need to get the min_io size from the
> directory. Assuming that the "stat --printf=%o" command works for directories, I

Yes it does, this is with a 64k bs XFS fs:

stat --print=%o /mnt/poo
65536

The same fs statx would be used. Right now only XFS will
support LBS so its the only fs where we can test this currently on
x86_64.

The usage of statx --print=%o for this purpose is summarized in this
diagram:

https://docs.google.com/drawings/d/e/2PACX-1vQeZaBq2a0dgg9RDyd_XAJBSH-wbuGCtm95sLp2oFj66oghHWmXunib7tYOTPr84AlQ791VGiaKWvKF/pub?w=1006&h=929

> agree that this statx call approach will be simpler.

OK will follow through with this, thanks.

  Luis