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:
> > 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@xxxxxxxxxx>
> > > > Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > > > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx>
> > > > ---
> > > > 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.)
>
> I don't follow - xfs_trans_apply_sb_deltas() already logs the
> changes to the superblock made in the transaction.
>
> xfs_trans_unreserve_and_mod_sb() does the in-memory counter updates
> after xfs_trans_apply_sb_deltas() applies them to the on-disk
> superblock in the buffer and logs them.
>
> But nowhere on a !lazysbcount setup are mp->m_sb.sb_fdcount/ifree/
> icount values being updated, and hence they are not valid at any
> time except for during log quiesce where all the in memory
> reservations have been removed and the per-cpu counters are synced
> to mp->m_sb.
>
> I'm suggesting that xfs_trans_unreserve_and_mod_sb() also updates
> the mp->m_sb.sb_fdcount/ifree/icount values for !lazysbcount, as we
> currently do not do this and this will keep them uptodate for any
> caller of xfs_sb_to_disk().
>
> i.e. we have three choices:
>
> 1. avoid writing the counters in xfs_sb_to_disk() for !lazycount.
> 2. read them from the buffer before writing them back to the buffer.
> 3. keep them up to date correctly via xfs_trans_unreserve_and_mod_sb.
>
> #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()
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/
and I'm not sure if we be64_add_cpu() on-disk sb counters in
xfs_trans_apply_sb_deltas() and then mp->m_sb.sb_fdblocks +=
tp->t_fdblocks_delta again in xfs_trans_unreserve_and_mod_sb()
seems much better.
Thanks,
Gao Xiang
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
