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
