Re: [PATCH v3 1/3] xfs: don't unlock invalidated buf on aborted tx commit

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

> -	}
> -
>  	trace_xfs_buf_item_unlock(bip);

Can we keep an empty line between the variable declarations and the
trace call?

> +	freed = atomic_dec_and_test(&bip->bli_refcount);
> +	if (freed) {
> +		ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
> +		/*
> +		 * An aborted item may be in the AIL regardless of dirty state.
> +		 * For example, consider an aborted transaction that invalidated
> +		 * a dirty bli and cleared the dirty state.
> +		 */
> +		if (aborted)
>  			xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> +		if (aborted || !dirty)
>  			xfs_buf_item_relse(bp);
> +	} else if (stale) {
> +		/*
> +		 * Stale buffers remain locked until final unpin unless the bli
> +		 * was freed in the branch above. A freed stale bli implies an
> +		 * abort because buffer invalidation dirties the bli and
> +		 * transaction.
> +		 */
> +		ASSERT(!freed);

This assert doesn't make sense as we're already in the else statement
of the 'if (freed) check.

> +		return;
>  	}
> +	ASSERT(!stale || (aborted && freed));

This assert still look a little odd to me because it duplicates so
much of the above logic.  Why not:

	if (atomic_dec_and_test(&bip->bli_refcount)) {
		/*
		 * An aborted item may be in the AIL regardless of dirty state.
		 * For example, consider an aborted transaction that invalidated
		 * a dirty bli and cleared the dirty state.
		 */
		if (aborted) {
			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
  			xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
 			xfs_buf_item_relse(bp);
		} else {
			ASSERT(!stale);
			if (!dirty)
 				xfs_buf_item_relse(bp);
		}
	} else if (stale) {
		/*
		 * Stale buffers remain locked until final unpin unless the bli
		 * was freed in the branch above. A freed stale bli implies an
		 * abort because buffer invalidation dirties the bli and
		 * transaction.
		 */
		return;
	}