On Sat, Aug 19, 2023 at 05:25:35PM +0100, Filipe Manana wrote:
> 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.
I'll add above link into commit log when I merge it.
> 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.
Jens? Darrick? or anyone who can help to provide an explanation about this
question Filipe asked, I'll add it into commit log too.
Thanks,
Zorro
>
> 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
> > > >>
> >
>
