Re: A special case in write_flush which cause the umount busy

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

On Wed, Feb 14, 2018 at 06:57:06AM +1100, NeilBrown wrote:
> I think this change is safe.  It is a bit of a hack, but the "flush"
> interface was actually very poorly designed and I don't think it *can*
> be implemented cleanly.
> Maybe we should just ignore the number written in and *always* flush
> the cache completely.  That is was it always wanted.
> 
> See below.
> 
> Bruce: do you have any thoughts on this?

I'm fine with ignoring the number--I agree that the chance anyone is
doing anything fancier is vanishingly small.

--b.

> 
> Thanks,
> NeilBrown
> 
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index aa36dad32db1..43f117d1547e 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1450,7 +1450,7 @@ static ssize_t write_flush(struct file *file, const char __user *buf,
>  			   struct cache_detail *cd)
>  {
>  	char tbuf[20];
> -	char *bp, *ep;
> +	char *ep;
>  	time_t then, now;
>  
>  	if (*ppos || count > sizeof(tbuf)-1)
> @@ -1461,24 +1461,24 @@ static ssize_t write_flush(struct file *file, const char __user *buf,
>  	simple_strtoul(tbuf, &ep, 0);
>  	if (*ep && *ep != '\n')
>  		return -EINVAL;
> +	/* Note that while we check that 'buf' holds a valid number,
> +	 * we always ignore the value and just flush everything.
> +	 * Making use of the number leads to races.
> +	 */
>  
> -	bp = tbuf;
> -	then = get_expiry(&bp);
>  	now = seconds_since_boot();
> -	cd->nextcheck = now;
> -	/* Can only set flush_time to 1 second beyond "now", or
> +	/* Always flush everything, so behave like cache_purge()
> +	 * 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;
> -	}
>  
> -	cd->flush_time = then;
> +	if (cd->flush_time >= now)
> +		now = cd->flush_time + 1;
> +
> +	cd->flush_time = now;
> +	cd->nextcheck = now;
>  	cache_flush();
>  
>  	*ppos += count;
> 
> 
> >
> > --------------------------------------------------------------------------
> >        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)
> >               printk("%s %d then = %d, now = %d\n", __func__, __LINE__, then, now);
> > -      if (then >= now) {
> > +     if (then >= now - 1) {
> >               /* Want to flush eve