[PATCH 0/3 v2] fiemap: introduce EXTENT_DATA_COMPRESSED flag

[PATCH 0/3 v2] fiemap: introduce EXTENT_DATA_COMPRESSED flag

[David Sterba]

The original FIEMAP patch did not define this bit, btrfs will make use of
it.  The defined constant maintains the same value as originally proposed.

Currently, the 'filefrag' utility has no way to recognize and denote a
compressed extent. As implemented in btrfs right now, the compression step
splits a big extent into smaller chunks and this is reported as a heavily
fragmented file. Adding the flag to filefrag will at least give some
explanation why, this has been confusing users for some time already.

V2:
Based on feedback from Andreas, the fiemap_extent is now able to hold the
physical extent length, to be filled by the filesystem callback.

The filesystems do not have access to the structure that is passed back to
userspace and are supposed to call fiemap_fill_next_extent, there's no direct
way to fill fe_phys_length. There are two ways to pass it:

1) extend fiemap_fill_next_extent to take phys_length and update all
   users (ext4, gfs2, ocfs2, nilfs2, xfs)

2) add new function that takes arguments for all the fiemap_extent items,
   newly added phys_length compared to fiemap_fill_next_extent

This patchset implements 2, the footprint is smaller, no need to change
filesystem that do not care about compression.


David Sterba (3):
  fiemap: fix comment at EXTENT_DATA_ENCRYPTED
  fiemap: add EXTENT_DATA_COMPRESSED flag
  btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents

 fs/btrfs/extent_io.c        | 11 ++++++++---
 fs/ioctl.c                  | 42 ++++++++++++++++++++++++++++++++++++++----
 include/linux/fs.h          |  2 ++
 include/uapi/linux/fiemap.h |  8 ++++++--
 4 files changed, 54 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/3 v2] fiemap: fix comment at EXTENT_DATA_ENCRYPTED

[David Sterba]

The flag was named EXTENT_NO_BYPASS in the original fiemap proposal[1], but
renamed to EXTENT_ENCODED afterwards.

[1] http://article.gmane.org/gmane.comp.file-systems.ext4/8871

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 include/uapi/linux/fiemap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index 0c51d617dae9..93abfcd9ac47 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h
@@ -51,7 +51,7 @@ struct fiemap {
 #define FIEMAP_EXTENT_ENCODED		0x00000008 /* Data can not be read
 						    * while fs is unmounted */
 #define FIEMAP_EXTENT_DATA_ENCRYPTED	0x00000080 /* Data is encrypted by fs.
-						    * Sets EXTENT_NO_BYPASS. */
+						    * Sets EXTENT_ENCODED */
 #define FIEMAP_EXTENT_NOT_ALIGNED	0x00000100 /* Extent offsets may not be
 						    * block aligned. */
 #define FIEMAP_EXTENT_DATA_INLINE	0x00000200 /* Data mixed with metadata.
-- 
1.8.3.1

[PATCH 2/3 v2] fiemap: add EXTENT_DATA_COMPRESSED flag

[David Sterba]

This flag was not accepted when fiemap was proposed [2] due to lack of
in-kernel users. Btrfs has compression for a long time and we'd like to
see that an extent is compressed in the output of 'filefrag' utility
once it's taught about it.

For that purpose, a reserved field from fiemap_extent is used to let the
filesystem store along the physcial extent length when the flag is set.
This keeps compatibility with applications that use FIEMAP.

In order to minimize changes to current ->fiemap users that do not
understand the compressed flag, a new filesystem helper is introduced,
fiemap_fill_next_extent_full. This is supposed to take parameters
matching all valid fiemap_extent fields, fe_phys_length in this case.

[1] http://article.gmane.org/gmane.comp.file-systems.ext4/8871
[2] http://thread.gmane.org/gmane.comp.file-systems.ext4/8870
[3] http://thread.gmane.org/gmane.linux.file-systems/77632 (v1)

CC: Andreas Dilger <adilger@dilger.ca>
CC: Christoph Hellwig <hch@infradead.org>
CC: Mark Fasheh <mfasheh@suse.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 fs/ioctl.c                  | 42 ++++++++++++++++++++++++++++++++++++++----
 include/linux/fs.h          |  2 ++
 include/uapi/linux/fiemap.h |  6 +++++-
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index fd507fb460f8..29db45020874 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -65,25 +65,30 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
 }
 
 /**
- * fiemap_fill_next_extent - Fiemap helper function
+ * fiemap_fill_next_extent_full - Fiemap helper function (extended argument list)
  * @fieinfo:	Fiemap context passed into ->fiemap
  * @logical:	Extent logical start offset, in bytes
  * @phys:	Extent physical start offset, in bytes
  * @len:	Extent length, in bytes
+ * @phys_len:   Physical extent length in bytes
  * @flags:	FIEMAP_EXTENT flags that describe this extent
  *
  * Called from file system ->fiemap callback. Will populate extent
  * info as passed in via arguments and copy to user memory. On
  * success, extent count on fieinfo is incremented.
  *
+ * This is an extended version of fiemap_fill_next_extent that fills all valid
+ * fiemap_extent fields.
+ *
  * Returns 0 on success, -errno on error, 1 if this was the last
  * extent that will fit in user array.
  */
 #define SET_UNKNOWN_FLAGS	(FIEMAP_EXTENT_DELALLOC)
-#define SET_NO_UNMOUNTED_IO_FLAGS	(FIEMAP_EXTENT_DATA_ENCRYPTED)
+#define SET_NO_UNMOUNTED_IO_FLAGS	(FIEMAP_EXTENT_DATA_ENCRYPTED | \
+					 FIEMAP_EXTENT_DATA_COMPRESSED)
 #define SET_NOT_ALIGNED_FLAGS	(FIEMAP_EXTENT_DATA_TAIL|FIEMAP_EXTENT_DATA_INLINE)
-int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
-			    u64 phys, u64 len, u32 flags)
+int fiemap_fill_next_extent_full(struct fiemap_extent_info *fieinfo,
+		u64 logical, u64 phys, u64 len, u64 phys_len, u32 flags)
 {
 	struct fiemap_extent extent;
 	struct fiemap_extent __user *dest = fieinfo->fi_extents_start;
@@ -110,6 +115,9 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
 	extent.fe_length = len;
 	extent.fe_flags = flags;
 
+	if (flags & FIEMAP_EXTENT_DATA_COMPRESSED)
+		extent.fe_phys_length = phys_len;
+
 	dest += fieinfo->fi_extents_mapped;
 	if (copy_to_user(dest, &extent, sizeof(extent)))
 		return -EFAULT;
@@ -119,6 +127,32 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
 		return 1;
 	return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0;
 }
+EXPORT_SYMBOL(fiemap_fill_next_extent_full);
+
+/**
+ * fiemap_fill_next_extent - Fiemap helper function
+ * @fieinfo:	Fiemap context passed into ->fiemap
+ * @logical:	Extent logical start offset, in bytes
+ * @phys:	Extent physical start offset, in bytes
+ * @len:	Extent length, in bytes
+ * @flags:	FIEMAP_EXTENT flags that describe this extent
+ *
+ * Called from file system ->fiemap callback. Will populate extent
+ * info as passed in via arguments and copy to user memory. On
+ * success, extent count on fieinfo is incremented.
+ *
+ * Returns 0 on success, -errno on error, 1 if this was the last
+ * extent that will fit in user array.
+ */
+int fiemap_fill_next_extent(struct fiemap_extent_info *fieinfo, u64 logical,
+			    u64 phys, u64 len, u32 flags)
+{
+	/*
+	 * Use fiemap_fill_next_extent_full if you see this
+	 */
+	WARN_ON_ONCE(flags & FIEMAP_EXTENT_DATA_COMPRESSED);
+	return fiemap_fill_next_extent_full(fieinfo, logical, phys, len, 0, flags);
+}
 EXPORT_SYMBOL(fiemap_fill_next_extent);
 
 /**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f40547ba191..40f6b59c910b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1485,6 +1485,8 @@ struct fiemap_extent_info {
 };
 int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 			    u64 phys, u64 len, u32 flags);
+int fiemap_fill_next_extent_full(struct fiemap_extent_info *info, u64 logical,
+			    u64 phys, u64 len, u64 phys_len, u32 flags);
 int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
 
 /*
diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index 93abfcd9ac47..0e32caeccedd 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h
@@ -19,7 +19,9 @@ struct fiemap_extent {
 	__u64 fe_physical; /* physical offset in bytes for the start
 			    * of the extent from the beginning of the disk */
 	__u64 fe_length;   /* length in bytes for this extent */
-	__u64 fe_reserved64[2];
+	__u64 fe_phys_length; /* physical length in bytes, undefined if
+			       * DATA_COMPRESSED not set */
+	__u64 fe_reserved64;
 	__u32 fe_flags;    /* FIEMAP_EXTENT_* flags for this extent */
 	__u32 fe_reserved[3];
 };
@@ -50,6 +52,8 @@ struct fiemap {
 						    * Sets EXTENT_UNKNOWN. */
 #define FIEMAP_EXTENT_ENCODED		0x00000008 /* Data can not be read
 						    * while fs is unmounted */
+#define FIEMAP_EXTENT_DATA_COMPRESSED	0x00000040 /* Data is compressed by fs.
+						    * Sets EXTENT_ENCODED */
 #define FIEMAP_EXTENT_DATA_ENCRYPTED	0x00000080 /* Data is encrypted by fs.
 						    * Sets EXTENT_ENCODED */
 #define FIEMAP_EXTENT_NOT_ALIGNED	0x00000100 /* Extent offsets may not be
-- 
1.8.3.1

[PATCH 3/3 v2] btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents

[David Sterba]

Set the EXTENT_DATA_COMPRESSED flag together with EXTENT_ENCODED as
defined by fiemap spec.

Signed-off-by: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/extent_io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c09a40db53db..6edb38e3a308 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4150,6 +4150,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 	while (!end) {
 		u64 offset_in_extent = 0;
+		u64 em_phys_len;
 
 		/* break if the extent we found is outside the range */
 		if (em->start >= max || extent_map_end(em) < off)
@@ -4174,6 +4175,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		em_end = extent_map_end(em);
 		em_len = em_end - em_start;
 		emflags = em->flags;
+		em_phys_len = em->len;
 		disko = 0;
 		flags = 0;
 
@@ -4195,9 +4197,12 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 				  FIEMAP_EXTENT_UNKNOWN);
 		} else {
 			disko = em->block_start + offset_in_extent;
+			em_phys_len = em->block_len;
 		}
-		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
+		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
 			flags |= FIEMAP_EXTENT_ENCODED;
+			flags |= FIEMAP_EXTENT_DATA_COMPRESSED;
+		}
 
 		free_extent_map(em);
 		em = NULL;
@@ -4218,8 +4223,8 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			flags |= FIEMAP_EXTENT_LAST;
 			end = 1;
 		}
-		ret = fiemap_fill_next_extent(fieinfo, em_start, disko,
-					      em_len, flags);
+		ret = fiemap_fill_next_extent_full(fieinfo, em_start, disko,
+					      em_len, em_phys_len, flags);
 		if (ret)
 			goto out_free;
 	}
-- 
1.8.3.1

Re: [PATCH 0/3 v2] fiemap: introduce EXTENT_DATA_COMPRESSED flag

[Andreas Dilger]

On 2013-10-02, at 9:02 AM, David Sterba wrote:
> The original FIEMAP patch did not define this bit, btrfs will make use of
> it.  The defined constant maintains the same value as originally proposed.
> 
> Currently, the 'filefrag' utility has no way to recognize and denote a
> compressed extent. As implemented in btrfs right now, the compression step
> splits a big extent into smaller chunks and this is reported as a heavily
> fragmented file. Adding the flag to filefrag will at least give some
> explanation why, this has been confusing users for some time already.
> 
> V2:
> Based on feedback from Andreas, the fiemap_extent is now able to hold the
> physical extent length, to be filled by the filesystem callback.

Thanks for this update.

> The filesystems do not have access to the structure that is passed back to
> userspace and are supposed to call fiemap_fill_next_extent, there's no
> way to fill fe_phys_length. There are two ways to pass it:
> 
> 1) extend fiemap_fill_next_extent to take phys_length and update all
>   users (ext4, gfs2, ocfs2, nilfs2, xfs)
> 
> 2) add new function that takes arguments for all the fiemap_extent items,
>   newly added phys_length compared to fiemap_fill_next_extent
> 
> This patchset implements 2, the footprint is smaller, no need to change
> filesystem that do not care about compression.

The fiemap_fill_next_extent_full() doesn't really take "all" of the
fields either - it just adds the one new phys_len field, but will
again be similarly lacking the next time a new field is used in
struct fiemap_extent.  For example, I've been wanting to add a 32-bit
fm_device field in place of fe_reserved[0] so that filesystems that
handle multiple devices directly (e.g. btrfs with internal RAID,
xfs with "realtime" volume, ext4 with external journal, lustre, etc)
could disambiguate extents between their backing devices.

I think in the fiemap_fill_next_extent() will just end up having
all of the fiemap_extent fields as arguments, and the callers may as
well just fill in an on-stack struct fiemap_extent and pass that as
a single pointer instead of passing 64 bytes of arguments and then
fiemap_fill_next_extent() allocates its own struct fiemap_extent on
the stack anyway.

So, I don't think it is worthwhile to introduce the "_full" version of
this function unless it is a short-term convenience for the sake of
this patch series, and it will be removed after converting the other
in-kernel users over to calling it.  The change is totally trivial -
just pass "0" for the new argument.  Keeping fiemap_fill_next_extent()
unchanged just adds 40+ bytes of stack usage to all of the existing
callers for no real benefit.

Cheers, Andreas