Re: [PATCH 13/45] xfs: xfs_log_force_lsn isn't passed a LSN

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 08, 2021 at 02:53:23PM -0800, Darrick J. Wong wrote:
> On Fri, Mar 05, 2021 at 04:11:11PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_types.h |  1 +
> >  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 ++--
> >  13 files changed, 54 insertions(+), 59 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> > index 064bd6e8c922..0870ef6f933d 100644
> > --- a/fs/xfs/libxfs/xfs_types.h
> > +++ b/fs/xfs/libxfs/xfs_types.h
> > @@ -21,6 +21,7 @@ typedef int32_t		xfs_suminfo_t;	/* type of bitmap summary info */
> >  typedef uint32_t	xfs_rtword_t;	/* word type for bitmap manipulations */
> >  
> >  typedef int64_t		xfs_lsn_t;	/* log sequence number */
> > +typedef int64_t		xfs_csn_t;	/* CIL sequence number */
> 
> I'm unfamiliar with the internal format of CIL sequence numbers.  Do
> they have the same cycle:offset segmented structure as LSNs do?  Or are
> they a simple linear integer that increases as we checkpoint committed
> items?

Monotonic increasing integer, only ever used in memory and never
written to disk.

> 
> Looking through the current code, I see a couple of places where we
> initialize them to 1, and I also see that when we create a new cil
> context we set its sequence to one more than the context that it will
> replace.
> 
> I also see a bunch of comparisons of cil context sequence numbers that
> use standard integer operators, but then I also see one instance of:
> 
> 	if (XFS_LSN_CMP(lip->li_seq, ctx->sequence) != 0)
> 		return false;
> 	return true
> 
> in xfs_log_item_in_current_chkpt.  AFAICT this could be replaced with a
> simple:
> 
> 	return lip->li_seq == ctx->sequence;

Yup, missed that, will fix.

-Dave.
-- 
Dave Chinner
david@fromorbit.com