[PATCHSET] fstests: random fixes for v2024.02.09

[PATCHSET] fstests: random fixes for v2024.02.09

[Darrick J. Wong]

Hi all,

Here's the usual odd fixes for fstests.  Most of these are cleanups and
bug fixes that have been aging in my djwong-wtf branch forever.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been running on the djcloud for months with no problems.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
Commits in this patchset:
 * generic/604: try to make race occur reliably
 * xfs/155: fail the test if xfs_repair hangs for too long
 * generic/192: fix spurious timeout?
 * generic/491: increase test timeout
 * xfs/599: reduce the amount of attrs created here
 * xfs/122: update test to pick up rtword/suminfo ondisk unions
 * xfs/43[4-6]: make module reloading optional
 * xfs: test for premature ENOSPC with large cow delalloc extents
---
 common/module      |   34 ++++++++++++++++++---
 common/rc          |   14 +++++++++
 tests/generic/192  |   16 ++++++++--
 tests/generic/491  |    2 +
 tests/generic/604  |    7 ++--
 tests/xfs/122      |    2 +
 tests/xfs/122.out  |    2 +
 tests/xfs/155      |    4 ++
 tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/1923.out |    8 +++++
 tests/xfs/434      |    3 +-
 tests/xfs/435      |    3 +-
 tests/xfs/436      |    3 +-
 tests/xfs/599      |    9 ++----
 14 files changed, 168 insertions(+), 24 deletions(-)
 create mode 100755 tests/xfs/1923
 create mode 100644 tests/xfs/1923.out

[PATCH 1/8] generic/604: try to make race occur reliably

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This test will occasionaly fail like so:

--- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
+++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
@@ -1,2 +1,5 @@
 QA output created by 604
-Silence is golden
+mount: /opt: /dev/sda4 already mounted on /opt.
+       dmesg(1) may have more information after failed mount system call.
+mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
+(see /var/tmp/fstests/generic/604.full for details)

As far as I can tell, the cause of this seems to be _scratch_mount
getting forked and exec'd before the backgrounded umount process has a
chance to enter the kernel.  When this occurs, the mount() system call
will return -EBUSY because this isn't an attempt to make a bind mount.
Slow things down slightly by stalling the mount by 10ms.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/604 |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)


diff --git a/tests/generic/604 b/tests/generic/604
index cc6a4b214f..a0dcdcd58e 100755
--- a/tests/generic/604
+++ b/tests/generic/604
@@ -24,10 +24,11 @@ _scratch_mount
 for i in $(seq 0 500); do
 	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
 done
-# For overlayfs, avoid unmouting the base fs after _scratch_mount
-# tries to mount the base fs
+# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
+# mount the base fs.  Delay the mount attempt by 0.1s in the hope that the
+# mount() call will try to lock s_umount /after/ umount has already taken it.
 $UMOUNT_PROG $SCRATCH_MNT &
-_scratch_mount
+sleep 0.01s ; _scratch_mount
 wait
 
 echo "Silence is golden"

Re: [PATCH 1/8] generic/604: try to make race occur reliably

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:00:47PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This test will occasionaly fail like so:
> 
> --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
> +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
> @@ -1,2 +1,5 @@
>  QA output created by 604
> -Silence is golden
> +mount: /opt: /dev/sda4 already mounted on /opt.
> +       dmesg(1) may have more information after failed mount system call.
> +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
> +(see /var/tmp/fstests/generic/604.full for details)
> 
> As far as I can tell, the cause of this seems to be _scratch_mount
> getting forked and exec'd before the backgrounded umount process has a
> chance to enter the kernel.  When this occurs, the mount() system call
> will return -EBUSY because this isn't an attempt to make a bind mount.
> Slow things down slightly by stalling the mount by 10ms.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/generic/604 |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/tests/generic/604 b/tests/generic/604
> index cc6a4b214f..a0dcdcd58e 100755
> --- a/tests/generic/604
> +++ b/tests/generic/604
> @@ -24,10 +24,11 @@ _scratch_mount
>  for i in $(seq 0 500); do
>  	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
>  done
> -# For overlayfs, avoid unmouting the base fs after _scratch_mount
> -# tries to mount the base fs
> +# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
> +# mount the base fs.  Delay the mount attempt by 0.1s in the hope that the
> +# mount() call will try to lock s_umount /after/ umount has already taken it.
>  $UMOUNT_PROG $SCRATCH_MNT &
> -_scratch_mount
> +sleep 0.01s ; _scratch_mount

0.1s or 0.01s ? Above comment says 0.1s, but it sleeps 0.01s actually :)

The comment of g/604 says "Evicting dirty inodes can take a long time during
umount." So how long time makes sense, how long is the bug?

Thanks,
Zorro

>  wait
>  
>  echo "Silence is golden"
> 

Re: [PATCH 1/8] generic/604: try to make race occur reliably

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 12:04:49PM +0800, Zorro Lang wrote:
> On Mon, Feb 26, 2024 at 06:00:47PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > This test will occasionaly fail like so:
> > 
> > --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
> > +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
> > @@ -1,2 +1,5 @@
> >  QA output created by 604
> > -Silence is golden
> > +mount: /opt: /dev/sda4 already mounted on /opt.
> > +       dmesg(1) may have more information after failed mount system call.
> > +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
> > +(see /var/tmp/fstests/generic/604.full for details)
> > 
> > As far as I can tell, the cause of this seems to be _scratch_mount
> > getting forked and exec'd before the backgrounded umount process has a
> > chance to enter the kernel.  When this occurs, the mount() system call
> > will return -EBUSY because this isn't an attempt to make a bind mount.
> > Slow things down slightly by stalling the mount by 10ms.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/generic/604 |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/tests/generic/604 b/tests/generic/604
> > index cc6a4b214f..a0dcdcd58e 100755
> > --- a/tests/generic/604
> > +++ b/tests/generic/604
> > @@ -24,10 +24,11 @@ _scratch_mount
> >  for i in $(seq 0 500); do
> >  	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
> >  done
> > -# For overlayfs, avoid unmouting the base fs after _scratch_mount
> > -# tries to mount the base fs
> > +# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
> > +# mount the base fs.  Delay the mount attempt by 0.1s in the hope that the
> > +# mount() call will try to lock s_umount /after/ umount has already taken it.
> >  $UMOUNT_PROG $SCRATCH_MNT &
> > -_scratch_mount
> > +sleep 0.01s ; _scratch_mount
> 
> 0.1s or 0.01s ? Above comment says 0.1s, but it sleeps 0.01s actually :)
> 
> The comment of g/604 says "Evicting dirty inodes can take a long time during
> umount." So how long time makes sense, how long is the bug?

/me has no idea. :/

The 10ms delay is a (racy bs) to try to to let the umount run just
enough that the mount will either end up blocked until the unmount
succeeds or find no contention and no mount either.

--D

> Thanks,
> Zorro
> 
> >  wait
> >  
> >  echo "Silence is golden"
> > 
> 
> 

[PATCH v1.1 1/8] generic/604: try to make race occur reliably

[Darrick J. Wong]

This test will occasionaly fail like so:

  --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
  +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
  @@ -1,2 +1,5 @@
   QA output created by 604
  -Silence is golden
  +mount: /opt: /dev/sda4 already mounted on /opt.
  +       dmesg(1) may have more information after failed mount system call.
  +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
  +(see /var/tmp/fstests/generic/604.full for details)

As far as I can tell, the cause of this seems to be _scratch_mount
getting forked and exec'd before the backgrounded umount process has a
chance to enter the kernel.  When this occurs, the mount() system call
will return -EBUSY because this isn't an attempt to make a bind mount.
Slow things down slightly by stalling the mount by 10ms.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: indent commit message, fix busted comment
---
 tests/generic/604 |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/generic/604 b/tests/generic/604
index cc6a4b214f..00da56dd70 100755
--- a/tests/generic/604
+++ b/tests/generic/604
@@ -24,10 +24,12 @@ _scratch_mount
 for i in $(seq 0 500); do
 	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
 done
-# For overlayfs, avoid unmouting the base fs after _scratch_mount
-# tries to mount the base fs
+# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
+# mount the base fs.  Delay the mount attempt by a small amount in the hope
+# that the mount() call will try to lock s_umount /after/ umount has already
+# taken it.
 $UMOUNT_PROG $SCRATCH_MNT &
-_scratch_mount
+sleep 0.01s ; _scratch_mount
 wait
 
 echo "Silence is golden"

Re: [PATCH v1.1 1/8] generic/604: try to make race occur reliably

[Zorro Lang]

On Mon, Feb 26, 2024 at 08:40:21PM -0800, Darrick J. Wong wrote:
> This test will occasionaly fail like so:
> 
>   --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
>   +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
>   @@ -1,2 +1,5 @@
>    QA output created by 604
>   -Silence is golden
>   +mount: /opt: /dev/sda4 already mounted on /opt.
>   +       dmesg(1) may have more information after failed mount system call.
>   +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
>   +(see /var/tmp/fstests/generic/604.full for details)
> 
> As far as I can tell, the cause of this seems to be _scratch_mount
> getting forked and exec'd before the backgrounded umount process has a
> chance to enter the kernel.  When this occurs, the mount() system call
> will return -EBUSY because this isn't an attempt to make a bind mount.
> Slow things down slightly by stalling the mount by 10ms.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: indent commit message, fix busted comment
> ---

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/generic/604 |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/generic/604 b/tests/generic/604
> index cc6a4b214f..00da56dd70 100755
> --- a/tests/generic/604
> +++ b/tests/generic/604
> @@ -24,10 +24,12 @@ _scratch_mount
>  for i in $(seq 0 500); do
>  	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
>  done
> -# For overlayfs, avoid unmouting the base fs after _scratch_mount
> -# tries to mount the base fs
> +# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
> +# mount the base fs.  Delay the mount attempt by a small amount in the hope
> +# that the mount() call will try to lock s_umount /after/ umount has already
> +# taken it.
>  $UMOUNT_PROG $SCRATCH_MNT &
> -_scratch_mount
> +sleep 0.01s ; _scratch_mount
>  wait
>  
>  echo "Silence is golden"
> 

Re: [PATCH v1.1 1/8] generic/604: try to make race occur reliably

[Christoph Hellwig]

On Mon, Feb 26, 2024 at 08:40:21PM -0800, Darrick J. Wong wrote:
> This test will occasionaly fail like so:
> 
>   --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
>   +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
>   @@ -1,2 +1,5 @@
>    QA output created by 604
>   -Silence is golden
>   +mount: /opt: /dev/sda4 already mounted on /opt.
>   +       dmesg(1) may have more information after failed mount system call.
>   +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
>   +(see /var/tmp/fstests/generic/604.full for details)
> 
> As far as I can tell, the cause of this seems to be _scratch_mount
> getting forked and exec'd before the backgrounded umount process has a
> chance to enter the kernel.  When this occurs, the mount() system call
> will return -EBUSY because this isn't an attempt to make a bind mount.
> Slow things down slightly by stalling the mount by 10ms.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: indent commit message, fix busted comment
> ---
>  tests/generic/604 |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/generic/604 b/tests/generic/604
> index cc6a4b214f..00da56dd70 100755
> --- a/tests/generic/604
> +++ b/tests/generic/604
> @@ -24,10 +24,12 @@ _scratch_mount
>  for i in $(seq 0 500); do
>  	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
>  done
> -# For overlayfs, avoid unmouting the base fs after _scratch_mount
> -# tries to mount the base fs
> +# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to

s/unmouting/unmounting/ ?

Re: [PATCH v1.1 1/8] generic/604: try to make race occur reliably

[Zorro Lang]

On Tue, Feb 27, 2024 at 06:52:21AM -0800, Christoph Hellwig wrote:
> On Mon, Feb 26, 2024 at 08:40:21PM -0800, Darrick J. Wong wrote:
> > This test will occasionaly fail like so:
> > 
> >   --- /tmp/fstests/tests/generic/604.out	2024-02-03 12:08:52.349924277 -0800
> >   +++ /var/tmp/fstests/generic/604.out.bad	2024-02-05 04:35:55.020000000 -0800
> >   @@ -1,2 +1,5 @@
> >    QA output created by 604
> >   -Silence is golden
> >   +mount: /opt: /dev/sda4 already mounted on /opt.
> >   +       dmesg(1) may have more information after failed mount system call.
> >   +mount -o usrquota,grpquota,prjquota, /dev/sda4 /opt failed
> >   +(see /var/tmp/fstests/generic/604.full for details)
> > 
> > As far as I can tell, the cause of this seems to be _scratch_mount
> > getting forked and exec'd before the backgrounded umount process has a
> > chance to enter the kernel.  When this occurs, the mount() system call
> > will return -EBUSY because this isn't an attempt to make a bind mount.
> > Slow things down slightly by stalling the mount by 10ms.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: indent commit message, fix busted comment
> > ---
> >  tests/generic/604 |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/generic/604 b/tests/generic/604
> > index cc6a4b214f..00da56dd70 100755
> > --- a/tests/generic/604
> > +++ b/tests/generic/604
> > @@ -24,10 +24,12 @@ _scratch_mount
> >  for i in $(seq 0 500); do
> >  	$XFS_IO_PROG -f -c "pwrite 0 4K" $SCRATCH_MNT/$i >/dev/null
> >  done
> > -# For overlayfs, avoid unmouting the base fs after _scratch_mount
> > -# tries to mount the base fs
> > +# For overlayfs, avoid unmouting the base fs after _scratch_mount tries to
> 
> s/unmouting/unmounting/ ?

Sure, I've changed that when I merged it. Thanks for pointing out that.

> 
> 

[PATCH 2/8] xfs/155: fail the test if xfs_repair hangs for too long

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

There are a few hard to reproduce bugs in xfs_repair where it can
deadlock trying to lock a buffer that it already owns.  These stalls
cause fstests never to finish, which is annoying!  To fix this, set up
the xfs_repair run to abort after 10 minutes, which will affect the
golden output and capture a core file.

This doesn't fix xfs_repair, obviously.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/155 |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/tests/xfs/155 b/tests/xfs/155
index 302607b510..fba557bff6 100755
--- a/tests/xfs/155
+++ b/tests/xfs/155
@@ -27,6 +27,10 @@ _require_scratch_xfs_crc		# needsrepair only exists for v5
 _require_populate_commands
 _require_libxfs_debug_flag LIBXFS_DEBUG_WRITE_CRASH
 
+# Inject a 10 minute abortive timeout on the repair program so that deadlocks
+# in the program do not cause fstests to hang indefinitely.
+XFS_REPAIR_PROG="timeout -s ABRT 10m $XFS_REPAIR_PROG"
+
 # Populate the filesystem
 _scratch_populate_cached nofill >> $seqres.full 2>&1
 

Re: [PATCH 2/8] xfs/155: fail the test if xfs_repair hangs for too long

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:01:03PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> There are a few hard to reproduce bugs in xfs_repair where it can
> deadlock trying to lock a buffer that it already owns.  These stalls
> cause fstests never to finish, which is annoying!  To fix this, set up
> the xfs_repair run to abort after 10 minutes, which will affect the
> golden output and capture a core file.
> 
> This doesn't fix xfs_repair, obviously.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/155 |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> 
> diff --git a/tests/xfs/155 b/tests/xfs/155
> index 302607b510..fba557bff6 100755
> --- a/tests/xfs/155
> +++ b/tests/xfs/155
> @@ -27,6 +27,10 @@ _require_scratch_xfs_crc		# needsrepair only exists for v5
>  _require_populate_commands
>  _require_libxfs_debug_flag LIBXFS_DEBUG_WRITE_CRASH
>  
> +# Inject a 10 minute abortive timeout on the repair program so that deadlocks
> +# in the program do not cause fstests to hang indefinitely.
> +XFS_REPAIR_PROG="timeout -s ABRT 10m $XFS_REPAIR_PROG"

Others cases of fstests always do:
  _require_command "$TIMEOUT_PROG" timeout
before using timeout.

Others looks good to me, as you only change single one case, it won't affect other testing.
Just hope the 10 minutes is enough even if on a big storage :)

Thanks,
Zorro

> +
>  # Populate the filesystem
>  _scratch_populate_cached nofill >> $seqres.full 2>&1
>  
> 

[PATCH v1.1 2/8] xfs/155: fail the test if xfs_repair hangs for too long

[Darrick J. Wong]

There are a few hard to reproduce bugs in xfs_repair where it can
deadlock trying to lock a buffer that it already owns.  These stalls
cause fstests never to finish, which is annoying!  To fix this, set up
the xfs_repair run to abort after 10 minutes, which will affect the
golden output and capture a core file.

This doesn't fix xfs_repair, obviously.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: require timeout command
---
 tests/xfs/155 |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/xfs/155 b/tests/xfs/155
index 302607b510..3181bfaf6f 100755
--- a/tests/xfs/155
+++ b/tests/xfs/155
@@ -26,6 +26,11 @@ _require_scratch_nocheck
 _require_scratch_xfs_crc		# needsrepair only exists for v5
 _require_populate_commands
 _require_libxfs_debug_flag LIBXFS_DEBUG_WRITE_CRASH
+_require_command "$TIMEOUT_PROG" timeout
+
+# Inject a 10 minute abortive timeout on the repair program so that deadlocks
+# in the program do not cause fstests to hang indefinitely.
+XFS_REPAIR_PROG="timeout -s ABRT 10m $XFS_REPAIR_PROG"
 
 # Populate the filesystem
 _scratch_populate_cached nofill >> $seqres.full 2>&1

Re: [PATCH v1.1 2/8] xfs/155: fail the test if xfs_repair hangs for too long

[Zorro Lang]

On Mon, Feb 26, 2024 at 08:41:00PM -0800, Darrick J. Wong wrote:
> There are a few hard to reproduce bugs in xfs_repair where it can
> deadlock trying to lock a buffer that it already owns.  These stalls
> cause fstests never to finish, which is annoying!  To fix this, set up
> the xfs_repair run to abort after 10 minutes, which will affect the
> golden output and capture a core file.
> 
> This doesn't fix xfs_repair, obviously.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: require timeout command
> ---
>  tests/xfs/155 |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/xfs/155 b/tests/xfs/155
> index 302607b510..3181bfaf6f 100755
> --- a/tests/xfs/155
> +++ b/tests/xfs/155
> @@ -26,6 +26,11 @@ _require_scratch_nocheck
>  _require_scratch_xfs_crc		# needsrepair only exists for v5
>  _require_populate_commands
>  _require_libxfs_debug_flag LIBXFS_DEBUG_WRITE_CRASH
> +_require_command "$TIMEOUT_PROG" timeout
> +
> +# Inject a 10 minute abortive timeout on the repair program so that deadlocks
> +# in the program do not cause fstests to hang indefinitely.
> +XFS_REPAIR_PROG="timeout -s ABRT 10m $XFS_REPAIR_PROG"

I'll help to change the time to $TIMEOUT_PROG when I merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  
>  # Populate the filesystem
>  _scratch_populate_cached nofill >> $seqres.full 2>&1
> 

Re: [PATCH v1.1 2/8] xfs/155: fail the test if xfs_repair hangs for too long

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/8] generic/192: fix spurious timeout

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

I have a theory that when the nfs server that hosts the root fs for my
testing VMs gets backed up, it can take a while for path resolution and
loading of echo, cat, or tee to finish.  That delays the test enough to
result in:

--- /tmp/fstests/tests/generic/192.out	2023-11-29 15:40:52.715517458 -0800
+++ /var/tmp/fstests/generic/192.out.bad	2023-12-15 21:28:02.860000000 -0800
@@ -1,5 +1,6 @@
 QA output created by 192
 sleep for 5 seconds
 test
-delta1 is in range
+delta1 has value of 12
+delta1 is NOT in range 5 .. 7
 delta2 is in range

Therefore, invoke all these utilities with --help before the critical
section to make sure they're all in memory.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/192 |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)


diff --git a/tests/generic/192 b/tests/generic/192
index 0d3cd03b4b..2825486635 100755
--- a/tests/generic/192
+++ b/tests/generic/192
@@ -29,17 +29,27 @@ delay=5
 testfile=$TEST_DIR/testfile
 rm -f $testfile
 
+# Preload every binary used between sampling time1 and time2 so that loading
+# them has minimal overhead even if the root fs is hosted over a slow network.
+# Also don't put pipe and tee creation in that critical section.
+for i in echo stat sleep cat; do
+	$i --help &>/dev/null
+done
+
 echo test >$testfile
-time1=`_access_time $testfile | tee -a $seqres.full`
+time1=`_access_time $testfile`
+echo $time1 >> $seqres.full
 
 echo "sleep for $delay seconds"
 sleep $delay # sleep to allow time to move on for access
 cat $testfile
-time2=`_access_time $testfile | tee -a $seqres.full`
+time2=`_access_time $testfile`
+echo $time2 >> $seqres.full
 
 cd /
 _test_cycle_mount
-time3=`_access_time $testfile | tee -a $seqres.full`
+time3=`_access_time $testfile`
+echo $time3 >> $seqres.full
 
 delta1=`expr $time2 - $time1`
 delta2=`expr $time3 - $time1`

Re: [PATCH 3/8] generic/192: fix spurious timeout

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:01:19PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I have a theory that when the nfs server that hosts the root fs for my
> testing VMs gets backed up, it can take a while for path resolution and
> loading of echo, cat, or tee to finish.  That delays the test enough to
> result in:
> 
> --- /tmp/fstests/tests/generic/192.out	2023-11-29 15:40:52.715517458 -0800
> +++ /var/tmp/fstests/generic/192.out.bad	2023-12-15 21:28:02.860000000 -0800
> @@ -1,5 +1,6 @@
>  QA output created by 192
>  sleep for 5 seconds
>  test
> -delta1 is in range
> +delta1 has value of 12
> +delta1 is NOT in range 5 .. 7
>  delta2 is in range



> 
> Therefore, invoke all these utilities with --help before the critical
> section to make sure they're all in memory.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

This patch makes sense to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

Just better to give 1 or 2 whitespaces to diff output message (especially the
lines with "+") in commit log :) I always need to change that manually before
merge the patch :-D

Thanks,
Zorro

>  tests/generic/192 |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/tests/generic/192 b/tests/generic/192
> index 0d3cd03b4b..2825486635 100755
> --- a/tests/generic/192
> +++ b/tests/generic/192
> @@ -29,17 +29,27 @@ delay=5
>  testfile=$TEST_DIR/testfile
>  rm -f $testfile
>  
> +# Preload every binary used between sampling time1 and time2 so that loading
> +# them has minimal overhead even if the root fs is hosted over a slow network.
> +# Also don't put pipe and tee creation in that critical section.
> +for i in echo stat sleep cat; do
> +	$i --help &>/dev/null
> +done
> +
>  echo test >$testfile
> -time1=`_access_time $testfile | tee -a $seqres.full`
> +time1=`_access_time $testfile`
> +echo $time1 >> $seqres.full
>  
>  echo "sleep for $delay seconds"
>  sleep $delay # sleep to allow time to move on for access
>  cat $testfile
> -time2=`_access_time $testfile | tee -a $seqres.full`
> +time2=`_access_time $testfile`
> +echo $time2 >> $seqres.full
>  
>  cd /
>  _test_cycle_mount
> -time3=`_access_time $testfile | tee -a $seqres.full`
> +time3=`_access_time $testfile`
> +echo $time3 >> $seqres.full
>  
>  delta1=`expr $time2 - $time1`
>  delta2=`expr $time3 - $time1`
> 

Re: [PATCH 3/8] generic/192: fix spurious timeout

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 12:23:46PM +0800, Zorro Lang wrote:
> On Mon, Feb 26, 2024 at 06:01:19PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > I have a theory that when the nfs server that hosts the root fs for my
> > testing VMs gets backed up, it can take a while for path resolution and
> > loading of echo, cat, or tee to finish.  That delays the test enough to
> > result in:
> > 
> > --- /tmp/fstests/tests/generic/192.out	2023-11-29 15:40:52.715517458 -0800
> > +++ /var/tmp/fstests/generic/192.out.bad	2023-12-15 21:28:02.860000000 -0800
> > @@ -1,5 +1,6 @@
> >  QA output created by 192
> >  sleep for 5 seconds
> >  test
> > -delta1 is in range
> > +delta1 has value of 12
> > +delta1 is NOT in range 5 .. 7
> >  delta2 is in range
> 
> 
> 
> > 
> > Therefore, invoke all these utilities with --help before the critical
> > section to make sure they're all in memory.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> This patch makes sense to me,
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> Just better to give 1 or 2 whitespaces to diff output message (especially the
> lines with "+") in commit log :) I always need to change that manually before
> merge the patch :-D

Oh, you mean indenting the diff output in the commit message?

Yeah, I'll try to remember that from now on:

I have a theory that when the nfs server that hosts the root fs for my
testing VMs gets backed up, it can take a while for path resolution and
loading of echo, cat, or tee to finish.  That delays the test enough to
result in:

  --- /tmp/fstests/tests/generic/192.out	2023-11-29 15:40:52.715517458 -0800
  +++ /var/tmp/fstests/generic/192.out.bad	2023-12-15 21:28:02.860000000 -0800
  @@ -1,5 +1,6 @@
   QA output created by 192
   sleep for 5 seconds
   test
  -delta1 is in range
  +delta1 has value of 12
  +delta1 is NOT in range 5 .. 7
   delta2 is in range

Therefore, invoke all these utilities with --help before the critical
section to make sure they're all in memory.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>

--D

> Thanks,
> Zorro
> 
> >  tests/generic/192 |   16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/tests/generic/192 b/tests/generic/192
> > index 0d3cd03b4b..2825486635 100755
> > --- a/tests/generic/192
> > +++ b/tests/generic/192
> > @@ -29,17 +29,27 @@ delay=5
> >  testfile=$TEST_DIR/testfile
> >  rm -f $testfile
> >  
> > +# Preload every binary used between sampling time1 and time2 so that loading
> > +# them has minimal overhead even if the root fs is hosted over a slow network.
> > +# Also don't put pipe and tee creation in that critical section.
> > +for i in echo stat sleep cat; do
> > +	$i --help &>/dev/null
> > +done
> > +
> >  echo test >$testfile
> > -time1=`_access_time $testfile | tee -a $seqres.full`
> > +time1=`_access_time $testfile`
> > +echo $time1 >> $seqres.full
> >  
> >  echo "sleep for $delay seconds"
> >  sleep $delay # sleep to allow time to move on for access
> >  cat $testfile
> > -time2=`_access_time $testfile | tee -a $seqres.full`
> > +time2=`_access_time $testfile`
> > +echo $time2 >> $seqres.full
> >  
> >  cd /
> >  _test_cycle_mount
> > -time3=`_access_time $testfile | tee -a $seqres.full`
> > +time3=`_access_time $testfile`
> > +echo $time3 >> $seqres.full
> >  
> >  delta1=`expr $time2 - $time1`
> >  delta2=`expr $time3 - $time1`
> > 
> 
> 

Re: [PATCH 3/8] generic/192: fix spurious timeout

[Christoph Hellwig]

On Mon, Feb 26, 2024 at 06:01:19PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I have a theory that when the nfs server that hosts the root fs for my
> testing VMs gets backed up, it can take a while for path resolution and
> loading of echo, cat, or tee to finish.  That delays the test enough to
> result in:

Heh, I've seen these warnings quite a lot in the past, but not recently.

The change looks good to me, though:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/8] generic/491: increase test timeout

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Bump the read timeout in this test to a few seconds just in case it
actually takes the IO system more than a second to retrieve the data
(e.g. cloud storage network lag).

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/491 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/tests/generic/491 b/tests/generic/491
index 797b08d506..5a586c122a 100755
--- a/tests/generic/491
+++ b/tests/generic/491
@@ -44,7 +44,7 @@ xfs_freeze -f $SCRATCH_MNT
 
 # Read file while filesystem is frozen should succeed
 # without blocking
-$TIMEOUT_PROG -s KILL 1s cat $testfile
+$TIMEOUT_PROG -s KILL 5s cat $testfile
 
 xfs_freeze -u $SCRATCH_MNT
 

Re: [PATCH 4/8] generic/491: increase test timeout

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:01:34PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Bump the read timeout in this test to a few seconds just in case it
> actually takes the IO system more than a second to retrieve the data
> (e.g. cloud storage network lag).
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/generic/491 |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/tests/generic/491 b/tests/generic/491
> index 797b08d506..5a586c122a 100755
> --- a/tests/generic/491
> +++ b/tests/generic/491
> @@ -44,7 +44,7 @@ xfs_freeze -f $SCRATCH_MNT
>  
>  # Read file while filesystem is frozen should succeed
>  # without blocking
> -$TIMEOUT_PROG -s KILL 1s cat $testfile
> +$TIMEOUT_PROG -s KILL 5s cat $testfile
>  
>  xfs_freeze -u $SCRATCH_MNT
>  
> 

Re: [PATCH 4/8] generic/491: increase test timeout

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/8] xfs/599: reduce the amount of attrs created here

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Luis Chamberlain reported insane runtimes in this test:

"xfs/599 takes a long time on LBS, but it passes. The amount of time it
takes, however, begs the question if the test is could be trimmed to do
less work because the larger the block size the larger the number of
dirents and xattrs are used to create. The large dirents are not a
problem. The amount of time it takes to create xattrs with hashcol
however grows exponentially in time.

"n=16k   takes 5   seconds
"n=32k   takes 30  seconds
"n=64k     takes 6-7 minutes
"n=1048576 takes 30 hours

"n=1048576 is what we use for block size 32k.

"Do we really need so many xattrs for larger block sizes for this test?"

No, we don't.  The goal of this test is to create a two-level dabtree of
xattrs having identical hashes.  However, the test author (me)
apparently forgot that if a dabtree is created in the attr fork, there
will be a dabtree entry for each extended attribute, not each attr leaf
block.  Hence it's a waste of time to multiply da_records_per_block by
attr_records_per_block.

Reported-by: Luis Chamberlain <mcgrof@kernel.org>
Fixes: 1cd6b61299 ("xfs: add a couple more tests for ascii-ci problems")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/599 |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)


diff --git a/tests/xfs/599 b/tests/xfs/599
index b55b62d7f5..57a797f0f5 100755
--- a/tests/xfs/599
+++ b/tests/xfs/599
@@ -43,14 +43,13 @@ longname="$(mktemp --dry-run "$(perl -e 'print "X" x 255;')" | tr ' ' 'X')"
 echo "creating $nr_dirents dirents from '$longname'" >> $seqres.full
 _scratch_xfs_db -r -c "hashcoll -n $nr_dirents -p $crash_dir $longname"
 
-# Create enough xattrs to fill two dabtree nodes.  Each attribute leaf block
-# gets its own record in the dabtree, so we have to create enough attr blocks
-# (each full of attrs) to get a dabtree of at least height 2.
+# Create enough xattrs to fill two dabtree nodes.  Each attribute entry gets
+# its own record in the dabtree, so we have to create enough attributes to get
+# a dabtree of at least height 2.
 blksz=$(_get_block_size "$SCRATCH_MNT")
 
-attr_records_per_block=$((blksz / 255))
 da_records_per_block=$((blksz / 8))	# 32-bit hash and 32-bit before
-nr_attrs=$((da_records_per_block * attr_records_per_block * 2))
+nr_attrs=$((da_records_per_block * 2))
 
 longname="$(mktemp --dry-run "$(perl -e 'print "X" x 249;')" | tr ' ' 'X')"
 echo "creating $nr_attrs attrs from '$longname'" >> $seqres.full

Re: [PATCH 5/8] xfs/599: reduce the amount of attrs created here

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:01:50PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Luis Chamberlain reported insane runtimes in this test:
> 
> "xfs/599 takes a long time on LBS, but it passes. The amount of time it
> takes, however, begs the question if the test is could be trimmed to do
> less work because the larger the block size the larger the number of
> dirents and xattrs are used to create. The large dirents are not a
> problem. The amount of time it takes to create xattrs with hashcol
> however grows exponentially in time.
> 
> "n=16k   takes 5   seconds
> "n=32k   takes 30  seconds
> "n=64k     takes 6-7 minutes
> "n=1048576 takes 30 hours
> 
> "n=1048576 is what we use for block size 32k.
> 
> "Do we really need so many xattrs for larger block sizes for this test?"
> 
> No, we don't.  The goal of this test is to create a two-level dabtree of
> xattrs having identical hashes.  However, the test author (me)
> apparently forgot that if a dabtree is created in the attr fork, there
> will be a dabtree entry for each extended attribute, not each attr leaf
> block.  Hence it's a waste of time to multiply da_records_per_block by
> attr_records_per_block.
> 
> Reported-by: Luis Chamberlain <mcgrof@kernel.org>
> Fixes: 1cd6b61299 ("xfs: add a couple more tests for ascii-ci problems")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Thanks for this fix, it save much time for us too :)

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/xfs/599 |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/tests/xfs/599 b/tests/xfs/599
> index b55b62d7f5..57a797f0f5 100755
> --- a/tests/xfs/599
> +++ b/tests/xfs/599
> @@ -43,14 +43,13 @@ longname="$(mktemp --dry-run "$(perl -e 'print "X" x 255;')" | tr ' ' 'X')"
>  echo "creating $nr_dirents dirents from '$longname'" >> $seqres.full
>  _scratch_xfs_db -r -c "hashcoll -n $nr_dirents -p $crash_dir $longname"
>  
> -# Create enough xattrs to fill two dabtree nodes.  Each attribute leaf block
> -# gets its own record in the dabtree, so we have to create enough attr blocks
> -# (each full of attrs) to get a dabtree of at least height 2.
> +# Create enough xattrs to fill two dabtree nodes.  Each attribute entry gets
> +# its own record in the dabtree, so we have to create enough attributes to get
> +# a dabtree of at least height 2.
>  blksz=$(_get_block_size "$SCRATCH_MNT")
>  
> -attr_records_per_block=$((blksz / 255))
>  da_records_per_block=$((blksz / 8))	# 32-bit hash and 32-bit before
> -nr_attrs=$((da_records_per_block * attr_records_per_block * 2))
> +nr_attrs=$((da_records_per_block * 2))
>  
>  longname="$(mktemp --dry-run "$(perl -e 'print "X" x 249;')" | tr ' ' 'X')"
>  echo "creating $nr_attrs attrs from '$longname'" >> $seqres.full
> 

[PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Update this test to check that the ondisk unions for rt bitmap word and
rt summary counts are always the correct size.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/122     |    2 +-
 tests/xfs/122.out |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)


diff --git a/tests/xfs/122 b/tests/xfs/122
index ba927c77c4..4e5ba1dfee 100755
--- a/tests/xfs/122
+++ b/tests/xfs/122
@@ -195,7 +195,7 @@ echo 'int main(int argc, char *argv[]) {' >>$cprog
 #
 cat /usr/include/xfs/xfs*.h | indent |\
 _attribute_filter |\
-grep -E '(} *xfs_.*_t|^struct xfs_[a-z0-9_]*$)' |\
+grep -E '(} *xfs_.*_t|^(union|struct) xfs_[a-z0-9_]*$)' |\
 grep -E -v -f $tmp.ignore |\
 sed -e 's/^.*}[[:space:]]*//g' -e 's/;.*$//g' -e 's/_t, /_t\n/g' |\
 sort | uniq |\
diff --git a/tests/xfs/122.out b/tests/xfs/122.out
index 067a0ec76b..a2b57cfb9b 100644
--- a/tests/xfs/122.out
+++ b/tests/xfs/122.out
@@ -124,6 +124,8 @@ sizeof(struct xfs_swap_extent) = 64
 sizeof(struct xfs_sxd_log_format) = 16
 sizeof(struct xfs_sxi_log_format) = 80
 sizeof(struct xfs_unmount_log_format) = 8
+sizeof(union xfs_rtword_raw) = 4
+sizeof(union xfs_suminfo_raw) = 4
 sizeof(xfs_agf_t) = 224
 sizeof(xfs_agfl_t) = 36
 sizeof(xfs_agi_t) = 344

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:02:05PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Update this test to check that the ondisk unions for rt bitmap word and
> rt summary counts are always the correct size.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/xfs/122     |    2 +-
>  tests/xfs/122.out |    2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/122 b/tests/xfs/122
> index ba927c77c4..4e5ba1dfee 100755
> --- a/tests/xfs/122
> +++ b/tests/xfs/122
> @@ -195,7 +195,7 @@ echo 'int main(int argc, char *argv[]) {' >>$cprog
>  #
>  cat /usr/include/xfs/xfs*.h | indent |\
>  _attribute_filter |\
> -grep -E '(} *xfs_.*_t|^struct xfs_[a-z0-9_]*$)' |\
> +grep -E '(} *xfs_.*_t|^(union|struct) xfs_[a-z0-9_]*$)' |\
>  grep -E -v -f $tmp.ignore |\
>  sed -e 's/^.*}[[:space:]]*//g' -e 's/;.*$//g' -e 's/_t, /_t\n/g' |\
>  sort | uniq |\
> diff --git a/tests/xfs/122.out b/tests/xfs/122.out
> index 067a0ec76b..a2b57cfb9b 100644
> --- a/tests/xfs/122.out
> +++ b/tests/xfs/122.out
> @@ -124,6 +124,8 @@ sizeof(struct xfs_swap_extent) = 64
>  sizeof(struct xfs_sxd_log_format) = 16
>  sizeof(struct xfs_sxi_log_format) = 80
>  sizeof(struct xfs_unmount_log_format) = 8
> +sizeof(union xfs_rtword_raw) = 4
> +sizeof(union xfs_suminfo_raw) = 4
>  sizeof(xfs_agf_t) = 224
>  sizeof(xfs_agfl_t) = 36
>  sizeof(xfs_agi_t) = 344
> 

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Christoph Hellwig]

Can we please just kill the goddamn test?  Just waiting for the
xfsprogs 6.8 resync to submit the static_asserts for libxfs that
will handle this much better.

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> Can we please just kill the goddamn test?  Just waiting for the
> xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> will handle this much better.

I'll be very happen when we scuttle xfs/122 finally.

However, in theory it's still be useful for QA departments to make sure
that xfsprogs backports (HA!) don't accidentally break things.

IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
detected, not removing it completely.

Unless someone wants to chime in and say that actually, nobody backports
stuff to old xfsprogs?  (We don't really...)

--D

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Christoph Hellwig]

On Tue, Feb 27, 2024 at 05:27:04PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> > Can we please just kill the goddamn test?  Just waiting for the
> > xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> > will handle this much better.
> 
> I'll be very happen when we scuttle xfs/122 finally.
> 
> However, in theory it's still be useful for QA departments to make sure
> that xfsprogs backports (HA!) don't accidentally break things.
> 
> IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
> detected, not removing it completely.
> 
> Unless someone wants to chime in and say that actually, nobody backports
> stuff to old xfsprogs?  (We don't really...)

Well, who is going to backport changes to the on-disk format in a way
that is complex enough to change strutures, and not also backport the
patch to actually check the sizes?  Sounds like a weird use case to
optimize for.

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Darrick J. Wong]

On Wed, Feb 28, 2024 at 07:39:29AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 27, 2024 at 05:27:04PM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> > > Can we please just kill the goddamn test?  Just waiting for the
> > > xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> > > will handle this much better.
> > 
> > I'll be very happen when we scuttle xfs/122 finally.
> > 
> > However, in theory it's still be useful for QA departments to make sure
> > that xfsprogs backports (HA!) don't accidentally break things.
> > 
> > IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
> > detected, not removing it completely.
> > 
> > Unless someone wants to chime in and say that actually, nobody backports
> > stuff to old xfsprogs?  (We don't really...)
> 
> Well, who is going to backport changes to the on-disk format in a way
> that is complex enough to change strutures, and not also backport the
> patch to actually check the sizes?  Sounds like a weird use case to
> optimize for.

It turns out that xfs/122 also captures ioctl structure sizes, and those
are /not/ captured by xfs_ondisk.h.  I think we should add those before
we kill xfs/122.

--D

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Christoph Hellwig]

On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> It turns out that xfs/122 also captures ioctl structure sizes, and those
> are /not/ captured by xfs_ondisk.h.  I think we should add those before
> we kill xfs/122.

Sure, I can look into that.

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Zorro Lang]

On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > we kill xfs/122.
> 
> Sure, I can look into that.

Hi Darrick,

Do you still want to have this patch?

Half of this patchset got RVB. As it's a random fix patchset, we can choose
merging those reviewed patches at first. Or you'd like to have them together
in next next release?

Thanks,
Zorro

> 
> 

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Darrick J. Wong]

On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > we kill xfs/122.
> > 
> > Sure, I can look into that.
> 
> Hi Darrick,
> 
> Do you still want to have this patch?
> 
> Half of this patchset got RVB. As it's a random fix patchset, we can choose
> merging those reviewed patches at first. Or you'd like to have them together
> in next next release?

I was about to resend the second to last patch.  If you decide to remove
xfs/122 then I'll drop this one.

--D

> Thanks,
> Zorro
> 
> > 
> > 
> 
> 

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Zorro Lang]

On Fri, Mar 01, 2024 at 09:50:20AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> > On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > > we kill xfs/122.
> > > 
> > > Sure, I can look into that.
> > 
> > Hi Darrick,
> > 
> > Do you still want to have this patch?
> > 
> > Half of this patchset got RVB. As it's a random fix patchset, we can choose
> > merging those reviewed patches at first. Or you'd like to have them together
> > in next next release?
> 
> I was about to resend the second to last patch.  If you decide to remove
> xfs/122 then I'll drop this one.

xfs/122 is a xfs specific test case, it's more important for xfs list than me.
As it doesn't break the fstests testing, I respect the decision from xfs folks,
about keeping or removing it :)

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > 
> > 
> > 
> 

Re: [PATCH 6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

[Darrick J. Wong]

On Sat, Mar 02, 2024 at 12:55:52PM +0800, Zorro Lang wrote:
> On Fri, Mar 01, 2024 at 09:50:20AM -0800, Darrick J. Wong wrote:
> > On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> > > On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > > > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > > > we kill xfs/122.
> > > > 
> > > > Sure, I can look into that.
> > > 
> > > Hi Darrick,
> > > 
> > > Do you still want to have this patch?
> > > 
> > > Half of this patchset got RVB. As it's a random fix patchset, we can choose
> > > merging those reviewed patches at first. Or you'd like to have them together
> > > in next next release?
> > 
> > I was about to resend the second to last patch.  If you decide to remove
> > xfs/122 then I'll drop this one.
> 
> xfs/122 is a xfs specific test case, it's more important for xfs list than me.
> As it doesn't break the fstests testing, I respect the decision from xfs folks,
> about keeping or removing it :)

I think we shouldn't consider dropping this until there's an xfsprogs
release with xfs_ondisk.h in the build process.  Even then, my
preference would be to leave a mark in xfs_db somewhere so that we keep
running this test for old userspace (i.e. the mark isn't found).

--D

> Thanks,
> Zorro
> 
> > 
> > --D
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> 
> 

[PATCH 7/8] xfs/43[4-6]: make module reloading optional

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

These three tests examine two things -- first, can xfs CoW staging
extent recovery handle corruptions in the refcount btree gracefully; and
second, can we avoid leaking incore inodes and dquots.

The only cheap way to check the second condition is to rmmod and
modprobe the XFS module, which triggers leak detection when rmmod tears
down the caches.  Currently, the entire test is _notrun if module
reloading doesn't work.

Unfortunately, these tests never run for the majority of XFS developers
because their testbeds either compile the xfs kernel driver into vmlinux
statically or the rootfs is xfs so the module cannot be reloaded.  The
author's testbed boots from NFS and does not have this limitation.

Because we've had repeated instances of CoW recovery regressions not
being caught by testing until for-next hits my machine, let's make the
module reloading optional in all three tests to improve coverage.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/module |   34 +++++++++++++++++++++++++++++-----
 tests/xfs/434 |    3 +--
 tests/xfs/435 |    3 +--
 tests/xfs/436 |    3 +--
 4 files changed, 32 insertions(+), 11 deletions(-)


diff --git a/common/module b/common/module
index 6efab71d34..f6814be34e 100644
--- a/common/module
+++ b/common/module
@@ -48,12 +48,15 @@ _require_loadable_module()
 	modprobe "${module}" || _notrun "${module} load failed"
 }
 
-# Check that the module for FSTYP can be loaded.
-_require_loadable_fs_module()
+# Test if the module for FSTYP can be unloaded and reloaded.
+#
+# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could
+# not be unloaded; or 3 if loading the module fails.
+_test_loadable_fs_module()
 {
 	local module="$1"
 
-	modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module."
+	modinfo "${module}" > /dev/null 2>&1 || return 1
 
 	# Unload test fs, try to reload module, remount
 	local had_testfs=""
@@ -68,8 +71,29 @@ _require_loadable_fs_module()
 	modprobe "${module}" || load_ok=0
 	test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null
 	test -n "${had_testfs}" && _test_mount 2> /dev/null
-	test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable"
-	test -z "${load_ok}" || _notrun "${module} load failed"
+	test -z "${unload_ok}" || return 2
+	test -z "${load_ok}" || return 3
+	return 0
+}
+
+_require_loadable_fs_module()
+{
+	local module="$1"
+
+	_test_loadable_fs_module "${module}"
+	ret=$?
+	case "$ret" in
+	1)
+		_notrun "${module}: must be a module."
+		;;
+	2)
+		_notrun "${module}: module could not be unloaded"
+		;;
+	3)
+		_notrun "${module}: module reload failed"
+		;;
+	esac
+	return "${ret}"
 }
 
 # Print the value of a filesystem module parameter
diff --git a/tests/xfs/434 b/tests/xfs/434
index 12d1a0c9da..ca80e12753 100755
--- a/tests/xfs/434
+++ b/tests/xfs/434
@@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_quota
 _require_scratch_reflink
 _require_cp_reflink
@@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0
diff --git a/tests/xfs/435 b/tests/xfs/435
index 44135c7653..b52e9287df 100755
--- a/tests/xfs/435
+++ b/tests/xfs/435
@@ -24,7 +24,6 @@ _begin_fstest auto quick clone
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_quota
 _require_scratch_reflink
 _require_cp_reflink
@@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0
diff --git a/tests/xfs/436 b/tests/xfs/436
index d010362785..02bcd66900 100755
--- a/tests/xfs/436
+++ b/tests/xfs/436
@@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_scratch_reflink
 _require_cp_reflink
 _require_xfs_io_command falloc # fsr requires support for preallocation
@@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0

Re: [PATCH 7/8] xfs/43[4-6]: make module reloading optional

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:02:21PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> These three tests examine two things -- first, can xfs CoW staging
> extent recovery handle corruptions in the refcount btree gracefully; and
> second, can we avoid leaking incore inodes and dquots.
> 
> The only cheap way to check the second condition is to rmmod and
> modprobe the XFS module, which triggers leak detection when rmmod tears
> down the caches.  Currently, the entire test is _notrun if module
> reloading doesn't work.
> 
> Unfortunately, these tests never run for the majority of XFS developers
> because their testbeds either compile the xfs kernel driver into vmlinux
> statically or the rootfs is xfs so the module cannot be reloaded.  The
> author's testbed boots from NFS and does not have this limitation.
> 
> Because we've had repeated instances of CoW recovery regressions not
> being caught by testing until for-next hits my machine, let's make the
> module reloading optional in all three tests to improve coverage.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/module |   34 +++++++++++++++++++++++++++++-----
>  tests/xfs/434 |    3 +--
>  tests/xfs/435 |    3 +--
>  tests/xfs/436 |    3 +--
>  4 files changed, 32 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/common/module b/common/module
> index 6efab71d34..f6814be34e 100644
> --- a/common/module
> +++ b/common/module
> @@ -48,12 +48,15 @@ _require_loadable_module()
>  	modprobe "${module}" || _notrun "${module} load failed"
>  }
>  
> -# Check that the module for FSTYP can be loaded.
> -_require_loadable_fs_module()
> +# Test if the module for FSTYP can be unloaded and reloaded.
> +#
> +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could
> +# not be unloaded; or 3 if loading the module fails.
> +_test_loadable_fs_module()
>  {
>  	local module="$1"
>  
> -	modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module."
> +	modinfo "${module}" > /dev/null 2>&1 || return 1
>  
>  	# Unload test fs, try to reload module, remount
>  	local had_testfs=""
> @@ -68,8 +71,29 @@ _require_loadable_fs_module()
>  	modprobe "${module}" || load_ok=0
>  	test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null
>  	test -n "${had_testfs}" && _test_mount 2> /dev/null
> -	test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable"
> -	test -z "${load_ok}" || _notrun "${module} load failed"
> +	test -z "${unload_ok}" || return 2
> +	test -z "${load_ok}" || return 3
> +	return 0
> +}
> +
> +_require_loadable_fs_module()
> +{
> +	local module="$1"
> +
> +	_test_loadable_fs_module "${module}"
> +	ret=$?
> +	case "$ret" in
> +	1)
> +		_notrun "${module}: must be a module."
> +		;;
> +	2)
> +		_notrun "${module}: module could not be unloaded"
> +		;;
> +	3)
> +		_notrun "${module}: module reload failed"
> +		;;
> +	esac
> +	return "${ret}"

I think nobody checks the return value of a _require_xxx helper. The
_require helper generally notrun or keep running. So if ret=0, then
return directly, other return values trigger different _notrun.

>  }
>  
>  # Print the value of a filesystem module parameter
> diff --git a/tests/xfs/434 b/tests/xfs/434
> index 12d1a0c9da..ca80e12753 100755
> --- a/tests/xfs/434
> +++ b/tests/xfs/434
> @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_quota
>  _require_scratch_reflink
>  _require_cp_reflink
> @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"
>  
>  # success, all done
>  status=0
> diff --git a/tests/xfs/435 b/tests/xfs/435
> index 44135c7653..b52e9287df 100755
> --- a/tests/xfs/435
> +++ b/tests/xfs/435
> @@ -24,7 +24,6 @@ _begin_fstest auto quick clone
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_quota
>  _require_scratch_reflink
>  _require_cp_reflink
> @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"

So we don't care about if the fs module reload success or not, just
try it then keep running?

Thanks,
Zorro

>  
>  # success, all done
>  status=0
> diff --git a/tests/xfs/436 b/tests/xfs/436
> index d010362785..02bcd66900 100755
> --- a/tests/xfs/436
> +++ b/tests/xfs/436
> @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_scratch_reflink
>  _require_cp_reflink
>  _require_xfs_io_command falloc # fsr requires support for preallocation
> @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"
>  
>  # success, all done
>  status=0
> 
> 

Re: [PATCH 7/8] xfs/43[4-6]: make module reloading optional

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 01:31:36PM +0800, Zorro Lang wrote:
> On Mon, Feb 26, 2024 at 06:02:21PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > These three tests examine two things -- first, can xfs CoW staging
> > extent recovery handle corruptions in the refcount btree gracefully; and
> > second, can we avoid leaking incore inodes and dquots.
> > 
> > The only cheap way to check the second condition is to rmmod and
> > modprobe the XFS module, which triggers leak detection when rmmod tears
> > down the caches.  Currently, the entire test is _notrun if module
> > reloading doesn't work.
> > 
> > Unfortunately, these tests never run for the majority of XFS developers
> > because their testbeds either compile the xfs kernel driver into vmlinux
> > statically or the rootfs is xfs so the module cannot be reloaded.  The
> > author's testbed boots from NFS and does not have this limitation.
> > 
> > Because we've had repeated instances of CoW recovery regressions not
> > being caught by testing until for-next hits my machine, let's make the
> > module reloading optional in all three tests to improve coverage.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/module |   34 +++++++++++++++++++++++++++++-----
> >  tests/xfs/434 |    3 +--
> >  tests/xfs/435 |    3 +--
> >  tests/xfs/436 |    3 +--
> >  4 files changed, 32 insertions(+), 11 deletions(-)
> > 
> > 
> > diff --git a/common/module b/common/module
> > index 6efab71d34..f6814be34e 100644
> > --- a/common/module
> > +++ b/common/module
> > @@ -48,12 +48,15 @@ _require_loadable_module()
> >  	modprobe "${module}" || _notrun "${module} load failed"
> >  }
> >  
> > -# Check that the module for FSTYP can be loaded.
> > -_require_loadable_fs_module()
> > +# Test if the module for FSTYP can be unloaded and reloaded.
> > +#
> > +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could
> > +# not be unloaded; or 3 if loading the module fails.
> > +_test_loadable_fs_module()
> >  {
> >  	local module="$1"
> >  
> > -	modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module."
> > +	modinfo "${module}" > /dev/null 2>&1 || return 1
> >  
> >  	# Unload test fs, try to reload module, remount
> >  	local had_testfs=""
> > @@ -68,8 +71,29 @@ _require_loadable_fs_module()
> >  	modprobe "${module}" || load_ok=0
> >  	test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null
> >  	test -n "${had_testfs}" && _test_mount 2> /dev/null
> > -	test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable"
> > -	test -z "${load_ok}" || _notrun "${module} load failed"
> > +	test -z "${unload_ok}" || return 2
> > +	test -z "${load_ok}" || return 3
> > +	return 0
> > +}
> > +
> > +_require_loadable_fs_module()
> > +{
> > +	local module="$1"
> > +
> > +	_test_loadable_fs_module "${module}"
> > +	ret=$?
> > +	case "$ret" in
> > +	1)
> > +		_notrun "${module}: must be a module."
> > +		;;
> > +	2)
> > +		_notrun "${module}: module could not be unloaded"
> > +		;;
> > +	3)
> > +		_notrun "${module}: module reload failed"
> > +		;;
> > +	esac
> > +	return "${ret}"
> 
> I think nobody checks the return value of a _require_xxx helper. The
> _require helper generally notrun or keep running. So if ret=0, then
> return directly, other return values trigger different _notrun.

Ok.  It's fine to let it run off the end, then.

> >  }
> >  
> >  # Print the value of a filesystem module parameter
> > diff --git a/tests/xfs/434 b/tests/xfs/434
> > index 12d1a0c9da..ca80e12753 100755
> > --- a/tests/xfs/434
> > +++ b/tests/xfs/434
> > @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr
> >  
> >  # real QA test starts here
> >  _supported_fs xfs
> > -_require_loadable_fs_module "xfs"
> >  _require_quota
> >  _require_scratch_reflink
> >  _require_cp_reflink
> > @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null
> >  rm -f ${RESULT_DIR}/require_scratch
> >  
> >  echo "See if we leak"
> > -_reload_fs_module "xfs"
> > +_test_loadable_fs_module "xfs"
> >  
> >  # success, all done
> >  status=0
> > diff --git a/tests/xfs/435 b/tests/xfs/435
> > index 44135c7653..b52e9287df 100755
> > --- a/tests/xfs/435
> > +++ b/tests/xfs/435
> > @@ -24,7 +24,6 @@ _begin_fstest auto quick clone
> >  
> >  # real QA test starts here
> >  _supported_fs xfs
> > -_require_loadable_fs_module "xfs"
> >  _require_quota
> >  _require_scratch_reflink
> >  _require_cp_reflink
> > @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null
> >  rm -f ${RESULT_DIR}/require_scratch
> >  
> >  echo "See if we leak"
> > -_reload_fs_module "xfs"
> > +_test_loadable_fs_module "xfs"
> 
> So we don't care about if the fs module reload success or not, just
> try it then keep running?

Welll... the "test" actually does everything that we wanted to do
(unmount, rmmod, modprobe, remount) so that's why I use it here.

--D

> Thanks,
> Zorro
> 
> >  
> >  # success, all done
> >  status=0
> > diff --git a/tests/xfs/436 b/tests/xfs/436
> > index d010362785..02bcd66900 100755
> > --- a/tests/xfs/436
> > +++ b/tests/xfs/436
> > @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr
> >  
> >  # real QA test starts here
> >  _supported_fs xfs
> > -_require_loadable_fs_module "xfs"
> >  _require_scratch_reflink
> >  _require_cp_reflink
> >  _require_xfs_io_command falloc # fsr requires support for preallocation
> > @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null
> >  rm -f ${RESULT_DIR}/require_scratch
> >  
> >  echo "See if we leak"
> > -_reload_fs_module "xfs"
> > +_test_loadable_fs_module "xfs"
> >  
> >  # success, all done
> >  status=0
> > 
> > 
> 
> 

[PATCH v1.1 7/8] xfs/43[4-6]: make module reloading optional

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

These three tests examine two things -- first, can xfs CoW staging
extent recovery handle corruptions in the refcount btree gracefully; and
second, can we avoid leaking incore inodes and dquots.

The only cheap way to check the second condition is to rmmod and
modprobe the XFS module, which triggers leak detection when rmmod tears
down the caches.  Currently, the entire test is _notrun if module
reloading doesn't work.

Unfortunately, these tests never run for the majority of XFS developers
because their testbeds either compile the xfs kernel driver into vmlinux
statically or the rootfs is xfs so the module cannot be reloaded.  The
author's testbed boots from NFS and does not have this limitation.

Because we've had repeated instances of CoW recovery regressions not
being caught by testing until for-next hits my machine, let's make the
module reloading optional in all three tests to improve coverage.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: address some review comments from maintainer
---
 common/module |   33 ++++++++++++++++++++++++++++-----
 tests/xfs/434 |    3 +--
 tests/xfs/435 |    3 +--
 tests/xfs/436 |    3 +--
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/common/module b/common/module
index 6efab71d34..a8d5f492d3 100644
--- a/common/module
+++ b/common/module
@@ -48,12 +48,15 @@ _require_loadable_module()
 	modprobe "${module}" || _notrun "${module} load failed"
 }
 
-# Check that the module for FSTYP can be loaded.
-_require_loadable_fs_module()
+# Test if the module for FSTYP can be unloaded and reloaded.
+#
+# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could
+# not be unloaded; or 3 if loading the module fails.
+_test_loadable_fs_module()
 {
 	local module="$1"
 
-	modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module."
+	modinfo "${module}" > /dev/null 2>&1 || return 1
 
 	# Unload test fs, try to reload module, remount
 	local had_testfs=""
@@ -68,8 +71,28 @@ _require_loadable_fs_module()
 	modprobe "${module}" || load_ok=0
 	test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null
 	test -n "${had_testfs}" && _test_mount 2> /dev/null
-	test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable"
-	test -z "${load_ok}" || _notrun "${module} load failed"
+	test -z "${unload_ok}" || return 2
+	test -z "${load_ok}" || return 3
+	return 0
+}
+
+_require_loadable_fs_module()
+{
+	local module="$1"
+
+	_test_loadable_fs_module "${module}"
+	ret=$?
+	case "$ret" in
+	1)
+		_notrun "${module}: must be a module."
+		;;
+	2)
+		_notrun "${module}: module could not be unloaded"
+		;;
+	3)
+		_notrun "${module}: module reload failed"
+		;;
+	esac
 }
 
 # Print the value of a filesystem module parameter
diff --git a/tests/xfs/434 b/tests/xfs/434
index 12d1a0c9da..ca80e12753 100755
--- a/tests/xfs/434
+++ b/tests/xfs/434
@@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_quota
 _require_scratch_reflink
 _require_cp_reflink
@@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0
diff --git a/tests/xfs/435 b/tests/xfs/435
index 44135c7653..b52e9287df 100755
--- a/tests/xfs/435
+++ b/tests/xfs/435
@@ -24,7 +24,6 @@ _begin_fstest auto quick clone
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_quota
 _require_scratch_reflink
 _require_cp_reflink
@@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0
diff --git a/tests/xfs/436 b/tests/xfs/436
index d010362785..02bcd66900 100755
--- a/tests/xfs/436
+++ b/tests/xfs/436
@@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr
 
 # real QA test starts here
 _supported_fs xfs
-_require_loadable_fs_module "xfs"
 _require_scratch_reflink
 _require_cp_reflink
 _require_xfs_io_command falloc # fsr requires support for preallocation
@@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null
 rm -f ${RESULT_DIR}/require_scratch
 
 echo "See if we leak"
-_reload_fs_module "xfs"
+_test_loadable_fs_module "xfs"
 
 # success, all done
 status=0

Re: [PATCH v1.1 7/8] xfs/43[4-6]: make module reloading optional

[Zorro Lang]

On Fri, Mar 01, 2024 at 09:51:24AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> These three tests examine two things -- first, can xfs CoW staging
> extent recovery handle corruptions in the refcount btree gracefully; and
> second, can we avoid leaking incore inodes and dquots.
> 
> The only cheap way to check the second condition is to rmmod and
> modprobe the XFS module, which triggers leak detection when rmmod tears
> down the caches.  Currently, the entire test is _notrun if module
> reloading doesn't work.
> 
> Unfortunately, these tests never run for the majority of XFS developers
> because their testbeds either compile the xfs kernel driver into vmlinux
> statically or the rootfs is xfs so the module cannot be reloaded.  The
> author's testbed boots from NFS and does not have this limitation.
> 
> Because we've had repeated instances of CoW recovery regressions not
> being caught by testing until for-next hits my machine, let's make the
> module reloading optional in all three tests to improve coverage.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: address some review comments from maintainer

This version looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

> ---
>  common/module |   33 ++++++++++++++++++++++++++++-----
>  tests/xfs/434 |    3 +--
>  tests/xfs/435 |    3 +--
>  tests/xfs/436 |    3 +--
>  4 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/common/module b/common/module
> index 6efab71d34..a8d5f492d3 100644
> --- a/common/module
> +++ b/common/module
> @@ -48,12 +48,15 @@ _require_loadable_module()
>  	modprobe "${module}" || _notrun "${module} load failed"
>  }
>  
> -# Check that the module for FSTYP can be loaded.
> -_require_loadable_fs_module()
> +# Test if the module for FSTYP can be unloaded and reloaded.
> +#
> +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could
> +# not be unloaded; or 3 if loading the module fails.
> +_test_loadable_fs_module()
>  {
>  	local module="$1"
>  
> -	modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module."
> +	modinfo "${module}" > /dev/null 2>&1 || return 1
>  
>  	# Unload test fs, try to reload module, remount
>  	local had_testfs=""
> @@ -68,8 +71,28 @@ _require_loadable_fs_module()
>  	modprobe "${module}" || load_ok=0
>  	test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null
>  	test -n "${had_testfs}" && _test_mount 2> /dev/null
> -	test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable"
> -	test -z "${load_ok}" || _notrun "${module} load failed"
> +	test -z "${unload_ok}" || return 2
> +	test -z "${load_ok}" || return 3
> +	return 0
> +}
> +
> +_require_loadable_fs_module()
> +{
> +	local module="$1"
> +
> +	_test_loadable_fs_module "${module}"
> +	ret=$?
> +	case "$ret" in
> +	1)
> +		_notrun "${module}: must be a module."
> +		;;
> +	2)
> +		_notrun "${module}: module could not be unloaded"
> +		;;
> +	3)
> +		_notrun "${module}: module reload failed"
> +		;;
> +	esac
>  }
>  
>  # Print the value of a filesystem module parameter
> diff --git a/tests/xfs/434 b/tests/xfs/434
> index 12d1a0c9da..ca80e12753 100755
> --- a/tests/xfs/434
> +++ b/tests/xfs/434
> @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_quota
>  _require_scratch_reflink
>  _require_cp_reflink
> @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"
>  
>  # success, all done
>  status=0
> diff --git a/tests/xfs/435 b/tests/xfs/435
> index 44135c7653..b52e9287df 100755
> --- a/tests/xfs/435
> +++ b/tests/xfs/435
> @@ -24,7 +24,6 @@ _begin_fstest auto quick clone
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_quota
>  _require_scratch_reflink
>  _require_cp_reflink
> @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"
>  
>  # success, all done
>  status=0
> diff --git a/tests/xfs/436 b/tests/xfs/436
> index d010362785..02bcd66900 100755
> --- a/tests/xfs/436
> +++ b/tests/xfs/436
> @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr
>  
>  # real QA test starts here
>  _supported_fs xfs
> -_require_loadable_fs_module "xfs"
>  _require_scratch_reflink
>  _require_cp_reflink
>  _require_xfs_io_command falloc # fsr requires support for preallocation
> @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null
>  rm -f ${RESULT_DIR}/require_scratch
>  
>  echo "See if we leak"
> -_reload_fs_module "xfs"
> +_test_loadable_fs_module "xfs"
>  
>  # success, all done
>  status=0
> 

[PATCH 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
even though the filesystem has sufficient number of free blocks.

This occurs if the file offset range on which the write operation is being
performed has a delalloc extent in the cow fork and this delalloc extent
begins much before the Direct IO range.

In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
allocate the blocks mapped by the delalloc extent. The extent thus allocated
may not cover the beginning of file offset range on which the Direct IO write
was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.

This test addresses this issue.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/rc          |   14 +++++++++
 tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/1923.out |    8 +++++
 3 files changed, 107 insertions(+)
 create mode 100755 tests/xfs/1923
 create mode 100644 tests/xfs/1923.out


diff --git a/common/rc b/common/rc
index 30c44dddd9..d3a2a0718b 100644
--- a/common/rc
+++ b/common/rc
@@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
 	_scratch_unmount
 }
 
+# Require test fs supports delay allocation.
+_require_test_delalloc()
+{
+	_require_command "$FILEFRAG_PROG" filefrag
+
+	rm -f $TEST_DIR/testy
+	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
+	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
+	res=$?
+	rm -f $TEST_DIR/testy
+	test $res -eq 0 || \
+		_notrun "test requires delayed allocation buffered writes"
+}
+
 # this test needs a test partition - check we're ok & mount it
 #
 _require_test()
diff --git a/tests/xfs/1923 b/tests/xfs/1923
new file mode 100755
index 0000000000..4e494ad8c2
--- /dev/null
+++ b/tests/xfs/1923
@@ -0,0 +1,85 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 1923
+#
+# This is a regression test for "xfs: Fix false ENOSPC when performing direct
+# write on a delalloc extent in cow fork".  If there is a lot of free space but
+# it is very fragmented, it's possible that a very large delalloc reservation
+# could be created in the CoW fork by a buffered write.  If a directio write
+# tries to convert the delalloc reservation to a real extent, it's possible
+# that the allocation will succeed but fail to convert even the first block of
+# the directio write range.  In this case, XFS will return ENOSPC even though
+# all it needed to do was to keep converting until the allocator returns ENOSPC
+# or the first block of the direct write got some space.
+#
+. ./common/preamble
+_begin_fstest auto quick clone
+
+_cleanup()
+{
+	cd /
+	rm -f $file1 $file2 $fragmentedfile
+}
+
+# Import common functions.
+. ./common/reflink
+. ./common/inject
+
+# real QA test starts here
+_fixed_by_kernel_commit XXXXX \
+	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
+
+# Modify as appropriate.
+_supported_fs generic
+_require_test_program "punch-alternating"
+_require_test_reflink
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+_require_test_delalloc
+
+file1=$TEST_DIR/file1.$seq
+file2=$TEST_DIR/file2.$seq
+fragmentedfile=$TEST_DIR/fragmentedfile.$seq
+
+# COW operates on pages, so we must not perform operations in units smaller
+# than a page.
+blksz=$(_get_file_block_size $TEST_DIR)
+pagesz=$(_get_page_size)
+if (( $blksz < $pagesz )); then
+	blksz=$pagesz
+fi
+
+echo "Create source file"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
+
+sync
+
+echo "Create Reflinked file"
+_cp_reflink $file1 $file2 >> $seqres.full
+#$XFS_IO_PROG -f -c "reflink $file1" $file2 >> $seqres.full
+
+echo "Set cowextsize"
+$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
+
+echo "Fragment FS"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
+sync
+$here/src/punch-alternating $fragmentedfile
+
+echo "Allocate block sized extent from now onwards"
+_test_inject_error bmap_alloc_minlen_extent 1
+
+echo "Create big delalloc extent in CoW fork"
+$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
+
+sync
+
+$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full
+
+echo "Direct I/O write at offset 3FSB"
+$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
new file mode 100644
index 0000000000..a0553cf3ee
--- /dev/null
+++ b/tests/xfs/1923.out
@@ -0,0 +1,8 @@
+QA output created by 1923
+Create source file
+Create Reflinked file
+Set cowextsize
+Fragment FS
+Allocate block sized extent from now onwards
+Create big delalloc extent in CoW fork
+Direct I/O write at offset 3FSB

Re: [PATCH 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:02:37PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> even though the filesystem has sufficient number of free blocks.
> 
> This occurs if the file offset range on which the write operation is being
> performed has a delalloc extent in the cow fork and this delalloc extent
> begins much before the Direct IO range.
> 
> In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> allocate the blocks mapped by the delalloc extent. The extent thus allocated
> may not cover the beginning of file offset range on which the Direct IO write
> was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> 
> This test addresses this issue.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  common/rc          |   14 +++++++++
>  tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/1923.out |    8 +++++
>  3 files changed, 107 insertions(+)
>  create mode 100755 tests/xfs/1923
>  create mode 100644 tests/xfs/1923.out
> 
> 
> diff --git a/common/rc b/common/rc
> index 30c44dddd9..d3a2a0718b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
>  	_scratch_unmount
>  }
>  
> +# Require test fs supports delay allocation.
> +_require_test_delalloc()
> +{
> +	_require_command "$FILEFRAG_PROG" filefrag
> +
> +	rm -f $TEST_DIR/testy
> +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc

I'm wondering if it's a 100% reliable way to get the "delalloc" flag when
the delalloc is supported? If not, is it worth testing with several files
or a loop, then return 0 if one of them get delalloc ?

> +	res=$?
> +	rm -f $TEST_DIR/testy
> +	test $res -eq 0 || \
> +		_notrun "test requires delayed allocation buffered writes"
> +}
> +
>  # this test needs a test partition - check we're ok & mount it
>  #
>  _require_test()
> diff --git a/tests/xfs/1923 b/tests/xfs/1923
> new file mode 100755
> index 0000000000..4e494ad8c2
> --- /dev/null
> +++ b/tests/xfs/1923
> @@ -0,0 +1,85 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 1923
> +#
> +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> +# it is very fragmented, it's possible that a very large delalloc reservation
> +# could be created in the CoW fork by a buffered write.  If a directio write
> +# tries to convert the delalloc reservation to a real extent, it's possible
> +# that the allocation will succeed but fail to convert even the first block of
> +# the directio write range.  In this case, XFS will return ENOSPC even though
> +# all it needed to do was to keep converting until the allocator returns ENOSPC
> +# or the first block of the direct write got some space.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick clone
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $file1 $file2 $fragmentedfile
> +}
> +
> +# Import common functions.
> +. ./common/reflink
> +. ./common/inject
> +
> +# real QA test starts here
> +_fixed_by_kernel_commit XXXXX \
> +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"

Is it this commit below, or a new fix with same subject?

commit d62113303d691bcd8d0675ae4ac63e7769afc56c
Author: Chandan Babu R <chandan.babu@oracle.com>
Date:   Thu Aug 4 08:59:27 2022 -0700

    xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork

If it's an old commit, please replace the "XXXXX" to "d62113303d69".

> +
> +# Modify as appropriate.
> +_supported_fs generic

"xfs"? As it's in tests/xfs/ directory.

> +_require_test_program "punch-alternating"
> +_require_test_reflink
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +_require_test_delalloc
> +
> +file1=$TEST_DIR/file1.$seq
> +file2=$TEST_DIR/file2.$seq
> +fragmentedfile=$TEST_DIR/fragmentedfile.$seq

As you use the $TEST_DIR, it might be worth making sure these files
aren't existed. Due to the files (from other cases) in $TEST_DIR might
not be cleaned, especially if they don't take much free space.

So if these 3 files won't take much space, we can keep them, don't need
a specific _cleanup(). And move the "rm -f $file1 $file2 $fragmentedfile"
at here.

> +
> +# COW operates on pages, so we must not perform operations in units smaller
> +# than a page.
> +blksz=$(_get_file_block_size $TEST_DIR)
> +pagesz=$(_get_page_size)
> +if (( $blksz < $pagesz )); then
> +	blksz=$pagesz
> +fi

Just curious, this's a xfs specific test case, can xfs support blocksize >
pagesize? If not, can we just use pagesz directly at here ?

> +
> +echo "Create source file"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> +
> +sync
> +
> +echo "Create Reflinked file"
> +_cp_reflink $file1 $file2 >> $seqres.full
> +#$XFS_IO_PROG -f -c "reflink $file1" $file2 >> $seqres.full

There's a "#", do you hope to run it or not?

> +
> +echo "Set cowextsize"
> +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> +
> +echo "Fragment FS"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> +sync
> +$here/src/punch-alternating $fragmentedfile
> +
> +echo "Allocate block sized extent from now onwards"
> +_test_inject_error bmap_alloc_minlen_extent 1
> +
> +echo "Create big delalloc extent in CoW fork"
> +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> +
> +sync
> +
> +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full
> +
> +echo "Direct I/O write at offset 3FSB"
> +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> new file mode 100644
> index 0000000000..a0553cf3ee
> --- /dev/null
> +++ b/tests/xfs/1923.out
> @@ -0,0 +1,8 @@
> +QA output created by 1923
> +Create source file
> +Create Reflinked file
> +Set cowextsize
> +Fragment FS
> +Allocate block sized extent from now onwards
> +Create big delalloc extent in CoW fork
> +Direct I/O write at offset 3FSB
> 

Re: [PATCH 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 02:00:21PM +0800, Zorro Lang wrote:
> On Mon, Feb 26, 2024 at 06:02:37PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> > even though the filesystem has sufficient number of free blocks.
> > 
> > This occurs if the file offset range on which the write operation is being
> > performed has a delalloc extent in the cow fork and this delalloc extent
> > begins much before the Direct IO range.
> > 
> > In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> > allocate the blocks mapped by the delalloc extent. The extent thus allocated
> > may not cover the beginning of file offset range on which the Direct IO write
> > was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> > 
> > This test addresses this issue.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/rc          |   14 +++++++++
> >  tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/1923.out |    8 +++++
> >  3 files changed, 107 insertions(+)
> >  create mode 100755 tests/xfs/1923
> >  create mode 100644 tests/xfs/1923.out
> > 
> > 
> > diff --git a/common/rc b/common/rc
> > index 30c44dddd9..d3a2a0718b 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
> >  	_scratch_unmount
> >  }
> >  
> > +# Require test fs supports delay allocation.
> > +_require_test_delalloc()
> > +{
> > +	_require_command "$FILEFRAG_PROG" filefrag
> > +
> > +	rm -f $TEST_DIR/testy
> > +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> > +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> 
> I'm wondering if it's a 100% reliable way to get the "delalloc" flag when
> the delalloc is supported? If not, is it worth testing with several files
> or a loop, then return 0 if one of them get delalloc ?
> 
> > +	res=$?
> > +	rm -f $TEST_DIR/testy
> > +	test $res -eq 0 || \
> > +		_notrun "test requires delayed allocation buffered writes"
> > +}
> > +
> >  # this test needs a test partition - check we're ok & mount it
> >  #
> >  _require_test()
> > diff --git a/tests/xfs/1923 b/tests/xfs/1923
> > new file mode 100755
> > index 0000000000..4e494ad8c2
> > --- /dev/null
> > +++ b/tests/xfs/1923
> > @@ -0,0 +1,85 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 1923
> > +#
> > +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> > +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> > +# it is very fragmented, it's possible that a very large delalloc reservation
> > +# could be created in the CoW fork by a buffered write.  If a directio write
> > +# tries to convert the delalloc reservation to a real extent, it's possible
> > +# that the allocation will succeed but fail to convert even the first block of
> > +# the directio write range.  In this case, XFS will return ENOSPC even though
> > +# all it needed to do was to keep converting until the allocator returns ENOSPC
> > +# or the first block of the direct write got some space.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick clone
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $file1 $file2 $fragmentedfile
> > +}
> > +
> > +# Import common functions.
> > +. ./common/reflink
> > +. ./common/inject
> > +
> > +# real QA test starts here
> > +_fixed_by_kernel_commit XXXXX \
> > +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> 
> Is it this commit below, or a new fix with same subject?
> 
> commit d62113303d691bcd8d0675ae4ac63e7769afc56c
> Author: Chandan Babu R <chandan.babu@oracle.com>
> Date:   Thu Aug 4 08:59:27 2022 -0700
> 
>     xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork
> 
> If it's an old commit, please replace the "XXXXX" to "d62113303d69".

Ah, yes, it is that one.  Sorry I forgot to update that.  As you can
tell, this is a really old test that ... I guess we never sent
upstream.

> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> 
> "xfs"? As it's in tests/xfs/ directory.

<nod>

> > +_require_test_program "punch-alternating"
> > +_require_test_reflink
> > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > +_require_test_delalloc
> > +
> > +file1=$TEST_DIR/file1.$seq
> > +file2=$TEST_DIR/file2.$seq
> > +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> 
> As you use the $TEST_DIR, it might be worth making sure these files
> aren't existed. Due to the files (from other cases) in $TEST_DIR might
> not be cleaned, especially if they don't take much free space.

Ok.

> So if these 3 files won't take much space, we can keep them, don't need
> a specific _cleanup(). And move the "rm -f $file1 $file2 $fragmentedfile"
> at here.

Well you make a good point that we should remove those files so that
they end up in a known state (i.e. nonexistence) where we can't get
tripped up by someone creating, say, a /dev/sda symlink to
$TEST_DIR/file1.$seq.

I don't think the files are all that big, but I'll continue removing
them anyway.  Deletion is part of aging.

> > +
> > +# COW operates on pages, so we must not perform operations in units smaller
> > +# than a page.
> > +blksz=$(_get_file_block_size $TEST_DIR)
> > +pagesz=$(_get_page_size)
> > +if (( $blksz < $pagesz )); then
> > +	blksz=$pagesz
> > +fi
> 
> Just curious, this's a xfs specific test case, can xfs support blocksize >
> pagesize? If not, can we just use pagesz directly at here ?

It doesn't support bs > ps yet, but the point of clamping $blksz here
is the fact that we'll never try to writeback less than a page, so we
might as well run with those units.

(This test has nothing to do with the ongoing bs>ps effort.)

> > +
> > +echo "Create source file"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +echo "Create Reflinked file"
> > +_cp_reflink $file1 $file2 >> $seqres.full
> > +#$XFS_IO_PROG -f -c "reflink $file1" $file2 >> $seqres.full
> 
> There's a "#", do you hope to run it or not?

Oops.  That's redundant with the _cp_reflink.

--D

> > +
> > +echo "Set cowextsize"
> > +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> > +
> > +echo "Fragment FS"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> > +sync
> > +$here/src/punch-alternating $fragmentedfile
> > +
> > +echo "Allocate block sized extent from now onwards"
> > +_test_inject_error bmap_alloc_minlen_extent 1
> > +
> > +echo "Create big delalloc extent in CoW fork"
> > +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full
> > +
> > +echo "Direct I/O write at offset 3FSB"
> > +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> > new file mode 100644
> > index 0000000000..a0553cf3ee
> > --- /dev/null
> > +++ b/tests/xfs/1923.out
> > @@ -0,0 +1,8 @@
> > +QA output created by 1923
> > +Create source file
> > +Create Reflinked file
> > +Set cowextsize
> > +Fragment FS
> > +Allocate block sized extent from now onwards
> > +Create big delalloc extent in CoW fork
> > +Direct I/O write at offset 3FSB
> > 
> 
> 

[PATCH v1.1 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
even though the filesystem has sufficient number of free blocks.

This occurs if the file offset range on which the write operation is being
performed has a delalloc extent in the cow fork and this delalloc extent
begins much before the Direct IO range.

In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
allocate the blocks mapped by the delalloc extent. The extent thus allocated
may not cover the beginning of file offset range on which the Direct IO write
was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.

This test addresses this issue.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: address some missing bits and remove extraneous code
---
 common/rc          |   14 ++++++++
 tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/1923.out |    8 +++++
 3 files changed, 108 insertions(+)
 create mode 100755 tests/xfs/1923
 create mode 100644 tests/xfs/1923.out

diff --git a/common/rc b/common/rc
index 30c44dddd9..d3a2a0718b 100644
--- a/common/rc
+++ b/common/rc
@@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
 	_scratch_unmount
 }
 
+# Require test fs supports delay allocation.
+_require_test_delalloc()
+{
+	_require_command "$FILEFRAG_PROG" filefrag
+
+	rm -f $TEST_DIR/testy
+	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
+	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
+	res=$?
+	rm -f $TEST_DIR/testy
+	test $res -eq 0 || \
+		_notrun "test requires delayed allocation buffered writes"
+}
+
 # this test needs a test partition - check we're ok & mount it
 #
 _require_test()
diff --git a/tests/xfs/1923 b/tests/xfs/1923
new file mode 100755
index 0000000000..7068fda64c
--- /dev/null
+++ b/tests/xfs/1923
@@ -0,0 +1,86 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 1923
+#
+# This is a regression test for "xfs: Fix false ENOSPC when performing direct
+# write on a delalloc extent in cow fork".  If there is a lot of free space but
+# it is very fragmented, it's possible that a very large delalloc reservation
+# could be created in the CoW fork by a buffered write.  If a directio write
+# tries to convert the delalloc reservation to a real extent, it's possible
+# that the allocation will succeed but fail to convert even the first block of
+# the directio write range.  In this case, XFS will return ENOSPC even though
+# all it needed to do was to keep converting until the allocator returns ENOSPC
+# or the first block of the direct write got some space.
+#
+. ./common/preamble
+_begin_fstest auto quick clone
+
+_cleanup()
+{
+	cd /
+	rm -f $file1 $file2 $fragmentedfile
+}
+
+# Import common functions.
+. ./common/reflink
+. ./common/inject
+
+# real QA test starts here
+_fixed_by_kernel_commit d62113303d69 \
+	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
+
+# Modify as appropriate.
+_supported_fs xfs
+_require_test_program "punch-alternating"
+_require_test_reflink
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+_require_test_delalloc
+
+file1=$TEST_DIR/file1.$seq
+file2=$TEST_DIR/file2.$seq
+fragmentedfile=$TEST_DIR/fragmentedfile.$seq
+
+rm -f $file1 $file2 $fragmentedfile
+
+# COW operates on pages, so we must not perform operations in units smaller
+# than a page.
+blksz=$(_get_file_block_size $TEST_DIR)
+pagesz=$(_get_page_size)
+if (( $blksz < $pagesz )); then
+	blksz=$pagesz
+fi
+
+echo "Create source file"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
+
+sync
+
+echo "Create Reflinked file"
+_cp_reflink $file1 $file2 >> $seqres.full
+
+echo "Set cowextsize"
+$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
+
+echo "Fragment FS"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
+sync
+$here/src/punch-alternating $fragmentedfile
+
+echo "Allocate block sized extent from now onwards"
+_test_inject_error bmap_alloc_minlen_extent 1
+
+echo "Create big delalloc extent in CoW fork"
+$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
+
+sync
+
+$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full
+
+echo "Direct I/O write at offset 3FSB"
+$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
new file mode 100644
index 0000000000..a0553cf3ee
--- /dev/null
+++ b/tests/xfs/1923.out
@@ -0,0 +1,8 @@
+QA output created by 1923
+Create source file
+Create Reflinked file
+Set cowextsize
+Fragment FS
+Allocate block sized extent from now onwards
+Create big delalloc extent in CoW fork
+Direct I/O write at offset 3FSB

Re: [PATCH v1.1 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Zorro Lang]

On Fri, Mar 01, 2024 at 09:52:02AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> even though the filesystem has sufficient number of free blocks.
> 
> This occurs if the file offset range on which the write operation is being
> performed has a delalloc extent in the cow fork and this delalloc extent
> begins much before the Direct IO range.
> 
> In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> allocate the blocks mapped by the delalloc extent. The extent thus allocated
> may not cover the beginning of file offset range on which the Direct IO write
> was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> 
> This test addresses this issue.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: address some missing bits and remove extraneous code
> ---
>  common/rc          |   14 ++++++++
>  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/1923.out |    8 +++++
>  3 files changed, 108 insertions(+)
>  create mode 100755 tests/xfs/1923
>  create mode 100644 tests/xfs/1923.out
> 
> diff --git a/common/rc b/common/rc
> index 30c44dddd9..d3a2a0718b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
>  	_scratch_unmount
>  }
>  
> +# Require test fs supports delay allocation.
> +_require_test_delalloc()
> +{
> +	_require_command "$FILEFRAG_PROG" filefrag
> +
> +	rm -f $TEST_DIR/testy
> +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> +	res=$?
> +	rm -f $TEST_DIR/testy
> +	test $res -eq 0 || \
> +		_notrun "test requires delayed allocation buffered writes"
> +}
> +
>  # this test needs a test partition - check we're ok & mount it
>  #
>  _require_test()
> diff --git a/tests/xfs/1923 b/tests/xfs/1923
> new file mode 100755
> index 0000000000..7068fda64c
> --- /dev/null
> +++ b/tests/xfs/1923
> @@ -0,0 +1,86 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 1923
> +#
> +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> +# it is very fragmented, it's possible that a very large delalloc reservation
> +# could be created in the CoW fork by a buffered write.  If a directio write
> +# tries to convert the delalloc reservation to a real extent, it's possible
> +# that the allocation will succeed but fail to convert even the first block of
> +# the directio write range.  In this case, XFS will return ENOSPC even though
> +# all it needed to do was to keep converting until the allocator returns ENOSPC
> +# or the first block of the direct write got some space.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick clone
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $file1 $file2 $fragmentedfile
> +}
> +
> +# Import common functions.
> +. ./common/reflink
> +. ./common/inject
> +
> +# real QA test starts here
> +_fixed_by_kernel_commit d62113303d69 \
> +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_require_test_program "punch-alternating"
> +_require_test_reflink
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +_require_test_delalloc
> +
> +file1=$TEST_DIR/file1.$seq
> +file2=$TEST_DIR/file2.$seq
> +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> +
> +rm -f $file1 $file2 $fragmentedfile
> +
> +# COW operates on pages, so we must not perform operations in units smaller
> +# than a page.
> +blksz=$(_get_file_block_size $TEST_DIR)
> +pagesz=$(_get_page_size)
> +if (( $blksz < $pagesz )); then
> +	blksz=$pagesz
> +fi
> +
> +echo "Create source file"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> +
> +sync
> +
> +echo "Create Reflinked file"
> +_cp_reflink $file1 $file2 >> $seqres.full
> +
> +echo "Set cowextsize"
> +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> +
> +echo "Fragment FS"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> +sync
> +$here/src/punch-alternating $fragmentedfile
> +
> +echo "Allocate block sized extent from now onwards"
> +_test_inject_error bmap_alloc_minlen_extent 1
> +
> +echo "Create big delalloc extent in CoW fork"
> +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> +
> +sync
> +
> +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full

The "-c" option might get:

  $ xfs_io -c "bmap -celpv" testfile 
  xfs_io: xfsctl(XFS_IOC_GETBMAPX) iflags=0x28 ["testfile"]: Invalid argument

It will break the golden image, can we redirect stderr to $seqres.full,
as it's just for debug? If you agree, I can help to change that by adding
"2>&1" at the end. Or you hope to deal with it by other method?

Thanks,
Zorro


> +
> +echo "Direct I/O write at offset 3FSB"
> +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> new file mode 100644
> index 0000000000..a0553cf3ee
> --- /dev/null
> +++ b/tests/xfs/1923.out
> @@ -0,0 +1,8 @@
> +QA output created by 1923
> +Create source file
> +Create Reflinked file
> +Set cowextsize
> +Fragment FS
> +Allocate block sized extent from now onwards
> +Create big delalloc extent in CoW fork
> +Direct I/O write at offset 3FSB
> 

Re: [PATCH v1.1 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

On Sun, Mar 03, 2024 at 04:50:56AM +0800, Zorro Lang wrote:
> On Fri, Mar 01, 2024 at 09:52:02AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> > even though the filesystem has sufficient number of free blocks.
> > 
> > This occurs if the file offset range on which the write operation is being
> > performed has a delalloc extent in the cow fork and this delalloc extent
> > begins much before the Direct IO range.
> > 
> > In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> > allocate the blocks mapped by the delalloc extent. The extent thus allocated
> > may not cover the beginning of file offset range on which the Direct IO write
> > was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> > 
> > This test addresses this issue.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: address some missing bits and remove extraneous code
> > ---
> >  common/rc          |   14 ++++++++
> >  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/1923.out |    8 +++++
> >  3 files changed, 108 insertions(+)
> >  create mode 100755 tests/xfs/1923
> >  create mode 100644 tests/xfs/1923.out
> > 
> > diff --git a/common/rc b/common/rc
> > index 30c44dddd9..d3a2a0718b 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1873,6 +1873,20 @@ _require_scratch_delalloc()
> >  	_scratch_unmount
> >  }
> >  
> > +# Require test fs supports delay allocation.
> > +_require_test_delalloc()
> > +{
> > +	_require_command "$FILEFRAG_PROG" filefrag
> > +
> > +	rm -f $TEST_DIR/testy
> > +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> > +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> > +	res=$?
> > +	rm -f $TEST_DIR/testy
> > +	test $res -eq 0 || \
> > +		_notrun "test requires delayed allocation buffered writes"
> > +}
> > +
> >  # this test needs a test partition - check we're ok & mount it
> >  #
> >  _require_test()
> > diff --git a/tests/xfs/1923 b/tests/xfs/1923
> > new file mode 100755
> > index 0000000000..7068fda64c
> > --- /dev/null
> > +++ b/tests/xfs/1923
> > @@ -0,0 +1,86 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 1923
> > +#
> > +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> > +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> > +# it is very fragmented, it's possible that a very large delalloc reservation
> > +# could be created in the CoW fork by a buffered write.  If a directio write
> > +# tries to convert the delalloc reservation to a real extent, it's possible
> > +# that the allocation will succeed but fail to convert even the first block of
> > +# the directio write range.  In this case, XFS will return ENOSPC even though
> > +# all it needed to do was to keep converting until the allocator returns ENOSPC
> > +# or the first block of the direct write got some space.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick clone
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $file1 $file2 $fragmentedfile
> > +}
> > +
> > +# Import common functions.
> > +. ./common/reflink
> > +. ./common/inject
> > +
> > +# real QA test starts here
> > +_fixed_by_kernel_commit d62113303d69 \
> > +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs
> > +_require_test_program "punch-alternating"
> > +_require_test_reflink
> > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > +_require_test_delalloc
> > +
> > +file1=$TEST_DIR/file1.$seq
> > +file2=$TEST_DIR/file2.$seq
> > +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> > +
> > +rm -f $file1 $file2 $fragmentedfile
> > +
> > +# COW operates on pages, so we must not perform operations in units smaller
> > +# than a page.
> > +blksz=$(_get_file_block_size $TEST_DIR)
> > +pagesz=$(_get_page_size)
> > +if (( $blksz < $pagesz )); then
> > +	blksz=$pagesz
> > +fi
> > +
> > +echo "Create source file"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +echo "Create Reflinked file"
> > +_cp_reflink $file1 $file2 >> $seqres.full
> > +
> > +echo "Set cowextsize"
> > +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> > +
> > +echo "Fragment FS"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> > +sync
> > +$here/src/punch-alternating $fragmentedfile
> > +
> > +echo "Allocate block sized extent from now onwards"
> > +_test_inject_error bmap_alloc_minlen_extent 1
> > +
> > +echo "Create big delalloc extent in CoW fork"
> > +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 >> $seqres.full
> 
> The "-c" option might get:
> 
>   $ xfs_io -c "bmap -celpv" testfile 
>   xfs_io: xfsctl(XFS_IOC_GETBMAPX) iflags=0x28 ["testfile"]: Invalid argument
> 
> It will break the golden image, can we redirect stderr to $seqres.full,
> as it's just for debug? If you agree, I can help to change that by adding
> "2>&1" at the end. Or you hope to deal with it by other method?

That's fine, I don't need the stderr output.  "&>> $seqres.full" works
just as well.

--D

> Thanks,
> Zorro
> 
> 
> > +
> > +echo "Direct I/O write at offset 3FSB"
> > +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> > new file mode 100644
> > index 0000000000..a0553cf3ee
> > --- /dev/null
> > +++ b/tests/xfs/1923.out
> > @@ -0,0 +1,8 @@
> > +QA output created by 1923
> > +Create source file
> > +Create Reflinked file
> > +Set cowextsize
> > +Fragment FS
> > +Allocate block sized extent from now onwards
> > +Create big delalloc extent in CoW fork
> > +Direct I/O write at offset 3FSB
> > 
> 
> 

[PATCH v1.2 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
even though the filesystem has sufficient number of free blocks.

This occurs if the file offset range on which the write operation is being
performed has a delalloc extent in the cow fork and this delalloc extent
begins much before the Direct IO range.

In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
allocate the blocks mapped by the delalloc extent. The extent thus allocated
may not cover the beginning of file offset range on which the Direct IO write
was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.

This test addresses this issue.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: address some missing bits and remove extraneous code
v1.2: fix cow fork dumping screwing up golden output
---
 common/rc          |   14 ++++++++
 tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/1923.out |    8 +++++
 3 files changed, 108 insertions(+)
 create mode 100755 tests/xfs/1923
 create mode 100644 tests/xfs/1923.out

diff --git a/common/rc b/common/rc
index 50dde313b8..9f54ab1e77 100644
--- a/common/rc
+++ b/common/rc
@@ -1883,6 +1883,20 @@ _require_scratch_delalloc()
 	_scratch_unmount
 }
 
+# Require test fs supports delay allocation.
+_require_test_delalloc()
+{
+	_require_command "$FILEFRAG_PROG" filefrag
+
+	rm -f $TEST_DIR/testy
+	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
+	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
+	res=$?
+	rm -f $TEST_DIR/testy
+	test $res -eq 0 || \
+		_notrun "test requires delayed allocation buffered writes"
+}
+
 # this test needs a test partition - check we're ok & mount it
 #
 _require_test()
diff --git a/tests/xfs/1923 b/tests/xfs/1923
new file mode 100755
index 0000000000..4ad3dfa764
--- /dev/null
+++ b/tests/xfs/1923
@@ -0,0 +1,86 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 1923
+#
+# This is a regression test for "xfs: Fix false ENOSPC when performing direct
+# write on a delalloc extent in cow fork".  If there is a lot of free space but
+# it is very fragmented, it's possible that a very large delalloc reservation
+# could be created in the CoW fork by a buffered write.  If a directio write
+# tries to convert the delalloc reservation to a real extent, it's possible
+# that the allocation will succeed but fail to convert even the first block of
+# the directio write range.  In this case, XFS will return ENOSPC even though
+# all it needed to do was to keep converting until the allocator returns ENOSPC
+# or the first block of the direct write got some space.
+#
+. ./common/preamble
+_begin_fstest auto quick clone
+
+_cleanup()
+{
+	cd /
+	rm -f $file1 $file2 $fragmentedfile
+}
+
+# Import common functions.
+. ./common/reflink
+. ./common/inject
+
+# real QA test starts here
+_fixed_by_kernel_commit d62113303d69 \
+	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
+
+# Modify as appropriate.
+_supported_fs xfs
+_require_test_program "punch-alternating"
+_require_test_reflink
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+_require_test_delalloc
+
+file1=$TEST_DIR/file1.$seq
+file2=$TEST_DIR/file2.$seq
+fragmentedfile=$TEST_DIR/fragmentedfile.$seq
+
+rm -f $file1 $file2 $fragmentedfile
+
+# COW operates on pages, so we must not perform operations in units smaller
+# than a page.
+blksz=$(_get_file_block_size $TEST_DIR)
+pagesz=$(_get_page_size)
+if (( $blksz < $pagesz )); then
+	blksz=$pagesz
+fi
+
+echo "Create source file"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
+
+sync
+
+echo "Create Reflinked file"
+_cp_reflink $file1 $file2 >> $seqres.full
+
+echo "Set cowextsize"
+$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
+
+echo "Fragment FS"
+$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
+sync
+$here/src/punch-alternating $fragmentedfile
+
+echo "Allocate block sized extent from now onwards"
+_test_inject_error bmap_alloc_minlen_extent 1
+
+echo "Create big delalloc extent in CoW fork"
+$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
+
+sync
+
+$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 &>> $seqres.full
+
+echo "Direct I/O write at offset 3FSB"
+$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
new file mode 100644
index 0000000000..a0553cf3ee
--- /dev/null
+++ b/tests/xfs/1923.out
@@ -0,0 +1,8 @@
+QA output created by 1923
+Create source file
+Create Reflinked file
+Set cowextsize
+Fragment FS
+Allocate block sized extent from now onwards
+Create big delalloc extent in CoW fork
+Direct I/O write at offset 3FSB

Re: [PATCH v1.2 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Zorro Lang]

On Thu, Mar 07, 2024 at 03:22:55PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> even though the filesystem has sufficient number of free blocks.
> 
> This occurs if the file offset range on which the write operation is being
> performed has a delalloc extent in the cow fork and this delalloc extent
> begins much before the Direct IO range.
> 
> In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> allocate the blocks mapped by the delalloc extent. The extent thus allocated
> may not cover the beginning of file offset range on which the Direct IO write
> was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> 
> This test addresses this issue.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: address some missing bits and remove extraneous code
> v1.2: fix cow fork dumping screwing up golden output

This version is good to me, I'll merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

BTW, I only see this patch for [PATCH 8/8], but I didn't see the "later" patch
for [PATCH 6/8], just to make sure if I missed something :)

Thanks,
Zorro

> ---
>  common/rc          |   14 ++++++++
>  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/1923.out |    8 +++++
>  3 files changed, 108 insertions(+)
>  create mode 100755 tests/xfs/1923
>  create mode 100644 tests/xfs/1923.out
> 
> diff --git a/common/rc b/common/rc
> index 50dde313b8..9f54ab1e77 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1883,6 +1883,20 @@ _require_scratch_delalloc()
>  	_scratch_unmount
>  }
>  
> +# Require test fs supports delay allocation.
> +_require_test_delalloc()
> +{
> +	_require_command "$FILEFRAG_PROG" filefrag
> +
> +	rm -f $TEST_DIR/testy
> +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> +	res=$?
> +	rm -f $TEST_DIR/testy
> +	test $res -eq 0 || \
> +		_notrun "test requires delayed allocation buffered writes"
> +}
> +
>  # this test needs a test partition - check we're ok & mount it
>  #
>  _require_test()
> diff --git a/tests/xfs/1923 b/tests/xfs/1923
> new file mode 100755
> index 0000000000..4ad3dfa764
> --- /dev/null
> +++ b/tests/xfs/1923
> @@ -0,0 +1,86 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 1923
> +#
> +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> +# it is very fragmented, it's possible that a very large delalloc reservation
> +# could be created in the CoW fork by a buffered write.  If a directio write
> +# tries to convert the delalloc reservation to a real extent, it's possible
> +# that the allocation will succeed but fail to convert even the first block of
> +# the directio write range.  In this case, XFS will return ENOSPC even though
> +# all it needed to do was to keep converting until the allocator returns ENOSPC
> +# or the first block of the direct write got some space.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick clone
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $file1 $file2 $fragmentedfile
> +}
> +
> +# Import common functions.
> +. ./common/reflink
> +. ./common/inject
> +
> +# real QA test starts here
> +_fixed_by_kernel_commit d62113303d69 \
> +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> +
> +# Modify as appropriate.
> +_supported_fs xfs
> +_require_test_program "punch-alternating"
> +_require_test_reflink
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +_require_test_delalloc
> +
> +file1=$TEST_DIR/file1.$seq
> +file2=$TEST_DIR/file2.$seq
> +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> +
> +rm -f $file1 $file2 $fragmentedfile
> +
> +# COW operates on pages, so we must not perform operations in units smaller
> +# than a page.
> +blksz=$(_get_file_block_size $TEST_DIR)
> +pagesz=$(_get_page_size)
> +if (( $blksz < $pagesz )); then
> +	blksz=$pagesz
> +fi
> +
> +echo "Create source file"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> +
> +sync
> +
> +echo "Create Reflinked file"
> +_cp_reflink $file1 $file2 >> $seqres.full
> +
> +echo "Set cowextsize"
> +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> +
> +echo "Fragment FS"
> +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> +sync
> +$here/src/punch-alternating $fragmentedfile
> +
> +echo "Allocate block sized extent from now onwards"
> +_test_inject_error bmap_alloc_minlen_extent 1
> +
> +echo "Create big delalloc extent in CoW fork"
> +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> +
> +sync
> +
> +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 &>> $seqres.full
> +
> +echo "Direct I/O write at offset 3FSB"
> +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> new file mode 100644
> index 0000000000..a0553cf3ee
> --- /dev/null
> +++ b/tests/xfs/1923.out
> @@ -0,0 +1,8 @@
> +QA output created by 1923
> +Create source file
> +Create Reflinked file
> +Set cowextsize
> +Fragment FS
> +Allocate block sized extent from now onwards
> +Create big delalloc extent in CoW fork
> +Direct I/O write at offset 3FSB
> 

Re: [PATCH v1.2 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

On Sun, Mar 10, 2024 at 05:17:34PM +0800, Zorro Lang wrote:
> On Thu, Mar 07, 2024 at 03:22:55PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> > even though the filesystem has sufficient number of free blocks.
> > 
> > This occurs if the file offset range on which the write operation is being
> > performed has a delalloc extent in the cow fork and this delalloc extent
> > begins much before the Direct IO range.
> > 
> > In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> > allocate the blocks mapped by the delalloc extent. The extent thus allocated
> > may not cover the beginning of file offset range on which the Direct IO write
> > was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> > 
> > This test addresses this issue.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: address some missing bits and remove extraneous code
> > v1.2: fix cow fork dumping screwing up golden output
> 
> This version is good to me, I'll merge it.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> BTW, I only see this patch for [PATCH 8/8], but I didn't see the "later" patch
> for [PATCH 6/8], just to make sure if I missed something :)

Oh!  Yeah, on re-reading that thread, I remembered that Christoph said
he'd look into making xfs_ondisk.h check ioctl structure sizes like
xfs/122 currently does.

In the meantime, there weren't any changes other than your RVB tag.  If
you want, I can resend it with that added.

--D

> Thanks,
> Zorro
> 
> > ---
> >  common/rc          |   14 ++++++++
> >  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/1923.out |    8 +++++
> >  3 files changed, 108 insertions(+)
> >  create mode 100755 tests/xfs/1923
> >  create mode 100644 tests/xfs/1923.out
> > 
> > diff --git a/common/rc b/common/rc
> > index 50dde313b8..9f54ab1e77 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1883,6 +1883,20 @@ _require_scratch_delalloc()
> >  	_scratch_unmount
> >  }
> >  
> > +# Require test fs supports delay allocation.
> > +_require_test_delalloc()
> > +{
> > +	_require_command "$FILEFRAG_PROG" filefrag
> > +
> > +	rm -f $TEST_DIR/testy
> > +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> > +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> > +	res=$?
> > +	rm -f $TEST_DIR/testy
> > +	test $res -eq 0 || \
> > +		_notrun "test requires delayed allocation buffered writes"
> > +}
> > +
> >  # this test needs a test partition - check we're ok & mount it
> >  #
> >  _require_test()
> > diff --git a/tests/xfs/1923 b/tests/xfs/1923
> > new file mode 100755
> > index 0000000000..4ad3dfa764
> > --- /dev/null
> > +++ b/tests/xfs/1923
> > @@ -0,0 +1,86 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 1923
> > +#
> > +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> > +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> > +# it is very fragmented, it's possible that a very large delalloc reservation
> > +# could be created in the CoW fork by a buffered write.  If a directio write
> > +# tries to convert the delalloc reservation to a real extent, it's possible
> > +# that the allocation will succeed but fail to convert even the first block of
> > +# the directio write range.  In this case, XFS will return ENOSPC even though
> > +# all it needed to do was to keep converting until the allocator returns ENOSPC
> > +# or the first block of the direct write got some space.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick clone
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $file1 $file2 $fragmentedfile
> > +}
> > +
> > +# Import common functions.
> > +. ./common/reflink
> > +. ./common/inject
> > +
> > +# real QA test starts here
> > +_fixed_by_kernel_commit d62113303d69 \
> > +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs
> > +_require_test_program "punch-alternating"
> > +_require_test_reflink
> > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > +_require_test_delalloc
> > +
> > +file1=$TEST_DIR/file1.$seq
> > +file2=$TEST_DIR/file2.$seq
> > +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> > +
> > +rm -f $file1 $file2 $fragmentedfile
> > +
> > +# COW operates on pages, so we must not perform operations in units smaller
> > +# than a page.
> > +blksz=$(_get_file_block_size $TEST_DIR)
> > +pagesz=$(_get_page_size)
> > +if (( $blksz < $pagesz )); then
> > +	blksz=$pagesz
> > +fi
> > +
> > +echo "Create source file"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +echo "Create Reflinked file"
> > +_cp_reflink $file1 $file2 >> $seqres.full
> > +
> > +echo "Set cowextsize"
> > +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> > +
> > +echo "Fragment FS"
> > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> > +sync
> > +$here/src/punch-alternating $fragmentedfile
> > +
> > +echo "Allocate block sized extent from now onwards"
> > +_test_inject_error bmap_alloc_minlen_extent 1
> > +
> > +echo "Create big delalloc extent in CoW fork"
> > +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> > +
> > +sync
> > +
> > +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 &>> $seqres.full
> > +
> > +echo "Direct I/O write at offset 3FSB"
> > +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> > new file mode 100644
> > index 0000000000..a0553cf3ee
> > --- /dev/null
> > +++ b/tests/xfs/1923.out
> > @@ -0,0 +1,8 @@
> > +QA output created by 1923
> > +Create source file
> > +Create Reflinked file
> > +Set cowextsize
> > +Fragment FS
> > +Allocate block sized extent from now onwards
> > +Create big delalloc extent in CoW fork
> > +Direct I/O write at offset 3FSB
> > 
> 

Re: [PATCH v1.2 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Zorro Lang]

On Sun, Mar 10, 2024 at 09:26:54AM -0700, Darrick J. Wong wrote:
> On Sun, Mar 10, 2024 at 05:17:34PM +0800, Zorro Lang wrote:
> > On Thu, Mar 07, 2024 at 03:22:55PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> > > even though the filesystem has sufficient number of free blocks.
> > > 
> > > This occurs if the file offset range on which the write operation is being
> > > performed has a delalloc extent in the cow fork and this delalloc extent
> > > begins much before the Direct IO range.
> > > 
> > > In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> > > allocate the blocks mapped by the delalloc extent. The extent thus allocated
> > > may not cover the beginning of file offset range on which the Direct IO write
> > > was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> > > 
> > > This test addresses this issue.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > > v1.1: address some missing bits and remove extraneous code
> > > v1.2: fix cow fork dumping screwing up golden output
> > 
> > This version is good to me, I'll merge it.
> > 
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > BTW, I only see this patch for [PATCH 8/8], but I didn't see the "later" patch
> > for [PATCH 6/8], just to make sure if I missed something :)
> 
> Oh!  Yeah, on re-reading that thread, I remembered that Christoph said
> he'd look into making xfs_ondisk.h check ioctl structure sizes like
> xfs/122 currently does.
> 
> In the meantime, there weren't any changes other than your RVB tag.  If
> you want, I can resend it with that added.

Oh, if that patch don't need to be changed, I'll merge it directly, don't
need resending it :)

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > ---
> > >  common/rc          |   14 ++++++++
> > >  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/xfs/1923.out |    8 +++++
> > >  3 files changed, 108 insertions(+)
> > >  create mode 100755 tests/xfs/1923
> > >  create mode 100644 tests/xfs/1923.out
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 50dde313b8..9f54ab1e77 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1883,6 +1883,20 @@ _require_scratch_delalloc()
> > >  	_scratch_unmount
> > >  }
> > >  
> > > +# Require test fs supports delay allocation.
> > > +_require_test_delalloc()
> > > +{
> > > +	_require_command "$FILEFRAG_PROG" filefrag
> > > +
> > > +	rm -f $TEST_DIR/testy
> > > +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> > > +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> > > +	res=$?
> > > +	rm -f $TEST_DIR/testy
> > > +	test $res -eq 0 || \
> > > +		_notrun "test requires delayed allocation buffered writes"
> > > +}
> > > +
> > >  # this test needs a test partition - check we're ok & mount it
> > >  #
> > >  _require_test()
> > > diff --git a/tests/xfs/1923 b/tests/xfs/1923
> > > new file mode 100755
> > > index 0000000000..4ad3dfa764
> > > --- /dev/null
> > > +++ b/tests/xfs/1923
> > > @@ -0,0 +1,86 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 1923
> > > +#
> > > +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> > > +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> > > +# it is very fragmented, it's possible that a very large delalloc reservation
> > > +# could be created in the CoW fork by a buffered write.  If a directio write
> > > +# tries to convert the delalloc reservation to a real extent, it's possible
> > > +# that the allocation will succeed but fail to convert even the first block of
> > > +# the directio write range.  In this case, XFS will return ENOSPC even though
> > > +# all it needed to do was to keep converting until the allocator returns ENOSPC
> > > +# or the first block of the direct write got some space.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick clone
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $file1 $file2 $fragmentedfile
> > > +}
> > > +
> > > +# Import common functions.
> > > +. ./common/reflink
> > > +. ./common/inject
> > > +
> > > +# real QA test starts here
> > > +_fixed_by_kernel_commit d62113303d69 \
> > > +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs xfs
> > > +_require_test_program "punch-alternating"
> > > +_require_test_reflink
> > > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > > +_require_test_delalloc
> > > +
> > > +file1=$TEST_DIR/file1.$seq
> > > +file2=$TEST_DIR/file2.$seq
> > > +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> > > +
> > > +rm -f $file1 $file2 $fragmentedfile
> > > +
> > > +# COW operates on pages, so we must not perform operations in units smaller
> > > +# than a page.
> > > +blksz=$(_get_file_block_size $TEST_DIR)
> > > +pagesz=$(_get_page_size)
> > > +if (( $blksz < $pagesz )); then
> > > +	blksz=$pagesz
> > > +fi
> > > +
> > > +echo "Create source file"
> > > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> > > +
> > > +sync
> > > +
> > > +echo "Create Reflinked file"
> > > +_cp_reflink $file1 $file2 >> $seqres.full
> > > +
> > > +echo "Set cowextsize"
> > > +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> > > +
> > > +echo "Fragment FS"
> > > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> > > +sync
> > > +$here/src/punch-alternating $fragmentedfile
> > > +
> > > +echo "Allocate block sized extent from now onwards"
> > > +_test_inject_error bmap_alloc_minlen_extent 1
> > > +
> > > +echo "Create big delalloc extent in CoW fork"
> > > +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> > > +
> > > +sync
> > > +
> > > +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 &>> $seqres.full
> > > +
> > > +echo "Direct I/O write at offset 3FSB"
> > > +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> > > new file mode 100644
> > > index 0000000000..a0553cf3ee
> > > --- /dev/null
> > > +++ b/tests/xfs/1923.out
> > > @@ -0,0 +1,8 @@
> > > +QA output created by 1923
> > > +Create source file
> > > +Create Reflinked file
> > > +Set cowextsize
> > > +Fragment FS
> > > +Allocate block sized extent from now onwards
> > > +Create big delalloc extent in CoW fork
> > > +Direct I/O write at offset 3FSB
> > > 
> > 
> 

Re: [PATCH v1.2 8/8] xfs: test for premature ENOSPC with large cow delalloc extents

[Darrick J. Wong]

On Mon, Mar 11, 2024 at 09:40:01PM +0800, Zorro Lang wrote:
> On Sun, Mar 10, 2024 at 09:26:54AM -0700, Darrick J. Wong wrote:
> > On Sun, Mar 10, 2024 at 05:17:34PM +0800, Zorro Lang wrote:
> > > On Thu, Mar 07, 2024 at 03:22:55PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > On a higly fragmented filesystem a Direct IO write can fail with -ENOSPC error
> > > > even though the filesystem has sufficient number of free blocks.
> > > > 
> > > > This occurs if the file offset range on which the write operation is being
> > > > performed has a delalloc extent in the cow fork and this delalloc extent
> > > > begins much before the Direct IO range.
> > > > 
> > > > In such a scenario, xfs_reflink_allocate_cow() invokes xfs_bmapi_write() to
> > > > allocate the blocks mapped by the delalloc extent. The extent thus allocated
> > > > may not cover the beginning of file offset range on which the Direct IO write
> > > > was issued. Hence xfs_reflink_allocate_cow() ends up returning -ENOSPC.
> > > > 
> > > > This test addresses this issue.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > > v1.1: address some missing bits and remove extraneous code
> > > > v1.2: fix cow fork dumping screwing up golden output
> > > 
> > > This version is good to me, I'll merge it.
> > > 
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > 
> > > BTW, I only see this patch for [PATCH 8/8], but I didn't see the "later" patch
> > > for [PATCH 6/8], just to make sure if I missed something :)
> > 
> > Oh!  Yeah, on re-reading that thread, I remembered that Christoph said
> > he'd look into making xfs_ondisk.h check ioctl structure sizes like
> > xfs/122 currently does.
> > 
> > In the meantime, there weren't any changes other than your RVB tag.  If
> > you want, I can resend it with that added.
> 
> Oh, if that patch don't need to be changed, I'll merge it directly, don't
> need resending it :)

That's correct -- the patch itself doesn't need changes, but the test
itself may get removed some day.

--D

> > 
> > --D
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > ---
> > > >  common/rc          |   14 ++++++++
> > > >  tests/xfs/1923     |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/xfs/1923.out |    8 +++++
> > > >  3 files changed, 108 insertions(+)
> > > >  create mode 100755 tests/xfs/1923
> > > >  create mode 100644 tests/xfs/1923.out
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 50dde313b8..9f54ab1e77 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -1883,6 +1883,20 @@ _require_scratch_delalloc()
> > > >  	_scratch_unmount
> > > >  }
> > > >  
> > > > +# Require test fs supports delay allocation.
> > > > +_require_test_delalloc()
> > > > +{
> > > > +	_require_command "$FILEFRAG_PROG" filefrag
> > > > +
> > > > +	rm -f $TEST_DIR/testy
> > > > +	$XFS_IO_PROG -f -c 'pwrite 0 64k' $TEST_DIR/testy &> /dev/null
> > > > +	$FILEFRAG_PROG -v $TEST_DIR/testy 2>&1 | grep -q delalloc
> > > > +	res=$?
> > > > +	rm -f $TEST_DIR/testy
> > > > +	test $res -eq 0 || \
> > > > +		_notrun "test requires delayed allocation buffered writes"
> > > > +}
> > > > +
> > > >  # this test needs a test partition - check we're ok & mount it
> > > >  #
> > > >  _require_test()
> > > > diff --git a/tests/xfs/1923 b/tests/xfs/1923
> > > > new file mode 100755
> > > > index 0000000000..4ad3dfa764
> > > > --- /dev/null
> > > > +++ b/tests/xfs/1923
> > > > @@ -0,0 +1,86 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2022-2024 Oracle.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test No. 1923
> > > > +#
> > > > +# This is a regression test for "xfs: Fix false ENOSPC when performing direct
> > > > +# write on a delalloc extent in cow fork".  If there is a lot of free space but
> > > > +# it is very fragmented, it's possible that a very large delalloc reservation
> > > > +# could be created in the CoW fork by a buffered write.  If a directio write
> > > > +# tries to convert the delalloc reservation to a real extent, it's possible
> > > > +# that the allocation will succeed but fail to convert even the first block of
> > > > +# the directio write range.  In this case, XFS will return ENOSPC even though
> > > > +# all it needed to do was to keep converting until the allocator returns ENOSPC
> > > > +# or the first block of the direct write got some space.
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick clone
> > > > +
> > > > +_cleanup()
> > > > +{
> > > > +	cd /
> > > > +	rm -f $file1 $file2 $fragmentedfile
> > > > +}
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/reflink
> > > > +. ./common/inject
> > > > +
> > > > +# real QA test starts here
> > > > +_fixed_by_kernel_commit d62113303d69 \
> > > > +	"xfs: Fix false ENOSPC when performing direct write on a delalloc extent in cow fork"
> > > > +
> > > > +# Modify as appropriate.
> > > > +_supported_fs xfs
> > > > +_require_test_program "punch-alternating"
> > > > +_require_test_reflink
> > > > +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> > > > +_require_test_delalloc
> > > > +
> > > > +file1=$TEST_DIR/file1.$seq
> > > > +file2=$TEST_DIR/file2.$seq
> > > > +fragmentedfile=$TEST_DIR/fragmentedfile.$seq
> > > > +
> > > > +rm -f $file1 $file2 $fragmentedfile
> > > > +
> > > > +# COW operates on pages, so we must not perform operations in units smaller
> > > > +# than a page.
> > > > +blksz=$(_get_file_block_size $TEST_DIR)
> > > > +pagesz=$(_get_page_size)
> > > > +if (( $blksz < $pagesz )); then
> > > > +	blksz=$pagesz
> > > > +fi
> > > > +
> > > > +echo "Create source file"
> > > > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 256))" $file1 >> $seqres.full
> > > > +
> > > > +sync
> > > > +
> > > > +echo "Create Reflinked file"
> > > > +_cp_reflink $file1 $file2 >> $seqres.full
> > > > +
> > > > +echo "Set cowextsize"
> > > > +$XFS_IO_PROG -c "cowextsize $((blksz * 128))" -c stat $file1 >> $seqres.full
> > > > +
> > > > +echo "Fragment FS"
> > > > +$XFS_IO_PROG -f -c "pwrite 0 $((blksz * 512))" $fragmentedfile >> $seqres.full
> > > > +sync
> > > > +$here/src/punch-alternating $fragmentedfile
> > > > +
> > > > +echo "Allocate block sized extent from now onwards"
> > > > +_test_inject_error bmap_alloc_minlen_extent 1
> > > > +
> > > > +echo "Create big delalloc extent in CoW fork"
> > > > +$XFS_IO_PROG -c "pwrite 0 $blksz" $file1 >> $seqres.full
> > > > +
> > > > +sync
> > > > +
> > > > +$XFS_IO_PROG -c 'bmap -elpv' -c 'bmap -celpv' $file1 &>> $seqres.full
> > > > +
> > > > +echo "Direct I/O write at offset 3FSB"
> > > > +$XFS_IO_PROG -d -c "pwrite $((blksz * 3)) $((blksz * 2))" $file1 >> $seqres.full
> > > > +
> > > > +# success, all done
> > > > +status=0
> > > > +exit
> > > > diff --git a/tests/xfs/1923.out b/tests/xfs/1923.out
> > > > new file mode 100644
> > > > index 0000000000..a0553cf3ee
> > > > --- /dev/null
> > > > +++ b/tests/xfs/1923.out
> > > > @@ -0,0 +1,8 @@
> > > > +QA output created by 1923
> > > > +Create source file
> > > > +Create Reflinked file
> > > > +Set cowextsize
> > > > +Fragment FS
> > > > +Allocate block sized extent from now onwards
> > > > +Create big delalloc extent in CoW fork
> > > > +Direct I/O write at offset 3FSB
> > > > 
> > > 
> > 
> 
> 

Re: [PATCHSET] fstests: random fixes for v2024.02.09

[Zorro Lang]

On Mon, Feb 26, 2024 at 06:00:41PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> Here's the usual odd fixes for fstests.  Most of these are cleanups and
> bug fixes that have been aging in my djwong-wtf branch forever.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> This has been running on the djcloud for months with no problems.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes
> 
> xfsprogs git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes
> 
> fstests git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
> ---
> Commits in this patchset:
>  * generic/604: try to make race occur reliably
>  * xfs/155: fail the test if xfs_repair hangs for too long
>  * generic/192: fix spurious timeout?
>  * generic/491: increase test timeout
>  * xfs/599: reduce the amount of attrs created here
>  * xfs/122: update test to pick up rtword/suminfo ondisk unions
>  * xfs/43[4-6]: make module reloading optional
>  * xfs: test for premature ENOSPC with large cow delalloc extents

Hi Darrick,

This patchset didn't catch last fstests release. Six of the 8 patches are
acked, I've merge those 6 patches in my local branch. They're in testing.
Now only the patch [6/8] and the [8/8] are waiting for your response.

As this's a random fix patchset, and I've merged 6 of it. If you have more
patches are waiting for reviewing, you can send those 2 patches in another
patchset :)

Thanks,
Zorro

> ---
>  common/module      |   34 ++++++++++++++++++---
>  common/rc          |   14 +++++++++
>  tests/generic/192  |   16 ++++++++--
>  tests/generic/491  |    2 +
>  tests/generic/604  |    7 ++--
>  tests/xfs/122      |    2 +
>  tests/xfs/122.out  |    2 +
>  tests/xfs/155      |    4 ++
>  tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/1923.out |    8 +++++
>  tests/xfs/434      |    3 +-
>  tests/xfs/435      |    3 +-
>  tests/xfs/436      |    3 +-
>  tests/xfs/599      |    9 ++----
>  14 files changed, 168 insertions(+), 24 deletions(-)
>  create mode 100755 tests/xfs/1923
>  create mode 100644 tests/xfs/1923.out
> 

Re: [PATCHSET] fstests: random fixes for v2024.02.09

[Darrick J. Wong]

On Sun, Mar 03, 2024 at 09:34:53PM +0800, Zorro Lang wrote:
> On Mon, Feb 26, 2024 at 06:00:41PM -0800, Darrick J. Wong wrote:
> > Hi all,
> > 
> > Here's the usual odd fixes for fstests.  Most of these are cleanups and
> > bug fixes that have been aging in my djwong-wtf branch forever.
> > 
> > If you're going to start using this code, I strongly recommend pulling
> > from my git trees, which are linked below.
> > 
> > This has been running on the djcloud for months with no problems.  Enjoy!
> > Comments and questions are, as always, welcome.
> > 
> > --D
> > 
> > kernel git tree:
> > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes
> > 
> > xfsprogs git tree:
> > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes
> > 
> > fstests git tree:
> > https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
> > ---
> > Commits in this patchset:
> >  * generic/604: try to make race occur reliably
> >  * xfs/155: fail the test if xfs_repair hangs for too long
> >  * generic/192: fix spurious timeout?
> >  * generic/491: increase test timeout
> >  * xfs/599: reduce the amount of attrs created here
> >  * xfs/122: update test to pick up rtword/suminfo ondisk unions
> >  * xfs/43[4-6]: make module reloading optional
> >  * xfs: test for premature ENOSPC with large cow delalloc extents
> 
> Hi Darrick,
> 
> This patchset didn't catch last fstests release. Six of the 8 patches are
> acked, I've merge those 6 patches in my local branch. They're in testing.
> Now only the patch [6/8] and the [8/8] are waiting for your response.
> 
> As this's a random fix patchset, and I've merged 6 of it. If you have more
> patches are waiting for reviewing, you can send those 2 patches in another
> patchset :)

The only pending things I have are resends of 6 and 8.  Will go do that
momentarily, sorry for being a bit late this week.

--D

> Thanks,
> Zorro
> 
> > ---
> >  common/module      |   34 ++++++++++++++++++---
> >  common/rc          |   14 +++++++++
> >  tests/generic/192  |   16 ++++++++--
> >  tests/generic/491  |    2 +
> >  tests/generic/604  |    7 ++--
> >  tests/xfs/122      |    2 +
> >  tests/xfs/122.out  |    2 +
> >  tests/xfs/155      |    4 ++
> >  tests/xfs/1923     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/1923.out |    8 +++++
> >  tests/xfs/434      |    3 +-
> >  tests/xfs/435      |    3 +-
> >  tests/xfs/436      |    3 +-
> >  tests/xfs/599      |    9 ++----
> >  14 files changed, 168 insertions(+), 24 deletions(-)
> >  create mode 100755 tests/xfs/1923
> >  create mode 100644 tests/xfs/1923.out
> > 
> 
>