Re: [RFC PATCH 1/2] errseq: split the SEEN flag into two new flags

[Jeffrey Layton <jlayton@poochiereds.net>]

On Mon, Dec 14, 2020 at 10:35:56AM +1100, NeilBrown wrote:
> On Sun, Dec 13 2020, 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.
> >
> > The original reason for the SEEN flag was to make it so that we didn't
> > need to increment the counter if nothing had observed the latest value
> > and the error was the same. Eventually, a regression was reported in
> > the errseq_t conversion, and we fixed that by using the SEEN flag to
> > also mean that the error had been reported to userland at least once
> > somewhere.
> >
> > Those are two different states, however. If we instead take a second
> > flag bit from the counter, we can track these two things separately,
> > and accomodate the overlayfs volatile mount use-case.
> >
> > Add a new MUSTINC flag that indicates that the counter must be
> > incremented the next time an error is set, and rework the errseq
> > functions to set and clear that flag whenever the SEEN bit is set or
> > cleared.
> >
> > Test only for the MUSTINC bit when deciding whether to increment the
> > counter and only for the SEEN bit when deciding what to return in
> > errseq_sample.
> >
> > Add a new errseq_peek function to allow for the overlayfs use-case.
> > This just grabs the latest counter and sets the MUSTINC bit, leaving
> > the SEEN bit untouched.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  include/linux/errseq.h |  2 ++
> >  lib/errseq.c           | 64 ++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 55 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/errseq.h b/include/linux/errseq.h
> > index fc2777770768..6d4b9bc629ac 100644
> > --- a/include/linux/errseq.h
> > +++ b/include/linux/errseq.h
> > @@ -9,6 +9,8 @@ typedef u32	errseq_t;
> >  
> >  errseq_t errseq_set(errseq_t *eseq, int err);
> >  errseq_t errseq_sample(errseq_t *eseq);
> > +errseq_t errseq_peek(errseq_t *eseq);
> > +errseq_t errseq_sample_advance(errseq_t *eseq);
> >  int errseq_check(errseq_t *eseq, errseq_t since);
> >  int errseq_check_and_advance(errseq_t *eseq, errseq_t *since);
> >  #endif
> > diff --git a/lib/errseq.c b/lib/errseq.c
> > index 81f9e33aa7e7..5cc830f0361b 100644
> > --- a/lib/errseq.c
> > +++ b/lib/errseq.c
> > @@ -38,8 +38,11 @@
> >  /* This bit is used as a flag to indicate whether the value has been seen */
> >  #define ERRSEQ_SEEN		(1 << ERRSEQ_SHIFT)
> 
> Would this look nicer using the BIT() macro?
> 
>   #define ERRSEQ_SEEN		BIT(ERRSEQ_SHIFT)
> 
> >  
> > +/* This bit indicates that value must be incremented even when error is same */
> > +#define ERRSEQ_MUSTINC		(1 << (ERRSEQ_SHIFT + 1))
> 
>  #define ERRSEQ_MUSTINC		BIT(ERRSEQ_SHIFT+1)
> 
> or if you don't like the BIT macro (not everyone does), then maybe
> 
>  #define ERR_SEQ_MUSTINC	(ERRSEQ_SEEN << 1 )
> 
> ??
> 
> > +
> >  /* The lowest bit of the counter */
> > -#define ERRSEQ_CTR_INC		(1 << (ERRSEQ_SHIFT + 1))
> > +#define ERRSEQ_CTR_INC		(1 << (ERRSEQ_SHIFT + 2))
> 
> Ditto.
> 

Yes, I can make that change. The BIT macro is much easier to read.

> >  
> >  /**
> >   * errseq_set - set a errseq_t for later reporting
> > @@ -77,11 +80,11 @@ errseq_t errseq_set(errseq_t *eseq, int err)
> >  	for (;;) {
> >  		errseq_t new;
> >  
> > -		/* Clear out error bits and set new error */
> > -		new = (old & ~(MAX_ERRNO|ERRSEQ_SEEN)) | -err;
> > +		/* Clear out flag bits and set new error */
> > +		new = (old & ~(MAX_ERRNO|ERRSEQ_SEEN|ERRSEQ_MUSTINC)) | -err;
> 
> This is starting to look clumsy (or maybe, this already looked clumsy,
> but now that is hard to ignore).
> 
> 		new = (old & (ERRSEQ_CTR_INC - 1)) | -err
> 

I think you mean:

		new = (old & ~(ERRSEQ_CTR_INC - 1)) | -err;

Maybe I can add a new ERRSEQ_CTR_MASK value though which makes it more
evident.

> Also this assumes MAX_ERRNO is a mask, which it is .. today.
> 
> 	BUILD_BUG_ON(MAX_ERRNO & (MAX_ERRNO + 1));
> ??
> 

We already have this in errseq_set:

        BUILD_BUG_ON_NOT_POWER_OF_2(MAX_ERRNO + 1);

> >  
> > -		/* Only increment if someone has looked at it */
> > -		if (old & ERRSEQ_SEEN)
> > +		/* Only increment if we have to */
> > +		if (old & ERRSEQ_MUSTINC)
> >  			new += ERRSEQ_CTR_INC;
> >  
> >  		/* If there would be no change, then call it done */
> > @@ -122,14 +125,50 @@ EXPORT_SYMBOL(errseq_set);
> >  errseq_t errseq_sample(errseq_t *eseq)
> >  {
> >  	errseq_t old = READ_ONCE(*eseq);
> > +	errseq_t new = old;
> >  
> > -	/* If nobody has seen this error yet, then we can be the first. */
> > -	if (!(old & ERRSEQ_SEEN))
> > -		old = 0;
> > -	return old;
> > +	/*
> > +	 * For the common case of no errors ever having been set, we can skip
> > +	 * marking the SEEN|MUSTINC bits. Once an error has been set, the value
> > +	 * will never go back to zero.
> > +	 */
> > +	if (old != 0) {
> > +		new |= ERRSEQ_SEEN|ERRSEQ_MUSTINC;
> 
> You lose me here.  Why is ERRSEQ_SEEN being set, where it wasn't before?
> 
> The ERRSEQ_SEEN flag not means precisely "The error has been reported to
> userspace".
> This operations isn't used to report errors - that is errseq_check().
> 
> I'm not saying the code it wrong - I really cannot tell.
> I'm just saying that I cannot see why it might be right.
> 

I think you're right. We should not be setting SEEN here, but we do
need to set MUSTINC if it's not already set. I'll fix (and re-test).

Thanks for the review!

> 
> 
> 
> > +		if (old != new)
> > +			cmpxchg(eseq, old, new);
> > +		if (!(old & ERRSEQ_SEEN))
> > +			return 0;
> > +	}
> > +	return new;
> >  }
> >  EXPORT_SYMBOL(errseq_sample);
> >  
> > +/**
> > + * errseq_peek - Grab current errseq_t value, but don't mark it SEEN
> > + * @eseq: Pointer to errseq_t to be sampled.
> > + *
> > + * In some cases, we need to be able to sample the errseq_t, but we're not
> > + * in a situation where we can report the value to userland. Use this
> > + * function to do that. This ensures that later errors will be recorded,
> > + * and that any current errors are reported at least once.
> > + *
> > + * Context: Any context.
> > + * Return: The current errseq value.
> > + */
> > +errseq_t errseq_peek(errseq_t *eseq)
> > +{
> > +	errseq_t old = READ_ONCE(*eseq);
> > +	errseq_t new = old;
> > +
> > +	if (old != 0) {
> > +		new |= ERRSEQ_MUSTINC;
> > +		if (old != new)
> > +			cmpxchg(eseq, old, new);
> > +	}
> > +	return new;
> > +}
> > +EXPORT_SYMBOL(errseq_peek);
> > +
> >  /**
> >   * errseq_check() - Has an error occurred since a particular sample point?
> >   * @eseq: Pointer to errseq_t value to be checked.
> > @@ -143,7 +182,10 @@ EXPORT_SYMBOL(errseq_sample);
> >   */
> >  int errseq_check(errseq_t *eseq, errseq_t since)
> >  {
> > -	errseq_t cur = READ_ONCE(*eseq);
> > +	errseq_t cur = READ_ONCE(*eseq) & ~(ERRSEQ_MUSTINC|ERRSEQ_SEEN);
> > +
> > +	/* Clear the flag bits for comparison */
> > +	since &= ~(ERRSEQ_MUSTINC|ERRSEQ_SEEN);
> >  
> >  	if (likely(cur == since))
> >  		return 0;
> > @@ -195,7 +237,7 @@ int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
> >  		 * can advance "since" and return an error based on what we
> >  		 * have.
> >  		 */
> > -		new = old | ERRSEQ_SEEN;
> > +		new = old | ERRSEQ_SEEN | ERRSEQ_MUSTINC;
> >  		if (new != old)
> >  			cmpxchg(eseq, old, new);
> >  		*since = new;
> > -- 
> > 2.29.2