diff mbox

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

Message ID 1457122300-28514-3-git-send-email-rpeterso@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bob Peterson March 4, 2016, 8:11 p.m. UTC 
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(-)

Comments

Jan Kara March 7, 2016, 10:18 a.m. UTC | #1 
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
Christoph Hellwig March 15, 2016, 7:33 a.m. UTC | #2 
> +	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.

--
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
diff mbox

Patch

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 */