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

[Gao Xiang <hsiangkao@redhat.com>]

Hi Dave,

On Wed, Apr 21, 2021 at 07:25:06AM +1000, Dave Chinner wrote:
> 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...

But if updating m_sb values in xfs_trans_apply_sb_deltas(), we
should also update on-disk sb counters in xfs_trans_apply_sb_deltas()
and log sb for !lazysbcount (since for such cases, sb counter update
should be considered immediately.)

That will indeed cause more modification, I'm not quite sure if it's
quite ok honestly. But if you assume that's more clear, I could submit
an alternative instead later.

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>