[PATCHSET 0/2] fstests: random fixes

[PATCHSET 0/2] fstests: random fixes

[Darrick J. Wong]

Hi all,

Here's the usual batch of odd fixes for fstests.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  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
---
 tests/xfs/187 |    2 +-
 tests/xfs/507 |    6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

[PATCH 1/2] xfs/187: don't rely on FSCOUNTS for free space data

[Darrick J. Wong]

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

Currently, this test relies on the XFS_IOC_FSCOUNTS ioctl to return
accurate free space information.  It doesn't.  Convert it to use statfs,
which uses the accurate versions of the percpu counters.  Obviously,
this only becomes a problem when we convert the free rtx count to use
(sloppier) percpu counters instead of the (more precise and previously
buggy) ondisk superblock counts.

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


diff --git a/tests/xfs/187 b/tests/xfs/187
index 1929e566..a9dfb30a 100755
--- a/tests/xfs/187
+++ b/tests/xfs/187
@@ -135,7 +135,7 @@ punch_off=$((bigfile_sz - frag_sz))
 $here/src/punch-alternating $SCRATCH_MNT/bigfile -o $((punch_off / fsbsize)) -i $((rtextsize_blks * 2)) -s $rtextsize_blks
 
 # Make sure we have some free rtextents.
-free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep counts.freertx | awk '{print $3}')
+free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep statfs.f_bavail | awk '{print $3}')
 if [ $free_rtx -eq 0 ]; then
 	echo "Expected fragmented free rt space, found none."
 fi

[PATCH 2/2] xfs/507: add test to auto group

[Darrick J. Wong]

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

Add this regression test to the auto group now that it's been quite a
while since the fix patches went upstream.

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


diff --git a/tests/xfs/507 b/tests/xfs/507
index aa3d8eeb..b9c9ab29 100755
--- a/tests/xfs/507
+++ b/tests/xfs/507
@@ -4,13 +4,17 @@
 #
 # FS QA Test No. 507
 #
+# Regression test for kernel commit:
+#
+# 394aafdc15da ("xfs: widen inode delalloc block counter to 64-bits")
+#
 # Try to overflow i_delayed_blks by setting the largest cowextsize hint
 # possible, creating a sparse file with a single byte every cowextsize bytes,
 # reflinking it, and retouching every written byte to see if we can create
 # enough speculative COW reservations to overflow i_delayed_blks.
 #
 . ./common/preamble
-_begin_fstest clone
+_begin_fstest auto clone
 
 _register_cleanup "_cleanup" BUS
 

Re: [PATCH 1/2] xfs/187: don't rely on FSCOUNTS for free space data

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Currently, this test relies on the XFS_IOC_FSCOUNTS ioctl to return
> accurate free space information.  It doesn't.  Convert it to use statfs,
> which uses the accurate versions of the percpu counters.  Obviously,
> this only becomes a problem when we convert the free rtx count to use
> (sloppier) percpu counters instead of the (more precise and previously
> buggy) ondisk superblock counts.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/187 |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/187 b/tests/xfs/187
> index 1929e566..a9dfb30a 100755
> --- a/tests/xfs/187
> +++ b/tests/xfs/187
> @@ -135,7 +135,7 @@ punch_off=$((bigfile_sz - frag_sz))
>  $here/src/punch-alternating $SCRATCH_MNT/bigfile -o $((punch_off / fsbsize)) -i $((rtextsize_blks * 2)) -s $rtextsize_blks
>  
>  # Make sure we have some free rtextents.
> -free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep counts.freertx | awk '{print $3}')
> +free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep statfs.f_bavail | awk '{print $3}')

Do you mean the "cnt->freertx = mp->m_sb.sb_frextents" in xfs_fs_counts() isn't
right?

Thanks,
Zorro

>  if [ $free_rtx -eq 0 ]; then
>  	echo "Expected fragmented free rt space, found none."
>  fi
> 

Re: [PATCH 2/2] xfs/507: add test to auto group

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add this regression test to the auto group now that it's been quite a
> while since the fix patches went upstream.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

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

>  tests/xfs/507 |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/507 b/tests/xfs/507
> index aa3d8eeb..b9c9ab29 100755
> --- a/tests/xfs/507
> +++ b/tests/xfs/507
> @@ -4,13 +4,17 @@
>  #
>  # FS QA Test No. 507
>  #
> +# Regression test for kernel commit:
> +#
> +# 394aafdc15da ("xfs: widen inode delalloc block counter to 64-bits")
> +#
>  # Try to overflow i_delayed_blks by setting the largest cowextsize hint
>  # possible, creating a sparse file with a single byte every cowextsize bytes,
>  # reflinking it, and retouching every written byte to see if we can create
>  # enough speculative COW reservations to overflow i_delayed_blks.
>  #
>  . ./common/preamble
> -_begin_fstest clone
> +_begin_fstest auto clone
>  
>  _register_cleanup "_cleanup" BUS
>  
> 

Re: [PATCH 1/2] xfs/187: don't rely on FSCOUNTS for free space data

[Darrick J. Wong]

On Tue, Apr 12, 2022 at 04:47:16PM +0800, Zorro Lang wrote:
> On Mon, Apr 11, 2022 at 03:54:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Currently, this test relies on the XFS_IOC_FSCOUNTS ioctl to return
> > accurate free space information.  It doesn't.  Convert it to use statfs,
> > which uses the accurate versions of the percpu counters.  Obviously,
> > this only becomes a problem when we convert the free rtx count to use
> > (sloppier) percpu counters instead of the (more precise and previously
> > buggy) ondisk superblock counts.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/187 |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/tests/xfs/187 b/tests/xfs/187
> > index 1929e566..a9dfb30a 100755
> > --- a/tests/xfs/187
> > +++ b/tests/xfs/187
> > @@ -135,7 +135,7 @@ punch_off=$((bigfile_sz - frag_sz))
> >  $here/src/punch-alternating $SCRATCH_MNT/bigfile -o $((punch_off / fsbsize)) -i $((rtextsize_blks * 2)) -s $rtextsize_blks
> >  
> >  # Make sure we have some free rtextents.
> > -free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep counts.freertx | awk '{print $3}')
> > +free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep statfs.f_bavail | awk '{print $3}')
> 
> Do you mean the "cnt->freertx = mp->m_sb.sb_frextents" in xfs_fs_counts() isn't
> right?

Correct -- prior to the patches introduced here:
https://lore.kernel.org/linux-xfs/164961485474.70555.18228016043917319266.stgit@magnolia/T/#t

The kernel would account actual ondisk rt extent usage *and* in-memory
transaction reservations in mp->m_sb.sb_frextents, which meant that one
thread calling xfs_log_sb racing with another thread allocating space on
the rt volume would write the wrong sb_frextents value to disk, which
corrupts the superblock counters.

The fix for that is to separate the two uses into separate counters --
now mp->m_sb.sb_frextents tracks the ondisk usage, and mp->m_frextents
also includes tx reservations.  m_frextents is a percpu counter, which
means that we won't be able to rely on it for a precise accounting after
the series is merged.  Hence the switch to statfs, which does use the
slow-but-accurate percpu_counter_sum method.

--D

> Thanks,
> Zorro
> 
> >  if [ $free_rtx -eq 0 ]; then
> >  	echo "Expected fragmented free rt space, found none."
> >  fi
> > 
> 

Re: [PATCH 1/2] xfs/187: don't rely on FSCOUNTS for free space data

[Zorro Lang]

On Tue, Apr 12, 2022 at 10:11:56AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 12, 2022 at 04:47:16PM +0800, Zorro Lang wrote:
> > On Mon, Apr 11, 2022 at 03:54:22PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Currently, this test relies on the XFS_IOC_FSCOUNTS ioctl to return
> > > accurate free space information.  It doesn't.  Convert it to use statfs,
> > > which uses the accurate versions of the percpu counters.  Obviously,
> > > this only becomes a problem when we convert the free rtx count to use
> > > (sloppier) percpu counters instead of the (more precise and previously
> > > buggy) ondisk superblock counts.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/xfs/187 |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/tests/xfs/187 b/tests/xfs/187
> > > index 1929e566..a9dfb30a 100755
> > > --- a/tests/xfs/187
> > > +++ b/tests/xfs/187
> > > @@ -135,7 +135,7 @@ punch_off=$((bigfile_sz - frag_sz))
> > >  $here/src/punch-alternating $SCRATCH_MNT/bigfile -o $((punch_off / fsbsize)) -i $((rtextsize_blks * 2)) -s $rtextsize_blks
> > >  
> > >  # Make sure we have some free rtextents.
> > > -free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep counts.freertx | awk '{print $3}')
> > > +free_rtx=$($XFS_IO_PROG -c 'statfs' $SCRATCH_MNT | grep statfs.f_bavail | awk '{print $3}')
> > 
> > Do you mean the "cnt->freertx = mp->m_sb.sb_frextents" in xfs_fs_counts() isn't
> > right?
> 
> Correct -- prior to the patches introduced here:
> https://lore.kernel.org/linux-xfs/164961485474.70555.18228016043917319266.stgit@magnolia/T/#t
> 
> The kernel would account actual ondisk rt extent usage *and* in-memory
> transaction reservations in mp->m_sb.sb_frextents, which meant that one
> thread calling xfs_log_sb racing with another thread allocating space on
> the rt volume would write the wrong sb_frextents value to disk, which
> corrupts the superblock counters.
> 
> The fix for that is to separate the two uses into separate counters --
> now mp->m_sb.sb_frextents tracks the ondisk usage, and mp->m_frextents
> also includes tx reservations.  m_frextents is a percpu counter, which
> means that we won't be able to rely on it for a precise accounting after
> the series is merged.  Hence the switch to statfs, which does use the
> slow-but-accurate percpu_counter_sum method.

Thanks for your detailed explanation, so it's about this patch:
[PATCH 3/3] xfs: use a separate frextents counter for rt extent reservations

It's good to me, it would be better if we can add more content (as above) into commit log.

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

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > >  if [ $free_rtx -eq 0 ]; then
> > >  	echo "Expected fragmented free rt space, found none."
> > >  fi
> > > 
> > 
>