From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from verein.lst.de ([213.95.11.211]:50707 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728124AbeKQB7h (ORCPT ); Fri, 16 Nov 2018 20:59:37 -0500 Date: Fri, 16 Nov 2018 16:46:42 +0100 From: Christoph Hellwig To: Carlos Maiolino Cc: linux-fsdevel@vger.kernel.org, sandeen@redhat.com, hch@lst.de, david@fromorbit.com, darrick.wong@oracle.com Subject: Re: [PATCH 05/20] fs: Introduce fiemap_ctx data structure Message-ID: <20181116154642.GC15119@lst.de> References: <20181030131823.29040-1-cmaiolino@redhat.com> <20181030131823.29040-6-cmaiolino@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181030131823.29040-6-cmaiolino@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Oct 30, 2018 at 02:18:08PM +0100, Carlos Maiolino wrote: > As the overall goal to deprecate fibmap, Christoph suggested a rework of > the ->fiemap API, in a way we could pass to it a callback to fill the > fiemap structure (one of these callbacks being fiemap_fill_next_extent). > > To avoid the need to add several fields into the ->fiemap method, just > aggregate everything into a single data structure, and pass it along. > > This patch isn't suppose to add any functional change, only to update > filesystems providing ->fiemap() method. > > Signed-off-by: Carlos Maiolino > --- > fs/bad_inode.c | 4 +--- > fs/btrfs/inode.c | 6 ++++-- > fs/ext2/ext2.h | 3 +-- > fs/ext2/inode.c | 6 ++---- > fs/ext4/ext4.h | 3 +-- > fs/ext4/extents.c | 9 +++++---- > fs/f2fs/data.c | 6 ++++-- > fs/f2fs/f2fs.h | 3 +-- > fs/gfs2/inode.c | 6 ++++-- > fs/hpfs/file.c | 4 ++-- > fs/ioctl.c | 23 +++++++++++++++-------- > fs/nilfs2/inode.c | 6 ++++-- > fs/nilfs2/nilfs.h | 3 +-- > fs/ocfs2/extent_map.c | 6 ++++-- > fs/ocfs2/extent_map.h | 3 +-- > fs/overlayfs/inode.c | 6 +++--- > fs/xfs/xfs_iops.c | 9 +++++---- > include/linux/fs.h | 27 +++++++++++++++++++-------- > 18 files changed, 77 insertions(+), 56 deletions(-) > > diff --git a/fs/bad_inode.c b/fs/bad_inode.c > index 8035d2a44561..b5fbe28b8d17 100644 > --- a/fs/bad_inode.c > +++ b/fs/bad_inode.c > @@ -119,9 +119,7 @@ static struct posix_acl *bad_inode_get_acl(struct inode *inode, int type) > return ERR_PTR(-EIO); > } > > -static int bad_inode_fiemap(struct inode *inode, > - struct fiemap_extent_info *fieinfo, u64 start, > - u64 len) > +static int bad_inode_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > return -EIO; > } > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index cd247a21afc4..ebe5aae69c44 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -8612,9 +8612,11 @@ static ssize_t btrfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter) > > #define BTRFS_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC) > > -static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - __u64 start, __u64 len) > +static int btrfs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 len = f_ctx->fc_len; > int ret; > > ret = fiemap_check_flags(fieinfo, BTRFS_FIEMAP_FLAGS); > diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h > index e770cd100a6a..dc5dec52a7c8 100644 > --- a/fs/ext2/ext2.h > +++ b/fs/ext2/ext2.h > @@ -775,8 +775,7 @@ extern void ext2_evict_inode(struct inode *); > extern int ext2_get_block(struct inode *, sector_t, struct buffer_head *, int); > extern int ext2_setattr (struct dentry *, struct iattr *); > extern void ext2_set_inode_flags(struct inode *inode); > -extern int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len); > +extern int ext2_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx); > > /* ioctl.c */ > extern long ext2_ioctl(struct file *, unsigned int, unsigned long); > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index e4bb9386c045..85e7edad58a3 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -855,11 +855,9 @@ const struct iomap_ops ext2_iomap_ops = { > const struct iomap_ops ext2_iomap_ops; > #endif /* CONFIG_FS_DAX */ > > -int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len) > +int ext2_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > - return generic_block_fiemap(inode, fieinfo, start, len, > - ext2_get_block); > + return generic_block_fiemap(inode, f_ctx, ext2_get_block); > } > > static int ext2_writepage(struct page *page, struct writeback_control *wbc) > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 3f89d0ab08fc..acc1ea0e9f40 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -3140,8 +3140,7 @@ extern struct ext4_ext_path *ext4_find_extent(struct inode *, ext4_lblk_t, > extern void ext4_ext_drop_refs(struct ext4_ext_path *); > extern int ext4_ext_check_inode(struct inode *inode); > extern ext4_lblk_t ext4_ext_next_allocated_block(struct ext4_ext_path *path); > -extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - __u64 start, __u64 len); > +extern int ext4_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx); > extern int ext4_ext_precache(struct inode *inode); > extern int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len); > extern int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len); > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 240b6dea5441..beccae768dac 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -5045,9 +5045,11 @@ static int ext4_xattr_fiemap(struct inode *inode, > return (error < 0 ? error : 0); > } > > -int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - __u64 start, __u64 len) > +int ext4_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 len = f_ctx->fc_len; > ext4_lblk_t start_blk; > int error = 0; > > @@ -5069,8 +5071,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > > /* fallback to generic here if not in extents fmt */ > if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) > - return generic_block_fiemap(inode, fieinfo, start, len, > - ext4_get_block); > + return generic_block_fiemap(inode, f_ctx, ext4_get_block); > > if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS)) > return -EBADR; > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 82b6e27a21cc..0f57403feb92 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -1397,9 +1397,11 @@ static int f2fs_xattr_fiemap(struct inode *inode, > return (err < 0 ? err : 0); > } > > -int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len) > +int f2fs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 len = f_ctx->fc_len; > struct buffer_head map_bh; > sector_t start_blk, last_blk; > pgoff_t next_pgofs; > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 4175445be2b0..185bb39dd82c 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -3075,8 +3075,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio); > void __do_map_lock(struct f2fs_sb_info *sbi, int flag, bool lock); > int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, > int create, int flag); > -int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len); > +int f2fs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx); > bool f2fs_should_update_inplace(struct inode *inode, struct f2fs_io_info *fio); > bool f2fs_should_update_outplace(struct inode *inode, struct f2fs_io_info *fio); > void f2fs_invalidate_page(struct page *page, unsigned int offset, > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c > index 648f0ca1ad57..2a4ce713c209 100644 > --- a/fs/gfs2/inode.c > +++ b/fs/gfs2/inode.c > @@ -2004,9 +2004,11 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat, > return 0; > } > > -static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len) > +static int gfs2_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 len = f_ctx->fc_len; > struct gfs2_inode *ip = GFS2_I(inode); > struct gfs2_holder gh; > int ret; > diff --git a/fs/hpfs/file.c b/fs/hpfs/file.c > index 1ecec124e76f..9e31278cbfe1 100644 > --- a/fs/hpfs/file.c > +++ b/fs/hpfs/file.c > @@ -190,9 +190,9 @@ static sector_t _hpfs_bmap(struct address_space *mapping, sector_t block) > return generic_block_bmap(mapping, block, hpfs_get_block); > } > > -static int hpfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, u64 start, u64 len) > +static int hpfs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > - return generic_block_fiemap(inode, fieinfo, start, len, hpfs_get_block); > + return generic_block_fiemap(inode, f_ctx, hpfs_get_block); > } > > const struct address_space_operations hpfs_aops = { > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 2005529af560..ddcc4f9a9cf1 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -181,6 +181,7 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg) > struct fiemap_extent_info fieinfo = { 0, }; > struct inode *inode = file_inode(filp); > struct super_block *sb = inode->i_sb; > + struct fiemap_ctx f_ctx; > u64 len; > int error; > > @@ -210,7 +211,12 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg) > if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC) > filemap_write_and_wait(inode->i_mapping); > > - error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len); > + f_ctx.fc_flags = fieinfo.fi_flags; > + f_ctx.fc_data = &fieinfo; > + f_ctx.fc_start = fiemap.fm_start; > + f_ctx.fc_len = len; > + > + error = inode->i_op->fiemap(inode, &f_ctx); > fiemap.fm_flags = fieinfo.fi_flags; > fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped; > if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap))) > @@ -278,10 +284,12 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk) > * generic_block_fiemap if you want the locking done for you. > */ > > -int __generic_block_fiemap(struct inode *inode, > - struct fiemap_extent_info *fieinfo, loff_t start, > - loff_t len, get_block_t *get_block) > +int __generic_block_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx, > + get_block_t *get_block) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + loff_t start = f_ctx->fc_start; > + loff_t len = f_ctx->fc_len; > struct buffer_head map_bh; > sector_t start_blk, last_blk; > loff_t isize = i_size_read(inode); > @@ -437,13 +445,12 @@ EXPORT_SYMBOL(__generic_block_fiemap); > * the inode's mutex lock. > */ > > -int generic_block_fiemap(struct inode *inode, > - struct fiemap_extent_info *fieinfo, u64 start, > - u64 len, get_block_t *get_block) > +int generic_block_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx, > + get_block_t *get_block) > { > int ret; > inode_lock(inode); > - ret = __generic_block_fiemap(inode, fieinfo, start, len, get_block); > + ret = __generic_block_fiemap(inode, f_ctx, get_block); > inode_unlock(inode); > return ret; > } > diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c > index 671085512e0f..529262f0ab7d 100644 > --- a/fs/nilfs2/inode.c > +++ b/fs/nilfs2/inode.c > @@ -992,9 +992,11 @@ void nilfs_dirty_inode(struct inode *inode, int flags) > nilfs_transaction_commit(inode->i_sb); /* never fails */ > } > > -int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - __u64 start, __u64 len) > +int nilfs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 len = f_ctx->fc_len; > struct the_nilfs *nilfs = inode->i_sb->s_fs_info; > __u64 logical = 0, phys = 0, size = 0; > __u32 flags = 0; > diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h > index a2f247b6a209..b403dae8c258 100644 > --- a/fs/nilfs2/nilfs.h > +++ b/fs/nilfs2/nilfs.h > @@ -276,8 +276,7 @@ extern int nilfs_inode_dirty(struct inode *); > int nilfs_set_file_dirty(struct inode *inode, unsigned int nr_dirty); > extern int __nilfs_mark_inode_dirty(struct inode *, int); > extern void nilfs_dirty_inode(struct inode *, int flags); > -int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - __u64 start, __u64 len); > +int nilfs_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx); > static inline int nilfs_mark_inode_dirty(struct inode *inode) > { > return __nilfs_mark_inode_dirty(inode, I_DIRTY); > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c > index 06cb96462bf9..1944f5659267 100644 > --- a/fs/ocfs2/extent_map.c > +++ b/fs/ocfs2/extent_map.c > @@ -749,9 +749,11 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh, > > #define OCFS2_FIEMAP_FLAGS (FIEMAP_FLAG_SYNC) > > -int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 map_start, u64 map_len) > +int ocfs2_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 map_start = f_ctx->fc_start; > + u64 map_len = f_ctx->fc_len; > int ret, is_last; > u32 mapping_end, cpos; > unsigned int hole_size; > diff --git a/fs/ocfs2/extent_map.h b/fs/ocfs2/extent_map.h > index 1057586ec19f..6c07fdf4d438 100644 > --- a/fs/ocfs2/extent_map.h > +++ b/fs/ocfs2/extent_map.h > @@ -50,8 +50,7 @@ int ocfs2_get_clusters(struct inode *inode, u32 v_cluster, u32 *p_cluster, > int ocfs2_extent_map_get_blocks(struct inode *inode, u64 v_blkno, u64 *p_blkno, > u64 *ret_count, unsigned int *extent_flags); > > -int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 map_start, u64 map_len); > +int ocfs2_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx); > > int ocfs2_overwrite_io(struct inode *inode, struct buffer_head *di_bh, > u64 map_start, u64 map_len); > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 3b7ed5d2279c..e56606c1ba92 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -456,9 +456,9 @@ int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags) > return 0; > } > > -static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > - u64 start, u64 len) > +static int ovl_fiemap(struct inode *inode, struct fiemap_ctx *f_ctx) > { > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > int err; > struct inode *realinode = ovl_inode_real(inode); > const struct cred *old_cred; > @@ -471,7 +471,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, > if (fieinfo->fi_flags & FIEMAP_FLAG_SYNC) > filemap_write_and_wait(realinode->i_mapping); > > - err = realinode->i_op->fiemap(realinode, fieinfo, start, len); > + err = realinode->i_op->fiemap(realinode, f_ctx); > revert_creds(old_cred); > > return err; > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > index f48ffd7a8d3e..d33d56fa8097 100644 > --- a/fs/xfs/xfs_iops.c > +++ b/fs/xfs/xfs_iops.c > @@ -1093,11 +1093,12 @@ xfs_vn_update_time( > STATIC int > xfs_vn_fiemap( > struct inode *inode, > - struct fiemap_extent_info *fieinfo, > - u64 start, > - u64 length) > + struct fiemap_ctx *f_ctx) > { > - int error; > + struct fiemap_extent_info *fieinfo = f_ctx->fc_data; > + u64 start = f_ctx->fc_start; > + u64 length = f_ctx->fc_len; > + int error; > > xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED); > if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) { > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 1ac714e95d6b..bd8e8a7dfd59 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1688,6 +1688,9 @@ extern bool may_open_dev(const struct path *path); > /* > * VFS FS_IOC_FIEMAP helper definitions. > */ > + > +struct fiemap_ctx; > + > struct fiemap_extent_info { > unsigned int fi_flags; /* Flags as passed from user */ > unsigned int fi_extents_mapped; /* Number of mapped extents */ > @@ -1695,6 +1698,18 @@ struct fiemap_extent_info { > struct fiemap_extent __user *fi_extents_start; /* Start of > fiemap_extent array */ > }; > + > +typedef int (*fiemap_fill_cb)(struct fiemap_ctx *f_ctx, u64 logical, > + u64 phys, u64 len, u32 flags); > + > +struct fiemap_ctx { > + unsigned int fc_flags; > + void *fc_data; > + fiemap_fill_cb fc_cb; /* Unused by now */ Please only add field when you actually start using them. Also I much prefer aligning the names of the struct members to ease readability, e.g.: struct fiemap_ctx { unsigned int fc_flags; u64 fc_start; u64 fc_len; }; Otherwise this looks fine to me: Reviewed-by: Christoph Hellwig