On Mon, 2023-06-12 at 19:04 +0000, Trond Myklebust wrote:

On Mon, 2023-06-12 at 13:30 -0400, Jeff Layton wrote:

quoted

On Mon, 2023-06-12 at 11:58 -0400, Jeff Layton wrote:

quoted

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 [off-list ref]