Re: [PATCH 02/17] libxfs: shut down filesystem if we xfs_trans_cancel with deferred work items

[Eric Sandeen <sandeen@sandeen.net>]

On 1/19/22 6:21 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While debugging some very strange rmap corruption reports in connection
> with the online directory repair code.  I root-caused the error to the
> following incorrect sequence:
> 
> <start repair transaction>
> <expand directory, causing a deferred rmap to be queued>
> <roll transaction>
> <cancel transaction>
> 
> Obviously, we should have committed the transaction instead of
> cancelling it.  Thinking more broadly, however, xfs_trans_cancel should
> have warned us that we were throwing away work item that we already
> committed to performing.  This is not correct, and we need to shut down
> the filesystem.
> 
> Change xfs_trans_cancel to complain in the loudest manner if we're
> cancelling any transaction with deferred work items attached.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

So this is basically:

Source kernel commit: 47a6df7cd3174b91c6c862eae0b8d4e13591df52

plus the actual shutting down / aborting part 

Seems ok; did you run into this in practice, in userspace?

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  libxfs/trans.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index fd2e6f9d..8c16cb8d 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -318,13 +318,30 @@ void
>  libxfs_trans_cancel(
>  	struct xfs_trans	*tp)
>  {
> +	bool			dirty;
> +
>  	trace_xfs_trans_cancel(tp, _RET_IP_);
>  
>  	if (tp == NULL)
>  		return;
> +	dirty = (tp->t_flags & XFS_TRANS_DIRTY);
>  
> -	if (tp->t_flags & XFS_TRANS_PERM_LOG_RES)
> +	/*
> +	 * It's never valid to cancel a transaction with deferred ops attached,
> +	 * because the transaction is effectively dirty.  Complain about this
> +	 * loudly before freeing the in-memory defer items.
> +	 */
> +	if (!list_empty(&tp->t_dfops)) {
> +		ASSERT(list_empty(&tp->t_dfops));
> +		ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> +		dirty = true;
>  		xfs_defer_cancel(tp);
> +	}
> +
> +	if (dirty) {
> +		fprintf(stderr, _("Cancelling dirty transaction!\n"));
> +		abort();
> +	}
>  
>  	xfs_trans_free_items(tp);
>  	xfs_trans_free(tp);
>