Re: [PATCH v3] errseq: split the ERRSEQ_SEEN flag into two new flags

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2020-12-19 at 06:13 +0000, Matthew Wilcox wrote:
> On Thu, Dec 17, 2020 at 10:00:37AM -0500, Jeff Layton wrote:
> > Overlayfs's volatile mounts want to be able to sample an error for their
> > own purposes, without preventing a later opener from potentially seeing
> > the error.
> 
> umm ... can't they just copy the errseq_t they're interested in, followed
> by calling errseq_check() later?
> 

They don't want the sampling for the volatile mount to prevent later
openers from seeing an error that hasn't yet been reported.

If they copy the errseq_t (or just do an errseq_sample), and then follow
it with a errseq_check_and_advance then the SEEN bit will end up being
set and a later opener wouldn't see the error.

Aside from that though, I think this patch clarifies things a bit since
the SEEN flag currently means two different things:

1/ do I need to increment the counter when recording another error?

2/ do I need to report this error to new samplers (at open time)

That was ok before, since we those conditions were always changed
together, but with the overlayfs volatile mount use-case, it no longer
does.

> actually, isn't errseq_check() buggy in the face of multiple
> watchers?  consider this:
> 
> worker.es starts at 0
> t2.es = errseq_sample(&worker.es)
> errseq_set(&worker.es, -EIO)
> t1.es = errseq_sample(&worker.es)
> t2.err = errseq_check_and_advance(&es, t2.es)
> 	** this sets ERRSEQ_SEEN **
> t1.err = errseq_check(&worker.es, t1.es)
> 	** reports an error, even though the only change is that
> 	   ERRSEQ_SEEN moved **.
> 
> i think errseq_check() should be:
> 
> 	if (likely(cur | ERRSEQ_SEEN) == (since | ERRSEQ_SEEN))
> 		return 0;
> 
> i'm not yet convinced other changes are needed to errseq.  but i am
> having great trouble understanding exactly what overlayfs is trying to do.

I think you're right on errseq_check. I'll plan to do a patch to fix
that up as well.

I too am having a bit of trouble understanding all of the nuances here.
My current understanding is that it depends on the "volatility" of the
mount:

normal (non-volatile): they basically want to be able to track errors as
if the files were being opened on the upper layer. For this case I think
they should aim to just do all of the error checking against the upper
sb and ignore the overlayfs s_wb_err field. This does mean pushing the
errseq_check_and_advance down into the individual filesystems in some
fashion though.

volatile: they want to sample at mount time and always return an error
to syncfs if there has been another error since the original sample
point. This sampling should also not affect later openers on the upper
layer (or on other overlayfs mounts).

I'm not 100% clear on whether I understand both use-cases correctly
though.
-- 
Jeff Layton <jlayton@kernel.org>