On Fri, Aug 18, 2023 at 1:43 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
>
> On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote:
> > on 2023/08/15 18:44, Filipe Manana wrote:
> > > On Mon, Aug 14, 2023 at 4:04 PM Yang Xu <xuyang2018.jy@xxxxxxxxxxx> wrote:
> > >>
> > >> I remember this case fails on last year becuase of
> > >> kernel commit cae2de69 ("iomap: Add async buffered write support")
> > >> kernel commit 1aa91d9 ("xfs: Add async buffered write support").
> > >> as below:
> > >> pwrite: Resource temporarily unavailable
> > >> wrote 8388608/8388608 bytes at offset 0
> > >> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > >> -RWF_NOWAIT time is within limits.
> > >> +pwrite: Resource temporarily unavailable
> > >> +(standard_in) 1: syntax error
> > >> +RWF_NOWAIT took seconds
> > >>
> > >> So For async buffered write requests, the request will return -EAGAIN
> > >
> > > I'm curious about this.
> > >
> > > All the xfs_io pwrite calls are being done with Direct IO (-d)
> > > argument, so how does that explain the failure?
> >
> > I am not understand async buffer write, but with the following
> > discussion link[1] maybe explain this failure and explain why btrfs passed.
> > >
> > >> if the ilock cannot be obtained immediately.
> > >
> > > What is the ilock? That seems to be xfs specific.
> >
> > yes, I guess it is xfs_ilock and they are xfs specific.
> > >
> > > The test passes on btrfs and btrfs supports async buffered writes -
> > > but I'm still puzzled, because the test only does direct IO writes.
> > > Does xfs falls back from direct IO to buffered IO?
> > >
> > >>
> > >> Here also a discussion[1] that seems generic/471 has been broken.
> > >
> > > Well that discussion doesn't help me understand things. It mentions
> > > some other discussion, presumably with more details. Where's that
> > > other discussion?
> >
> > I think the url that Jens mention should be this[1] when he reviewed
> > Stefan V7 patch for "io-uring/xfs: support async buffered writes".
> >
> > [1]https://lkml.kernel.org/linux-fsdevel/ca60a7dc-b16d-d8ce-f56c-547b449da8c9@xxxxxxxxx/
>
> Hi Filipe,
>
> Does above explanation make sense to you?
Not completely.
It justifies that the test's assumptions are not necessarily correct,
that I understood and it's reasonable.
However I don't see, neither in that thread nor in this patch's
changelog, why the test started to fail (on xfs, it still passes on
btrfs and ext4) after adding support for async buffered IO writes to
xfs and iomap - as all the writes done by the test are using direct
IO.
At least the changelog should point to that thread, and not the one it
currently refers to because it doesn't provide any useful information.
For completeness it should also justify why the async buffered write
support broke the test, as it points out the test fails after those 2
commits for buffered write support, yet there are no buffered writes
performed by the test.
Anyway, don't make me hold back a patch just because I'm too curious.
Thanks.
>
> This patch got several RVBs, but as this patch removes a generic patch, so
> I'd like to see there's not objection from any fs list. If btrfs still have
> more review points, I'll hold this patch, won't merge it in next release.
>
> Thanks,
> Zorro
>
> >
> >
> > Best Regards
> > Yang Xu
> > >
> > > Thanks.
> > >
> > >>
> > >> Now, I met this problem in my linux distribution, then I found the above
> > >> discussion. IMO, remove this case is ok and then we can avoid to meet this
> > >> false report again.
> > >>
> > >> [1]https://lore.kernel.org/linux-xfs/b2865bd6-2346-8f4d-168b-17f06bbedbed@xxxxxxxxx/
> > >> Signed-off-by: Yang Xu <xuyang2018.jy@xxxxxxxxxxx>
> > >> ---
> > >> tests/generic/471 | 67 -------------------------------------------
> > >> tests/generic/471.out | 13 ---------
> > >> 2 files changed, 80 deletions(-)
> > >> delete mode 100755 tests/generic/471
> > >> delete mode 100644 tests/generic/471.out
> > >>
> > >> diff --git a/tests/generic/471 b/tests/generic/471
> > >> deleted file mode 100755
> > >> index fbd0b12a..00000000
> > >> --- a/tests/generic/471
> > >> +++ /dev/null
> > >> @@ -1,67 +0,0 @@
> > >> -#! /bin/bash
> > >> -# SPDX-License-Identifier: GPL-2.0
> > >> -# Copyright (c) 2017, SUSE Linux Products. All Rights Reserved.
> > >> -#
> > >> -# FS QA Test No. 471
> > >> -#
> > >> -# write a file with RWF_NOWAIT and it would fail because there are no
> > >> -# blocks allocated. Create a file with direct I/O and re-write it
> > >> -# using RWF_NOWAIT. I/O should finish within 50 microsecods since
> > >> -# block allocations are already performed.
> > >> -#
> > >> -. ./common/preamble
> > >> -_begin_fstest auto quick rw
> > >> -
> > >> -# Import common functions.
> > >> -. ./common/populate
> > >> -. ./common/filter
> > >> -. ./common/attr
> > >> -
> > >> -# real QA test starts here
> > >> -_require_odirect
> > >> -_require_test
> > >> -_require_xfs_io_command pwrite -N
> > >> -
> > >> -# Remove reminiscence of previously run tests
> > >> -testdir=$TEST_DIR/$seq
> > >> -if [ -e $testdir ]; then
> > >> - rm -Rf $testdir
> > >> -fi
> > >> -
> > >> -mkdir $testdir
> > >> -
> > >> -# Btrfs is a COW filesystem, so a RWF_NOWAIT write will always fail with -EAGAIN
> > >> -# when writing to a file range except if it's a NOCOW file and an extent for the
> > >> -# range already exists or if it's a COW file and preallocated/unwritten extent
> > >> -# exists in the target range. So to make sure that the last write succeeds on
> > >> -# all filesystems, use a NOCOW file on btrfs.
> > >> -if [ $FSTYP == "btrfs" ]; then
> > >> - _require_chattr C
> > >> - # Zoned btrfs does not support NOCOW
> > >> - _require_non_zoned_device $TEST_DEV
> > >> - touch $testdir/f1
> > >> - $CHATTR_PROG +C $testdir/f1
> > >> -fi
> > >> -
> > >> -# Create a file with pwrite nowait (will fail with EAGAIN)
> > >> -$XFS_IO_PROG -f -d -c "pwrite -N -V 1 -b 1M 0 1M" $testdir/f1
> > >> -
> > >> -# Write the file without nowait
> > >> -$XFS_IO_PROG -f -d -c "pwrite -S 0xaa -W -w -V 1 -b 1M 0 8M" $testdir/f1 | _filter_xfs_io
> > >> -
> > >> -time_taken=`$XFS_IO_PROG -d -c "pwrite -S 0xbb -N -V 1 -b 1M 2M 1M" $testdir/f1 | awk '/^1/ {print $5}'`
> > >> -
> > >> -# RWF_NOWAIT should finish within a short period of time so we are choosing
> > >> -# a conservative value of 50 ms. Anything longer means it is waiting
> > >> -# for something in the kernel which would be a fail.
> > >> -if (( $(echo "$time_taken < 0.05" | bc -l) )); then
> > >> - echo "RWF_NOWAIT time is within limits."
> > >> -else
> > >> - echo "RWF_NOWAIT took $time_taken seconds"
> > >> -fi
> > >> -
> > >> -$XFS_IO_PROG -c "pread -v 0 8M" $testdir/f1 | _filter_xfs_io_unique
> > >> -
> > >> -# success, all done
> > >> -status=0
> > >> -exit
> > >> diff --git a/tests/generic/471.out b/tests/generic/471.out
> > >> deleted file mode 100644
> > >> index ab23272e..00000000
> > >> --- a/tests/generic/471.out
> > >> +++ /dev/null
> > >> @@ -1,13 +0,0 @@
> > >> -QA output created by 471
> > >> -pwrite: Resource temporarily unavailable
> > >> -wrote 8388608/8388608 bytes at offset 0
> > >> -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > >> -RWF_NOWAIT time is within limits.
> > >> -00000000: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................
> > >> -*
> > >> -00200000: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
> > >> -*
> > >> -00300000: aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ................
> > >> -*
> > >> -read 8388608/8388608 bytes at offset 0
> > >> -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > >> --
> > >> 2.27.0
> > >>
>
