Re: fstests generic/441 -- occasional bcachefs failure

[Kent Overstreet <kent.overstreet@linux.dev>]

On Wed, Jan 25, 2023 at 10:45:24AM -0500, Brian Foster wrote:
> Hi Kent, list,
> 
> I've noticed occasional failures of generic/441 on bcachefs that look
> something like this:
> 
> generic/441 11s ... - output mismatch (see /root/xfstests-dev/results//generic/441.out.bad)
>     --- tests/generic/441.out   2022-10-11 14:20:36.986202162 -0400
>     +++ /root/xfstests-dev/results//generic/441.out.bad 2023-01-24 13:33:54.977729904 -0500
>     @@ -1,3 +1,3 @@
>      QA output created by 441
>      Format and mount
>     -Test passed!
>     +First fsync after reopen of fd[0] failed: Input/output error
>     ...
>     (Run 'diff -u /root/xfstests-dev/tests/generic/441.out /root/xfstests-dev/results//generic/441.out.bad'  to see the entire diff)
> 
> This test covers fsync error reporting across multiple fds. What it does
> in a nutshell is open a bunch of fds, write to them and fsync them, and
> then repeat that a couple times over after switching to an error table
> (i.e. dm-error) and back to a working table, verifying error reporting
> expectations at each step along the way.
> 
> The error above is associated with an fsync failure after switching from
> an error table back to a working table, and essentially occurs because
> the filesystem has shutdown and the bcachefs inode has a journal
> sequence number ahead of what the journal shows has been flushed to
> disk. That makes sense in general because even though the vfs error
> reporting might not expect an error, the filesystem has been shutdown
> and fsync has work to do. I've been digging into it a bit regardless to
> try and understand why the behavior seems transient..
> 
> What I'm seeing looks like a matter of timing at the point the test
> switches to an error table and issues the first fsync (where failure is
> expected). IIUC, what I think is happening here in the scenario where
> the test passes is something like this:

Hey Brian - my understanding of this bug was that it was even simpler.

As I understand it, the decreed semantics of fsync by the Powers That Be
are that it should return a given error _once_  - if a subsequent fsync
returns an error, then there must have been another write that errored.

So we just need to plumb our errors from the journal through the
errseq_t mechanism. The tricky bit is that bch2_fsync() doesn't record
or check if it's actually doing a journal flush or if the journal flush
was already done.

So I think we'd need to add another sequence number to
bch_inode_info to track the sequence that was flushed, and then only
call bch2_journal_flush_seq() if flushed_seq !=
journal_seq_of_last_update, and then to make this work correctly when an
inode has been evicted it might need to go in struct bch_inode, stored
in the btree, as well.