On Thu, Nov 30, 2023 at 12:06:17AM -0800, Christoph Hellwig wrote:
> > +static inline void
> > +xfs_defer_recover_work_item(
> > + struct xfs_defer_pending *dfp,
> > + struct list_head *work)
> > +{
> > + list_add_tail(work, &dfp->dfp_work);
> > + dfp->dfp_count++;
> > +}
>
> The same (arguably trivial) logic is also duplicated in xfs_defer_add.
> Maybe give the helper a more generic name and use it there as well?
Done.
> > + INIT_LIST_HEAD(&attr->xattri_list);
>
> No need to initialize this (and all the other list_heads in the items)
> as we're only adding them to the work list, but never actualy using them
> as the head of a list or do list_empty checks on them.
Ah, right, list_add_tail doesn't care about the current contents of
new->{next,prev}. Fixed.
> > + xfs_defer_recover_work_item(dfp, &bi->bi_list);
>
> Does this commit on it's own actually work? We add the items to
> the work list, but I don't think we ever take it off without the next
> patch?
Oops, I think "xfs: use xfs_defer_pending objects to recover intent
items" leaks the dfp in the success case. After the loop in
xlog_recover_process_intents sets dfp->dfp_intent = NULL, it ought to be
calling xfs_defer_cancel_recovery to free the dfp_work items and the dfp
itself.
> > - struct xfs_bmap_intent fake = { };
> > struct xfs_trans_res resv;
> > struct xfs_log_item *lip = dfp->dfp_intent;
> > struct xfs_bui_log_item *buip = BUI_ITEM(lip);
> > @@ -498,6 +520,7 @@ xfs_bui_item_recover(
> > struct xfs_mount *mp = lip->li_log->l_mp;
> > struct xfs_map_extent *map;
> > struct xfs_bud_log_item *budp;
> > + struct xfs_bmap_intent *fake;
>
> So this patch moves the intent structures off the stack, and the next
> one then renames it to work. Maybe do the renames here so that we don't
> have to touch every single use of them twice?
The next patch removes those variables altogether, so I'll remove the
fake -> work rename entirely.
> Otherwise this looks nice, and I really like the better structure of
> the recovery code that this is leading towards.
Oh good!
--D
