Re: [PATCH 2/4] xfs: proper replay of deferred ops queued during log recovery

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Sep 28, 2020 at 08:37:17AM +0200, Christoph Hellwig wrote:
> On Mon, Sep 28, 2020 at 03:26:18PM +1000, Dave Chinner wrote:
> > > +	struct xfs_mount		*mp = tp->t_mountp;
> > > +	struct xfs_defer_capture	*dfc = xfs_defer_capture(tp);
> > > +	int				error;
> > > +
> > > +	/* If we don't capture anything, commit tp and exit. */
> > > +	if (!dfc)
> > > +		return xfs_trans_commit(tp);
> > > +
> > > +	/*
> > > +	 * Commit the transaction.  If that fails, clean up the defer ops and
> > > +	 * the dfc that we just created.  Otherwise, add the dfc to the list.
> > > +	 */
> > > +	error = xfs_trans_commit(tp);
> > > +	if (error) {
> > > +		xfs_defer_capture_free(mp, dfc);
> > > +		return error;
> > > +	}
> > > +
> > > +	list_add_tail(&dfc->dfc_list, capture_list);
> > > +	return 0;
> > > +}
> > 
> > And, really, this is more than a "transaction commit" operation; it
> > doesn't have anything recovery specific to it, so if the
> > xfs_defer_capture() API is "generic xfs_defer" functionality, why
> > isn't this placed next to it and nameed
> > xfs_defer_capture_and_commit()?
> 
> Agreed.  I find the xlog_recover_trans_commit naming pretty weird.

<nod>

The final list of functions are:

xfs_defer_ops_capture_and_commit: capture a transaction's dfops, commit
	the transaction, and add the capture structure to the list, just like
	xlog_recover_trans_commit did in this patch.

xfs_defer_ops_continue: restore the captured dfops and transaction state
	to a fresh transaction, and free the capture structure.

xfs_defer_ops_release: free all captured dfops and the structure, in case
	recovery failed somewhere and we have to bail out.

> > > @@ -2533,28 +2577,28 @@ xlog_recover_process_intents(
> > >  		 */
> > >  		ASSERT(XFS_LSN_CMP(last_lsn, lip->li_lsn) >= 0);
> > >  
> > > +		if (test_and_set_bit(XFS_LI_RECOVERED, &lip->li_flags))
> > > +			continue;
> > > +
> > 
> > Why do we still need XFS_LI_RECOVERED here? This log item is going to get
> > removed from the AIL by the committing of the first transaction
> > in the ->iop_recover() sequence we are running, so we'll never find
> > it again in the AIL. Nothing else checks for XFS_LI_RECOVERED
> > anymore, so this seems unnecessary now...
> 
> We also never restart the list walk as far as I can tell.  So yes,
> XFS_LI_RECOVERED seems entirely superflous and should probably be
> removed in a prep patch.

Ok.

> > > -out:
> > > +
> > >  	xfs_trans_ail_cursor_done(&cur);
> > >  	spin_unlock(&ailp->ail_lock);
> > >  	if (!error)
> > > -		error = xlog_finish_defer_ops(parent_tp);
> > > -	xfs_trans_cancel(parent_tp);
> > > +		error = xlog_finish_defer_ops(log->l_mp, &capture_list);
> > >  
> > > +	xlog_cancel_defer_ops(log->l_mp, &capture_list);
> > >  	return error;
> > >  }
> > 
> > Again, why are we cancelling the capture list if we just
> > successfully processed the defer ops on the capture list?
> 
> Yes, we'll probably just want to assert it is non-empty at the end of
> xlog_finish_defer_ops.

Done.

--D