[PATCH] generic/471: Remove this broken case

[PATCH] generic/471: Remove this broken case

[Yang Xu]

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

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

Re: [PATCH] generic/471: Remove this broken case

[Darrick J. Wong]

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
> 

Re: [PATCH] generic/471: Remove this broken case

[Jens Axboe]

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>

-- 
Jens Axboe

Re: [PATCH] generic/471: Remove this broken case

[Bill O'Donnell]

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
> 

Re: [PATCH] generic/471: Remove this broken case

[Filipe Manana]

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
>

Re: [PATCH] generic/471: Remove this broken case

[Yang Xu (Fujitsu)]

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
>>

Re: [PATCH] generic/471: Remove this broken case

[Zorro Lang]

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
> >>

Re: [PATCH] generic/471: Remove this broken case

[Filipe Manana]

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
> > >>
>

Re: [PATCH] generic/471: Remove this broken case

[Zorro Lang]

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
> > > >>
> >
> 

Re: [PATCH] generic/471: Remove this broken case

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/471: Remove this broken case

[Filipe Manana]

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

Re: [PATCH] generic/471: Remove this broken case

[Yang Xu (Fujitsu)]

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

Re: [PATCH] generic/471: Remove this broken case

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com