Re: [PATCH 02/11] cachefiles: Calculate the blockshift in terms of bytes, not pages

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2022-01-18 at 13:53 +0000, David Howells wrote:
> Cachefiles keeps track of how much space is available on the backing
> filesystem and refuses new writes permission to start if there isn't enough
> (we especially don't want ENOSPC happening).  It also tracks the amount of
> data pending in DIO writes (cache->b_writing) and reduces the amount of
> free space available by this amount before deciding if it can set up a new
> write.
> 
> However, the old fscache I/O API was very much page-granularity dependent
> and, as such, cachefiles's cache->bshift was meant to be a multiplier to
> get from PAGE_SIZE to block size (ie. a blocksize of 512 would give a shift
> of 3 for a 4KiB page) - and this was incorrectly being used to turn the
> number of bytes in a DIO write into a number of blocks, leading to a
> massive over estimation of the amount of data in flight.
> 
> Fix this by changing cache->bshift to be a multiplier from bytes to
> blocksize and deal with quantities of blocks, not quantities of pages.
> 
> Fix also the rounding in the calculation in cachefiles_write() which needs
> a "- 1" inserting.
> 
> Fixes: 047487c947e8 ("cachefiles: Implement the I/O routines")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-cachefs@redhat.com
> ---
> 
>  fs/cachefiles/cache.c    |    7 ++-----
>  fs/cachefiles/internal.h |    2 +-
>  fs/cachefiles/io.c       |    2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cachefiles/cache.c b/fs/cachefiles/cache.c
> index ce4d4785003c..1e9c71666c6a 100644
> --- a/fs/cachefiles/cache.c
> +++ b/fs/cachefiles/cache.c
> @@ -84,9 +84,7 @@ int cachefiles_add_cache(struct cachefiles_cache *cache)
>  		goto error_unsupported;
>  
>  	cache->bsize = stats.f_bsize;
> -	cache->bshift = 0;
> -	if (stats.f_bsize < PAGE_SIZE)
> -		cache->bshift = PAGE_SHIFT - ilog2(stats.f_bsize);
> +	cache->bshift = ilog2(stats.f_bsize);
>  
>  	_debug("blksize %u (shift %u)",
>  	       cache->bsize, cache->bshift);
> @@ -106,7 +104,6 @@ int cachefiles_add_cache(struct cachefiles_cache *cache)
>  	       (unsigned long long) cache->fcull,
>  	       (unsigned long long) cache->fstop);
>  
> -	stats.f_blocks >>= cache->bshift;
>  	do_div(stats.f_blocks, 100);
>  	cache->bstop = stats.f_blocks * cache->bstop_percent;
>  	cache->bcull = stats.f_blocks * cache->bcull_percent;
> @@ -209,7 +206,7 @@ int cachefiles_has_space(struct cachefiles_cache *cache,
>  		return ret;
>  	}
>  
> -	b_avail = stats.f_bavail >> cache->bshift;
> +	b_avail = stats.f_bavail;
>  	b_writing = atomic_long_read(&cache->b_writing);
>  	if (b_avail > b_writing)
>  		b_avail -= b_writing;
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index 8dd54d9375b6..c793d33b0224 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -86,7 +86,7 @@ struct cachefiles_cache {
>  	unsigned			bcull_percent;	/* when to start culling (% blocks) */
>  	unsigned			bstop_percent;	/* when to stop allocating (% blocks) */
>  	unsigned			bsize;		/* cache's block size */
> -	unsigned			bshift;		/* min(ilog2(PAGE_SIZE / bsize), 0) */
> +	unsigned			bshift;		/* ilog2(bsize) */
>  	uint64_t			frun;		/* when to stop culling */
>  	uint64_t			fcull;		/* when to start culling */
>  	uint64_t			fstop;		/* when to stop allocating */
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index 60b1eac2ce78..04eb52736990 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -264,7 +264,7 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
>  	ki->term_func		= term_func;
>  	ki->term_func_priv	= term_func_priv;
>  	ki->was_async		= true;
> -	ki->b_writing		= (len + (1 << cache->bshift)) >> cache->bshift;
> +	ki->b_writing		= (len + (1 << cache->bshift) - 1) >> cache->bshift;
>  
>  	if (ki->term_func)
>  		ki->iocb.ki_complete = cachefiles_write_complete;
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>