Re: [PATCH 29/30] xfs: factor xfs_iflush_done

[Brian Foster <bfoster@redhat.com>]

On Wed, Jun 10, 2020 at 08:14:31AM +1000, Dave Chinner wrote:
> On Tue, Jun 09, 2020 at 09:12:49AM -0400, Brian Foster wrote:
> > On Thu, Jun 04, 2020 at 05:46:05PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > xfs_iflush_done() does 3 distinct operations to the inodes attached
> > > to the buffer. Separate these operations out into functions so that
> > > it is easier to modify these operations independently in future.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/xfs_inode_item.c | 154 +++++++++++++++++++++-------------------
> > >  1 file changed, 81 insertions(+), 73 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > > index dee7385466f83..3894d190ea5b9 100644
> > > --- a/fs/xfs/xfs_inode_item.c
> > > +++ b/fs/xfs/xfs_inode_item.c
> > > @@ -640,101 +640,64 @@ xfs_inode_item_destroy(
> > >  
> > >  
> > >  /*
> > > - * This is the inode flushing I/O completion routine.  It is called
> > > - * from interrupt level when the buffer containing the inode is
> > > - * flushed to disk.  It is responsible for removing the inode item
> > > - * from the AIL if it has not been re-logged, and unlocking the inode's
> > > - * flush lock.
> > > - *
> > > - * To reduce AIL lock traffic as much as possible, we scan the buffer log item
> > > - * list for other inodes that will run this function. We remove them from the
> > > - * buffer list so we can process all the inode IO completions in one AIL lock
> > > - * traversal.
> > > - *
> > > - * Note: Now that we attach the log item to the buffer when we first log the
> > > - * inode in memory, we can have unflushed inodes on the buffer list here. These
> > > - * inodes will have a zero ili_last_fields, so skip over them here.
> > > + * We only want to pull the item from the AIL if it is actually there
> > > + * and its location in the log has not changed since we started the
> > > + * flush.  Thus, we only bother if the inode's lsn has not changed.
> > >   */
> > >  void
> > > -xfs_iflush_done(
> > > -	struct xfs_buf		*bp)
> > > +xfs_iflush_ail_updates(
> > > +	struct xfs_ail		*ailp,
> > > +	struct list_head	*list)
> > >  {
> > > -	struct xfs_inode_log_item *iip;
> > > -	struct xfs_log_item	*lip, *n;
> > > -	struct xfs_ail		*ailp = bp->b_mount->m_ail;
> > > -	int			need_ail = 0;
> > > -	LIST_HEAD(tmp);
> > > +	struct xfs_log_item	*lip;
> > > +	xfs_lsn_t		tail_lsn = 0;
> > >  
> > > -	/*
> > > -	 * Pull the attached inodes from the buffer one at a time and take the
> > > -	 * appropriate action on them.
> > > -	 */
> > > -	list_for_each_entry_safe(lip, n, &bp->b_li_list, li_bio_list) {
> > > -		iip = INODE_ITEM(lip);
> > > +	/* this is an opencoded batch version of xfs_trans_ail_delete */
> > > +	spin_lock(&ailp->ail_lock);
> > > +	list_for_each_entry(lip, list, li_bio_list) {
> > > +		xfs_lsn_t	lsn;
> > >  
> > > -		if (xfs_iflags_test(iip->ili_inode, XFS_ISTALE)) {
> > > -			xfs_iflush_abort(iip->ili_inode);
> > > +		if (INODE_ITEM(lip)->ili_flush_lsn != lip->li_lsn) {
> > > +			clear_bit(XFS_LI_FAILED, &lip->li_flags);
> > >  			continue;
> > >  		}
> > 
> > That seems like strange logic. Shouldn't we clear LI_FAILED regardless?
> 
> It's the same logic as before this patch series:
> 
>                        if (INODE_ITEM(blip)->ili_logged &&
>                             blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) {
>                                 /*
>                                  * xfs_ail_update_finish() only cares about the
>                                  * lsn of the first tail item removed, any
>                                  * others will be at the same or higher lsn so
>                                  * we just ignore them.
>                                  */
>                                 xfs_lsn_t lsn = xfs_ail_delete_one(ailp, blip);
>                                 if (!tail_lsn && lsn)
>                                         tail_lsn = lsn;
>                         } else {
>                                 xfs_clear_li_failed(blip);
>                         }
> 
> I've just re-ordered it to check for relogged inodes first instead
> of handling that in the else branch.
> 

Hmm.. I guess I'm confused why the logic seems to be swizzled around. An
earlier patch lifted the bit clear outside of this check, then we seem
to put it back in place in a different order for some reason..?

> i.e. we do clear XFS_LI_FAILED always: xfs_ail_delete_one() does it
> for the log items that are being removed from the AIL....
> 
> > > +/*
> > > + * Inode buffer IO completion routine.  It is responsible for removing inodes
> > > + * attached to the buffer from the AIL if they have not been re-logged, as well
> > > + * as completing the flush and unlocking the inode.
> > > + */
> > > +void
> > > +xfs_iflush_done(
> > > +	struct xfs_buf		*bp)
> > > +{
> > > +	struct xfs_log_item	*lip, *n;
> > > +	LIST_HEAD(flushed_inodes);
> > > +	LIST_HEAD(ail_updates);
> > > +
> > > +	/*
> > > +	 * Pull the attached inodes from the buffer one at a time and take the
> > > +	 * appropriate action on them.
> > > +	 */
> > > +	list_for_each_entry_safe(lip, n, &bp->b_li_list, li_bio_list) {
> > > +		struct xfs_inode_log_item *iip = INODE_ITEM(lip);
> > > +
> > > +		if (xfs_iflags_test(iip->ili_inode, XFS_ISTALE)) {
> > > +			xfs_iflush_abort(iip->ili_inode);
> > > +			continue;
> > > +		}
> > > +		if (!iip->ili_last_fields)
> > > +			continue;
> > > +
> > > +		/* Do an unlocked check for needing the AIL lock. */
> > > +		if (iip->ili_flush_lsn == lip->li_lsn ||
> > > +		    test_bit(XFS_LI_FAILED, &lip->li_flags))
> > > +			list_move_tail(&lip->li_bio_list, &ail_updates);
> > > +		else
> > > +			list_move_tail(&lip->li_bio_list, &flushed_inodes);
> > 
> > Not sure I see the point of having two lists here, particularly since
> > this is all based on lockless logic.
> 
> It's not lockless - it's all done under the buffer lock which
> protects the buffer log item list...
> 
> > At the very least it's a subtle
> > change in AIL processing logic and I don't think that should be buried
> > in a refactoring patch.
> 
> I don't think it changes logic at all - what am I missing?
> 

I'm referring to the fact that we no longer check the lsn of each
(flushed) log item attached to the buffer under the ail lock. Note that
I am not saying it's necessarily wrong, but rather that IMO it's too
subtle a change to silently squash into a refactoring patch.

> FWIW, I untangled the function this way because the "track dirty
> inodes by ordered buffers" patchset completely removes the AIL stuff
> - the ail_updates list and the xfs_iflush_ail_updates() function go
> away completely and the rest of the refactoring remains unchanged.
> i.e.  as the commit messages says, this change makes follow-on
> patches much easier to understand...
> 

The general function breakdown seems fine to me. I find the multiple
list processing to be a bit overdone, particularly if it doesn't serve a
current functional purpose. If the purpose is to support a future patch
series, I'd suggest to continue using the existing logic of moving all
flushed inodes to a single list and leave the separate list bits to the
start of the series where it's useful so it's possible to review with
the associated context (or alternatively just defer the entire patch).

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>