[PATCH 0/3] fiemap: introduce EXTENT_DATA_COMPRESSED flag

[PATCH 0/3] 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.

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        |    4 +++-
 include/uapi/linux/fiemap.h |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
1.7.9

[PATCH 1/3] 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 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index d830747..0f52849 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h
@@ -50,7 +50,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.7.9

[PATCH 2/3] 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.

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

CC: Christoph Hellwig <hch@infradead.org>
CC: Mark Fasheh <mfasheh@suse.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 include/uapi/linux/fiemap.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
index 0f52849..74d1da9 100644
--- a/include/uapi/linux/fiemap.h
+++ b/include/uapi/linux/fiemap.h
@@ -49,6 +49,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.7.9

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

[Andreas Dilger]

On 2013-09-03, at 6:11 AM, David Sterba wrote:
> 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.
> 
> [1] http://article.gmane.org/gmane.comp.file-systems.ext4/8871
> [2] http://thread.gmane.org/gmane.comp.file-systems.ext4/8870
> 
> +#define FIEMAP_EXTENT_DATA_COMPRESSED	0x00000040 /* Data compressed by fs
> +						    * Sets EXTENT_ENCODED */

If the data is compressed, I think that just setting a flag in the
fiemap_extent structure is not enough.  It should also distinguish
between the logical extent length and the physical extent length,
by adding something like the fe_log_length field:

struct fiemap_extent {
        __u64 fe_logical;  /* logical offset in bytes for the start of
                            * the extent from the beginning of the file */
        __u64 fe_physical; /* physical offset in bytes for the start
                            * of the extent from the beginning of the disk */
        __u64 fe_length;   /* physical length in bytes for this extent */
        __u64 fe_log_length;/* logical length in bytes (DATA_COMPRESSED) */
        __u64 fe_reserved64;
        __u32 fe_flags;    /* FIEMAP_EXTENT_* flags for this extent */
        __u32 fe_reserved[3];
};

I'm a bit indecisive on whether the existing fe_length should be
the physical length (which is what filefrag is reporting for the
existing compressed files), or if it should be the logical length.
For uncompressed files they would be the same, except fe_log_length
is currently always zero.  I don't think it makes sense to ever
have an allocated extent with a logical length of zero, so tools
could easily distinguish this case (without the DATA_COMPRESSED flag).

Without this separation for compressed files, it isn't clear to
the user if the file is sparse with holes all over it, even in the
presence of the DATA_COMPRESSED flag.  Also, it isn't clear for
tools like tar or cp what parts of the file they should copy.

Cheers, Andreas

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

[David Sterba]

On Thu, Sep 05, 2013 at 11:28:20PM -0600, Andreas Dilger wrote:
> If the data is compressed, I think that just setting a flag in the
> fiemap_extent structure is not enough.  It should also distinguish
> between the logical extent length and the physical extent length,
> by adding something like the fe_log_length field:

That's right, I've overlooked the spare 64bit fields.

> I'm a bit indecisive on whether the existing fe_length should be
> the physical length (which is what filefrag is reporting for the
> existing compressed files), or if it should be the logical length.

IMO it must be the logical length, otherwise it would break existing
applications (cp).

The relevant parts of the structure would look like this:

         __u64 fe_length;   /* logical length in bytes for this extent */
         __u64 fe_phys_length; /* physical length in bytes (DATA_COMPRESSED) */

> For uncompressed files they would be the same, except fe_log_length
> is currently always zero.  I don't think it makes sense to ever
> have an allocated extent with a logical length of zero, so tools
> could easily distinguish this case (without the DATA_COMPRESSED flag).

Looks like if fe_length contains the physical length it complicates
things unnecessarily.

I'd expect that fe_phys_length is valid iff an additional fe_flag bit is
set.  That way we don't need to touch any other existing filesystem and
won't break userspace.

> Without this separation for compressed files, it isn't clear to
> the user if the file is sparse with holes all over it, even in the
> presence of the DATA_COMPRESSED flag.  Also, it isn't clear for
> tools like tar or cp what parts of the file they should copy.

A hole is represented by a missing extent, right? Then I don't see how
the tools could get confused, the compression is completely transparent.

The main point of the flag is to enhance extent description, and with
the physical length available, to compute the compressed size of the
file. (*)

I'll spin V2 if the proposed change is ok.

david


(*)
This has been asked for for a long time, a separate ioctl patch
exists [1] but we did not completely agree that it's the right approach
and that some other interface should be used instead.

[1] http://repo.or.cz/w/linux-2.6/btrfs-unstable.git/commit/aa50490d57dd2035fc82f4070c13edba40926c69

Similar to FIEMAP, the ioctl goes through extents in a given file range
but calculates only the compressed (physical) size.

[PATCH 3/3] 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 |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fe443fe..bb2e346 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4095,8 +4095,10 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		} else {
 			disko = em->block_start + offset_in_extent;
 		}
-		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;
-- 
1.7.9