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
