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. 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. 3) in-memory sb counters are serialized by some spinlock now, so I'm not sure sb per-CPU counters behave for lazysbcount cases, are these used for better performance? > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > > xfs: update superblock counters correctly for !lazysbcount > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > 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. > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > --- > fs/xfs/libxfs/xfs_sb.c | 16 +++++++++++++--- > fs/xfs/xfs_trans.c | 3 +++ > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c > index 9630f9e2f540..7d4c238540d4 100644 > --- a/fs/xfs/libxfs/xfs_sb.c > +++ b/fs/xfs/libxfs/xfs_sb.c > @@ -794,9 +794,19 @@ xfs_log_sb( > struct xfs_mount *mp = tp->t_mountp; > struct xfs_buf *bp = xfs_trans_getsb(tp); > > - 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); > + /* > + * Lazy sb counters don't update the in-core superblock so do that now. > + * If this is at unmount, the counters will be exactly correct, but at > + * any other time they will only be ballpark correct because of > + * reservations that have been taken out percpu counters. If we have an > + * unclean shutdown, this will be corrected by log recovery rebuilding > + * the counters from the AGF block counts. > + */ > + 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); > 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(). Thanks, Gao Xiang > + mp->m_sb.sb_icount += idelta; > + mp->m_sb.sb_ifree += ifreedelta; > mp->m_sb.sb_frextents += rtxdelta; > mp->m_sb.sb_dblocks += tp->t_dblocks_delta; > mp->m_sb.sb_agcount += tp->t_agcount_delta; >
