On Wed, May 24, 2023 at 08:11:21PM +0800, Long Li wrote:
> The following error occurred when do IO fault injection test:
>
> XFS: Assertion failed: xlog_is_shutdown(lip->li_log), file: fs/xfs/xfs_inode_item.c, line: 748
This line of code does not match to any assert in the current
upstream code base.
I'm assuming that the assert is this one:
735 /*
736 * dgc: Not sure how this happens, but it happens very
737 * occassionaly via generic/388. xfs_iflush_abort() also
738 * silently handles this same "under writeback but not in AIL at
739 * shutdown" condition via xfs_trans_ail_delete().
740 */
741 if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
742 >>>>>>> ASSERT(xlog_is_shutdown(lip->li_log));
743 continue;
744 }
Is that correct? I am going to assume that it is from this point
onwards.
Also, please include the full stack trace with the assert - the assert by
itself does not give us any context about where the failure
occurred, and the above code path can be run from several different
contexts, especially when a shutdown is in progress.
> In commit 3c4cb76bce43 (xfs: xfs_trans_commit() path must check for log
> shutdown) fix a problem that dirty transaction was canceled before log
> shutdown, because of the log is still running, it result dirty and
> unlogged inode item that isn't in the AIL in memory that can be flushed
> to disk via writeback clustering.
Yes, but this was a race checking shutdown state before inserting the
dirty item into the CIL, moving it from filesystem level context
to log level context. i.e. it caused the object to be handled as
valid instead of being aborted.
> xfs_trans_cancel() has the same problem, if a shut down races with
> xfs_trans_cancel() and we have shut down the filesystem but not the log,
> we will still cancel the transaction before log shutdown. So
> xfs_trans_cancel() needs to check log state for shutdown, not mount.
Yet this is cancelling a transaction that has dirty objects that
will be aborted. It is not the same context as the race fixed in
3c4cb76bce43, especially as the log item is marked as aborted as
part of the transaction cancel. As this is operating at the
filesytsem level, checking for filesystem level shutdown before
issuing a filesystem level shutdown is appropriate - we're about to
release items, not hand them off to the log.
Transaction cancel this ends up in xfs_trans_free_items(abort =
true) which sets XFS_LI_ABORTED then releases the log items. This
then calls xfs_inode_item_release() for inodes, which does nothing
special with XFS_LI_ABORTED inode items, nor does it check for log
shutdown.
I may be missing something obvious that hasn't been explained, but
from the information provided I can't see how the above assert is
related to not doing a log shutdown check in the transaction cancel
path. Nothing in the transaction cancel path removes the inode item
from the AIL, so it can't be responsible for the ASSERT firing...
However, looking at this abort path does point out that the code in
xfs_inode_item_release() is probably wrong - it probably should check
for XFS_LI_ABORTED and log shutdown and mark the inode as stale if
it is dirty and then remove it from the AIL. In comparison, the
buffer log item release method does these checks and removes the
buffer log item from the AIL on abort/log shutdown, so I suspect
that inodes log items should be doing something similar.
I also supsect that the AIL push (metadata writeback) should skip
over aborted log items, too.
But none of these things explain to me how the change is in any way
related to the assert I have assumed has fired. More information,
please.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
