> + int freed;
I think this should be a bool..
> bool aborted;
> bool hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> bool dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> + bool stale = !!(bip->bli_flags & XFS_BLI_STALE);
> #if defined(DEBUG) || defined(XFS_WARN)
> bool ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> #endif
> + /*
> + * The bli dirty state should match whether the blf has logged segments
> + * except for ordered buffers, where only the bli should be dirty.
> + */
> + ASSERT((!ordered && dirty == xfs_buf_item_dirty_format(bip)) ||
> + (ordered && dirty && !xfs_buf_item_dirty_format(bip)));
> + ASSERT(!stale || (bip->__bli_format.blf_flags & XFS_BLF_CANCEL));
>
> + trace_xfs_buf_item_unlock(bip);
>
> + aborted = test_bit(XFS_LI_ABORTED, &lip->li_flags);
>
> /*
> + * Clear the buffer's association with this transaction and
> + * per-transaction state from the bli, which has been copied above.
> */
> + bp->b_transp = NULL;
> bip->bli_flags &= ~(XFS_BLI_LOGGED | XFS_BLI_HOLD | XFS_BLI_ORDERED);
>
> /*
> + * Drop the transaction bli reference and free the item if clean or
> + * aborted and we had the last reference. In either case this is the
> + * last opportunity to free the item since it won't be written back.
> + * Otherwise, the bli is still referenced or dirty and will be freed on
> + * final unpin of the buffer (if stale) or writeback completion.
> */
> + freed = atomic_dec_and_test(&bip->bli_refcount);
> + if (freed && (aborted || !dirty)) {
> + ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
> + ASSERT(!stale || aborted);
> + /* an aborted item may be in the AIL, remove it first */
> + if (aborted)
> + xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> + xfs_buf_item_relse(bp);
> }
>
> /*
> + * If the buffer was invalidated, leave it locked on transaction commit
> + * unless we just dropped the final reference. The latter case should
> + * only ever happen on abort because invalidation dirties the
> + * transaction and the log would have grabbed another bli reference when
> + * the buffer was pinned. In the non-abort case, the buffer is unlocked
> + * on final unpin and the bli freed since stale buffers are not written
> + * back.
> */
> + if (stale && !freed)
> + return;
> + ASSERT(!stale || (aborted && freed));
>
> if (!hold)
> xfs_buf_relse(bp);
I find the logic much more convoluted than what was there before.
I seems like we could apply your stale fix without reshuffling the
code like this:
if (atomic_dec_and_test(&bip->bli_refcount)) {
if (aborted) {
ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
xfs_buf_item_relse(bp);
} else if (!dirty) {
ASSERT(!stale);
xfs_buf_item_relse(bp);
}
} else {
if (stale)
return;
}
if (!hold)
xfs_buf_relse(bp);
which at least to me is a lot easier to read.
