Re: [PATCH v4 13/27] lib: add errseq_t type and infrastructure for handling it

[Jeff Layton <jlayton@redhat.com>]

On Wed, 2017-05-10 at 13:34 +0200, Jan Kara wrote:
> On Tue 09-05-17 11:49:16, Jeff Layton wrote:
> > An errseq_t is a way of recording errors in one place, and allowing any
> > number of "subscribers" to tell whether an error has been set again
> > since a previous time.
> > 
> > It's implemented as an unsigned 32-bit value that is managed with atomic
> > operations. The low order bits are designated to hold an error code
> > (max size of MAX_ERRNO). The upper bits are used as a counter.
> > 
> > The API works with consumers sampling an errseq_t value at a particular
> > point in time. Later, that value can be used to tell whether new errors
> > have been set since that time.
> > 
> > Note that there is a 1 in 512k risk of collisions here if new errors
> > are being recorded frequently, since we have so few bits to use as a
> > counter. To mitigate this, one bit is used as a flag to tell whether the
> > value has been sampled since a new value was recorded. That allows
> > us to avoid bumping the counter if no one has sampled it since it
> > was last bumped.
> > 
> > Later patches will build on this infrastructure to change how writeback
> > errors are tracked in the kernel.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> The patch looks good to me. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just two nits below:
> ...
> > +int errseq_check_and_advance(errseq_t *eseq, errseq_t *since)
> > +{
> > +	int err = 0;
> > +	errseq_t old, new;
> > +
> > +	/*
> > +	 * Most callers will want to use the inline wrapper to check this,
> > +	 * so that the common case of no error is handled without needing
> > +	 * to lock.
> > +	 */
> 
> I'm not sure which locking you are speaking about here. Is the comment
> stale?
> 

No it's not stale, just unclear. In the kerneldoc comment, I have this
blurb:

 * Note that no locking is provided here for concurrent updates to the "since"
 * value. The caller must provide that if necessary. Because of this, callers
 * may want to do a lockless errseq_check before taking the lock and calling
 * this.

...so the lock here refers to any locking that the caller needs to do to
serialize changes to "since" in this function. For the usual case of
file->f_wb_err, we use f_lock to protect it so by doing an errseq_check
beforehand we can avoid taking the f_lock and calling this function if
the value hasn't changed.

I'll see if I can rephrase that to be more clear.

> > +	old = READ_ONCE(*eseq);
> > +	if (old != *since) {
> > +		/*
> > +		 * Set the flag and try to swap it into place if it has
> > +		 * changed.
> > +		 *
> > +		 * We don't care about the outcome of the swap here. If the
> > +		 * swap doesn't occur, then it has either been updated by a
> > +		 * writer who is bumping the seq count anyway, or another
> 
> "bumping the seq count anyway" part is not quite true. Writer may see
> ERRSEQ_SEEN not set and so just update the error code and leave seq count
> as is. But since you compare full errseq_t for equality, this works out as
> designed...
> 

Yes, I think the code is correct, but the comment is not clear. Maybe
"bumping the seq count or resetting the error code, or..."

In any case, I'll clean up the comments for the next respin here.

> > +		 * reader who is just setting the "seen" flag. Either outcome
> > +		 * is OK, and we can advance "since" and return an error based
> > +		 * on what we have.
> > +		 */
> > +		new = old | ERRSEQ_SEEN;
> > +		if (new != old)
> > +			cmpxchg(eseq, old, new);
> > +		*since = new;
> > +		err = -(new & MAX_ERRNO);
> > +	}
> > +	return err;
> > +}
> > +EXPORT_SYMBOL(errseq_check_and_advance);
> 
> 								Honza

Thanks!
-- 
Jeff Layton <jlayton@redhat.com>