On Mon, 2023-06-12 at 19:04 +0000, Trond Myklebust wrote: > On Mon, 2023-06-12 at 13:30 -0400, Jeff Layton wrote: > > On Mon, 2023-06-12 at 11:58 -0400, Jeff Layton wrote: > > > > > > > > Got it: I think I see what's happening. filemap_sample_wb_err just > > > calls > > > errseq_sample, which does this: > > > > > > errseq_t errseq_sample(errseq_t > > > *eseq) > > > { > > > > > > errseq_t old = > > > READ_ONCE(*eseq); > > > > > > > > > > > > /* If nobody has seen this error yet, then we can be the > > > first. */ > > > if (!(old & > > > ERRSEQ_SEEN)) > > > > > > old = > > > 0; > > > > > > return > > > old; > > > > > > } > > > > > > Because no one has seen that error yet (ERRSEQ_SEEN is clear), the > > > write > > > ends up being the first to see it and it gets back a 0, even though > > > the > > > error happened before the sample. > > > > > > The above behavior is what we want for the sample that we do at > > > open() > > > time, but not what's needed for this use-case. We need a new helper > > > that > > > samples the value regardless of whether it has already been seen: > > > > > > errseq_t errseq_peek(errseq_t *eseq) > > > { > > > return READ_ONCE(*eseq); > > > } > > > > > > ...but we'll also need to fix up errseq_check to handle differences > > > between the SEEN bit. > > > > > > I'll see if I can spin up a patch for that. Stay tuned. > > > > This may not be fixable with the way that NFS is trying to use > > errseq_t. > > > > The fundamental problem is that we need to mark the errseq_t in the > > mapping as SEEN when we sample it, to ensure that a later error is > > recorded and not ignored. > > > > But...if the error hasn't been reported yet and we mark it SEEN here, > > and then a later error doesn't occur, then a later open won't have > > its > > errseq_t set to 0, and that unseen error could be lost. > > > > It's a bit of a pity: as originally envisioned, the errseq_t > > mechanism > > would provide for this sort of use case, but we added this patch not > > long after the original code went in, and it changed those semantics: > > > > b4678df184b3 errseq: Always report a writeback error once > > > > I don't see a good way to do this using the current errseq_t > > mechanism, > > given these competing needs. I'll keep thinking about it though. > > Maybe > > we could add some sort of store and forward mechanism for fsync on > > NFS? > > That could get rather complex though. > > > > Cheers, > > Does RHEL-8 have commit 6c984083ec24, 064109db53ec, d95b26650e86, > e6005436f6cc, 9641d9bc9b75, and cea9ba7239dc applied? > Ben is working on backporting those as we speak. Hopefully we can get RHEL8's state closer to where upstream is. I'm also working on a patch for upstream that should give Chris the expected behavior in this test. -- Jeff Layton <jlayton@xxxxxxxxxx>