Re: [PATCH 5/7] xfs: recreate work items when recovering intent items

["Darrick J. Wong" <djwong@kernel.org>]

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