[PATCH 0/3] Replace direct ->bmap calls by bmap() with error support

[PATCH 0/3] Replace direct ->bmap calls by bmap() with error support

[Carlos Maiolino]

Hi, this is a non-RFC version of the attempt to replace direct ->bmap() calls
by calls to bmap() helper.

This is a preparatory work to remove ->bmap() interface in a not so distant
future.

I decided to send this replacement separated from everything else, because I
want to make sure to get the ->bmap replacement correct before doing work which
will rely on it.

This also enables bmap() helper to properly return an error, as, by now,
->bmap() uses '0' as an error return.

With this patchset, the following behavior is set:

int bmap(struct inode *, sector_t *);

the passed pointer sector_t* will be changed internally and after return, will
contain either the physical block number of the requested mapping, or 0, if it
falls into a hole.

The return value is either a negative in case of error, or 0.

The ability of bmap() to return an error, has been suggested by Christoph, and
this is based on his suggestion, with small modifications.

Also, with the ability to return errors, Eric Sandeen suggested we may actually
use it to return -EOVERFLOW in ioctl_fibmap() calls, to signal userspace the
requested value has been truncated, since, by now, there is no easy way for it
to be detected.

Once I can get this patchset properly set, I'll work on needed modifications
into ->bmap() itself.

I did split the ->bmap() to bmap() replacements into different patches,
separated by projects, because I thought it would be easier to review, I do
apologize if I should have added everything into a single patch.

I also didn't change ioctl_fibmap(), because for me at least, it makes more
sense to call ->bmap() there directly by now, so we we avoid copying the
userspace data if the method doesn't even exist.

Comments are appreciated.
Cheers.


Carlos Maiolino (3):
  fs: Enable bmap() function to properly return errors
  cachefiles: drop direct usage of ->bmap method.
  ecryptfs: drop direct calls to ->bmap

 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/cachefiles/rdwr.c   | 27 ++++++++++++++-------------
 fs/ecryptfs/mmap.c     | 11 ++++++-----
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  2 +-
 mm/page_io.c           | 11 +++++++----
 7 files changed, 70 insertions(+), 49 deletions(-)

-- 
2.14.4

[PATCH 1/3] fs: Enable bmap() function to properly return errors

[Carlos Maiolino]

By now, bmap() will either return the physical block number related to
the requested file offset or 0 in case of error or the requested offset
maps into a hole.
This patch makes the needed changes to enable bmap() to proper return
errors, using the return value as an error return, and now, a pointer
must be passed to bmap() to be filled with the mapped physical block.

It will change the behavior of bmap() on return:

- negative value in case of error
- zero on success or if map fell into a hole

In case of a hole, the *block will be zero too

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

P.S. Since this is a prep patch, by now, the only error return is -EINVAL if
->bmap doesn't exist.

 drivers/md/md-bitmap.c | 16 ++++++++++------
 fs/inode.c             | 30 +++++++++++++++++-------------
 fs/jbd2/journal.c      | 22 +++++++++++++++-------
 include/linux/fs.h     |  2 +-
 mm/page_io.c           | 11 +++++++----
 5 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 2fc8c113977f..0bbd55f45b25 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -363,7 +363,7 @@ static int read_page(struct file *file, unsigned long index,
 	int ret = 0;
 	struct inode *inode = file_inode(file);
 	struct buffer_head *bh;
-	sector_t block;
+	sector_t block, blk_cur;
 
 	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
 		 (unsigned long long)index << PAGE_SHIFT);
@@ -374,17 +374,21 @@ static int read_page(struct file *file, unsigned long index,
 		goto out;
 	}
 	attach_page_buffers(page, bh);
-	block = index << (PAGE_SHIFT - inode->i_blkbits);
+	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
 	while (bh) {
+		block = blk_cur;
+
 		if (count == 0)
 			bh->b_blocknr = 0;
 		else {
-			bh->b_blocknr = bmap(inode, block);
-			if (bh->b_blocknr == 0) {
-				/* Cannot use this file! */
+			ret = bmap(inode, &block);
+			if (ret || !block) {
 				ret = -EINVAL;
+				bh->b_blocknr = 0;
 				goto out;
 			}
+
+			bh->b_blocknr = block;
 			bh->b_bdev = inode->i_sb->s_bdev;
 			if (count < (1<<inode->i_blkbits))
 				count = 0;
@@ -398,7 +402,7 @@ static int read_page(struct file *file, unsigned long index,
 			set_buffer_mapped(bh);
 			submit_bh(REQ_OP_READ, 0, bh);
 		}
-		block++;
+		blk_cur++;
 		bh = bh->b_this_page;
 	}
 	page->index = index;
diff --git a/fs/inode.c b/fs/inode.c
index 41812a3e829e..99154ce81cd9 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1577,21 +1577,25 @@ EXPORT_SYMBOL(iput);
 
 /**
  *	bmap	- find a block number in a file
- *	@inode: inode of file
- *	@block: block to find
- *
- *	Returns the block number on the device holding the inode that
- *	is the disk block number for the block of the file requested.
- *	That is, asked for block 4 of inode 1 the function will return the
- *	disk block relative to the disk start that holds that block of the
- *	file.
+ *	@inode:  inode owning the block number being requested
+ *	@*block: pointer containing the block to find
+ *
+ *	Replaces the value in *block with the block number on the device holding
+ *	corresponding to the requested block number in the file.
+ *	That is, asked for block 4 of inode 1 the function will replace the
+ *	4 in *block, with disk block relative to the disk start that holds that
+ *	block of the file.
+ *
+ *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
+ *	hole, returns 0 and *block is also set to 0.
  */
-sector_t bmap(struct inode *inode, sector_t block)
+int bmap(struct inode *inode, sector_t *block)
 {
-	sector_t res = 0;
-	if (inode->i_mapping->a_ops->bmap)
-		res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
-	return res;
+	if (!inode->i_mapping->a_ops->bmap)
+		return -EINVAL;
+
+	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
+	return 0;
 }
 EXPORT_SYMBOL(bmap);
 
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8ef6b6daaa7a..7acaf6f55404 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -814,18 +814,23 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
 {
 	int err = 0;
 	unsigned long long ret;
+	sector_t block = 0;
 
 	if (journal->j_inode) {
-		ret = bmap(journal->j_inode, blocknr);
-		if (ret)
-			*retp = ret;
-		else {
+		block = blocknr;
+		ret = bmap(journal->j_inode, &block);
+
+		if (ret || !block) {
 			printk(KERN_ALERT "%s: journal block not found "
 					"at offset %lu on %s\n",
 			       __func__, blocknr, journal->j_devname);
 			err = -EIO;
 			__journal_abort_soft(journal, err);
+
+		} else {
+			*retp = block;
 		}
+
 	} else {
 		*retp = blocknr; /* +journal->j_blk_offset */
 	}
@@ -1251,11 +1256,14 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
 journal_t *jbd2_journal_init_inode(struct inode *inode)
 {
 	journal_t *journal;
+	sector_t blocknr;
 	char *p;
-	unsigned long long blocknr;
+	int err = 0;
+
+	blocknr = 0;
+	err = bmap(inode, &blocknr);
 
-	blocknr = bmap(inode, 0);
-	if (!blocknr) {
+	if (err || !blocknr) {
 		pr_err("%s: Cannot locate journal superblock\n",
 			__func__);
 		return NULL;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bf0cad65b9b7..15a79f67ad87 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2762,7 +2762,7 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK
-extern sector_t bmap(struct inode *, sector_t);
+extern int bmap(struct inode *, sector_t *);
 #endif
 extern int notify_change(struct dentry *, struct iattr *, struct inode **);
 extern int inode_permission(struct inode *, int);
diff --git a/mm/page_io.c b/mm/page_io.c
index aafd19ec1db4..994c996414c3 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -177,8 +177,9 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
 
 		cond_resched();
 
-		first_block = bmap(inode, probe_block);
-		if (first_block == 0)
+		first_block = probe_block;
+		ret = bmap(inode, &first_block);
+		if (ret || !first_block)
 			goto bad_bmap;
 
 		/*
@@ -193,9 +194,11 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
 					block_in_page++) {
 			sector_t block;
 
-			block = bmap(inode, probe_block + block_in_page);
-			if (block == 0)
+			block = probe_block + block_in_page;
+			ret = bmap(inode, &block);
+			if (ret || !block)
 				goto bad_bmap;
+
 			if (block != first_block + block_in_page) {
 				/* Discontiguity */
 				probe_block++;
-- 
2.14.4

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

[Carlos Maiolino]

Replace the direct usage of ->bmap method by a bmap() call.

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 40f7595aad10..a3ee23fe9269 100644
--- a/fs/cachefiles/rdwr.c
+++ b/fs/cachefiles/rdwr.c
@@ -400,7 +400,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;
 
@@ -416,7 +416,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 */
@@ -432,12 +431,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);
 
-	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) {
@@ -709,7 +710,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 */
@@ -726,7 +726,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
@@ -734,13 +734,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);
 
-		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.14.4

[PATCH 3/3] ecryptfs: drop direct calls to ->bmap

[Carlos Maiolino]

Replace direct ->bmap calls by bmap() method.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/ecryptfs/mmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index cdf358b209d9..256721eb6a03 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -538,16 +538,17 @@ static int ecryptfs_write_end(struct file *file,
 
 static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
 {
-	int rc = 0;
+	sector_t blk_map = 0;
+	int ret;
 	struct inode *inode;
 	struct inode *lower_inode;
 
 	inode = (struct inode *)mapping->host;
 	lower_inode = ecryptfs_inode_to_lower(inode);
-	if (lower_inode->i_mapping->a_ops->bmap)
-		rc = lower_inode->i_mapping->a_ops->bmap(lower_inode->i_mapping,
-							 block);
-	return rc;
+
+	ret = bmap(lower_inode, &blk_map);
+
+	return !ret ? blk_map : 0;
 }
 
 const struct address_space_operations ecryptfs_aops = {
-- 
2.14.4

Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors

[Christoph Hellwig]

On Wed, Sep 12, 2018 at 02:25:34PM +0200, Carlos Maiolino wrote:
>  	attach_page_buffers(page, bh);
> -	block = index << (PAGE_SHIFT - inode->i_blkbits);
> +	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
>  	while (bh) {
> +		block = blk_cur;
> +
>  		if (count == 0)
>  			bh->b_blocknr = 0;
>  		else {
> -			bh->b_blocknr = bmap(inode, block);
> -			if (bh->b_blocknr == 0) {
> -				/* Cannot use this file! */
> +			ret = bmap(inode, &block);
> +			if (ret || !block) {
>  				ret = -EINVAL;
> +				bh->b_blocknr = 0;
>  				goto out;


This conversion look good, but the code you are starting with is
completely broken.  It really needs to switch to use normal read/write_iter
interfaces ASAP.

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

On Wed, Sep 12, 2018 at 02:25:35PM +0200, Carlos Maiolino wrote:
> Replace the direct usage of ->bmap method by a bmap() call.

Would be nice to have some real error handling instead of the assert
on an error return, but otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] ecryptfs: drop direct calls to ->bmap

[Christoph Hellwig]

On Wed, Sep 12, 2018 at 02:25:36PM +0200, Carlos Maiolino wrote:
>  static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
>  {
> +	sector_t blk_map = 0;
> +	int ret;
>  	struct inode *inode;
>  	struct inode *lower_inode;
>  
>  	inode = (struct inode *)mapping->host;
>  	lower_inode = ecryptfs_inode_to_lower(inode);
> +
> +	ret = bmap(lower_inode, &blk_map);
> +
> +	return !ret ? blk_map : 0;

This could be simplified to:

static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
{
 	struct inode *lower_inode = ecryptfs_inode_to_lower(mapping->host);
	int ret = bmap(lower_inode, &block);

	if (ret)
		return 0;
	return block;
}

But the idea that we even support ->bmap on ecryptfs sounds way too
dangerous.

Re: [PATCH 3/3] ecryptfs: drop direct calls to ->bmap

[Carlos Maiolino]

On Fri, Sep 14, 2018 at 03:26:16PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 12, 2018 at 02:25:36PM +0200, Carlos Maiolino wrote:
> >  static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
> >  {
> > +	sector_t blk_map = 0;
> > +	int ret;
> >  	struct inode *inode;
> >  	struct inode *lower_inode;
> >  
> >  	inode = (struct inode *)mapping->host;
> >  	lower_inode = ecryptfs_inode_to_lower(inode);
> > +
> > +	ret = bmap(lower_inode, &blk_map);
> > +
> > +	return !ret ? blk_map : 0;
> 
> This could be simplified to:
> 
> static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
> {
>  	struct inode *lower_inode = ecryptfs_inode_to_lower(mapping->host);
> 	int ret = bmap(lower_inode, &block);
> 
> 	if (ret)
> 		return 0;
> 	return block;
> }

I can change, without problem.
> 
> But the idea that we even support ->bmap on ecryptfs sounds way too
> dangerous.

I can't argue here, I don't know much about ecryptfs, I just replaced the ->bmap
call keeping the same logic, but thanks a lot for the review, I'll update it and
send a V2

-- 
Carlos

Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors

[Carlos Maiolino]

On Fri, Sep 14, 2018 at 03:23:13PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 12, 2018 at 02:25:34PM +0200, Carlos Maiolino wrote:
> >  	attach_page_buffers(page, bh);
> > -	block = index << (PAGE_SHIFT - inode->i_blkbits);
> > +	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
> >  	while (bh) {
> > +		block = blk_cur;
> > +
> >  		if (count == 0)
> >  			bh->b_blocknr = 0;
> >  		else {
> > -			bh->b_blocknr = bmap(inode, block);
> > -			if (bh->b_blocknr == 0) {
> > -				/* Cannot use this file! */
> > +			ret = bmap(inode, &block);
> > +			if (ret || !block) {
> >  				ret = -EINVAL;
> > +				bh->b_blocknr = 0;
> >  				goto out;
> 
> 
> This conversion look good, but the code you are starting with is
> completely broken.  It really needs to switch to use normal read/write_iter
> interfaces ASAP.
> 
Thanks for the review Christoph, I'll add this to my todo list. Thanks a lot.


> Otherwise looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>


-- 
Carlos

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

[Carlos Maiolino]

On Fri, Sep 14, 2018 at 03:23:53PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 12, 2018 at 02:25:35PM +0200, Carlos Maiolino wrote:
> > Replace the direct usage of ->bmap method by a bmap() call.
> 
> Would be nice to have some real error handling instead of the assert
> on an error return, but otherwise this looks fine:
> 

Thanks. I'll see if I can do something about it and write a patch for it. /me
doesn't know much about cachefiles yet.

Thanks for the review.

Cheers

> Reviewed-by: Christoph Hellwig <hch@lst.de>

-- 
Carlos

[PATCH 3/3 V2] ecryptfs: drop direct calls to ->bmap

[Carlos Maiolino]

Replace direct ->bmap calls by bmap() method.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

Changelog:

V2: Simplify logic as suggested by Christoph

 fs/ecryptfs/mmap.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index cdf358b209d9..ff323dccef36 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -538,16 +538,12 @@ static int ecryptfs_write_end(struct file *file,
 
 static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block)
 {
-	int rc = 0;
-	struct inode *inode;
-	struct inode *lower_inode;
-
-	inode = (struct inode *)mapping->host;
-	lower_inode = ecryptfs_inode_to_lower(inode);
-	if (lower_inode->i_mapping->a_ops->bmap)
-		rc = lower_inode->i_mapping->a_ops->bmap(lower_inode->i_mapping,
-							 block);
-	return rc;
+	struct inode *lower_inode = ecryptfs_inode_to_lower(mapping->host);
+	int ret = bmap(lower_inode, &block);
+
+	if (ret)
+		return 0;
+	return block;
 }
 
 const struct address_space_operations ecryptfs_aops = {
-- 
2.14.4

Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors

[Darrick J. Wong]

On Wed, Sep 12, 2018 at 02:25:34PM +0200, Carlos Maiolino wrote:
> By now, bmap() will either return the physical block number related to
> the requested file offset or 0 in case of error or the requested offset
> maps into a hole.
> This patch makes the needed changes to enable bmap() to proper return
> errors, using the return value as an error return, and now, a pointer
> must be passed to bmap() to be filled with the mapped physical block.
> 
> It will change the behavior of bmap() on return:
> 
> - negative value in case of error
> - zero on success or if map fell into a hole
> 
> In case of a hole, the *block will be zero too
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> P.S. Since this is a prep patch, by now, the only error return is -EINVAL if
> ->bmap doesn't exist.
> 
>  drivers/md/md-bitmap.c | 16 ++++++++++------
>  fs/inode.c             | 30 +++++++++++++++++-------------
>  fs/jbd2/journal.c      | 22 +++++++++++++++-------
>  include/linux/fs.h     |  2 +-
>  mm/page_io.c           | 11 +++++++----
>  5 files changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 2fc8c113977f..0bbd55f45b25 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -363,7 +363,7 @@ static int read_page(struct file *file, unsigned long index,
>  	int ret = 0;
>  	struct inode *inode = file_inode(file);
>  	struct buffer_head *bh;
> -	sector_t block;
> +	sector_t block, blk_cur;
>  
>  	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
>  		 (unsigned long long)index << PAGE_SHIFT);
> @@ -374,17 +374,21 @@ static int read_page(struct file *file, unsigned long index,
>  		goto out;
>  	}
>  	attach_page_buffers(page, bh);
> -	block = index << (PAGE_SHIFT - inode->i_blkbits);
> +	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
>  	while (bh) {
> +		block = blk_cur;
> +
>  		if (count == 0)
>  			bh->b_blocknr = 0;
>  		else {
> -			bh->b_blocknr = bmap(inode, block);
> -			if (bh->b_blocknr == 0) {
> -				/* Cannot use this file! */
> +			ret = bmap(inode, &block);
> +			if (ret || !block) {
>  				ret = -EINVAL;
> +				bh->b_blocknr = 0;
>  				goto out;
>  			}
> +
> +			bh->b_blocknr = block;
>  			bh->b_bdev = inode->i_sb->s_bdev;
>  			if (count < (1<<inode->i_blkbits))
>  				count = 0;
> @@ -398,7 +402,7 @@ static int read_page(struct file *file, unsigned long index,
>  			set_buffer_mapped(bh);
>  			submit_bh(REQ_OP_READ, 0, bh);
>  		}
> -		block++;
> +		blk_cur++;
>  		bh = bh->b_this_page;
>  	}
>  	page->index = index;
> diff --git a/fs/inode.c b/fs/inode.c
> index 41812a3e829e..99154ce81cd9 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1577,21 +1577,25 @@ EXPORT_SYMBOL(iput);
>  
>  /**
>   *	bmap	- find a block number in a file
> - *	@inode: inode of file
> - *	@block: block to find
> - *
> - *	Returns the block number on the device holding the inode that
> - *	is the disk block number for the block of the file requested.
> - *	That is, asked for block 4 of inode 1 the function will return the
> - *	disk block relative to the disk start that holds that block of the
> - *	file.
> + *	@inode:  inode owning the block number being requested
> + *	@*block: pointer containing the block to find
> + *
> + *	Replaces the value in *block with the block number on the device holding
> + *	corresponding to the requested block number in the file.
> + *	That is, asked for block 4 of inode 1 the function will replace the
> + *	4 in *block, with disk block relative to the disk start that holds that
> + *	block of the file.
> + *
> + *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
> + *	hole, returns 0 and *block is also set to 0.
>   */
> -sector_t bmap(struct inode *inode, sector_t block)
> +int bmap(struct inode *inode, sector_t *block)
>  {
> -	sector_t res = 0;
> -	if (inode->i_mapping->a_ops->bmap)
> -		res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
> -	return res;
> +	if (!inode->i_mapping->a_ops->bmap)
> +		return -EINVAL;
> +
> +	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);

Hm.  Could we change the aops ->bmap interface to return negative error
codes too?

Also... ioctl_fibmap blindly copies the bottom 32 bits of *block out to
userspace without checking for overflows.  Shouldn't that also be
changed?

--D

> +	return 0;
>  }
>  EXPORT_SYMBOL(bmap);
>  
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 8ef6b6daaa7a..7acaf6f55404 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -814,18 +814,23 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
>  {
>  	int err = 0;
>  	unsigned long long ret;
> +	sector_t block = 0;
>  
>  	if (journal->j_inode) {
> -		ret = bmap(journal->j_inode, blocknr);
> -		if (ret)
> -			*retp = ret;
> -		else {
> +		block = blocknr;
> +		ret = bmap(journal->j_inode, &block);
> +
> +		if (ret || !block) {
>  			printk(KERN_ALERT "%s: journal block not found "
>  					"at offset %lu on %s\n",
>  			       __func__, blocknr, journal->j_devname);
>  			err = -EIO;
>  			__journal_abort_soft(journal, err);
> +
> +		} else {
> +			*retp = block;
>  		}
> +
>  	} else {
>  		*retp = blocknr; /* +journal->j_blk_offset */
>  	}
> @@ -1251,11 +1256,14 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
>  journal_t *jbd2_journal_init_inode(struct inode *inode)
>  {
>  	journal_t *journal;
> +	sector_t blocknr;
>  	char *p;
> -	unsigned long long blocknr;
> +	int err = 0;
> +
> +	blocknr = 0;
> +	err = bmap(inode, &blocknr);
>  
> -	blocknr = bmap(inode, 0);
> -	if (!blocknr) {
> +	if (err || !blocknr) {
>  		pr_err("%s: Cannot locate journal superblock\n",
>  			__func__);
>  		return NULL;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bf0cad65b9b7..15a79f67ad87 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2762,7 +2762,7 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
>  extern void emergency_sync(void);
>  extern void emergency_remount(void);
>  #ifdef CONFIG_BLOCK
> -extern sector_t bmap(struct inode *, sector_t);
> +extern int bmap(struct inode *, sector_t *);
>  #endif
>  extern int notify_change(struct dentry *, struct iattr *, struct inode **);
>  extern int inode_permission(struct inode *, int);
> diff --git a/mm/page_io.c b/mm/page_io.c
> index aafd19ec1db4..994c996414c3 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -177,8 +177,9 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
>  
>  		cond_resched();
>  
> -		first_block = bmap(inode, probe_block);
> -		if (first_block == 0)
> +		first_block = probe_block;
> +		ret = bmap(inode, &first_block);
> +		if (ret || !first_block)
>  			goto bad_bmap;
>  
>  		/*
> @@ -193,9 +194,11 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
>  					block_in_page++) {
>  			sector_t block;
>  
> -			block = bmap(inode, probe_block + block_in_page);
> -			if (block == 0)
> +			block = probe_block + block_in_page;
> +			ret = bmap(inode, &block);
> +			if (ret || !block)
>  				goto bad_bmap;
> +
>  			if (block != first_block + block_in_page) {
>  				/* Discontiguity */
>  				probe_block++;
> -- 
> 2.14.4
> 

Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors

[Carlos Maiolino]

Hi Darrick

> > -	sector_t res = 0;
> > -	if (inode->i_mapping->a_ops->bmap)
> > -		res = inode->i_mapping->a_ops->bmap(inode->i_mapping, block);
> > -	return res;
> > +	if (!inode->i_mapping->a_ops->bmap)
> > +		return -EINVAL;
> > +
> > +	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> 
> Hm.  Could we change the aops ->bmap interface to return negative error
> codes too?

This could be done, yes, but, the goal here, is to get rid of ->bmap()
interface, so, this will (hopefully) soon be gone, so I don't see any reason to
change it by now. Could be done, yes, I am just not sure if it's worth, once the
main goal is to remove ->bmap calls and replace it by ->fiemap().
> 
> Also... ioctl_fibmap blindly copies the bottom 32 bits of *block out to
> userspace without checking for overflows.  Shouldn't that also be
> changed?
> 

Eric suggested it, and this is in my road map to do, I just didn't want to
attempt implementing it in this patchset, which I aimed just to pave the road
for the next changes I'm working on.

Cheers.

> --D
> 
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL(bmap);
> >  
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 8ef6b6daaa7a..7acaf6f55404 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -814,18 +814,23 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
> >  {
> >  	int err = 0;
> >  	unsigned long long ret;
> > +	sector_t block = 0;
> >  
> >  	if (journal->j_inode) {
> > -		ret = bmap(journal->j_inode, blocknr);
> > -		if (ret)
> > -			*retp = ret;
> > -		else {
> > +		block = blocknr;
> > +		ret = bmap(journal->j_inode, &block);
> > +
> > +		if (ret || !block) {
> >  			printk(KERN_ALERT "%s: journal block not found "
> >  					"at offset %lu on %s\n",
> >  			       __func__, blocknr, journal->j_devname);
> >  			err = -EIO;
> >  			__journal_abort_soft(journal, err);
> > +
> > +		} else {
> > +			*retp = block;
> >  		}
> > +
> >  	} else {
> >  		*retp = blocknr; /* +journal->j_blk_offset */
> >  	}
> > @@ -1251,11 +1256,14 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
> >  journal_t *jbd2_journal_init_inode(struct inode *inode)
> >  {
> >  	journal_t *journal;
> > +	sector_t blocknr;
> >  	char *p;
> > -	unsigned long long blocknr;
> > +	int err = 0;
> > +
> > +	blocknr = 0;
> > +	err = bmap(inode, &blocknr);
> >  
> > -	blocknr = bmap(inode, 0);
> > -	if (!blocknr) {
> > +	if (err || !blocknr) {
> >  		pr_err("%s: Cannot locate journal superblock\n",
> >  			__func__);
> >  		return NULL;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index bf0cad65b9b7..15a79f67ad87 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2762,7 +2762,7 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
> >  extern void emergency_sync(void);
> >  extern void emergency_remount(void);
> >  #ifdef CONFIG_BLOCK
> > -extern sector_t bmap(struct inode *, sector_t);
> > +extern int bmap(struct inode *, sector_t *);
> >  #endif
> >  extern int notify_change(struct dentry *, struct iattr *, struct inode **);
> >  extern int inode_permission(struct inode *, int);
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index aafd19ec1db4..994c996414c3 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -177,8 +177,9 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
> >  
> >  		cond_resched();
> >  
> > -		first_block = bmap(inode, probe_block);
> > -		if (first_block == 0)
> > +		first_block = probe_block;
> > +		ret = bmap(inode, &first_block);
> > +		if (ret || !first_block)
> >  			goto bad_bmap;
> >  
> >  		/*
> > @@ -193,9 +194,11 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
> >  					block_in_page++) {
> >  			sector_t block;
> >  
> > -			block = bmap(inode, probe_block + block_in_page);
> > -			if (block == 0)
> > +			block = probe_block + block_in_page;
> > +			ret = bmap(inode, &block);
> > +			if (ret || !block)
> >  				goto bad_bmap;
> > +
> >  			if (block != first_block + block_in_page) {
> >  				/* Discontiguity */
> >  				probe_block++;
> > -- 
> > 2.14.4
> > 

-- 
Carlos