Re: [PATCH v2 1/2] xfs: don't use in-core per-cpu fdblocks for !lazysbcount

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 20, 2021 at 07:08:54PM +0800, Gao Xiang wrote:
> There are many paths which could trigger xfs_log_sb(), e.g.
>   xfs_bmap_add_attrfork()
>     -> xfs_log_sb()
> , which overrides on-disk fdblocks by in-core per-CPU fdblocks.
> 
> However, for !lazysbcount cases, on-disk fdblocks is actually updated
> by xfs_trans_apply_sb_deltas(), and generally it isn't equal to
> in-core per-CPU fdblocks due to xfs_reserve_blocks() or whatever,
> see the comment in xfs_unmountfs().
> 
> It could be observed by the following steps reported by Zorro:
> 
> 1. mkfs.xfs -f -l lazy-count=0 -m crc=0 $dev
> 2. mount $dev $mnt
> 3. fsstress -d $mnt -p 100 -n 1000 (maybe need more or less io load)
> 4. umount $mnt
> 5. xfs_repair -n $dev
> 
> yet due to commit f46e5a174655 ("xfs: fold sbcount quiesce logging
> into log covering"), xfs_sync_sb() will also be triggered if log
> covering is needed and !lazysbcount when xfs_unmountfs(), so hard
> to reproduce on kernel 5.12+ for clean unmount.
> 
> on-disk sb_icount and sb_ifree are also updated in
> xfs_trans_apply_sb_deltas() for !lazysbcount cases, however, which
> are always equal to per-CPU counters, so only fdblocks matters.
> 
> After this patch, I've seen no strange so far on older kernels
> for the testcase above without lazysbcount.
> 
> Reported-by: Zorro Lang <zlang@redhat.com>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v1:
>  - update commit message.
> 
>  fs/xfs/libxfs/xfs_sb.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 60e6d255e5e2..423dada3f64c 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -928,7 +928,13 @@ xfs_log_sb(
>  
>  	mp->m_sb.sb_icount = percpu_counter_sum(&mp->m_icount);
>  	mp->m_sb.sb_ifree = percpu_counter_sum(&mp->m_ifree);
> -	mp->m_sb.sb_fdblocks = percpu_counter_sum(&mp->m_fdblocks);
> +	if (!xfs_sb_version_haslazysbcount(&mp->m_sb)) {
> +		struct xfs_dsb	*dsb = bp->b_addr;
> +
> +		mp->m_sb.sb_fdblocks = be64_to_cpu(dsb->sb_fdblocks);
> +	} else {
> +		mp->m_sb.sb_fdblocks = percpu_counter_sum(&mp->m_fdblocks);
> +	}

THis really needs a comment explaining why this is done this way.
It's not obvious from reading the code why we pull the the fdblock
count off disk and then, in  xfs_sb_to_disk(), we write it straight
back to disk.

It's also not clear to me that summing the inode counters is correct
in the case of the !lazysbcount for the similar reasons - the percpu
counter is not guaranteed to be absolutely accurate here, yet the
values in the disk buffer are. Perhaps we should be updating the
m_sb values in xfs_trans_apply_sb_deltas() for the !lazycount case,
and only summing them here for the lazycount case...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com