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@xxxxxxxxxx> > > (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@xxxxxxxxxxxxx > > >
