Re: [PATCH] xfs: update superblock counters correctly for !lazysbcount

[Gao Xiang <hsiangkao@redhat.com>]

On Tue, Apr 27, 2021 at 10:25:31AM -0400, Brian Foster wrote:
> On Tue, Apr 27, 2021 at 09:13:18PM +0800, Gao Xiang wrote:
> > On Tue, Apr 27, 2021 at 08:46:25AM -0400, Brian Foster wrote:
> > > On Tue, Apr 27, 2021 at 09:12:01AM +0800, Gao Xiang wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Keep the mount superblock counters up to date for !lazysbcount
> > > > filesystems so that when we log the superblock they do not need
> > > > updating in any way because they are already correct.
> > > > 
> > > > It's found by what Zorro reported:
> > > > 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
> > > > and I've seen no problem with this patch.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > Reported-by: Zorro Lang <zlang@redhat.com>
> > > > Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
> > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > ---
> > > 
> > > Could you provide a bit more detail on the problem in the commit log?
> > > From the description and code change, it seems like there is some
> > > problem with doing the percpu aggregation in xfs_log_sb() on
> > > !lazysbcount filesystems. Therefore this patch reserves that behavior
> > > for lazysbcount, and instead enables per-transaction updates in the
> > > !lazysbcount specific cleanup path. Am I following that correctly?
> > 
> > This patch inherited from Dave's patch [1] (and I added reproduable
> > steps),
> > https://lore.kernel.org/r/20210422014446.GZ63242@dread.disaster.area
> > 
> > More details see my original patch v2:
> > https://lore.kernel.org/r/20210420110855.2961626-1-hsiangkao@redhat.com
> > 
> 
> Ok, thanks. So the bit about xfs_log_sb() is to avoid an incorrect
> overwrite of the in-core sb counters from the percpu counters on
> !lazysbcount. The xfs_trans_apply_sb_deltas() function already applies
> the transaction deltas to the on-disk superblock buffer, so the change
> to xfs_trans_unreserve_and_mod_sb() is basically to apply those same
> deltas to the in-core superblock so they are consistent in the
> !lazysbcount case... yes? If I'm following that correctly, this looks
> good to me:

Yeah, that's right :)

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for the review!

Thanks,
Gao Xiang

> 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Brian