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

[Brian Foster <bfoster@redhat.com>]

On Fri, Aug 31, 2018 at 12:24:37AM -0700, Christoph Hellwig wrote:
> > -	}
> > -
> >  	trace_xfs_buf_item_unlock(bip);
> 
> Can we keep an empty line between the variable declarations and the
> trace call?
> 

Sure.

> > +	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.
> 

It was intended to be defensive. I actually considered 'else if (freed
&& stale)' so the code was more clear, but settled on this (which is
eventually replaced).

> > +		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:
> 

Just optimizing out the freed variable doesn't eliminate the need for
the assert. freed is defined because I wanted an assert to check the
expected state where we consider an unlock. I wanted the assert because
the situation is non-trivial with regard to handling unlocks of stale
buffers in this path. To simplify this is partly the purpose of the
following patches, which also tweak this assert and replace the rest of
the code you've changed.

Brian

> 	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;
> 	}