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@xxxxxxxxxx>
<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@xxxxxxxxxx>
--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
>
