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

From: Gao Xiang <hsiangkao@redhat.com>
To: Dave Chinner <david@fromorbit.com>,
	"Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org, Zorro Lang <zlang@redhat.com>
Subject: Re: [PATCH] xfs: don't use in-core per-cpu fdblocks for !lazysbcount
Date: Sat, 17 Apr 2021 10:20:13 +0800	[thread overview]
Message-ID: <20210417022013.GA2266103@xiangao.remote.csb> (raw)
In-Reply-To: <20210417015702.GT63242@dread.disaster.area>

Hi Darrick and Dave,

On Sat, Apr 17, 2021 at 11:57:02AM +1000, Dave Chinner wrote:
> On Fri, Apr 16, 2021 at 05:19:41PM -0700, Darrick J. Wong wrote:
> > On Sat, Apr 17, 2021 at 05:13:20AM +0800, Gao Xiang wrote:

...

> 
> Nor is it necessary to fix the problem.
> 
> > > > > 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);
> > 
> > Hmm... is this really needed?  I thought in !lazysbcount mode,
> > xfs_trans_apply_sb_deltas updates the ondisk super buffer directly.
> > So aren't all three of these updates unnecessary?
> 
> Yup, now I understand the issue, the fix is simply to avoid these
> updates for !lazysb. i.e. it should just be:
> 
> 	if (xfs_sb_version_haslazysbcount(&mp->m_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);
> 	}
> 	xfs_sb_to_disk(bp->b_addr, &mp->m_sb);

I did as this because xfs_sb_to_disk() will override them, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/xfs/libxfs/xfs_sb.c#n629

...
	to->sb_icount = cpu_to_be64(from->sb_icount);
	to->sb_ifree = cpu_to_be64(from->sb_ifree);
	to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);

As an alternative, I was once to wrap it as:

xfs_sb_to_disk() {
...
	if (xfs_sb_version_haslazysbcount(&mp->m_sb)) {
		to->sb_icount = cpu_to_be64(from->sb_icount);
		to->sb_ifree = cpu_to_be64(from->sb_ifree);
		to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);
	}
...
}

Yet after I observed the other callers of xfs_sb_to_disk() (e.g. growfs
and online repair), I think a better modification is the way I proposed
here, so no need to update xfs_sb_to_disk() and the other callers (since
!lazysbcount is not recommended at all.)

It's easier to backport and less conflict, and btw !lazysbcount also need
to be warned out and deprecated from now.

Thanks,
Gao Xiang

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