[vfs PATCH v3 0/4] vfs: Expand iomap interface to fiemap

[vfs PATCH v3 0/4] vfs: Expand iomap interface to fiemap

[Bob Peterson]

Patch set history:
------------------
o Back in October 2014, I proposed an improvement to fiemap to accomodate
  very large sparse files. This was done because the existing blockmap-based
  code skips holes one block at a time, which is extremely slow when it comes
  to large holes. For example, doing fiemap on a 1PB file whose only byte
  exists at EOF can take an extremely long time. To recreate:

dd if=/dev/zero of=/mnt/holey-P bs=1 count=1 seek=1P
time filefrag -v /mnt/holey-P

  At that time, someone suggested I use the iomap interface proposed by
  Dave Chinner a long time ago, and so my patches never got off the ground
  (until now).

  Since then, Christoph apparently added the basics of iomap to exportfs.h.
  which today is minimal, and limited to exportfs users.

o On 11 January 2016, I posted a vfs patch that expanded the use of the iomap
  infrastructure, but iomap was added to address_space_operations.

o On 21 January 2016, Jan Kara suggested it NOT be included as an a_op,
  but rather, to have the new iomap function be passed in, similar to
  get_blocks(), along with some pretty generic flags, rather than a set of
  commands.

o on 03 March, I posted a v2 patch set to implement Jan's suggestions.

o on 04 March, Christoph made some suggestions. This v3 patch set is an
  attempt to implement those suggestions.

Patch description:
------------------
The first patch only moves the iomap declares from exportfs.h to its own
iomap.h.

The second patch declares a new iomap_get_t function, similar to get_block_t,
to be used to get iomap information, much like get_block_t is used to get
block information. It adds a new __generic_iomap_fiemap interface to be used
by file systems who choose to use it. My original version of this function
looked nearly identical to the blockmap version, but this one is smaller and
simpler. So if people don't like my implementation, I can revert to that one.

The third patch introduces a new __gfs2_io_map function to the GFS2 file
system.

The fourth patch hooks the new function into GFS2's fiemap to take
advantage of the new code.

The result: for a 1PB sparse file, filefrag takes milliseconds rather
than a week.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
Bob Peterson (4):
  VFS: move iomap from exportfs.h to iomap.h
  VFS: Add new __generic_iomap_fiemap interface
  GFS2: Add function gfs2_get_iomap
  GFS2: Use new iomap interface for fiemap

 fs/gfs2/bmap.c           | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/gfs2/bmap.h           |   2 +
 fs/gfs2/inode.c          |   4 +-
 fs/ioctl.c               |  96 ++++++++++++++++++++++++++++++
 include/linux/exportfs.h |  16 +----
 include/linux/fs.h       |  11 +++-
 include/linux/iomap.h    |  25 ++++++++
 7 files changed, 287 insertions(+), 18 deletions(-)
 create mode 100644 include/linux/iomap.h

-- 
2.5.0

[vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Bob Peterson]

This patch moves the iomap declares from its current location in
exportfs.h to a new iomap.h. This will facilitate future improvements
such as a more efficient fiemap for holey files. Hopefully it will
one day be used for multipage writes as well.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 include/linux/exportfs.h | 16 +---------------
 include/linux/iomap.h    | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 15 deletions(-)
 create mode 100644 include/linux/iomap.h

diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index fa05e04..bb564c1 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -2,6 +2,7 @@
 #define LINUX_EXPORTFS_H 1
 
 #include <linux/types.h>
+#include <linux/iomap.h>
 
 struct dentry;
 struct iattr;
@@ -181,21 +182,6 @@ struct fid {
  *    get_name is not (which is possibly inconsistent)
  */
 
-/* types of block ranges for multipage write mappings. */
-#define IOMAP_HOLE	0x01	/* no blocks allocated, need allocation */
-#define IOMAP_DELALLOC	0x02	/* delayed allocation blocks */
-#define IOMAP_MAPPED	0x03	/* blocks allocated @blkno */
-#define IOMAP_UNWRITTEN	0x04	/* blocks allocated @blkno in unwritten state */
-
-#define IOMAP_NULL_BLOCK -1LL	/* blkno is not valid */
-
-struct iomap {
-	sector_t	blkno;	/* first sector of mapping */
-	loff_t		offset;	/* file offset of mapping, bytes */
-	u64		length;	/* length of mapping, bytes */
-	int		type;	/* type of mapping */
-};
-
 struct export_operations {
 	int (*encode_fh)(struct inode *inode, __u32 *fh, int *max_len,
 			struct inode *parent);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
new file mode 100644
index 0000000..161c1c9
--- /dev/null
+++ b/include/linux/iomap.h
@@ -0,0 +1,20 @@
+#ifndef _IOMAP_H
+#define _IOMAP_H
+
+/* types of block ranges for multipage write mappings. */
+#define IOMAP_HOLE	0x01	/* no blocks allocated, need allocation */
+#define IOMAP_DELALLOC	0x02	/* delayed allocation blocks */
+#define IOMAP_MAPPED	0x03	/* blocks allocated @blkno */
+#define IOMAP_UNWRITTEN	0x04	/* blocks allocated @blkno in unwritten state */
+
+#define IOMAP_NULL_BLOCK -1LL	/* blkno is not valid */
+
+struct iomap {
+	sector_t	blkno;	/* first sector of mapping */
+	loff_t		offset;	/* file offset of mapping, bytes */
+	ssize_t		length;	/* length of mapping, bytes */
+	int		type;	/* type of mapping */
+	void		*priv;	/* fs private data associated with map */
+};
+
+#endif /* _IOMAP_H */
-- 
2.5.0

[vfs PATCH v3 2/4] VFS: Add new __generic_iomap_fiemap interface

[Bob Peterson]

This patch adds a new function __generic_iomap_fiemap similar to
__generic_block_fiemap, but it uses the new iomap interface.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/ioctl.c            | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h    | 11 +++++-
 include/linux/iomap.h |  5 +++
 3 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 29466c3..04b3799 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -15,6 +15,8 @@
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
+#include <linux/iomap.h>
+
 #include "internal.h"
 
 #include <asm/ioctls.h>
@@ -251,6 +253,100 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
 }
 
 /**
+ * __generic_iomap_fiemap - FIEMAP for iomap based inodes (no locking)
+ * @inode: the inode to map
+ * @fieinfo: the fiemap info struct that will be passed back to userspace
+ * @start: where to start mapping in the inode
+ * @len: how much space to map
+ *
+ * This does FIEMAP for iomap based inodes.  Basically it will just loop
+ * through iomap until we hit the number of extents we want to map, or we
+ * go past the end of the file and hit a hole.
+ *
+ * If it is possible to have data blocks beyond a hole past @inode->i_size, then
+ * please do not use this function, it will stop at the first unmapped block
+ * beyond i_size.
+ *
+ * If you use this function directly, you need to do your own locking. Use
+ * generic_iomap_fiemap if you want the locking done for you.
+ */
+
+int __generic_iomap_fiemap(struct inode *inode,
+			   struct fiemap_extent_info *fieinfo, loff_t start,
+			   loff_t len, iomap_get_t iomap)
+{
+	struct iomap iom, prev_iom;
+	loff_t isize = i_size_read(inode);
+	int ret = 0;
+
+	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+	memset(&prev_iom, 0, sizeof(prev_iom));
+	if (len >= isize)
+		len = isize;
+
+	while ((ret == 0) && (start < isize) && len) {
+		memset(&iom, 0, sizeof(iom));
+		ret = iomap(inode->i_mapping, start, len, &iom);
+		if (ret)
+			break;
+
+		if (!iomap_needs_allocation(&iom)) {
+			if (prev_iom.blkno)
+				ret = fiemap_fill_next_extent(fieinfo,
+							      prev_iom.offset,
+							      blk_to_logical(inode,
+							      prev_iom.blkno),
+							      prev_iom.length,
+							      FIEMAP_EXTENT_MERGED);
+			prev_iom = iom;
+		}
+		start += iom.length;
+		if (len < iom.length)
+			break;
+		len -= iom.length;
+		cond_resched();
+	}
+
+	if (prev_iom.blkno)
+		ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset,
+					      blk_to_logical(inode,
+							     prev_iom.blkno),
+					      prev_iom.length,
+					      FIEMAP_EXTENT_MERGED |
+					      FIEMAP_EXTENT_LAST);
+	/* If ret is 1 then we just hit the end of the extent array */
+	if (ret == 1)
+		ret = 0;
+
+	return ret;
+}
+EXPORT_SYMBOL(__generic_iomap_fiemap);
+
+/**
+ * generic_iomap_fiemap - FIEMAP for block based inodes
+ * @inode: The inode to map
+ * @fieinfo: The mapping information
+ * @start: The initial block to map
+ * @len: The length of the extect to attempt to map
+ * @get_block: The block mapping function for the fs
+ *
+ * Calls __generic_block_fiemap to map the inode, after taking
+ * the inode's mutex lock.
+ */
+
+int generic_iomap_fiemap(struct inode *inode,
+			 struct fiemap_extent_info *fieinfo, u64 start,
+			 u64 len, iomap_get_t iomap)
+{
+	int ret;
+	mutex_lock(&inode->i_mutex);
+	ret = __generic_iomap_fiemap(inode, fieinfo, start, len, iomap);
+	mutex_unlock(&inode->i_mutex);
+	return ret;
+}
+EXPORT_SYMBOL(generic_iomap_fiemap);
+
+/**
  * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
  * @inode: the inode to map
  * @fieinfo: the fiemap info struct that will be passed back to userspace
diff --git a/include/linux/fs.h b/include/linux/fs.h
index daf399d..c253d94 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -68,8 +68,12 @@ extern int sysctl_protected_symlinks;
 extern int sysctl_protected_hardlinks;
 
 struct buffer_head;
+struct address_space;
+struct iomap;
 typedef int (get_block_t)(struct inode *inode, sector_t iblock,
 			struct buffer_head *bh_result, int create);
+typedef int (iomap_get_t)(struct address_space *mapping, loff_t pos,
+			  ssize_t length, struct iomap *iomap);
 typedef void (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 			ssize_t bytes, void *private);
 typedef void (dax_iodone_t)(struct buffer_head *bh_map, int uptodate);
@@ -314,7 +318,6 @@ enum positive_aop_returns {
  * oh the beauties of C type declarations.
  */
 struct page;
-struct address_space;
 struct writeback_control;
 
 #define IOCB_EVENTFD		(1 << 0)
@@ -2777,6 +2780,12 @@ extern int vfs_lstat(const char __user *, struct kstat *);
 extern int vfs_fstat(unsigned int, struct kstat *);
 extern int vfs_fstatat(int , const char __user *, struct kstat *, int);
 
+extern int __generic_iomap_fiemap(struct inode *inode,
+				  struct fiemap_extent_info *fieinfo,
+				  loff_t start, loff_t len, iomap_get_t iomap);
+extern int generic_iomap_fiemap(struct inode *inode,
+				struct fiemap_extent_info *fieinfo, u64 start,
+				u64 len, iomap_get_t iomap);
 extern int __generic_block_fiemap(struct inode *inode,
 				  struct fiemap_extent_info *fieinfo,
 				  loff_t start, loff_t len,
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 161c1c9..05a0645 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -17,4 +17,9 @@ struct iomap {
 	void		*priv;	/* fs private data associated with map */
 };
 
+static inline bool iomap_needs_allocation(struct iomap *iomap)
+{
+	return iomap->type == IOMAP_HOLE;
+}
+
 #endif /* _IOMAP_H */
-- 
2.5.0

[vfs PATCH v3 3/4] GFS2: Add function gfs2_get_iomap

[Bob Peterson]

This patch adds a generic iomap_get_t interface to GFS2 for
block mapping.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/bmap.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/gfs2/bmap.h |   2 +
 2 files changed, 153 insertions(+)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 61296ec..5636257 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -13,6 +13,7 @@
 #include <linux/blkdev.h>
 #include <linux/gfs2_ondisk.h>
 #include <linux/crc32.h>
+#include <linux/iomap.h>
 
 #include "gfs2.h"
 #include "incore.h"
@@ -587,6 +588,156 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
 }
 
 /**
+ * hole_size - figure out the size of a hole
+ * @ip: The inode
+ * @lblock: The logical starting block number
+ * @mp: The metapath
+ *
+ * Returns: The hole size in bytes
+ *
+ */
+static u64 hole_size(struct inode *inode, sector_t lblock, struct metapath *mp)
+{
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+	struct metapath mp_eof;
+	unsigned int end_of_metadata = ip->i_height - 1;
+	u64 factor = 1;
+	int hgt = end_of_metadata;
+	u64 holesz = 0, holestep;
+	const __be64 *first, *end, *ptr;
+	const struct buffer_head *bh;
+	u64 isize = i_size_read(inode);
+	int zeroptrs;
+	bool done = false;
+
+	/* Get another metapath, to the very last byte */
+	find_metapath(sdp, (isize - 1) >> inode->i_blkbits, &mp_eof,
+		      ip->i_height);
+	for (hgt = end_of_metadata; hgt >= 0 && !done; hgt--) {
+		bh = mp->mp_bh[hgt];
+		if (bh) {
+			zeroptrs = 0;
+			first = metapointer(hgt, mp);
+			end = (const __be64 *)(bh->b_data + bh->b_size);
+
+			for (ptr = first; ptr < end; ptr++) {
+				if (*ptr) {
+					done = true;
+					break;
+				} else {
+					zeroptrs++;
+				}
+			}
+		} else {
+			zeroptrs = sdp->sd_inptrs;
+		}
+		holestep = min(factor * zeroptrs,
+			       isize - (lblock + (zeroptrs * holesz)));
+		holesz += holestep;
+		if (lblock + holesz >= isize)
+			return holesz << inode->i_blkbits;
+
+		factor *= sdp->sd_inptrs;
+		if (hgt && (mp->mp_list[hgt - 1] < mp_eof.mp_list[hgt - 1]))
+			(mp->mp_list[hgt - 1])++;
+	}
+	return holesz << inode->i_blkbits;
+}
+
+/**
+ * gfs2_get_iomap - Map blocks from an inode to disk blocks
+ * @mapping: The address space
+ * @pos: Starting position in bytes
+ * @length: Length to map, in bytes
+ * @iomap: The iomap structure
+ *
+ * Returns: errno
+ */
+
+int gfs2_get_iomap(struct address_space *mapping, loff_t pos, ssize_t length,
+		   struct iomap *iomap)
+{
+	struct inode *inode = mapping->host;
+	struct gfs2_inode *ip = GFS2_I(inode);
+	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
+	unsigned int bsize = sdp->sd_sb.sb_bsize;
+	const u64 *arr = sdp->sd_heightsize;
+	__be64 *ptr;
+	sector_t lblock = pos >> sdp->sd_sb.sb_bsize_shift;
+	u64 size;
+	struct metapath mp;
+	int ret, eob;
+	unsigned int len;
+	struct buffer_head *bh;
+	u8 height;
+	loff_t isize = i_size_read(inode);
+
+	if (length == 0)
+		return -EINVAL;
+
+	iomap->offset = pos;
+	iomap->blkno = 0;
+	iomap->type = IOMAP_HOLE;
+	iomap->length = length;
+
+	if (pos >= isize)
+		return 0;
+
+	memset(mp.mp_bh, 0, sizeof(mp.mp_bh));
+	bmap_lock(ip, 0);
+	if (gfs2_is_dir(ip)) {
+		bsize = sdp->sd_jbsize;
+		arr = sdp->sd_jheightsize;
+	}
+
+	ret = gfs2_meta_inode_buffer(ip, &mp.mp_bh[0]);
+	if (ret)
+		goto out_release;
+
+	height = ip->i_height;
+	size = (lblock + 1) * bsize;
+	while (size > arr[height])
+		height++;
+	find_metapath(sdp, lblock, &mp, height);
+	if (height > ip->i_height || gfs2_is_stuffed(ip)) {
+		ret = -EINVAL;
+		goto out_release;
+	}
+	ret = lookup_metapath(ip, &mp);
+	if (ret < 0)
+		goto out_release;
+
+	if (ret != ip->i_height) {
+		iomap->length = hole_size(inode, lblock, &mp);
+		goto out_meta_hole;
+	}
+
+	ptr = metapointer(ip->i_height - 1, &mp);
+	iomap->blkno = be64_to_cpu(*ptr);
+	if (*ptr)
+		iomap->type = IOMAP_MAPPED;
+	else
+		iomap->type = IOMAP_HOLE;
+
+	bh = mp.mp_bh[ip->i_height - 1];
+	len = gfs2_extent_length(bh->b_data, bh->b_size, ptr,
+				 length >> inode->i_blkbits, &eob);
+	iomap->length = len << sdp->sd_sb.sb_bsize_shift;
+	/* If we go past eof, round up to the nearest block */
+	if (iomap->offset + iomap->length >= isize)
+		iomap->length = (((isize - iomap->offset) + (bsize - 1)) &
+				 ~(bsize - 1));
+
+out_meta_hole:
+	ret = 0;
+out_release:
+	release_metapath(&mp);
+	bmap_unlock(ip, 0);
+	return ret;
+}
+
+/**
  * gfs2_block_map - Map a block from an inode to a disk block
  * @inode: The inode
  * @lblock: The logical block number
diff --git a/fs/gfs2/bmap.h b/fs/gfs2/bmap.h
index 81ded5e..815a527 100644
--- a/fs/gfs2/bmap.h
+++ b/fs/gfs2/bmap.h
@@ -47,6 +47,8 @@ static inline void gfs2_write_calc_reserv(const struct gfs2_inode *ip,
 extern int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page);
 extern int gfs2_block_map(struct inode *inode, sector_t lblock,
 			  struct buffer_head *bh, int create);
+extern int gfs2_get_iomap(struct address_space *mapping, loff_t pos,
+			  ssize_t length, struct iomap *iomap);
 extern int gfs2_extent_map(struct inode *inode, u64 lblock, int *new,
 			   u64 *dblock, unsigned *extlen);
 extern int gfs2_setattr_size(struct inode *inode, u64 size);
-- 
2.5.0

[vfs PATCH v3 4/4] GFS2: Use new iomap interface for fiemap

[Bob Peterson]

This patch switches the GFS2 fiemap interface so that it uses the
new iomap infrastructure rather than the old blockmap.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 1bae189..3625753 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -2090,8 +2090,8 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		if (ret == 1)
 			ret = 0;
 	} else {
-		ret = __generic_block_fiemap(inode, fieinfo, start, len,
-					     gfs2_block_map);
+		ret = __generic_iomap_fiemap(inode, fieinfo, start, len,
+					     gfs2_get_iomap);
 	}
 
 	gfs2_glock_dq_uninit(&gh);
-- 
2.5.0

Re: [vfs PATCH v3 2/4] VFS: Add new __generic_iomap_fiemap interface

[Jan Kara]

On Fri 04-03-16 15:11:38, Bob Peterson wrote:
> @@ -251,6 +253,100 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
>  }
>  
>  /**
> + * __generic_iomap_fiemap - FIEMAP for iomap based inodes (no locking)
> + * @inode: the inode to map
> + * @fieinfo: the fiemap info struct that will be passed back to userspace
> + * @start: where to start mapping in the inode
> + * @len: how much space to map

You are missing @iomap description here.

> + *
> + * This does FIEMAP for iomap based inodes.  Basically it will just loop
> + * through iomap until we hit the number of extents we want to map, or we
> + * go past the end of the file and hit a hole.
> + *
> + * If it is possible to have data blocks beyond a hole past @inode->i_size, then
> + * please do not use this function, it will stop at the first unmapped block
> + * beyond i_size.

Line over 80 chars here.

> + *
> + * If you use this function directly, you need to do your own locking. Use
> + * generic_iomap_fiemap if you want the locking done for you.
> + */
> +
> +int __generic_iomap_fiemap(struct inode *inode,
> +			   struct fiemap_extent_info *fieinfo, loff_t start,
> +			   loff_t len, iomap_get_t iomap)
> +{
> +	struct iomap iom, prev_iom;
> +	loff_t isize = i_size_read(inode);
> +	int ret = 0;
> +
> +	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
> +	memset(&prev_iom, 0, sizeof(prev_iom));
> +	if (len >= isize)
> +		len = isize;
> +
> +	while ((ret == 0) && (start < isize) && len) {
> +		memset(&iom, 0, sizeof(iom));
> +		ret = iomap(inode->i_mapping, start, len, &iom);
> +		if (ret)
> +			break;
> +
> +		if (!iomap_needs_allocation(&iom)) {

Why not directly check for hole here? The name iomap_needs_allocation()
only obscures what's going on here. At least to me... ;)

> +			if (prev_iom.blkno)
> +				ret = fiemap_fill_next_extent(fieinfo,
> +							      prev_iom.offset,
> +							      blk_to_logical(inode,
> +							      prev_iom.blkno),
> +							      prev_iom.length,
> +							      FIEMAP_EXTENT_MERGED);

I'd just format this as 
				ret = fiemap_fill_next_extent(fieinfo,
					prev_iom.offset,
					blk_to_logical(inode, prev_iom.blkno),
					prev_iom.length,
					FIEMAP_EXTENT_MERGED);

IMHO it is more readable and doesn't overflow 80 chars.

> +			prev_iom = iom;
> +		}
> +		start += iom.length;
> +		if (len < iom.length)
> +			break;
> +		len -= iom.length;
> +		cond_resched();

Add the fatal_signal_pending() check we have in __generic_block_fiemap()?

> +	}
> +
> +	if (prev_iom.blkno)
> +		ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset,
> +					      blk_to_logical(inode,
> +							     prev_iom.blkno),
> +					      prev_iom.length,
> +					      FIEMAP_EXTENT_MERGED |
> +					      FIEMAP_EXTENT_LAST);

FIEMAP_EXTENT_LAST should only be set if this is the last extent in the
file. Here you set it in other cases as well.

Also in some cases - e.g. when fiemap_fill_next_extent() in the loop above
runs out of space in fieinfo, or in case of error, you shouldn't try to
fill next extent, should you?

>  typedef int (get_block_t)(struct inode *inode, sector_t iblock,
>  			struct buffer_head *bh_result, int create);
> +typedef int (iomap_get_t)(struct address_space *mapping, loff_t pos,
> +			  ssize_t length, struct iomap *iomap);

Why do you pass 'struct address_space' and not 'struct inode'? That would
look more natural to me...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Fri, Mar 04, 2016 at 03:11:37PM -0500, Bob Peterson wrote:
> This patch moves the iomap declares from its current location in
> exportfs.h to a new iomap.h. This will facilitate future improvements
> such as a more efficient fiemap for holey files. Hopefully it will
> one day be used for multipage writes as well.

Still not a plain move.  This adds the fspriv member, but also reduces
the size of length which will probably break pnfs exporting in some
cases.

I have done a plain move in this series:
http://thread.gmane.org/gmane.comp.file-systems.xfs.general/73804

might make sense to grab this for both possible tree that could be used
for the infrastructure.

Re: [vfs PATCH v3 2/4] VFS: Add new __generic_iomap_fiemap interface

[Christoph Hellwig]

> +	while ((ret == 0) && (start < isize) && len) {

no need for both sets of inner braces.

> +		memset(&iom, 0, sizeof(iom));
> +		ret = iomap(inode->i_mapping, start, len, &iom);
> +		if (ret)
> +			break;
> +
> +		if (!iomap_needs_allocation(&iom)) {

I don't think the iomap_needs_allocation, because the decision if
something needs an allocation is up to the beholder.  E.g. in this case
you want to report block numbers of holes and unwritten extents.

> +			if (prev_iom.blkno)
> +				ret = fiemap_fill_next_extent(fieinfo,
> +							      prev_iom.offset,
> +							      blk_to_logical(inode,

the iomap interface as used for pnfs and the multi page writes that I
pointed you to in the last reply use file system blocks as unit, so the
blk_to_logical should go away.

Re: [vfs PATCH v3 4/4] GFS2: Use new iomap interface for fiemap

[Christoph Hellwig]

> -		ret = __generic_block_fiemap(inode, fieinfo, start, len,
> -					     gfs2_block_map);
> +		ret = __generic_iomap_fiemap(inode, fieinfo, start, len,
> +					     gfs2_get_iomap);

So it seems like the generic_iomap_fiemap you added is unused,
no need to add it then.

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Bob Peterson]

----- Original Message -----
> On Fri, Mar 04, 2016 at 03:11:37PM -0500, Bob Peterson wrote:
> > This patch moves the iomap declares from its current location in
> > exportfs.h to a new iomap.h. This will facilitate future improvements
> > such as a more efficient fiemap for holey files. Hopefully it will
> > one day be used for multipage writes as well.
> 
> Still not a plain move.  This adds the fspriv member, but also reduces
> the size of length which will probably break pnfs exporting in some
> cases.
> 
> I have done a plain move in this series:
> http://thread.gmane.org/gmane.comp.file-systems.xfs.general/73804
> 
> might make sense to grab this for both possible tree that could be used
> for the infrastructure.
> 
Hi Christoph,

Thanks for the link. I'm very pleased to see you're working on the
multi-page write and iomap stuff. I'd like to see these patches go
forward and I'll build my fiemap stuff off of them.

Does it make sense to split the first two vfs-related patches from the
others that are more xfs related? I didn't see these patches appear on
linux-fsdevel or lkml, so I assume they're only on the xfs list currently?

It would be nice to see them on linux-fsdevel, since I'm not subscribed to
that xfs list. It would also be useful to get them into some place more
central like Al's vfs tree.

Regards,

Bob Peterson
Red Hat File Systems

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Mon, Mar 28, 2016 at 03:53:04PM -0400, Bob Peterson wrote:
> Thanks for the link. I'm very pleased to see you're working on the
> multi-page write and iomap stuff. I'd like to see these patches go
> forward and I'll build my fiemap stuff off of them.
> 
> Does it make sense to split the first two vfs-related patches from the
> others that are more xfs related? I didn't see these patches appear on
> linux-fsdevel or lkml, so I assume they're only on the xfs list currently?

For now I'm not sure the infrastructure will stay in the generic code or
in XFS, so for the first round I just wanted to ask for XFS feedback.

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Dave Chinner]

On Tue, Mar 29, 2016 at 12:40:26AM -0700, Christoph Hellwig wrote:
> On Mon, Mar 28, 2016 at 03:53:04PM -0400, Bob Peterson wrote:
> > Thanks for the link. I'm very pleased to see you're working on the
> > multi-page write and iomap stuff. I'd like to see these patches go
> > forward and I'll build my fiemap stuff off of them.
> > 
> > Does it make sense to split the first two vfs-related patches from the
> > others that are more xfs related? I didn't see these patches appear on
> > linux-fsdevel or lkml, so I assume they're only on the xfs list currently?
> 
> For now I'm not sure the infrastructure will stay in the generic code or
> in XFS, so for the first round I just wanted to ask for XFS feedback.

QA over the past couple of days has indicatedno regressions, so I'm
going to seriously consider the multipage write code for the next
XFS merge. I'd be happy to also take fiemap code based on the same
iomap interfaces, especially if we can make it a generic, fully
functional fiemap implementation and use it in XFS, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Wed, Mar 30, 2016 at 09:20:36AM +1100, Dave Chinner wrote:
> > For now I'm not sure the infrastructure will stay in the generic code or
> > in XFS, so for the first round I just wanted to ask for XFS feedback.
> 
> QA over the past couple of days has indicatedno regressions, so I'm
> going to seriously consider the multipage write code for the next
> XFS merge. I'd be happy to also take fiemap code based on the same
> iomap interfaces, especially if we can make it a generic, fully
> functional fiemap implementation and use it in XFS, too.

Ok.  Bob, is the code going to be useful for your in gfs2 as-is?  I'm
torn a bit between adding it to common code because it actually is
nicely abstracted and generic, and keepin it in XFS to make future
changes easier (getting rid of buffer_heads is the big one here)

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Bob Peterson]

----- Original Message -----
> On Wed, Mar 30, 2016 at 09:20:36AM +1100, Dave Chinner wrote:
> > > For now I'm not sure the infrastructure will stay in the generic code or
> > > in XFS, so for the first round I just wanted to ask for XFS feedback.
> > 
> > QA over the past couple of days has indicatedno regressions, so I'm
> > going to seriously consider the multipage write code for the next
> > XFS merge. I'd be happy to also take fiemap code based on the same
> > iomap interfaces, especially if we can make it a generic, fully
> > functional fiemap implementation and use it in XFS, too.
> 
> Ok.  Bob, is the code going to be useful for your in gfs2 as-is?  I'm
> torn a bit between adding it to common code because it actually is
> nicely abstracted and generic, and keepin it in XFS to make future
> changes easier (getting rid of buffer_heads is the big one here)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Hi Christoph,

Sorry it's taken me a while to get back to you on this. I've been in fire-drill
mode way too long. Yes, your patches seem compatible with mine, with one
small exception, which I've adjusted for in the patch seen below, to
replace one of my previous patches.

Also, I joined the xfs mailing list and saw you were considering pulling
some of my stuff in. I guess I don't care where it gets pulled in; I thought
Al's vfs tree made sense.

I grabbed your iomap-related patches from the xfs set and applied them to
my copy of Al's VFS tree, added my GFS2 patches, and everything compiles.
I haven't tested it yet though.

Regards,

Bob Peterson
Red Hat File Systems
---
VFS: Add new __generic_iomap_fiemap interface

This patch adds a new function __generic_iomap_fiemap similar to
__generic_block_fiemap, but it uses the new iomap interface.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 116a333..0d04c2f 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -15,6 +15,8 @@
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
 #include <linux/falloc.h>
+#include <linux/iomap.h>
+
 #include "internal.h"
 
 #include <asm/ioctls.h>
@@ -251,6 +253,100 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
 }
 
 /**
+ * __generic_iomap_fiemap - FIEMAP for iomap based inodes (no locking)
+ * @inode: the inode to map
+ * @fieinfo: the fiemap info struct that will be passed back to userspace
+ * @start: where to start mapping in the inode
+ * @len: how much space to map
+ *
+ * This does FIEMAP for iomap based inodes.  Basically it will just loop
+ * through iomap until we hit the number of extents we want to map, or we
+ * go past the end of the file and hit a hole.
+ *
+ * If it is possible to have data blocks beyond a hole past @inode->i_size, then
+ * please do not use this function, it will stop at the first unmapped block
+ * beyond i_size.
+ *
+ * If you use this function directly, you need to do your own locking. Use
+ * generic_iomap_fiemap if you want the locking done for you.
+ */
+
+int __generic_iomap_fiemap(struct inode *inode,
+			   struct fiemap_extent_info *fieinfo, loff_t start,
+			   loff_t len, iomap_get_t iomap)
+{
+	struct iomap iom, prev_iom;
+	loff_t isize = i_size_read(inode);
+	int ret = 0;
+
+	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+	memset(&prev_iom, 0, sizeof(prev_iom));
+	if (len >= isize)
+		len = isize;
+
+	while ((ret == 0) && (start < isize) && len) {
+		memset(&iom, 0, sizeof(iom));
+		ret = iomap(inode->i_mapping, start, len, &iom);
+		if (ret)
+			break;
+
+		if (iom.type != IOMAP_HOLE) {
+			if (prev_iom.blkno)
+				ret = fiemap_fill_next_extent(fieinfo,
+							      prev_iom.offset,
+							      blk_to_logical(inode,
+							      prev_iom.blkno),
+							      prev_iom.length,
+							      FIEMAP_EXTENT_MERGED);
+			prev_iom = iom;
+		}
+		start += iom.length;
+		if (len < iom.length)
+			break;
+		len -= iom.length;
+		cond_resched();
+	}
+
+	if (prev_iom.blkno)
+		ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset,
+					      blk_to_logical(inode,
+							     prev_iom.blkno),
+					      prev_iom.length,
+					      FIEMAP_EXTENT_MERGED |
+					      FIEMAP_EXTENT_LAST);
+	/* If ret is 1 then we just hit the end of the extent array */
+	if (ret == 1)
+		ret = 0;
+
+	return ret;
+}
+EXPORT_SYMBOL(__generic_iomap_fiemap);
+
+/**
+ * generic_iomap_fiemap - FIEMAP for block based inodes
+ * @inode: The inode to map
+ * @fieinfo: The mapping information
+ * @start: The initial block to map
+ * @len: The length of the extect to attempt to map
+ * @get_block: The block mapping function for the fs
+ *
+ * Calls __generic_block_fiemap to map the inode, after taking
+ * the inode's mutex lock.
+ */
+
+int generic_iomap_fiemap(struct inode *inode,
+			 struct fiemap_extent_info *fieinfo, u64 start,
+			 u64 len, iomap_get_t iomap)
+{
+	int ret;
+	mutex_lock(&inode->i_mutex);
+	ret = __generic_iomap_fiemap(inode, fieinfo, start, len, iomap);
+	mutex_unlock(&inode->i_mutex);
+	return ret;
+}
+EXPORT_SYMBOL(generic_iomap_fiemap);
+
+/**
  * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
  * @inode: the inode to map
  * @fieinfo: the fiemap info struct that will be passed back to userspace
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c460175..31a0f5e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -70,8 +70,12 @@ extern int sysctl_protected_symlinks;
 extern int sysctl_protected_hardlinks;
 
 struct buffer_head;
+struct address_space;
+struct iomap;
 typedef int (get_block_t)(struct inode *inode, sector_t iblock,
 			struct buffer_head *bh_result, int create);
+typedef int (iomap_get_t)(struct address_space *mapping, loff_t pos,
+			  ssize_t length, struct iomap *iomap);
 typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 			ssize_t bytes, void *private);
 typedef void (dax_iodone_t)(struct buffer_head *bh_map, int uptodate);
@@ -316,7 +320,6 @@ enum positive_aop_returns {
  * oh the beauties of C type declarations.
  */
 struct page;
-struct address_space;
 struct writeback_control;
 
 #define IOCB_EVENTFD		(1 << 0)
@@ -2848,6 +2851,12 @@ extern int vfs_lstat(const char __user *, struct kstat *);
 extern int vfs_fstat(unsigned int, struct kstat *);
 extern int vfs_fstatat(int , const char __user *, struct kstat *, int);
 
+extern int __generic_iomap_fiemap(struct inode *inode,
+				  struct fiemap_extent_info *fieinfo,
+				  loff_t start, loff_t len, iomap_get_t iomap);
+extern int generic_iomap_fiemap(struct inode *inode,
+				struct fiemap_extent_info *fieinfo, u64 start,
+				u64 len, iomap_get_t iomap);
 extern int __generic_block_fiemap(struct inode *inode,
 				  struct fiemap_extent_info *fieinfo,
 				  loff_t start, loff_t len,

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Thu, Apr 07, 2016 at 03:58:35PM -0400, Bob Peterson wrote:
> Sorry it's taken me a while to get back to you on this. I've been in fire-drill
> mode way too long. Yes, your patches seem compatible with mine, with one
> small exception, which I've adjusted for in the patch seen below, to
> replace one of my previous patches.
> 
> Also, I joined the xfs mailing list and saw you were considering pulling
> some of my stuff in. I guess I don't care where it gets pulled in; I thought
> Al's vfs tree made sense.
> 
> I grabbed your iomap-related patches from the xfs set and applied them to
> my copy of Al's VFS tree, added my GFS2 patches, and everything compiles.
> I haven't tested it yet though.

I've done some minor work to better integrate them, and I expect to
finish off the integration by the nd of the week.  I'll Cc you on the
patch series.

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Wed, Mar 30, 2016 at 09:20:36AM +1100, Dave Chinner wrote:
> QA over the past couple of days has indicatedno regressions, so I'm
> going to seriously consider the multipage write code for the next
> XFS merge. I'd be happy to also take fiemap code based on the same
> iomap interfaces, especially if we can make it a generic, fully
> functional fiemap implementation and use it in XFS, too.

Now that's I've spent some more time with it: it depend on your
idea of fully functional.  For the actual file data the fiemap
implementation is simple and beautiful.  But XFS also supports the
odd FIEMAP_FLAG_XATTR flags, which makes no sense at all for an
fiemap based implementation.

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Dave Chinner]

On Sun, Apr 10, 2016 at 10:15:28AM -0700, Christoph Hellwig wrote:
> On Wed, Mar 30, 2016 at 09:20:36AM +1100, Dave Chinner wrote:
> > QA over the past couple of days has indicatedno regressions, so I'm
> > going to seriously consider the multipage write code for the next
> > XFS merge. I'd be happy to also take fiemap code based on the same
> > iomap interfaces, especially if we can make it a generic, fully
> > functional fiemap implementation and use it in XFS, too.
> 
> Now that's I've spent some more time with it: it depend on your
> idea of fully functional.  For the actual file data the fiemap
> implementation is simple and beautiful.  But XFS also supports the
> odd FIEMAP_FLAG_XATTR flags, which makes no sense at all for an
> fiemap based implementation.

In this case, I'd let xfs_vn_fiemap handle that. i.e. if the
FIEMAP_FLAG_XATTR is set, run the old code, otherwise call straight
into the new generic iomap based code.

I'm not sure if how your new code is structured, but n this version
__generic_iomap_fiemap() is passed an iomap_get_t callback function.
We could simply have XFS pass a different callback function that
looks up the attribute fork extent list rather than the data fork
extent list to support FIEMAP_FLAG_XATTR appropriately because other
than the different initial extent list root it seems to me like the
implementation should be identical....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Christoph Hellwig]

On Mon, Apr 11, 2016 at 04:17:40PM +1000, Dave Chinner wrote:
> In this case, I'd let xfs_vn_fiemap handle that. i.e. if the
> FIEMAP_FLAG_XATTR is set, run the old code, otherwise call straight
> into the new generic iomap based code.
> 
> I'm not sure if how your new code is structured, but n this version
> __generic_iomap_fiemap() is passed an iomap_get_t callback function.
> We could simply have XFS pass a different callback function that
> looks up the attribute fork extent list rather than the data fork
> extent list to support FIEMAP_FLAG_XATTR appropriately because other
> than the different initial extent list root it seems to me like the
> implementation should be identical....

Providing iomap_ops for the xattr flag look feasily, but currently
there is no real testing for it.  Only fsstress will call into it
at all and doesn't even verify the output.  I'll send the iomap
implementation and XFS support as RFC, and then you can decide what
to do with it.

Re: [vfs PATCH v3 1/4] VFS: move iomap from exportfs.h to iomap.h

[Dave Chinner]

On Tue, Apr 12, 2016 at 11:29:40AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 11, 2016 at 04:17:40PM +1000, Dave Chinner wrote:
> > In this case, I'd let xfs_vn_fiemap handle that. i.e. if the
> > FIEMAP_FLAG_XATTR is set, run the old code, otherwise call straight
> > into the new generic iomap based code.
> > 
> > I'm not sure if how your new code is structured, but n this version
> > __generic_iomap_fiemap() is passed an iomap_get_t callback function.
> > We could simply have XFS pass a different callback function that
> > looks up the attribute fork extent list rather than the data fork
> > extent list to support FIEMAP_FLAG_XATTR appropriately because other
> > than the different initial extent list root it seems to me like the
> > implementation should be identical....
> 
> Providing iomap_ops for the xattr flag look feasily, but currently
> there is no real testing for it.  Only fsstress will call into it
> at all and doesn't even verify the output.  I'll send the iomap
> implementation and XFS support as RFC, and then you can decide what
> to do with it.

Sure.

FWIW, the xfs_io fiemap command is able to query the attribute
fork rather than the data fork. I suspect it's not actually used in
any regression tests in xfstests, though.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com