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

[Zorro Lang <zlang@redhat.com>]

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