Re: fstests generic/441 -- occasional bcachefs failure

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

On Mon, Feb 06, 2023 at 10:33:14AM -0500, Brian Foster wrote:
> So yes, it seems the sync_inode_metadata() path is what bumps the
> journal seq and the explicit journal flush is what shuts down the fs,
> but I assume sync_inode_metadata() attempts the inode flush simply
> because the buffered write that precedes it works perfectly fine. This
> is what had me thinking that bch2_fsync() is being more aggressive than
> it really needs to be here. With the proposed logic tweak, the second
> issue is resolved because bcachefs no longer attempts the inode flush
> when page writeback has failed. So what happens in the test is that it
> switches back over to the functional dm table, the vfs inode flush
> occurs then and succeeds, and so the test sees no (unexpected) errors
> and the fs has survived without shutting down.

So, sync_inode_metadata() is just writing out timestamp updates.

But, there's no reason we couldn't be doing that as part of the write
path, if it was plumbed through - the write path has to do an inode
update anyways. Although that would involve an unpack/repack of the
inode, so maybe we don't want to do that - we'd want to change the inode
format to not encode timestamps as varints, and I'm not sure that's
worth the hassle and the increased inode size.

I was eyeing changing the way timestamp updates work to make them more
direct, it would get rid of the need to call sync_inode_metadata() and
regularize timestamp updates with the way inode updates work for
everything else - ISTR that's how it works in XFS as well. But it'd make
atime updates more expensive, and that's not something I've looked into
yet.

> All in all I think this test basically builds on some minor assumptions
> about how fsync error behavior is implemented in Linux, and bcachefs
> happens to slightly diverge in a way that leads to this shutdown. IOW,
> I'd expect the same problematic behavior out of XFS if it implemented
> this sort of fsync logic, but afaict no other fs does that, so the test
> is functional in practice. I admit that's not the best pure engineering
> justification for the change in bcachefs (so I understand any
> hesitation), but IMO it's reasonable in practice and worthwhile enough
> to improve test coverage. I haven't audited fstests for this or
> anything, but it wouldn't surprise me much if there are other tests that
> rely on this sort of "assumed behavior" for testing I/O failures.
> Thoughts?

Yeah, maybe we should hold off on making any decisions until we see
where else this comes up, I hoped this test was going to be easier to
fix but it doesn't seem like there's any clean obvious solutions. Ah
well :)

LSF is coming up and as I recall that's where the errseq stuff was
originally discussed - I'd like to hear from the people behind those
changes if they thing leaving our journal flush out of the errseq path
is reasonable (it seems so to me, but it's worth bringing up). I can
pencil it in to the bcachefs talk... need to start working on that...

We can just leave it failing in the CI for now - maybe make some notes
as to what our options are when we decide to come back to it, there's
definitely more straightforward and more impactful bugs to work on in
the meantime.