Re: fstests generic/441 -- occasional bcachefs failure

[Brian Foster <bfoster@redhat.com>]

On Mon, Feb 06, 2023 at 05:18:35PM -0500, Kent Overstreet wrote:
> 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.
> 

Yeah.. I hadn't confirmed, but that's what I figured.

> 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 :)
> 

Indeed. While the proposed logic change seems reasonable to me (for
reasons already explained)...

> 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...
> 

... I also think it's reasonable to sit on it, take stock of the scope,
and gain more input wrt to errseq and whatnot, should any of that lead
to potentially better solutions.

> 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.
> 

Ok. FWIW, I was just trying to pick off something that looked
approachable because that helps get more comfortable with the code,
etc., even if it doesn't result in an immediate fix. I.e., this
investigation/root-cause was helpful to grok journaling a bit more,
uncover (to me, at least) a use case for freeze, etc.

If you have any other open bugs that you think might be useful to look
into, feel free to send them along..

Brian