Re: [PATCH/RFC] sunrpc/cache: make cache flushing more reliable.

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Oct 15, 2015 at 05:39:47PM +1100, Neil Brown wrote:
> 
> Sorry.. I didn't refresh before posting....
> 
> > @@ -487,10 +496,13 @@ EXPORT_SYMBOL_GPL(cache_flush);
> >  
> >  void cache_purge(struct cache_detail *detail)
> >  {
> > -	detail->flush_time = LONG_MAX;
> > +	time_t now = seconds_since_boot();
> > +	if (detail->flush_time >= now)
> > +		now = detail->flush_time + 1;
> > +	/* 'now' is the maximum value any 'last_refresh' can have */
> > +	detail->flush_time = now + 1;
> 
> This should not have the '+ 1'.
> 
> >  	detail->nextcheck = seconds_since_boot();
> >  	cache_flush();
> > -	detail->flush_time = 1;
> >  }
> >  EXPORT_SYMBOL_GPL(cache_purge);
> >  
> > @@ -1447,8 +1460,22 @@ static ssize_t write_flush(struct file *file, const char __user *buf,
> >  		return -EINVAL;
> >  
> >  	bp = tbuf;
> > -	cd->flush_time = get_expiry(&bp);
> > -	cd->nextcheck = seconds_since_boot();
> > +	then = get_expiry(&bp);
> > +	now = seconds_since_boot();
> > +	cd->nextcheck = now;
> > +	/* Can only set flush_time to 1 second beyond "now", or
> > +	 * possibly 1 second beyond flushtime.  This is because
> > +	 * flush_time never goes backwards so it mustn't get too far
> > +	 * ahead of time.
> > +	 */
> > +	if (then >= now) {
> > +		/* Want to flush everything, so behave like cache_purge() */
> > +		if (cd->flush_time >= now)
> > +			now = cd->flush_time + 1;
> > +		then = now + 1;
> 
> Nor should.
> 
> I'll resend it people are otherwise happy.

I'm otherwise happy!  Looks like a reasonable approach.

(The one thing I wonder is whether it would be clearer to outright fail
writes that attempt to create already-expired cache entries.

But I think that's an unimportant corner case really.  And if there are
existing cases where that's happening then perhaps it's less disruptive
just to let them expire a second later rathern than to introduce a new
error.)

--b.