On Wed, Nov 29, 2023 at 11:53:31PM -0800, Christoph Hellwig wrote:
> > +/* Create a pending deferred work item for use with log recovery. */
> > +struct xfs_defer_pending *
> > +xfs_defer_start_recovery(
> > + struct xfs_log_item *lip,
> > + enum xfs_defer_ops_type dfp_type)
> > +{
> > + struct xfs_defer_pending *dfp;
> > +
> > + dfp = kmem_cache_zalloc(xfs_defer_pending_cache,
> > + GFP_NOFS | __GFP_NOFAIL);
> > + dfp->dfp_type = dfp_type;
> > + dfp->dfp_intent = lip;
> > + INIT_LIST_HEAD(&dfp->dfp_work);
> > + INIT_LIST_HEAD(&dfp->dfp_list);
> > + return dfp;
> > +}
>
> Initializing dfp_list here is a bit pointless as the caller instantly
> adds it to log->l_dfops.
I'd rather pass the list_head into xfs_defer_start_recovery so that it
can create a fully initialized dfp and add it to the tracker.
> > -#if defined(DEBUG) || defined(XFS_WARN)
> > + spin_lock(&log->l_ailp->ail_lock);
> > last_lsn = xlog_assign_lsn(log->l_curr_cycle, log->l_curr_block);
> > + spin_unlock(&log->l_ailp->ail_lock);
>
> ail_lock shouldn't be needed here.
Oh, right, the AIL lock was to protect the cursor, not the debug
statements.
> Otherwise this looks reasonable to me.
<nod>
--D
