| Message ID | 1692024101-3967-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/471: Remove this broken case | expand |
On Mon, Aug 14, 2023 at 10:41:41PM +0800, Yang Xu 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 > if the ilock cannot be obtained immediately. > > Here also a discussion[1] that seems generic/471 has been broken. > > 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@kernel.dk/ > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> Fine with me, since nobody thinks this test does anything useful, nor has anyone made it do something useful. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > 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 >
On 8/14/23 9:37 AM, Darrick J. Wong wrote: > On Mon, Aug 14, 2023 at 10:41:41PM +0800, Yang Xu 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 >> if the ilock cannot be obtained immediately. >> >> Here also a discussion[1] that seems generic/471 has been broken. >> >> 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@kernel.dk/ >> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > Fine with me, since nobody thinks this test does anything useful, nor > has anyone made it do something useful. It has never done anything useful, it's a test that was added so that someone could say they had a test case for a kernel change that they made. Killing it is the right choice, as I've argued for before. Reviewed-by: Jens Axboe <axboe@kernel.dk>
On Mon, Aug 14, 2023 at 10:41:41PM +0800, Yang Xu 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 > if the ilock cannot be obtained immediately. > > Here also a discussion[1] that seems generic/471 has been broken. > > 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@kernel.dk/ > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> Reviewed-by: Bill O'Donnell <bodonnel@redhat.com> > --- > 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 >
On Mon, Aug 14, 2023 at 4:04 PM Yang Xu <xuyang2018.jy@fujitsu.com> 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? > if the ilock cannot be obtained immediately. What is the ilock? That seems to be 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? 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@kernel.dk/ > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- > 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 >
on 2023/08/15 18:44, Filipe Manana wrote: > On Mon, Aug 14, 2023 at 4:04 PM Yang Xu <xuyang2018.jy@fujitsu.com> 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@kernel.dk/ 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@kernel.dk/ >> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> >> --- >> 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 >>
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@fujitsu.com> 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@kernel.dk/ Hi Filipe, Does above explanation make sense to you? 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@kernel.dk/ > >> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > >> --- > >> 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 > >>
On Fri, Aug 18, 2023 at 1:43 PM Zorro Lang <zlang@redhat.com> 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@fujitsu.com> 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@kernel.dk/ > > 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@kernel.dk/ > > >> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > >> --- > > >> 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 > > >> >
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@redhat.com> 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@fujitsu.com> 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@kernel.dk/ > > > > 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@kernel.dk/ > > > >> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > > >> --- > > > >> 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 > > > >> > > >
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@redhat.com> wrote: > > On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote: > > > 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@kernel.dk/ > > > > 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. We changed how timestamps are updated so that they are aware of IOCB_NOWAIT. If the IOCB_NOWIAT DIO write now needs to update the inode timestamps, it will return -EAGAIN instead of doing potentially blocking operations that require IO to complete (i.e. taking a transaction reservation). Hence the first time we go to do a DIO read an inode, it's going to do an atime update, which now occurrs from an IOCB_NOWAIT context and we return -EAGAIN.... Yes, we added non-blocking timestamp updates as part of the async buffered write support, but this was a general XFS IO path change of behaviour to address a potential blocking point in *all* IOCB_NOWAIT reads and writes, buffered or direct. > 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. async buffered writes didn't break the test. The addition of nonblocking timestamp updates in XFS is what causes the test to now fail. Indeed, we may change this XFS behaviour again some day - if we can guarantee that we can get a transaciton reservation without blocking then we -could- allow the timestamp update to run in IOCB_NOWAIT context. Doing this would then mean the test might randomly fail, depending on whether the timestamp update can be done without blocking or not.... Put simply, the test is not validating that RWF_NOWAIT is behaving correctly - it just was a simple operation that kinda exercised RWF_NOWAIT semantics when we had no other way to test this code. It has outlived it's original purpose, so it should be removed... -Dave.
On Mon, Aug 21, 2023 at 6:39 AM Dave Chinner <david@fromorbit.com> wrote: > > 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@redhat.com> wrote: > > > On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote: > > > > 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@kernel.dk/ > > > > > > 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. > > We changed how timestamps are updated so that they are aware of > IOCB_NOWAIT. If the IOCB_NOWIAT DIO write now needs to update the > inode timestamps, it will return -EAGAIN instead of doing > potentially blocking operations that require IO to complete (i.e. > taking a transaction reservation). > > Hence the first time we go to do a DIO read an inode, it's going to > do an atime update, which now occurrs from an IOCB_NOWAIT context > and we return -EAGAIN.... > > Yes, we added non-blocking timestamp updates as part of the async > buffered write support, but this was a general XFS IO path change of > behaviour to address a potential blocking point in *all* IOCB_NOWAIT > reads and writes, buffered or direct. Ok, now that's the kind of explanation I would expect in the changelog. > > > 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. > > async buffered writes didn't break the test. The addition of > nonblocking timestamp updates in XFS is what causes the test to now > fail. Ok, so the changelog is very misleading, as it points to commits that, as far as I can see at least, have nothing to do with the changes that make timestamp updates aware of NOWAIT semantics. So it should be the following commit to be referred (and possibly others): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=66fa3cedf16abc82d19b943e3289c82e685419d5 > > Indeed, we may change this XFS behaviour again some day - if we can > guarantee that we can get a transaciton reservation without blocking > then we -could- allow the timestamp update to run in IOCB_NOWAIT > context. Doing this would then mean the test might randomly fail, > depending on whether the timestamp update can be done without > blocking or not.... > > Put simply, the test is not validating that RWF_NOWAIT is behaving > correctly - it just was a simple operation that kinda exercised > RWF_NOWAIT semantics when we had no other way to test this code. It > has outlived it's original purpose, so it should be removed... Thanks for the detailed explanation. A simpler version of this, or perhaps a lore link to your reply should be added to the changelog, because the current one is more like "remove this test because someone else told to do so" without any relevant details. > > -Dave. > -- > Dave Chinner > david@fromorbit.com
on 2023/08/21 19:16, Filipe Manana wrote: > On Mon, Aug 21, 2023 at 6:39 AM Dave Chinner <david@fromorbit.com> wrote: >> >> 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@redhat.com> wrote: >>>> On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote: >>>>> 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@kernel.dk/ >>>> >>>> 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. >> >> We changed how timestamps are updated so that they are aware of >> IOCB_NOWAIT. If the IOCB_NOWIAT DIO write now needs to update the >> inode timestamps, it will return -EAGAIN instead of doing >> potentially blocking operations that require IO to complete (i.e. >> taking a transaction reservation). >> >> Hence the first time we go to do a DIO read an inode, it's going to >> do an atime update, which now occurrs from an IOCB_NOWAIT context >> and we return -EAGAIN.... >> >> Yes, we added non-blocking timestamp updates as part of the async >> buffered write support, but this was a general XFS IO path change of >> behaviour to address a potential blocking point in *all* IOCB_NOWAIT >> reads and writes, buffered or direct. > > Ok, now that's the kind of explanation I would expect in the changelog. > >> >>> 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. >> >> async buffered writes didn't break the test. The addition of >> nonblocking timestamp updates in XFS is what causes the test to now >> fail. > > Ok, so the changelog is very misleading, as it points to commits that, > as far as I can see at least, > have nothing to do with the changes that make timestamp updates aware > of NOWAIT semantics. > > So it should be the following commit to be referred (and possibly others): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=66fa3cedf16abc82d19b943e3289c82e685419d5 > >> >> Indeed, we may change this XFS behaviour again some day - if we can >> guarantee that we can get a transaciton reservation without blocking >> then we -could- allow the timestamp update to run in IOCB_NOWAIT >> context. Doing this would then mean the test might randomly fail, >> depending on whether the timestamp update can be done without >> blocking or not.... >> >> Put simply, the test is not validating that RWF_NOWAIT is behaving >> correctly - it just was a simple operation that kinda exercised >> RWF_NOWAIT semantics when we had no other way to test this code. It >> has outlived it's original purpose, so it should be removed... > > Thanks for the detailed explanation. > > A simpler version of this, or perhaps a lore link to your reply should > be added to the changelog, > because the current one is more like "remove this test because someone > else told to do so" without > any relevant details. Sorry, I just saw part of the generic/471 history and discussion last week, then wrote the misleading changelog. But now , it is better than do nothing. At least, we figured out why xfs fail reason and why remove this test. So I guess I only need to add the following word as commit message is enough "The test is not validating that RWF_NOWAIT is behaving correctly - it just was a simple operation that kinda exercised RWF_NOWAIT semantics when we had no other way to test this code. It has outlived it's original purpose, so it should be removed... " Is it right? Best Regards Yang Xu > >> >> -Dave. >> -- >> Dave Chinner >> david@fromorbit.com
On Mon, Aug 21, 2023 at 12:16:54PM +0100, Filipe Manana wrote: > On Mon, Aug 21, 2023 at 6:39 AM Dave Chinner <david@fromorbit.com> wrote: > > > > 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@redhat.com> wrote: > > > > On Wed, Aug 16, 2023 at 02:58:10PM +0000, Yang Xu (Fujitsu) wrote: > > > > > 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@kernel.dk/ > > > > > > > > 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. > > > > We changed how timestamps are updated so that they are aware of > > IOCB_NOWAIT. If the IOCB_NOWIAT DIO write now needs to update the > > inode timestamps, it will return -EAGAIN instead of doing > > potentially blocking operations that require IO to complete (i.e. > > taking a transaction reservation). > > > > Hence the first time we go to do a DIO read an inode, it's going to > > do an atime update, which now occurrs from an IOCB_NOWAIT context > > and we return -EAGAIN.... > > > > Yes, we added non-blocking timestamp updates as part of the async > > buffered write support, but this was a general XFS IO path change of > > behaviour to address a potential blocking point in *all* IOCB_NOWAIT > > reads and writes, buffered or direct. > > Ok, now that's the kind of explanation I would expect in the changelog. Why? It was clearly obvious in the patch series that this infrastructure was being added to the VFS and XFS was being converted to use it. All the VFS support for this was done in earlier patches in the series, but the bisect doesn't land on them - it lands on the commit that enabled that functionality. Indeed, just because the commit a bisect lands on doesn't describe all the specific changes in behaviour that occur across an entire series, it doesn't mean the change logs for the patches are bad or incomplete. It just means there's more context to the new feature the patch enables than what is in the commit that the bisect lands on. > > > 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. > > > > async buffered writes didn't break the test. The addition of > > nonblocking timestamp updates in XFS is what causes the test to now > > fail. > > Ok, so the changelog is very misleading, as it points to commits that, > as far as I can see at least, > have nothing to do with the changes that make timestamp updates aware > of NOWAIT semantics. > > So it should be the following commit to be referred (and possibly others): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=66fa3cedf16abc82d19b943e3289c82e685419d5 Precisely my point. This is the async timestamp NOWAIT VFS support patch that the XFS changes depended on. You have to look at the change as a whole, not just individual commits. But this patch didn't enable that functionality in XFS - it's just the infrastructure - and so the bisect is *never* going to land on this patch because nothing actually uses the code at this point. The bisect will always land on the commit that enables the new functionality, and those commits rarely explain all the details of the new functionality that it is enabling. If you don't like that, then review the new feature code as it goes past and ask the authors to rewrite all their commit messages to explain everything in every patch in the series. We just don't do that because it is unnecessary - everyone else who reviewed the series was happy with the commit messages for each commit because they looked at the whole series, not just one specific patch in the large work like you have done and then complained about. > > Indeed, we may change this XFS behaviour again some day - if we can > > guarantee that we can get a transaciton reservation without blocking > > then we -could- allow the timestamp update to run in IOCB_NOWAIT > > context. Doing this would then mean the test might randomly fail, > > depending on whether the timestamp update can be done without > > blocking or not.... > > > > Put simply, the test is not validating that RWF_NOWAIT is behaving > > correctly - it just was a simple operation that kinda exercised > > RWF_NOWAIT semantics when we had no other way to test this code. It > > has outlived it's original purpose, so it should be removed... > > Thanks for the detailed explanation. > > A simpler version of this, or perhaps a lore link to your reply should > be added to the changelog, > because the current one is more like "remove this test because someone > else told to do so" without > any relevant details. <shrug> I don't care if that is done or not, and it should not hold up removing the test. We know why it needs to be removed, and this discussion is already in lore under the patch name, so everyone can find it simply by searching for the commit title in the lore archive. Cheers, Dave.
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)
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 if the ilock cannot be obtained immediately. Here also a discussion[1] that seems generic/471 has been broken. 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@kernel.dk/ Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- 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