[Cluster-devel] [PATCH v2 3/3] GFS2: Switch fiemap implementation to use iomap

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 28/10/16 20:29, Bob Peterson wrote:
> This patch switches GFS2's implementation of fiemap from the old
> block_map code to the new iomap interface.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/Kconfig       |  1 +
>   fs/gfs2/inode.c       | 60 ++++++++++++++++++++++++++++++++++++++++-----------
>   include/linux/iomap.h |  1 +
>   3 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/fs/gfs2/Kconfig b/fs/gfs2/Kconfig
> index 90c6a8f..43c827a 100644
> --- a/fs/gfs2/Kconfig
> +++ b/fs/gfs2/Kconfig
> @@ -4,6 +4,7 @@ config GFS2_FS
>   	select FS_POSIX_ACL
>   	select CRC32
>   	select QUOTACTL
> +	select FS_IOMAP
>   	help
>   	  A cluster filesystem.
>   
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index fe3f849..322d178 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -17,7 +17,7 @@
>   #include <linux/posix_acl.h>
>   #include <linux/gfs2_ondisk.h>
>   #include <linux/crc32.h>
> -#include <linux/fiemap.h>
> +#include <linux/iomap.h>
>   #include <linux/security.h>
>   #include <asm/uaccess.h>
>   
> @@ -1994,28 +1994,64 @@ static int gfs2_getattr(struct vfsmount *mnt, struct dentry *dentry,
>   	return 0;
>   }
>   
> +static int gfs2_iomap_fiemap_begin(struct inode *inode, loff_t offset,
> +				   loff_t length, unsigned flags,
> +				   struct iomap *iomap)
> +{
> +	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_holder *gh;
> +	int ret;
> +
> +	gh = kzalloc(sizeof(struct gfs2_holder), GFP_NOFS);
> +	if (!gh)
> +		return -ENOMEM;
> +
> +	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, gh);
> +	if (ret) {
> +		kfree(gh);
> +		return ret;
> +	}
> +	iomap->private = gh;
> +	ret = gfs2_get_iomap(inode, offset, length, iomap);
> +	if (ret)
> +		gfs2_glock_dq_uninit(gh);
> +	return ret;
> +}
> +
> +static int gfs2_iomap_fiemap_end(struct inode *inode, loff_t offset,
> +				 loff_t length, ssize_t written,
> +				 unsigned flags, struct iomap *iomap)
> +{
> +	struct gfs2_holder *gh = iomap->private;
> +
> +	BUG_ON(gh == NULL);
> +	gfs2_glock_dq_uninit(gh);
> +	return 0;
> +}
> +
>   static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>   		       u64 start, u64 len)
>   {
>   	struct gfs2_inode *ip = GFS2_I(inode);
> -	struct gfs2_holder gh;
>   	int ret;
>   
> -	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> -	if (ret)
> -		return ret;
> +	struct iomap_ops gfs2_iomap_fiemap_ops = {
> +		.iomap_begin = gfs2_iomap_fiemap_begin,
> +		.iomap_end = gfs2_iomap_fiemap_end,
> +	};
>   
>   	inode_lock(inode);
>   
> -	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
> -	if (ret)
> -		goto out;
> -
>   	if (gfs2_is_stuffed(ip)) {
> +		struct gfs2_holder gh;
>   		u64 phys = ip->i_no_addr << inode->i_blkbits;
>   		u64 size = i_size_read(inode);
>   		u32 flags = FIEMAP_EXTENT_LAST|FIEMAP_EXTENT_NOT_ALIGNED|
>   			    FIEMAP_EXTENT_DATA_INLINE;
> +		ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
> +		if (ret)
> +			goto out;
> +
>   		phys += sizeof(struct gfs2_dinode);
>   		phys += start;
>   		if (start + len > size)
> @@ -2025,12 +2061,12 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>   						      len, flags);
>   		if (ret == 1)
>   			ret = 0;
> +		gfs2_glock_dq_uninit(&gh);
>   	} else {
> -		ret = __generic_block_fiemap(inode, fieinfo, start, len,
> -					     gfs2_block_map);

> +		ret = iomap_fiemap(inode, fieinfo, start, len,
> +				   &gfs2_iomap_fiemap_ops);
Is there a reason that we don't simply wrap this function with the 
glock? That would simplify the gfs2_iomap_fiemap_ops and reduce locking 
for multi-extent files a lot,

Steve.

>   	}
>   
> -	gfs2_glock_dq_uninit(&gh);
>   out:
>   	inode_unlock(inode);
>   	return ret;
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index e63e288..0f177d3 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -37,6 +37,7 @@ struct iomap {
>   	u16			type;	/* type of mapping */
>   	u16			flags;	/* flags for mapping */
>   	struct block_device	*bdev;	/* block device for I/O */
> +	void			*private; /* private value for fs use */
>   };
>   
>   /*