On Tue, Aug 21, 2018 at 11:01:00PM -0700, Christoph Hellwig wrote:
> > + int freed;
>
> I think this should be a bool..
>
Yep.
> > 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.
What you have here looks logically equivalent and exactly like the 1-2
liner I originally introduced to test the fix. I refactored the function
because I found it duplicated some logic, was unnecessarily long and
some of the comments actually look stale/out of place.
The logic fix here is indeed very small, as you and Dave have both
pointed out. I think what Dave proposed is roughly equivalent to what
you suggest and more closely resembles the original flow, so I'll take a
closer look at that and post a v2.
Brian
