Re: [PATCH 15/19] xfs: refactor releasing finished intents during log recovery

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, Apr 25, 2020 at 11:34:10AM -0700, Christoph Hellwig wrote:
> > +STATIC bool
> > +xlog_release_bui(
> > +	struct xlog		*log,
> > +	struct xfs_log_item	*lip,
> > +	uint64_t		intent_id)
> > +{
> > +	struct xfs_bui_log_item	*buip = BUI_ITEM(lip);
> > +	struct xfs_ail		*ailp = log->l_ailp;
> > +
> > +	if (buip->bui_format.bui_id == intent_id) {
> > +		/*
> > +		 * Drop the BUD reference to the BUI. This
> > +		 * removes the BUI from the AIL and frees it.
> > +		 */
> > +		spin_unlock(&ailp->ail_lock);
> > +		xfs_bui_release(buip);
> > +		spin_lock(&ailp->ail_lock);
> > +		return true;
> > +	}
> > +
> > +	return false;
> 
> Early returns for the no match case would seem a little more clear for
> all the callbacks.

Done.

> Also the boilerplate comments in all the callback
> seem rather pointless.  If you think they have enough value I'd
> consolidate that information once in the xlog_recover_release_intent
> description.

Yeah, I'll move it.

> 
> > +	spin_lock(&ailp->ail_lock);
> > +	lip = xfs_trans_ail_cursor_first(ailp, &cur, 0);
> > +	while (lip != NULL) {
> > +		if (lip->li_type == intent_type && fn(log, lip, intent_id))
> > +			break;
> > +		lip = xfs_trans_ail_cursor_next(ailp, &cur);
> > +	}
> 
> What about:
> 
> 	for (lip = xfs_trans_ail_cursor_first(ailp, &cur, 0); lip;
> 	     lip = xfs_trans_ail_cursor_next(ailp, &cur) {
> 		if (lip->li_type != intent_type)
> 			continue;
> 		if (fn(log, lip, intent_id))
> 			break;
> 	}

Done.

--D