On Thu, Jan 09 2020, Carlos Maiolino wrote:
> Replace the direct usage of ->bmap method by a bmap() call.
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
