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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Apr 22, 2021 at 11:12:15AM +0800, Gao Xiang wrote:
> On Thu, Apr 22, 2021 at 01:01:02PM +1000, Dave Chinner wrote:
> > On Thu, Apr 22, 2021 at 10:06:13AM +0800, Gao Xiang wrote:
> > > Hi Dave,
> > > 
> > > On Thu, Apr 22, 2021 at 11:44:46AM +1000, Dave Chinner wrote:
> > > > On Wed, Apr 21, 2021 at 11:01:29AM +0800, Gao Xiang wrote:
> > > > > On Wed, Apr 21, 2021 at 11:45:26AM +1000, Dave Chinner wrote:
> > > > > > On Wed, Apr 21, 2021 at 05:54:43AM +0800, Gao Xiang wrote:
> > > > > > #1 is bad because there are cases where we want to write the
> > > > > > counters even for !lazysbcount filesystems (e.g. mkfs, repair, etc).
> > > > > > 
> > > > > > #2 is essentially a hack around the fact that mp->m_sb is not kept
> > > > > > up to date in the in-memory superblock for !lazysbcount filesystems.
> > > > > > 
> > > > > > #3 keeps the in-memory superblock up to date for !lazysbcount case
> > > > > > so they are coherent with the on-disk values and hence we only need
> > > > > > to update the in-memory superblock counts for lazysbcount
> > > > > > filesystems before calling xfs_sb_to_disk().
> > > > > > 
> > > > > > #3 is my preferred solution.
> > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > I think the version you posted doesn't fix the entire problem. It
> > > > > > merely slaps a band-aid over the symptom that is being seen, and
> > > > > > doesn't address all the non-coherent data that can be written to the
> > > > > > superblock here.
> > > > > 
> > > > > As I explained on IRC as well, I think for !lazysbcount cases, fdblocks,
> > > > > icount and ifree are protected by sb buffer lock. and the only users of
> > > > > these three are:
> > > > >  1) xfs_trans_apply_sb_deltas()
> > > > >  2) xfs_log_sb()
> > > > 
> > > > That's just a happy accident and not intentional in any way. Just
> > > > fixing the case that occurs while holding the sb buffer lock doesn't
> > > > actually fix the underlying problem, it just uses this as a bandaid.
> > > 
> > > I think for !lazysbcases, sb buffer lock is only a reliable lock that
> > > can be relied on for serialzing (since we need to make sure each sb
> > > write matches the corresponding fdblocks, ifree, icount. So sb buffer
> > > needs be locked every time. So so need to recalc on dirty log.)
> > > > 
> > > > > 
> > > > > So I've seen no need to update sb_icount, sb_ifree in that way (I mean
> > > > > my v2, although I agree it's a bit hacky.) only sb_fdblocks matters.
> > > > > 
> > > > > But the reason why this patch exist is only to backport to old stable
> > > > > kernels, since after [PATCH v2 2/2], we can get rid of all of
> > > > > !lazysbcount cases upstream.
> > > > > 
> > > > > But if we'd like to do more e.g. by taking m_sb_lock, I've seen the
> > > > > xfs codebase quite varies these years. and I modified some version
> > > > > like http://paste.debian.net/1194481/
> > > > 
> > > > I said on IRC that this is what xfs_trans_unreserve_and_mod_sb() is
> > > > for. For !lazysbcount filesystems the transaction will be marked
> > > > dirty (i.e XFS_TRANS_SB_DIRTY is set) and so we'll always run the
> > > > slow path that takes the m_sb_lock and updates mp->m_sb. 
> > > > 
> > > > It's faster for me to explain this by patch than any other way. See
> > > > below.
> > > 
> > > I know what you mean, but there exists 3 things:
> > >  1) we be64_add_cpu() on-disk fdblocks, ifree, icount at
> > >     xfs_trans_apply_sb_deltas(), and then do the same bahavior in
> > >     xfs_trans_unreserve_and_mod_sb() for in-memory counters again.
> > >     that is (somewhat) fragile.
> > 
> > That's exactly how the superblock updates have been done since the
> > mid 1990s. It's the way it was intended to work:
> > 
> > - xfs_trans_apply_sb_deltas() applies the changes to the on
> >   disk superblock
> > - xfs_trans_unreserve_and_mod_sb() applies the changes to the
> >   in-memory superblock.
> > 
> > All my patch does is follow the long established separation of
> > update responsibilities. It is actually returning the code to the
> > behaviour we had before lazy superblock counters were introduced.
> > 
> > >  2) m_sb_lock behaves no effect at this. This lock between
> > >     xfs_log_sb() and xfs_trans_unreserve_and_mod_sb() is still
> > >     sb buffer lock for !lazysbcount cases.
> > 
> > The m_sb_lock doesn't need to have any effect on this. It's to
> > prevent concurrent updates of the in-core superblock, not to prevent
> > access to the superblock buffer.
> > 
> > i.e. the superblock buffer lock protects against concurrent updates
> > of the superblock buffer, and hence while progating and logging
> > changes to the superblock buffer we have to have the superblock
> > buffer locked.
> > 
> > >  3) in-memory sb counters are serialized by some spinlock now,
> > 
> > No, they are not. Lazysbcount does not set XFS_TRANS_SB_DIRTY
> > for pure ifree/icount/fdblock updates, so it never runs the code
> > I modified in xfs_trans_unreserve_and_mod_sb() unless some other
> > part of the superblock is changed.
> > 
> > For !lazysbcount, we always run this path because XFS_TRANS_SB_DIRTY
> > is always set.
> > 
> > >     so I'm not sure sb per-CPU counters behave for lazysbcount
> > >     cases, are these used for better performance?
> > 
> > It does not change behaviour of anything at all, execpt the counter
> > values for !lazysbcount filesystems are now always kept correctly up
> > to date.
> > 
> > > >  	xfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > > >  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
> > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > > > index bcc978011869..438e41931b55 100644
> > > > --- a/fs/xfs/xfs_trans.c
> > > > +++ b/fs/xfs/xfs_trans.c
> > > > @@ -629,6 +629,9 @@ xfs_trans_unreserve_and_mod_sb(
> > > >  
> > > >  	/* apply remaining deltas */
> > > >  	spin_lock(&mp->m_sb_lock);
> > > > +	mp->m_sb.sb_fdblocks += blkdelta;
> > > 
> > > not sure that is quite equal to blkdelta, since (I think) we might need
> > > to apply t_res_fdblocks_delta for !lazysbcount cases but not lazysbcount
> > > cases, but I'm not quite sure, just saw the comment above
> > > xfs_trans_unreserve_and_mod_sb() and the implementation of
> > > xfs_trans_apply_sb_deltas().
> > 
> > Yes, I forgot about the special delayed allocation space accounting.
> > We'll have to add that, too, so it becomes:
> > 
> > +	mp->m_sb.sb_fdblocks += blkdelta + tp->t_res_fdblocks_delta;
> > +	mp->m_sb.sb_icount += idelta;
> > +	mp->m_sb.sb_ifree += ifreedelta;
> > 
> > But this doesn't change the structure of the patch in any way.
> 
> Anyway, I think this'd be absolutely fine to fix this issue as well,
> so:
> 
> Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
> 
> (Although I still insist on my v2 [just my own thought] since in-memory
>  sb counters are totally unused/reserved compared with on-disk sb counters
>  for sb_fdblocks and per-CPU sb counters for sb_ifree / sb_icount for
>  the whole !lazysbcount cases, maybe adding some comments is better.
>  But I'm also fine if the patch goes like this ;) )

Does this patch (+ other fixes) fix the problem?  If Zorro says it's ok,
please send this as a formal patch submission so it isn't buried in a
thread.

--D

> Thanks,
> Gao Xiang
> 
> > 
> > CHeers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
>