| Message ID | 20180905135748.30098-2-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ->bmap interface retirement | expand |
On 9/5/18 8:57 AM, Carlos Maiolino wrote: > Prepare the field to use ->fiemap for FIBMAP ioctl > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/cachefiles/rdwr.c | 5 ++--- > fs/ecryptfs/mmap.c | 5 ++--- > fs/ioctl.c | 14 ++++++++------ > 3 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c > index 40f7595aad10..186e203d64a7 100644 > --- a/fs/cachefiles/rdwr.c > +++ b/fs/cachefiles/rdwr.c > @@ -435,7 +435,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op, > block0 = page->index; > block0 <<= shift; > > - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0); > + block = bmap(inode, block0); Prior to this there's an ASSERT that inode->i_mapping->a_ops->bmap exists. Should that stay, if the goal is to move all ->bmap use out of calling code? OTOH, what will this code do if bmap() finds that there is no ->bmap present and returns 0? > _debug("%llx -> %llx", > (unsigned long long) block0, > (unsigned long long) block); > @@ -737,8 +737,7 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op, > block0 = page->index; > block0 <<= shift; > > - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, > - block0); > + block = bmap(inode, block0); > _debug("%llx -> %llx", > (unsigned long long) block0, > (unsigned long long) block); > diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c > index cdf358b209d9..16626fce5754 100644 > --- a/fs/ecryptfs/mmap.c > +++ b/fs/ecryptfs/mmap.c > @@ -544,9 +544,8 @@ static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block) > > 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); > + > + rc = bmap(lower_inode, block); > return rc; > } > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 3212c29235ce..413585d58415 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -53,19 +53,21 @@ EXPORT_SYMBOL(vfs_ioctl); > > static int ioctl_fibmap(struct file *filp, int __user *p) > { > - struct address_space *mapping = filp->f_mapping; > + struct inode *inode = filp->f_inode; > int res, block; > > - /* do we support this mess? */ > - if (!mapping->a_ops->bmap) > - return -EINVAL; > if (!capable(CAP_SYS_RAWIO)) > return -EPERM; > res = get_user(block, p); > if (res) > return res; > - res = mapping->a_ops->bmap(mapping, block); > - return put_user(res, p); > + > + res = bmap(inode, block); > + > + if (res) > + return put_user(res, p); > + else > + return -EINVAL; So now mapping a hole will return -EINVAL? I don't think that change in behavior is ok. -Eric > } > > /** >
On Wed, Sep 05, 2018 at 09:23:26AM -0500, Eric Sandeen wrote: > > block0 = page->index; > > block0 <<= shift; > > > > - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0); > > + block = bmap(inode, block0); > > Prior to this there's an ASSERT that inode->i_mapping->a_ops->bmap > exists. Should that stay, if the goal is to move all ->bmap use out > of calling code? I think it needs to go away. > OTOH, what will this code do if bmap() finds that there > is no ->bmap present and returns 0? I think we just need to fix the bmap() prototype so that it can return error, which would be very useful for various reasons. Something like this: int bmap(struct address_space *mapping, sector_t *block) { if (!mapping->a_ops->bmap) return -EINVAL; *block = mapping->a_ops->bmap(mapping, *block); return 0; } then add fiemap support with real error handling later.
On Wed, Sep 05, 2018 at 09:23:26AM -0500, Eric Sandeen wrote: > On 9/5/18 8:57 AM, Carlos Maiolino wrote: > > Prepare the field to use ->fiemap for FIBMAP ioctl > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > fs/cachefiles/rdwr.c | 5 ++--- > > fs/ecryptfs/mmap.c | 5 ++--- > > fs/ioctl.c | 14 ++++++++------ > > 3 files changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c > > index 40f7595aad10..186e203d64a7 100644 > > --- a/fs/cachefiles/rdwr.c > > +++ b/fs/cachefiles/rdwr.c > > @@ -435,7 +435,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op, > > block0 = page->index; > > block0 <<= shift; > > > > - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0); > > + block = bmap(inode, block0); > > Prior to this there's an ASSERT that inode->i_mapping->a_ops->bmap > exists. Should that stay, if the goal is to move all ->bmap use out > of calling code? OTOH, what will this code do if bmap() finds that there > is no ->bmap present and returns 0? > > > + res = bmap(inode, block); > > + > > + if (res) > > + return put_user(res, p); > > + else > > + return -EINVAL; > > So now mapping a hole will return -EINVAL? I don't think that change > in behavior is ok. > Thanks for catching it, I messed the return value here. I'm going to think about your 2 points for the next review and work on a real patch for retiring ->bmap once I've got more information now. > -Eric > > > } > > > > /** > > >
On Wed, Sep 05, 2018 at 08:55:39PM +0200, Christoph Hellwig wrote: > On Wed, Sep 05, 2018 at 09:23:26AM -0500, Eric Sandeen wrote: > > > block0 = page->index; > > > block0 <<= shift; > > > > > > - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0); > > > + block = bmap(inode, block0); > > > > Prior to this there's an ASSERT that inode->i_mapping->a_ops->bmap > > exists. Should that stay, if the goal is to move all ->bmap use out > > of calling code? > > I think it needs to go away. /me will keep it in mind > > > OTOH, what will this code do if bmap() finds that there > > is no ->bmap present and returns 0? > > I think we just need to fix the bmap() prototype so that it can return > error, which would be very useful for various reasons. Something like > this: > > int bmap(struct address_space *mapping, sector_t *block) > { > if (!mapping->a_ops->bmap) > return -EINVAL; > *block = mapping->a_ops->bmap(mapping, *block); > return 0; > } > > then add fiemap support with real error handling later. Will do, thanks for the suggestion.
diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c index 40f7595aad10..186e203d64a7 100644 --- a/fs/cachefiles/rdwr.c +++ b/fs/cachefiles/rdwr.c @@ -435,7 +435,7 @@ int cachefiles_read_or_alloc_page(struct fscache_retrieval *op, block0 = page->index; block0 <<= shift; - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, block0); + block = bmap(inode, block0); _debug("%llx -> %llx", (unsigned long long) block0, (unsigned long long) block); @@ -737,8 +737,7 @@ int cachefiles_read_or_alloc_pages(struct fscache_retrieval *op, block0 = page->index; block0 <<= shift; - block = inode->i_mapping->a_ops->bmap(inode->i_mapping, - block0); + block = bmap(inode, block0); _debug("%llx -> %llx", (unsigned long long) block0, (unsigned long long) block); diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c index cdf358b209d9..16626fce5754 100644 --- a/fs/ecryptfs/mmap.c +++ b/fs/ecryptfs/mmap.c @@ -544,9 +544,8 @@ static sector_t ecryptfs_bmap(struct address_space *mapping, sector_t block) 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); + + rc = bmap(lower_inode, block); return rc; } diff --git a/fs/ioctl.c b/fs/ioctl.c index 3212c29235ce..413585d58415 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -53,19 +53,21 @@ EXPORT_SYMBOL(vfs_ioctl); static int ioctl_fibmap(struct file *filp, int __user *p) { - struct address_space *mapping = filp->f_mapping; + struct inode *inode = filp->f_inode; int res, block; - /* do we support this mess? */ - if (!mapping->a_ops->bmap) - return -EINVAL; if (!capable(CAP_SYS_RAWIO)) return -EPERM; res = get_user(block, p); if (res) return res; - res = mapping->a_ops->bmap(mapping, block); - return put_user(res, p); + + res = bmap(inode, block); + + if (res) + return put_user(res, p); + else + return -EINVAL; } /**
Prepare the field to use ->fiemap for FIBMAP ioctl Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/cachefiles/rdwr.c | 5 ++--- fs/ecryptfs/mmap.c | 5 ++--- fs/ioctl.c | 14 ++++++++------ 3 files changed, 12 insertions(+), 12 deletions(-)