| Message ID | 20180912122536.31977-2-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Replace direct ->bmap calls by bmap() with error support | expand |
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>
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>
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 >
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 > >
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++;
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(-)