From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com ([209.85.221.67]:38139 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726414AbeIRLbS (ORCPT ); Tue, 18 Sep 2018 07:31:18 -0400 Received: by mail-wr1-f67.google.com with SMTP id w11-v6so642216wrc.5 for ; Mon, 17 Sep 2018 23:00:17 -0700 (PDT) Date: Tue, 18 Sep 2018 08:00:14 +0200 From: Carlos Maiolino To: "Darrick J. Wong" Cc: linux-fsdevel@vger.kernel.org, hch@lst.de, sandeen@redhat.com, david@fromorbit.com Subject: Re: [PATCH 1/3] fs: Enable bmap() function to properly return errors Message-ID: <20180918060013.6xna6ddkac7fm7hn@odin.usersys.redhat.com> References: <20180912122536.31977-1-cmaiolino@redhat.com> <20180912122536.31977-2-cmaiolino@redhat.com> <20180917205541.GA4591@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180917205541.GA4591@magnolia> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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