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.
> > @@ -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.
> > -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.
