Re: [PATCH v3 1/2] ext4: fix fast commit inode enqueueing during a full journal commit

["Theodore Ts'o" <tytso@mit.edu>]

On Wed, May 29, 2024 at 10:20:29AM +0100, Luis Henriques (SUSE) wrote:
> When a full journal commit is on-going, any fast commit has to be enqueued
> into a different queue: FC_Q_STAGING instead of FC_Q_MAIN.  This enqueueing
> is done only once, i.e. if an inode is already queued in a previous fast
> commit entry it won't be enqueued again.  However, if a full commit starts
> _after_ the inode is enqueued into FC_Q_MAIN, the next fast commit needs to
> be done into FC_Q_STAGING.  And this is not being done in function
> ext4_fc_track_template().
> 
> This patch fixes the issue by re-enqueuing an inode into the STAGING queue
> during the fast commit clean-up callback if it has a tid (i_sync_tid)
> greater than the one being handled.  The STAGING queue will then be spliced
> back into MAIN.
> 
> This bug was found using fstest generic/047.  This test creates several 32k
> bytes files, sync'ing each of them after it's creation, and then shutting
> down the filesystem.  Some data may be loss in this operation; for example a
> file may have it's size truncated to zero.
> 
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>

This patch is causing a regression for the test generic/472
generic/496 generic/643 if fast_commit is enabled.  So using the
ext4/adv or ext4/fast_commit configuration, e.g:

% kvm-xfstests  -c ext4/fast_commit  generic/472 generic/496 generic/643

For all of these test, the failures seem to involve the swapon command
erroring out:

    --- tests/generic/496.out   2024-06-13 18:57:39.000000000 -0400
    +++ /results/ext4/results-fast_commit/generic/496.out.bad   2024-07-08 23:46:39.720
    @@ -1,3 +1,4 @@
     QA output created by 496
     fallocate swap
     mixed swap
    +swapon: Invalid argument
    ...

but it's unclear why this patch would affect swapon.

I've never been able to see generic/047 failure in any of my ext4/dev
testing, nor in any of my daily fs-next CI testing.  So for that
reason, I'm going to drop this patch from my tree.

The second patch in this series appears to be independent at least
from a logical perspective --- although a minor change is needed to
resolve a merge conflict after dropping this change.

Luis, Harshad, could you look in this failure and then resubmit once
it's been fixed?  Thanks!!  Also, Luis, can you give more details
about the generic/047 failure that you had seen?  Is it one of those
flaky tests where you need to run the test dozens or hundreds of time
to see the failure?

Many thanks!!

					- Ted