Re: [PATCH 2/5] cachefiles: drop direct usage of ->bmap method.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

On Thu, Jan 09 2020, Carlos Maiolino wrote:

> Replace the direct usage of ->bmap method by a bmap() call.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/cachefiles/rdwr.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
> index 44a3ce1e4ce4..1dc97f2d6201 100644
> --- a/fs/cachefiles/rdwr.c
> +++ b/fs/cachefiles/rdwr.c
> @@ -396,7 +396,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
>  	struct cachefiles_object *object;
>  	struct cachefiles_cache *cache;
>  	struct inode *inode;
> -	sector_t block0, block;
> +	sector_t block;
>  	unsigned shift;
>  	int ret;
>  
> @@ -412,7 +412,6 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
>  
>  	inode = d_backing_inode(object->backer);
>  	ASSERT(S_ISREG(inode->i_mode));
> -	ASSERT(inode->i_mapping->a_ops->bmap);
>  	ASSERT(inode->i_mapping->a_ops->readpages);
>  
>  	/* calculate the shift required to use bmap */
> @@ -428,12 +427,14 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op,
>  	 *   enough for this as it doesn't indicate errors, but it's all we've
>  	 *   got for the moment
>  	 */
> -	block0 = page->index;
> -	block0 <<= shift;
> +	block = page->index;
> +	block <<= shift;
> +
> +	ret = bmap(inode, &block);
> +	ASSERT(ret < 0);
>  
> -	block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0);
>  	_debug("%llx -> %llx",
> -	       (unsigned long long) block0,
> +	       (unsigned long long) (page->index << shift),
>  	       (unsigned long long) block);
>  
>  	if (block) {
> @@ -711,7 +712,6 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
>  
>  	inode = d_backing_inode(object->backer);
>  	ASSERT(S_ISREG(inode->i_mode));
> -	ASSERT(inode->i_mapping->a_ops->bmap);
>  	ASSERT(inode->i_mapping->a_ops->readpages);
>  
>  	/* calculate the shift required to use bmap */
> @@ -728,7 +728,7 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
>  
>  	ret = space ? -ENODATA : -ENOBUFS;
>  	list_for_each_entry_safe(page, _n, pages, lru) {
> -		sector_t block0, block;
> +		sector_t block;
>  
>  		/* we assume the absence or presence of the first block is a
>  		 * good enough indication for the page as a whole
> @@ -736,13 +736,14 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op,
>  		 *   good enough for this as it doesn't indicate errors, but
>  		 *   it's all we've got for the moment
>  		 */
> -		block0 = page->index;
> -		block0 <<= shift;
> +		block = page->index;
> +		block <<= shift;
> +
> +		ret = bmap(inode, &block);
> +		ASSERT(!ret);

Hi,
 this change corrupts 'ret'.
 Some paths from here down change ret, but some paths to do not.
 So it is possible that this function would previously return -ENODATA
 or -ENOBUFS, but now it returns 0.

 This gets caught by a BUG_ON in __nfs_readpages_from_fscache().

 Maybe a new variable should be introduced?
 or maybe ASSERT(!bmap(..));
 Or maybe if (bmap() != 0) ASSET(0);

??

https://bugzilla.opensuse.org/show_bug.cgi?id=1168841

NeilBrown


>  
> -		block = inode->i_mapping->a_ops->bmap(inode->i_mapping,
> -						      block0);
>  		_debug("%llx -> %llx",
> -		       (unsigned long long) block0,
> +		       (unsigned long long) (page->index << shift),
>  		       (unsigned long long) block);
>  
>  		if (block) {
> -- 
> 2.23.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]