On Wed, Feb 24, 2021 at 01:42:35PM -0800, Darrick J. Wong wrote: > On Tue, Feb 23, 2021 at 04:32:10PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > In doing an investigation into AIL push stalls, I was looking at the > > log force code to see if an async CIL push could be done instead. > > This lead me to xfs_log_force_lsn() and looking at how it works. > > > > xfs_log_force_lsn() is only called from inode synchronisation > > contexts such as fsync(), and it takes the ip->i_itemp->ili_last_lsn > > value as the LSN to sync the log to. This gets passed to > > xlog_cil_force_lsn() via xfs_log_force_lsn() to flush the CIL to the > > journal, and then used by xfs_log_force_lsn() to flush the iclogs to > > the journal. > > > > The problem with is that ip->i_itemp->ili_last_lsn does not store a > > log sequence number. What it stores is passed to it from the > > ->iop_committing method, which is called by xfs_log_commit_cil(). > > The value this passes to the iop_committing method is the CIL > > context sequence number that the item was committed to. > > > > As it turns out, xlog_cil_force_lsn() converts the sequence to an > > actual commit LSN for the related context and returns that to > > xfs_log_force_lsn(). xfs_log_force_lsn() overwrites it's "lsn" > > variable that contained a sequence with an actual LSN and then uses > > that to sync the iclogs. > > > > This caused me some confusion for a while, even though I originally > > wrote all this code a decade ago. ->iop_committing is only used by > > a couple of log item types, and only inode items use the sequence > > number it is passed. > > > > Let's clean up the API, CIL structures and inode log item to call it > > a sequence number, and make it clear that the high level code is > > using CIL sequence numbers and not on-disk LSNs for integrity > > synchronisation purposes. > > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > > --- > > fs/xfs/xfs_buf_item.c | 2 +- > > fs/xfs/xfs_dquot_item.c | 2 +- > > fs/xfs/xfs_file.c | 14 +++++++------- > > fs/xfs/xfs_inode.c | 10 +++++----- > > fs/xfs/xfs_inode_item.c | 4 ++-- > > fs/xfs/xfs_inode_item.h | 2 +- > > fs/xfs/xfs_log.c | 27 ++++++++++++++------------- > > fs/xfs/xfs_log.h | 4 +--- > > fs/xfs/xfs_log_cil.c | 22 +++++++++------------- > > fs/xfs/xfs_log_priv.h | 15 +++++++-------- > > fs/xfs/xfs_trans.c | 6 +++--- > > fs/xfs/xfs_trans.h | 4 ++-- > > 12 files changed, 53 insertions(+), 59 deletions(-) > > > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c > > index 14d1fefcbf4c..7affe1aa16da 100644 > > --- a/fs/xfs/xfs_buf_item.c > > +++ b/fs/xfs/xfs_buf_item.c > > @@ -713,7 +713,7 @@ xfs_buf_item_release( > > STATIC void > > xfs_buf_item_committing( > > struct xfs_log_item *lip, > > - xfs_lsn_t commit_lsn) > > + uint64_t seq) > > FWIW I rather wish you'd defined a new type for cil sequence numbers, > since uint64_t is rather generic. Even if checkpatch whines about new > typedefs. I don't use checkpatch so I don't care about all the idiotic "crusade of the month" stuff it is always whining about. > I was kind of hoping that we'd be able to mark xfs_lsn_t and xfs_csn_t > with __bitwise and so static checkers could catch us if we accidentally > feed a CIL sequence number into a function that wants an LSN. Seems reasonable to hide that behind a typedef. No idea how to set it up and test it, though, and I don't really have time right now. I'll change it to a typedef to make this easier to do in future, though. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
