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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <725c4bcacded089553341003117a3f49104c971b.camel@kernel.org>
Date:   Fri, 21 Jan 2022 12:47:24 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     David Howells <dhowells@...hat.com>, linux-cachefs@...hat.com
Cc:     Trond Myklebust <trondmy@...merspace.com>,
        Anna Schumaker <anna.schumaker@...app.com>,
        Steve French <smfrench@...il.com>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        Matthew Wilcox <willy@...radead.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Omar Sandoval <osandov@...ndov.com>,
        JeffleXu <jefflexu@...ux.alibaba.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-afs@...ts.infradead.org, linux-nfs@...r.kernel.org,
        linux-cifs@...r.kernel.org, ceph-devel@...r.kernel.org,
        v9fs-developer@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/11] cachefiles: Calculate the blockshift in terms of
 bytes, not pages

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@...hat.com>
> cc: linux-cachefs@...hat.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@...nel.org>