On Tue, 2020-12-15 at 09:00 +1100, NeilBrown wrote:
> On Mon, Dec 14 2020, Jeffrey Layton wrote:
>
> > 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@xxxxxxxxxx>
> > > > ---
> > > > 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.
>
> Sounds good.
>
> >
> > > 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);
>
> Oh good - I didn't see.
>
> >
> > > >
> > > >
> > > >
> > > >
> > > > - /* 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. Though it isn't clear to me why MUSTINC needs to be set there,
> so if you could make that clear, it would help me.
>
> Also, the two flags seem similar in how they are handled, only tracking
> different states, but their names don't reflect that.
> I imagine changing "SEEN" to "MUST_REPORT" or similar, so both flags are
> "MUST_XXX".
> Only I think we would then need to invert "SEEN" - as it currently means
> "MUSTN'T_REPORT" .. approximately.
>
> Or maybe we could replace MUST_INC by DID_INC, so it says what has been
> done, rather than what must be done.
>
> Or maybe not. Certainly it would be useful to have a clear picture of
> how the two flags are similar, and how they are different.
>
You need to set MUSTINC in errseq_peek to ensure that the next error
that occurs will be recorded, via the counter being bumped. Otherwise
that increment may be skipped (if no one else observed the last error).
I sent a v2 set before I saw your mail. Hopefully it addresses some of
your concerns.
You're right that the flag naming is a bit awkward. I'm open to
suggestions for names, but I'd probably like to keep the "sense" of the
flags so that I don't need to sort out the logic again. It also works
better with 0 being a special value that way.
> Thanks,
> NeilBrown
>
>
> >
> > 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
--
Jeff Layton <jlayton@xxxxxxxxxx>
