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@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.
I guessed that the 16k XFS filesystem will allow 4k writes to regular files
(In case this is wrong, correct me). Do you mean that 16k writes will be more
appropriate blktests condition on 16k XFS filesystem? If this is the case, I
agree with it.
> 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
agree that this statx call approach will be simpler.
