Re: fstests generic/441 -- occasional bcachefs failure

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

On Sat, Feb 04, 2023 at 04:33:41PM -0500, Brian Foster wrote:
> On Thu, Feb 02, 2023 at 05:56:32PM -0500, Kent Overstreet wrote:
> > On Tue, Jan 31, 2023 at 11:04:11AM -0500, Brian Foster wrote:
> > > On Mon, Jan 30, 2023 at 12:06:34PM -0500, Kent Overstreet wrote:
> > > > On Fri, Jan 27, 2023 at 09:50:05AM -0500, Brian Foster wrote:
> > > > > Something else that occurred to me while looking further at this is we
> > > > > can also avoid the error in this case fairly easily by bailing out of
> > > > > bch2_fsync() if page writeback fails, as opposed to the unconditional
> > > > > flush -> sync meta -> flush log sequence that returns the first error
> > > > > anyways. That would prevent marking the inode with a new sequence number
> > > > > when I/Os are obviously failing. The caveat is that the test still
> > > > > fails, now with a "Read-only file system" error instead of EIO, because
> > > > > the filesystem is shutdown by the time the vfs write inode path actually
> > > > > runs.
> > > > 
> > > > If some pages did write successfully we don't want to skip the rest of
> > > > the fsync, though.
> > > > 
> > > 
> > > What does it matter if the fsync() has already failed? ISTM this is
> > > pretty standard error handling behavior across major fs', but it's not
> > > clear to me if there's some bcachefs specific quirk that warrants
> > > different handling..
> > > 
> > > FWIW, I think I've been able to fix this test with a couple small
> > > tweaks:
> > > 
> > > 1. Change bch2_fsync() to return on first error.
> > 
> > I suppose it doesn't make sense to flush the journal if
> > file_write_and_wait_range() didn't successfully do anything - but I
> > would prefer to keep the behaviour where we do flush the journal on
> > partial writeback.
> > 
> 
> That seems reasonable to me in principle...
> 
> > What if we plumbed a did_work parameter through?
> > 
> 
> ... but I'm not sure how that is supposed to work..? Tracking submits
> from the current flush wouldn't be hard, but wouldn't tell us about
> writeback that might have occurred in the background before fsync was
> called. It also doesn't give any information about what I/Os succeeded
> or failed.

Good point.

Now that I think about it, inode->bi_journal_seq won't be getting
updated unless a write actually did successfully complete - meaning
bch2_flush_inode() won't do anything unless there was a successful
transaction commit; either data was written or the inode was updated for
some other reason.

Maybe it's the sync_inode_metadata() call that's causing the journal
flush to happen?