* [PATCH 0/3] Replace direct ->bmap calls by bmap() with error support @ 2018-09-12 12:25 Carlos Maiolino 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-12 12:25 UTC (permalink / raw) To: linux-fsdevel; +Cc: hch, sandeen, david 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] fs: Enable bmap() function to properly return errors 2018-09-12 12:25 [PATCH 0/3] Replace direct ->bmap calls by bmap() with error support Carlos Maiolino @ 2018-09-12 12:25 ` Carlos Maiolino 2018-09-14 13:23 ` Christoph Hellwig 2018-09-17 20:55 ` Darrick J. Wong 2018-09-12 12:25 ` [PATCH 2/3] cachefiles: drop direct usage of ->bmap method Carlos Maiolino 2018-09-12 12:25 ` [PATCH 3/3] ecryptfs: drop direct calls to ->bmap Carlos Maiolino 2 siblings, 2 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-12 12:25 UTC (permalink / raw) To: linux-fsdevel; +Cc: hch, sandeen, david 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino @ 2018-09-14 13:23 ` Christoph Hellwig 2018-09-14 18:48 ` Carlos Maiolino 2018-09-17 20:55 ` Darrick J. Wong 1 sibling, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2018-09-14 13:23 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-fsdevel, hch, sandeen, david 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors 2018-09-14 13:23 ` Christoph Hellwig @ 2018-09-14 18:48 ` Carlos Maiolino 0 siblings, 0 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-14 18:48 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-fsdevel, sandeen, david 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino 2018-09-14 13:23 ` Christoph Hellwig @ 2018-09-17 20:55 ` Darrick J. Wong 2018-09-18 6:00 ` Carlos Maiolino 1 sibling, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2018-09-17 20:55 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-fsdevel, hch, sandeen, david 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors 2018-09-17 20:55 ` Darrick J. Wong @ 2018-09-18 6:00 ` Carlos Maiolino 0 siblings, 0 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-18 6:00 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-fsdevel, hch, sandeen, david 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] cachefiles: drop direct usage of ->bmap method. 2018-09-12 12:25 [PATCH 0/3] Replace direct ->bmap calls by bmap() with error support Carlos Maiolino 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino @ 2018-09-12 12:25 ` Carlos Maiolino 2018-09-14 13:23 ` Christoph Hellwig 2018-09-12 12:25 ` [PATCH 3/3] ecryptfs: drop direct calls to ->bmap Carlos Maiolino 2 siblings, 1 reply; 13+ messages in thread From: Carlos Maiolino @ 2018-09-12 12:25 UTC (permalink / raw) To: linux-fsdevel; +Cc: hch, sandeen, david 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] cachefiles: drop direct usage of ->bmap method. 2018-09-12 12:25 ` [PATCH 2/3] cachefiles: drop direct usage of ->bmap method Carlos Maiolino @ 2018-09-14 13:23 ` Christoph Hellwig 2018-09-14 18:56 ` Carlos Maiolino 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2018-09-14 13:23 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-fsdevel, hch, sandeen, david 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] cachefiles: drop direct usage of ->bmap method. 2018-09-14 13:23 ` Christoph Hellwig @ 2018-09-14 18:56 ` Carlos Maiolino 0 siblings, 0 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-14 18:56 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-fsdevel, sandeen, david 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] ecryptfs: drop direct calls to ->bmap 2018-09-12 12:25 [PATCH 0/3] Replace direct ->bmap calls by bmap() with error support Carlos Maiolino 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino 2018-09-12 12:25 ` [PATCH 2/3] cachefiles: drop direct usage of ->bmap method Carlos Maiolino @ 2018-09-12 12:25 ` Carlos Maiolino 2018-09-14 13:26 ` Christoph Hellwig 2018-09-17 11:04 ` [PATCH 3/3 V2] " Carlos Maiolino 2 siblings, 2 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-12 12:25 UTC (permalink / raw) To: linux-fsdevel; +Cc: hch, sandeen, david 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] ecryptfs: drop direct calls to ->bmap 2018-09-12 12:25 ` [PATCH 3/3] ecryptfs: drop direct calls to ->bmap Carlos Maiolino @ 2018-09-14 13:26 ` Christoph Hellwig 2018-09-14 18:47 ` Carlos Maiolino 2018-09-17 11:04 ` [PATCH 3/3 V2] " Carlos Maiolino 1 sibling, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2018-09-14 13:26 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-fsdevel, hch, sandeen, david 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] ecryptfs: drop direct calls to ->bmap 2018-09-14 13:26 ` Christoph Hellwig @ 2018-09-14 18:47 ` Carlos Maiolino 0 siblings, 0 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-14 18:47 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-fsdevel, sandeen, david 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3 V2] ecryptfs: drop direct calls to ->bmap 2018-09-12 12:25 ` [PATCH 3/3] ecryptfs: drop direct calls to ->bmap Carlos Maiolino 2018-09-14 13:26 ` Christoph Hellwig @ 2018-09-17 11:04 ` Carlos Maiolino 1 sibling, 0 replies; 13+ messages in thread From: Carlos Maiolino @ 2018-09-17 11:04 UTC (permalink / raw) To: linux-fsdevel; +Cc: hch, sandeen, david 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-09-18 11:31 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-12 12:25 [PATCH 0/3] Replace direct ->bmap calls by bmap() with error support Carlos Maiolino 2018-09-12 12:25 ` [PATCH 1/3] fs: Enable bmap() function to properly return errors Carlos Maiolino 2018-09-14 13:23 ` Christoph Hellwig 2018-09-14 18:48 ` Carlos Maiolino 2018-09-17 20:55 ` Darrick J. Wong 2018-09-18 6:00 ` Carlos Maiolino 2018-09-12 12:25 ` [PATCH 2/3] cachefiles: drop direct usage of ->bmap method Carlos Maiolino 2018-09-14 13:23 ` Christoph Hellwig 2018-09-14 18:56 ` Carlos Maiolino 2018-09-12 12:25 ` [PATCH 3/3] ecryptfs: drop direct calls to ->bmap Carlos Maiolino 2018-09-14 13:26 ` Christoph Hellwig 2018-09-14 18:47 ` Carlos Maiolino 2018-09-17 11:04 ` [PATCH 3/3 V2] " Carlos Maiolino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).