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@xxxxxxxxxx>
> ---
>
> 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
>
