Re: [PATCH 2/2] xfs: remove dead stale buf unpin handling code

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 11, 2021 at 09:52:57AM -0400, Brian Foster wrote:
> This code goes back to a time when transaction commits wrote
> directly to iclogs. The associated log items were pinned, written to
> the log, and then "uncommitted" if some part of the log write had
> failed. This uncommit sequence called an ->iop_unpin_remove()
> handler that was eventually folded into ->iop_unpin() via the remove
> parameter. The log subsystem has since changed significantly in that
> transactions commit to the CIL instead of direct to iclogs, though
> log items must still be aborted in the event of an eventual log I/O
> error. However, the context for a log item abort is now asynchronous
> from transaction commit, which means the committing transaction has
> been freed by this point in time and the transaction uncommit
> sequence of events is no longer relevant.
> 
> Further, since stale buffers remain locked at transaction commit
> through unpin, we can be certain that the buffer is not associated
> with any transaction when the unpin callback executes. Remove this
> unused hunk of code and replace it with an assertion that the buffer
> is disassociated from transaction context.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

<nod> my brain kinda hurts now, but I have a vague recollection of
wondering how you could get a stale buffer that was also being
removed and not being able to figure out how one might stumble into this
chunk of code. :)

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf_item.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 7ff31788512b..634abf30b5bc 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -517,28 +517,10 @@ xfs_buf_item_unpin(
>  		ASSERT(xfs_buf_islocked(bp));
>  		ASSERT(bp->b_flags & XBF_STALE);
>  		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
> +		ASSERT(list_empty(&lip->li_trans) && !bp->b_transp);
>  
>  		trace_xfs_buf_item_unpin_stale(bip);
>  
> -		if (remove) {
> -			/*
> -			 * If we are in a transaction context, we have to
> -			 * remove the log item from the transaction as we are
> -			 * about to release our reference to the buffer.  If we
> -			 * don't, the unlock that occurs later in
> -			 * xfs_trans_uncommit() will try to reference the
> -			 * buffer which we no longer have a hold on.
> -			 */
> -			if (!list_empty(&lip->li_trans))
> -				xfs_trans_del_item(lip);
> -
> -			/*
> -			 * Since the transaction no longer refers to the buffer,
> -			 * the buffer should no longer refer to the transaction.
> -			 */
> -			bp->b_transp = NULL;
> -		}
> -
>  		/*
>  		 * If we get called here because of an IO error, we may or may
>  		 * not have the item on the AIL. xfs_trans_ail_delete() will
> -- 
> 2.26.3
>