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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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
>