| Message ID | 20190911134315.27380-10-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New ->fiemap infrastructure and ->bmap removal | expand |
On Wed, Sep 11, 2019 at 03:43:15PM +0200, Carlos Maiolino wrote: > We don't need ->bmap anymore, only usage for it was FIBMAP, which is now > gone. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > Changelog: > V5: > - Properly rebase against 5.3 > - iomap_{bmap(),bmap_actor()} are now used also by GFS2, so > don't remove them anymore > V2: > - Kill iomap_bmap() and iomap_bmap_actor() > > fs/xfs/xfs_aops.c | 24 ------------------------ > fs/xfs/xfs_trace.h | 1 - > 2 files changed, 25 deletions(-) > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index 4e4a4d7df5ac..a2884537d2c2 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -1138,29 +1138,6 @@ xfs_vm_releasepage( > return iomap_releasepage(page, gfp_mask); > } > > -STATIC sector_t > -xfs_vm_bmap( > - struct address_space *mapping, > - sector_t block) > -{ > - struct xfs_inode *ip = XFS_I(mapping->host); > - > - trace_xfs_vm_bmap(ip); > - > - /* > - * The swap code (ab-)uses ->bmap to get a block mapping and then > - * bypasses the file system for actual I/O. We really can't allow > - * that on reflinks inodes, so we have to skip out here. And yes, > - * 0 is the magic code for a bmap error. > - * > - * Since we don't pass back blockdev info, we can't return bmap > - * information for rt files either. > - */ > - if (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip)) Uhhhh where does this check happen now? --D > - return 0; > - return iomap_bmap(mapping, block, &xfs_iomap_ops); > -} > - > STATIC int > xfs_vm_readpage( > struct file *unused, > @@ -1199,7 +1176,6 @@ const struct address_space_operations xfs_address_space_operations = { > .set_page_dirty = iomap_set_page_dirty, > .releasepage = xfs_vm_releasepage, > .invalidatepage = xfs_vm_invalidatepage, > - .bmap = xfs_vm_bmap, > .direct_IO = noop_direct_IO, > .migratepage = iomap_migrate_page, > .is_partially_uptodate = iomap_is_partially_uptodate, > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > index eaae275ed430..c226b562f5da 100644 > --- a/fs/xfs/xfs_trace.h > +++ b/fs/xfs/xfs_trace.h > @@ -626,7 +626,6 @@ DEFINE_INODE_EVENT(xfs_readdir); > #ifdef CONFIG_XFS_POSIX_ACL > DEFINE_INODE_EVENT(xfs_get_acl); > #endif > -DEFINE_INODE_EVENT(xfs_vm_bmap); > DEFINE_INODE_EVENT(xfs_file_ioctl); > DEFINE_INODE_EVENT(xfs_file_compat_ioctl); > DEFINE_INODE_EVENT(xfs_ioctl_setattr); > -- > 2.20.1 >
On Mon, Sep 16, 2019 at 10:50:49AM -0700, Darrick J. Wong wrote: > On Wed, Sep 11, 2019 at 03:43:15PM +0200, Carlos Maiolino wrote: > > We don't need ->bmap anymore, only usage for it was FIBMAP, which is now > > gone. > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > > > Changelog: > > V5: > > - Properly rebase against 5.3 > > - iomap_{bmap(),bmap_actor()} are now used also by GFS2, so > > don't remove them anymore > > V2: > > - Kill iomap_bmap() and iomap_bmap_actor() > > > > fs/xfs/xfs_aops.c | 24 ------------------------ > > fs/xfs/xfs_trace.h | 1 - > > 2 files changed, 25 deletions(-) > > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > > index 4e4a4d7df5ac..a2884537d2c2 100644 > > --- a/fs/xfs/xfs_aops.c > > +++ b/fs/xfs/xfs_aops.c > > @@ -1138,29 +1138,6 @@ xfs_vm_releasepage( > > return iomap_releasepage(page, gfp_mask); > > } > > > > -STATIC sector_t > > -xfs_vm_bmap( > > - struct address_space *mapping, > > - sector_t block) > > -{ > > - struct xfs_inode *ip = XFS_I(mapping->host); > > - > > - trace_xfs_vm_bmap(ip); > > - > > - /* > > - * The swap code (ab-)uses ->bmap to get a block mapping and then > > - * bypasses the file system for actual I/O. We really can't allow > > - * that on reflinks inodes, so we have to skip out here. And yes, > > - * 0 is the magic code for a bmap error. > > - * > > - * Since we don't pass back blockdev info, we can't return bmap > > - * information for rt files either. > > - */ > > - if (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip)) > > Uhhhh where does this check happen now? All checks are now made in the caller, bmap_fiemap() based on the filesystem's returned flags in the fiemap structure. So, it will decide to pass the result back, or just return -EINVAL. Well, there is no way for iomap (or bmap_fiemap now) detect the block is in a realtime device, since we have no flags for that. Following Christoph's line of thought here, maybe we can add a new IOMAP_F_* so the filesystem can notify iomap the extent is in a different device? I don't know, just a thought. This would still keep the consistency of leaving bmap_fiemap() with the decision of passing or not. Cheers. > > --D > > > - return 0; > > - return iomap_bmap(mapping, block, &xfs_iomap_ops); > > -} > > - > > STATIC int > > xfs_vm_readpage( > > struct file *unused, > > @@ -1199,7 +1176,6 @@ const struct address_space_operations xfs_address_space_operations = { > > .set_page_dirty = iomap_set_page_dirty, > > .releasepage = xfs_vm_releasepage, > > .invalidatepage = xfs_vm_invalidatepage, > > - .bmap = xfs_vm_bmap, > > .direct_IO = noop_direct_IO, > > .migratepage = iomap_migrate_page, > > .is_partially_uptodate = iomap_is_partially_uptodate, > > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > > index eaae275ed430..c226b562f5da 100644 > > --- a/fs/xfs/xfs_trace.h > > +++ b/fs/xfs/xfs_trace.h > > @@ -626,7 +626,6 @@ DEFINE_INODE_EVENT(xfs_readdir); > > #ifdef CONFIG_XFS_POSIX_ACL > > DEFINE_INODE_EVENT(xfs_get_acl); > > #endif > > -DEFINE_INODE_EVENT(xfs_vm_bmap); > > DEFINE_INODE_EVENT(xfs_file_ioctl); > > DEFINE_INODE_EVENT(xfs_file_compat_ioctl); > > DEFINE_INODE_EVENT(xfs_ioctl_setattr); > > -- > > 2.20.1 > >
On Wed, Sep 18, 2019 at 10:13:04AM +0200, Carlos Maiolino wrote: > All checks are now made in the caller, bmap_fiemap() based on the filesystem's > returned flags in the fiemap structure. So, it will decide to pass the result > back, or just return -EINVAL. > > Well, there is no way for iomap (or bmap_fiemap now) detect the block is in a > realtime device, since we have no flags for that. > > Following Christoph's line of thought here, maybe we can add a new IOMAP_F_* so > the filesystem can notify iomap the extent is in a different device? I don't > know, just a thought. > > This would still keep the consistency of leaving bmap_fiemap() with the decision > of passing or not. I think this actually is a problem with FIEMAP as well, as it doesn't report that things are on a different device. So I guess for now we should fail FIEMAP on the RT device as well.
On Wed, Sep 18, 2019 at 03:24:36PM +0200, Christoph Hellwig wrote: > On Wed, Sep 18, 2019 at 10:13:04AM +0200, Carlos Maiolino wrote: > > All checks are now made in the caller, bmap_fiemap() based on the filesystem's > > returned flags in the fiemap structure. So, it will decide to pass the result > > back, or just return -EINVAL. > > > > Well, there is no way for iomap (or bmap_fiemap now) detect the block is in a > > realtime device, since we have no flags for that. > > > > Following Christoph's line of thought here, maybe we can add a new IOMAP_F_* so > > the filesystem can notify iomap the extent is in a different device? I don't > > know, just a thought. > > > > This would still keep the consistency of leaving bmap_fiemap() with the decision > > of passing or not. > > I think this actually is a problem with FIEMAP as well, as it > doesn't report that things are on a different device. So I guess for > now we should fail FIEMAP on the RT device as well. Or enhance FIEMAP to report some kind of device id like I suggested a while back... --D
On Wed, Sep 18, 2019 at 09:12:41AM -0700, Darrick J. Wong wrote: > On Wed, Sep 18, 2019 at 03:24:36PM +0200, Christoph Hellwig wrote: > > On Wed, Sep 18, 2019 at 10:13:04AM +0200, Carlos Maiolino wrote: > > > All checks are now made in the caller, bmap_fiemap() based on the filesystem's > > > returned flags in the fiemap structure. So, it will decide to pass the result > > > back, or just return -EINVAL. > > > > > > Well, there is no way for iomap (or bmap_fiemap now) detect the block is in a > > > realtime device, since we have no flags for that. > > > > > > Following Christoph's line of thought here, maybe we can add a new IOMAP_F_* so > > > the filesystem can notify iomap the extent is in a different device? I don't > > > know, just a thought. > > > > > > This would still keep the consistency of leaving bmap_fiemap() with the decision > > > of passing or not. > > > > I think this actually is a problem with FIEMAP as well, as it > > doesn't report that things are on a different device. So I guess for > > now we should fail FIEMAP on the RT device as well. > > Or enhance FIEMAP to report some kind of device id like I suggested a > while back... Yes, this is in my todo list Darrick, but as agreed previously, this will be in a different patchset. It simply does not belong here and will make this patchset much more complex than it should be. About this patch itself, there isn't much I can do here, and I think a XFS fix to make it reject FIEMAP for RT devices as Christoph suggested, belongs to a xfs-only patch, not to this one. I can do that too, but on a different patch, changing FS semantics simply does not belong in this patchset. Cheers. > > --D
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 4e4a4d7df5ac..a2884537d2c2 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -1138,29 +1138,6 @@ xfs_vm_releasepage( return iomap_releasepage(page, gfp_mask); } -STATIC sector_t -xfs_vm_bmap( - struct address_space *mapping, - sector_t block) -{ - struct xfs_inode *ip = XFS_I(mapping->host); - - trace_xfs_vm_bmap(ip); - - /* - * The swap code (ab-)uses ->bmap to get a block mapping and then - * bypasses the file system for actual I/O. We really can't allow - * that on reflinks inodes, so we have to skip out here. And yes, - * 0 is the magic code for a bmap error. - * - * Since we don't pass back blockdev info, we can't return bmap - * information for rt files either. - */ - if (xfs_is_cow_inode(ip) || XFS_IS_REALTIME_INODE(ip)) - return 0; - return iomap_bmap(mapping, block, &xfs_iomap_ops); -} - STATIC int xfs_vm_readpage( struct file *unused, @@ -1199,7 +1176,6 @@ const struct address_space_operations xfs_address_space_operations = { .set_page_dirty = iomap_set_page_dirty, .releasepage = xfs_vm_releasepage, .invalidatepage = xfs_vm_invalidatepage, - .bmap = xfs_vm_bmap, .direct_IO = noop_direct_IO, .migratepage = iomap_migrate_page, .is_partially_uptodate = iomap_is_partially_uptodate, diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index eaae275ed430..c226b562f5da 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -626,7 +626,6 @@ DEFINE_INODE_EVENT(xfs_readdir); #ifdef CONFIG_XFS_POSIX_ACL DEFINE_INODE_EVENT(xfs_get_acl); #endif -DEFINE_INODE_EVENT(xfs_vm_bmap); DEFINE_INODE_EVENT(xfs_file_ioctl); DEFINE_INODE_EVENT(xfs_file_compat_ioctl); DEFINE_INODE_EVENT(xfs_ioctl_setattr);
We don't need ->bmap anymore, only usage for it was FIBMAP, which is now gone. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- Changelog: V5: - Properly rebase against 5.3 - iomap_{bmap(),bmap_actor()} are now used also by GFS2, so don't remove them anymore V2: - Kill iomap_bmap() and iomap_bmap_actor() fs/xfs/xfs_aops.c | 24 ------------------------ fs/xfs/xfs_trace.h | 1 - 2 files changed, 25 deletions(-)