From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:33222 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726189AbeIESxw (ORCPT ); Wed, 5 Sep 2018 14:53:52 -0400 Subject: Re: [PATCH 1/2] fs: Replace direct ->bmap calls by bmap() To: Carlos Maiolino , linux-fsdevel@vger.kernel.org Cc: hch@lst.de, david@fromorbit.com References: <20180905135748.30098-1-cmaiolino@redhat.com> <20180905135748.30098-2-cmaiolino@redhat.com> From: Eric Sandeen Message-ID: Date: Wed, 5 Sep 2018 09:23:26 -0500 MIME-Version: 1.0 In-Reply-To: <20180905135748.30098-2-cmaiolino@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 9/5/18 8:57 AM, Carlos Maiolino wrote: > Prepare the field to use ->fiemap for FIBMAP ioctl > > Signed-off-by: Carlos Maiolino > --- > 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 > } > > /** >