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

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

On Fri, Feb 04, 2022 at 03:36:18PM -0600, Eric Sandeen wrote:
> 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?

Nope, I was merely porting it to userspace and decided the source was
different enough not to bother trying to libxfs-apply it.

--D

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