[PATCH 0/4 v3] fiemap: introduce EXTENT_DATA_COMPRESSED flag

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

V3:
Based on feedback from Andreas, implement #1 from V2, current users of
fiemap_fill_next_extent (fs/, ext4, gfs2, ocfs2, nilfs2, xfs) updated
accordingly, no functional change.

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

David Sterba (4):
  fiemap: fix comment at EXTENT_DATA_ENCRYPTED
  fiemap: add EXTENT_DATA_COMPRESSED flag
  btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents
  Documentation/fiemap: Document the DATA_COMPRESSED flag

 Documentation/filesystems/fiemap.txt |   17 +++++++++++++----
 fs/btrfs/extent_io.c                 |    9 +++++++--
 fs/ext4/extents.c                    |    3 ++-
 fs/ext4/inline.c                     |    2 +-
 fs/gfs2/inode.c                      |    2 +-
 fs/ioctl.c                           |   18 ++++++++++++------
 fs/nilfs2/inode.c                    |    8 +++++---
 fs/ocfs2/extent_map.c                |    4 ++--
 fs/xfs/xfs_iops.c                    |    2 +-
 include/linux/fs.h                   |    2 +-
 include/uapi/linux/fiemap.h          |    8 ++++++--
 11 files changed, 51 insertions(+), 24 deletions(-)

-- 
1.7.9

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/4 v3] 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 0c51d61..93abfcd 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.7.9

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

Extend arguments of fiemap_fill_next_extent and update all users.

[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)
[4] http://www.spinics.net/lists/linux-fsdevel/msg69078.html (v2)

Cc: Al Viro <viro@zeniv.linux.org.uk>
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/btrfs/extent_io.c        |    2 +-
 fs/ext4/extents.c           |    3 ++-
 fs/ext4/inline.c            |    2 +-
 fs/gfs2/inode.c             |    2 +-
 fs/ioctl.c                  |   18 ++++++++++++------
 fs/nilfs2/inode.c           |    8 +++++---
 fs/ocfs2/extent_map.c       |    4 ++--
 fs/xfs/xfs_iops.c           |    2 +-
 include/linux/fs.h          |    2 +-
 include/uapi/linux/fiemap.h |    6 +++++-
 10 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ff43802..5ea0ef5 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4244,7 +4244,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			end = 1;
 		}
 		ret = fiemap_fill_next_extent(fieinfo, em_start, disko,
-					      em_len, flags);
+					      em_len, 0, flags);
 		if (ret)
 			goto out_free;
 	}
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 35f65cf..00ffd18 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2224,6 +2224,7 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 				(__u64)es.es_lblk << blksize_bits,
 				(__u64)es.es_pblk << blksize_bits,
 				(__u64)es.es_len << blksize_bits,
+				0,
 				flags);
 			if (err < 0)
 				break;
@@ -4798,7 +4799,7 @@ static int ext4_xattr_fiemap(struct inode *inode,
 
 	if (physical)
 		error = fiemap_fill_next_extent(fieinfo, 0, physical,
-						length, flags);
+						length, 0, flags);
 	return (error < 0 ? error : 0);
 }
 
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index bae9875..c5da773 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1816,7 +1816,7 @@ int ext4_inline_data_fiemap(struct inode *inode,
 
 	if (physical)
 		error = fiemap_fill_next_extent(fieinfo, 0, physical,
-						length, flags);
+						length, 0, flags);
 	brelse(iloc.bh);
 out:
 	up_read(&EXT4_I(inode)->xattr_sem);
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 7119504..86e9e9b 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1817,7 +1817,7 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			len = size - start;
 		if (start < size)
 			ret = fiemap_fill_next_extent(fieinfo, start, phys,
-						      len, flags);
+						      len, 0, flags);
 		if (ret == 1)
 			ret = 0;
 	} else {
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 8ac3fad..e7902c4 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -70,6 +70,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
  * @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
@@ -80,10 +81,11 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
  * 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)
+			    u64 phys, u64 len, u64 phys_len, u32 flags)
 {
 	struct fiemap_extent extent;
 	struct fiemap_extent __user *dest = fieinfo->fi_extents_start;
@@ -110,6 +112,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;
@@ -318,10 +323,11 @@ int __generic_block_fiemap(struct inode *inode,
 				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
-							      flags);
+							      0, flags);
 			} else if (size) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
-							      phys, size, flags);
+							      phys, size,
+							      0, flags);
 				size = 0;
 			}
 
@@ -347,7 +353,7 @@ int __generic_block_fiemap(struct inode *inode,
 			if (start_blk > last_blk && !whole_file) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
-							      flags);
+							      0, flags);
 				break;
 			}
 
@@ -358,7 +364,7 @@ int __generic_block_fiemap(struct inode *inode,
 			if (size) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
-							      flags);
+							      0, flags);
 				if (ret)
 					break;
 			}
diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 7e350c5..b03917a 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -1018,7 +1018,8 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			if (size) {
 				/* End of the current extent */
 				ret = fiemap_fill_next_extent(
-					fieinfo, logical, phys, size, flags);
+					fieinfo, logical, phys, size, 0,
+					flags);
 				if (ret)
 					break;
 			}
@@ -1068,7 +1069,8 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 					flags |= FIEMAP_EXTENT_LAST;
 
 				ret = fiemap_fill_next_extent(
-					fieinfo, logical, phys, size, flags);
+					fieinfo, logical, phys, size, 0,
+					flags);
 				if (ret)
 					break;
 				size = 0;
@@ -1084,7 +1086,7 @@ int nilfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 					/* Terminate the current extent */
 					ret = fiemap_fill_next_extent(
 						fieinfo, logical, phys, size,
-						flags);
+						0, flags);
 					if (ret || blkoff > end_blkoff)
 						break;
 
diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c
index 767370b..521b0f2 100644
--- a/fs/ocfs2/extent_map.c
+++ b/fs/ocfs2/extent_map.c
@@ -735,7 +735,7 @@ static int ocfs2_fiemap_inline(struct inode *inode, struct buffer_head *di_bh,
 			phys += offsetof(struct ocfs2_dinode,
 					 id2.i_data.id_data);
 
-		ret = fiemap_fill_next_extent(fieinfo, 0, phys, id_count,
+		ret = fiemap_fill_next_extent(fieinfo, 0, phys, id_count, 0,
 					      flags);
 		if (ret < 0)
 			return ret;
@@ -809,7 +809,7 @@ int ocfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		virt_bytes = (u64)le32_to_cpu(rec.e_cpos) << osb->s_clustersize_bits;
 
 		ret = fiemap_fill_next_extent(fieinfo, virt_bytes, phys_bytes,
-					      len_bytes, fe_flags);
+					      len_bytes, 0, fe_flags);
 		if (ret)
 			break;
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 27e0e54..31e9f53 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1000,7 +1000,7 @@ xfs_fiemap_format(
 		fiemap_flags |= FIEMAP_EXTENT_LAST;
 
 	error = fiemap_fill_next_extent(fieinfo, logical, physical,
-					length, fiemap_flags);
+					length, 0, fiemap_flags);
 	if (error > 0) {
 		error = 0;
 		*full = 1;	/* user array now full */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 121f11f..1a96f9b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1479,7 +1479,7 @@ struct fiemap_extent_info {
 							fiemap_extent array */
 };
 int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
-			    u64 phys, u64 len, u32 flags);
+			    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 93abfcd..0e32cae 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.7.9

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Dave Chinner]

On Thu, Dec 12, 2013 at 04:25:59PM +0100, 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.
> 
> 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.

I'd prefer to just see the new physical length field always filled
out, regardless of whether it is a compressed extent or not. In
terms of backwards compatibility to userspace, it makes no
difference because the value of reserved/unused fields is undefined
by the API. Yes, the implementation zeros them, but there's nothing
in the documentation that says "reserved fields must be zero".
Hence I think we should just set it for every extent.

>From the point of view of the kernel API (fiemap_fill_next_extent),
passing the physical extent size in the "len" parameter for normal
extents, then passing 0 for the "physical length" makes absolutely
no sense.

IOWs, what you have created is a distinction between the extent's
"logical length" and it's "physical length". For uncompressed
extents, they are both equal and they should both be passed to
fiemap_fill_next_extent as the same value. Extents where they are
different (i.e.  encoded extents) is when they can be different.
Perhaps fiemap_fill_next_extent() should check and warn about
mismatches when they differ and the relevant flags are not set...

> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
> index 93abfcd..0e32cae 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];
>  };

The comment for fe_length needs to change, too, because it needs to
indicate that it is the logical extent length and that it may be
different to the fe_phys_length depending on the flags that are set
on the extent.

And, FWIW, I wouldn't mention specific flags in the comment here,
but do it at the definition of the flags that indicate there is
a difference between physical and logical extent lengths....

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

i.e. here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5028 bytes --]

On Dec 12, 2013, at 4:24 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> On Thu, Dec 12, 2013 at 04:25:59PM +0100, 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.
>> 
>> 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.
> 
> I'd prefer to just see the new physical length field always filled
> out, regardless of whether it is a compressed extent or not. In
> terms of backwards compatibility to userspace, it makes no
> difference because the value of reserved/unused fields is undefined
> by the API. Yes, the implementation zeros them, but there's nothing
> in the documentation that says "reserved fields must be zero".
> Hence I think we should just set it for every extent.

I'd actually thought the same thing while reading the patch, but I figured
people would object because it implies that old kernels will return a
physical length of 0 bytes (which might be valid) and badly-written tools
will not work correctly on older kernels.  That said, applications _should_
be checking the FIEMAP_EXTENT_DATA_COMPRESSED flag, and I suspect in the
future fewer developers will be confused if fe_phys_length == fe_length
going forward.

If the initial tools get it right (in particular filefrag), then hopefully
others will get it correct also.

> From the point of view of the kernel API (fiemap_fill_next_extent),
> passing the physical extent size in the "len" parameter for normal
> extents, then passing 0 for the "physical length" makes absolutely
> no sense.
> 
> IOWs, what you have created is a distinction between the extent's
> "logical length" and it's "physical length". For uncompressed
> extents, they are both equal and they should both be passed to
> fiemap_fill_next_extent as the same value. Extents where they are
> different (i.e.  encoded extents) is when they can be different.
> Perhaps fiemap_fill_next_extent() should check and warn about
> mismatches when they differ and the relevant flags are not set...

Seems reasonable to have a WARN_ONCE() in that case.  That would catch bugs
in the filesystem, code as well:

	WARN_ONCE(phys_len != lgcl_len &&
		  !(flags & FIEMAP_EXTENT_DATA_COMPRESSED),
		  "physical len %llu != logical length %llu without DATA_COMPRESSED\n",
		  phys_len, logical_len, phys_len, logical_len);

>> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
>> index 93abfcd..0e32cae 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];
>> };
> 
> The comment for fe_length needs to change, too, because it needs to
> indicate that it is the logical extent length and that it may be
> different to the fe_phys_length depending on the flags that are set
> on the extent.

Would it make sense to rename fe_length to fe_logi_length (or something,
I'm open to suggestions), and have a compat macro:

#define fe_length fe_logi_length

around for older applications?  That way, new developers would start to
use the new name, old applications would still compile for both newer and
older interfaces, and it doesn't affect the ABI at all.

> And, FWIW, I wouldn't mention specific flags in the comment here,
> but do it at the definition of the flags that indicate there is
> a difference between physical and logical extent lengths....

Actually, I was thinking just the opposite for this field.  It seems useful
that the requirement for DATA_COMPRESSED being set is beside fe_phys_length
so that anyone using this field sees the correlation clearly.  I don't expect
everyone would read and understand the meaning of all the flags when looking
at the data structure.

Cheers, Andreas

>> @@ -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 */
> 
> i.e. here.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Dave Chinner]

On Thu, Dec 12, 2013 at 05:02:57PM -0700, Andreas Dilger wrote:
> On Dec 12, 2013, at 4:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Dec 12, 2013 at 04:25:59PM +0100, 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.
> >> 
> >> 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.
> > 
> > I'd prefer to just see the new physical length field always filled
> > out, regardless of whether it is a compressed extent or not. In
> > terms of backwards compatibility to userspace, it makes no
> > difference because the value of reserved/unused fields is undefined
> > by the API. Yes, the implementation zeros them, but there's nothing
> > in the documentation that says "reserved fields must be zero".
> > Hence I think we should just set it for every extent.
> 
> I'd actually thought the same thing while reading the patch, but I figured
> people would object because it implies that old kernels will return a
> physical length of 0 bytes (which might be valid) and badly-written tools
> will not work correctly on older kernels. 

Well, that's a problem regardless of whether new kernels return a
physical length by default or not. I think I'd prefer a flag that
says specifically whether the fe_phys_len field is valid or not. Old
kernels will never set the flag, new kernels can always set the
flag...

> That said, applications _should_
> be checking the FIEMAP_EXTENT_DATA_COMPRESSED flag, and I suspect in the
> future fewer developers will be confused if fe_phys_length == fe_length
> going forward.

I think an explicit flag is better than relying on a flag that
defines the encoding to imply the physical length field is valid.

> If the initial tools get it right (in particular filefrag),

I'd think xfs_io is the first target - because we'll need xfstests
coverage of this before there's a filefrag release that supports
it...

> then hopefully others will get it correct also.

Agreed.

> > From the point of view of the kernel API (fiemap_fill_next_extent),
> > passing the physical extent size in the "len" parameter for normal
> > extents, then passing 0 for the "physical length" makes absolutely
> > no sense.
> > 
> > IOWs, what you have created is a distinction between the extent's
> > "logical length" and it's "physical length". For uncompressed
> > extents, they are both equal and they should both be passed to
> > fiemap_fill_next_extent as the same value. Extents where they are
> > different (i.e.  encoded extents) is when they can be different.
> > Perhaps fiemap_fill_next_extent() should check and warn about
> > mismatches when they differ and the relevant flags are not set...
> 
> Seems reasonable to have a WARN_ONCE() in that case.  That would catch bugs
> in the filesystem, code as well:
> 
> 	WARN_ONCE(phys_len != lgcl_len &&
> 		  !(flags & FIEMAP_EXTENT_DATA_COMPRESSED),
> 		  "physical len %llu != logical length %llu without DATA_COMPRESSED\n",
> 		  phys_len, logical_len, phys_len, logical_len);

Yup, pretty much what I was thinking.

> >> --- 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];
> >> };
> > 
> > The comment for fe_length needs to change, too, because it needs to
> > indicate that it is the logical extent length and that it may be
> > different to the fe_phys_length depending on the flags that are set
> > on the extent.
> 
> Would it make sense to rename fe_length to fe_logi_length (or something,
> I'm open to suggestions), and have a compat macro:
> 
> #define fe_length fe_logi_length
> 
> around for older applications?  That way, new developers would start to
> use the new name, old applications would still compile for both newer and
> older interfaces, and it doesn't affect the ABI at all.

Sounds like a good idea.

> > And, FWIW, I wouldn't mention specific flags in the comment here,
> > but do it at the definition of the flags that indicate there is
> > a difference between physical and logical extent lengths....
> 
> Actually, I was thinking just the opposite for this field.  It seems useful
> that the requirement for DATA_COMPRESSED being set is beside fe_phys_length
> so that anyone using this field sees the correlation clearly.  I don't expect
> everyone would read and understand the meaning of all the flags when looking
> at the data structure.

Well, it's moot if we decide a specific flag for the fe_phys_len
field being valid is decided on ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[David Sterba]

Thanks all for feedback, the changes sound ok to me. I'll send v4.

david

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 6373 bytes --]

David,
any progress on this patch series?

I never saw an updated version of this patch series after the last round of
reviews, but it would be great to move it forward.  I have filefrag patches
in my e2fsprogs tree waiting for an updated version of your patch.

I recall the main changes were:
- add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid
- rename fe_length to fe_logi_length and #define fe_length fe_logi_length
- always fill in fe_phys_length (= fe_logi_length for uncompressed files)
  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
- add WARN_ONCE() in fiemap_fill_next_extent() as described below

I don't know if there was any clear statement about whether there should be
separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags,
or if the latter should be implicit?  Probably makes sense to have separate
flags.  It should be fine to use:

#define FIEMAP_EXTENT_PHYS_LENGTH	0x00000010

since this flag was never used.

Cheers, Andreas

On Dec 12, 2013, at 5:02 PM, Andreas Dilger <adilger-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org> wrote:
> On Dec 12, 2013, at 4:24 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
>> On Thu, Dec 12, 2013 at 04:25:59PM +0100, 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.
>>> 
>>> 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.
>> 
>> I'd prefer to just see the new physical length field always filled
>> out, regardless of whether it is a compressed extent or not. In
>> terms of backwards compatibility to userspace, it makes no
>> difference because the value of reserved/unused fields is undefined
>> by the API. Yes, the implementation zeros them, but there's nothing
>> in the documentation that says "reserved fields must be zero".
>> Hence I think we should just set it for every extent.
> 
> I'd actually thought the same thing while reading the patch, but I figured
> people would object because it implies that old kernels will return a
> physical length of 0 bytes (which might be valid) and badly-written tools
> will not work correctly on older kernels.  That said, applications _should_
> be checking the FIEMAP_EXTENT_DATA_COMPRESSED flag, and I suspect in the
> future fewer developers will be confused if fe_phys_length == fe_length
> going forward.
> 
> If the initial tools get it right (in particular filefrag), then hopefully
> others will get it correct also.
> 
>> From the point of view of the kernel API (fiemap_fill_next_extent),
>> passing the physical extent size in the "len" parameter for normal
>> extents, then passing 0 for the "physical length" makes absolutely
>> no sense.
>> 
>> IOWs, what you have created is a distinction between the extent's
>> "logical length" and it's "physical length". For uncompressed
>> extents, they are both equal and they should both be passed to
>> fiemap_fill_next_extent as the same value. Extents where they are
>> different (i.e.  encoded extents) is when they can be different.
>> Perhaps fiemap_fill_next_extent() should check and warn about
>> mismatches when they differ and the relevant flags are not set...
> 
> Seems reasonable to have a WARN_ONCE() in that case.  That would catch bugs
> in the filesystem, code as well:
> 
> 	WARN_ONCE(phys_len != lgcl_len &&
> 		  !(flags & FIEMAP_EXTENT_DATA_COMPRESSED),
> 		  "physical len %llu != logical length %llu without DATA_COMPRESSED\n",
> 		  phys_len, logical_len, phys_len, logical_len);
> 
>>> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
>>> index 93abfcd..0e32cae 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];
>>> };
>> 
>> The comment for fe_length needs to change, too, because it needs to
>> indicate that it is the logical extent length and that it may be
>> different to the fe_phys_length depending on the flags that are set
>> on the extent.
> 
> Would it make sense to rename fe_length to fe_logi_length (or something,
> I'm open to suggestions), and have a compat macro:
> 
> #define fe_length fe_logi_length
> 
> around for older applications?  That way, new developers would start to
> use the new name, old applications would still compile for both newer and
> older interfaces, and it doesn't affect the ABI at all.
> 
>> And, FWIW, I wouldn't mention specific flags in the comment here,
>> but do it at the definition of the flags that indicate there is
>> a difference between physical and logical extent lengths....
> 
> Actually, I was thinking just the opposite for this field.  It seems useful
> that the requirement for DATA_COMPRESSED being set is beside fe_phys_length
> so that anyone using this field sees the correlation clearly.  I don't expect
> everyone would read and understand the meaning of all the flags when looking
> at the data structure.
> 
> Cheers, Andreas
> 
>>> @@ -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 */
>> 
>> i.e. here.
>> 
>> Cheers,
>> 
>> Dave.
>> -- 
>> Dave Chinner
>> david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[David Sterba]

On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote:
> any progress on this patch series?

I'm sorry I got distracted at the end of year and did not finish the
series.

> I never saw an updated version of this patch series after the last round of
> reviews, but it would be great to move it forward.  I have filefrag patches
> in my e2fsprogs tree waiting for an updated version of your patch.
> 
> I recall the main changes were:
> - add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid

fe_phys_length will be always valid, so other the flags are set only if it's
not equal to the logical length.

> - rename fe_length to fe_logi_length and #define fe_length fe_logi_length
> - always fill in fe_phys_length (= fe_logi_length for uncompressed files)
>   and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not

This is my understanding and contradicts the first point.

> - add WARN_ONCE() in fiemap_fill_next_extent() as described below
>
> I don't know if there was any clear statement about whether there should be
> separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags,
> or if the latter should be implicit?  Probably makes sense to have separate
> flags.  It should be fine to use:
>
> #define FIEMAP_EXTENT_PHYS_LENGTH	0x00000010
> 
> since this flag was never used.

I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for
FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the
FIEMAP_EXTENT_DATA_ENCODED is also implied.

I'll send V4, we can discuss the PHYS_LENGTH flag then.
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]


On Jul 24, 2014, at 1:22 PM, David Sterba <dsterba@suse.cz> wrote:
> On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote:
>> any progress on this patch series?
> 
> I'm sorry I got distracted at the end of year and did not finish the
> series.
> 
>> I never saw an updated version of this patch series after the last round of
>> reviews, but it would be great to move it forward.  I have filefrag patches
>> in my e2fsprogs tree waiting for an updated version of your patch.
>> 
>> I recall the main changes were:
>> - add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid
> 
> fe_phys_length will be always valid, so other the flags are set only if it's
> not equal to the logical length.
> 
>> - rename fe_length to fe_logi_length and #define fe_length fe_logi_length
>> - always fill in fe_phys_length (= fe_logi_length for uncompressed files)
>>  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
> 
> This is my understanding and contradicts the first point.

I think Dave Chinner's former point was that having fe_phys_length validity
depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface.  It is
not true that fe_phys_length would always be valid, since that is not the
case for older kernels that currently always set this field to 0, so they
need some flag to indicate if fe_phys_length is valid.  Alternately,
userspace could do:

	if (ext->fe_phys_length == 0)
		ext->fe_phys_length = ext->fe_logi_length;

but that pre-supposes that fe_phys_length == 0 is never a valid value when
fe_logi_length is non-zero, and this might introduce errors in some cases.
I could imagine that some compression methods might not allocate any space
at all if it was all zeroes, and just store a bit in the blockpointer or
extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer
in the long run.  That opens up the question of whether a written zero
filled space that gets compressed away is different from a hole, but I'd
prefer to just return whatever the file mapping is than interpret it.

Cheers, Andreas

>> - add WARN_ONCE() in fiemap_fill_next_extent() as described below
>> 
>> I don't know if there was any clear statement about whether there should be
>> separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags,
>> or if the latter should be implicit?  Probably makes sense to have separate
>> flags.  It should be fine to use:
>> 
>> #define FIEMAP_EXTENT_PHYS_LENGTH	0x00000010
>> 
>> since this flag was never used.
> 
> I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for
> FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the
> FIEMAP_EXTENT_DATA_ENCODED is also implied.
> 
> I'll send V4, we can discuss the PHYS_LENGTH flag then.


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Rohan Puri]

On Fri, Jul 25, 2014 at 4:04 AM, Andreas Dilger <adilger-m1MBpc4rdrD3fQ9qLvQP4Q@public.gmane.org> wrote:
>
> On Jul 24, 2014, at 1:22 PM, David Sterba <dsterba-AlSwsSmVLrQ@public.gmane.org> wrote:
>> On Thu, Jul 17, 2014 at 12:07:57AM -0600, Andreas Dilger wrote:
>>> any progress on this patch series?
>>
>> I'm sorry I got distracted at the end of year and did not finish the
>> series.
>>
>>> I never saw an updated version of this patch series after the last round of
>>> reviews, but it would be great to move it forward.  I have filefrag patches
>>> in my e2fsprogs tree waiting for an updated version of your patch.
>>>
>>> I recall the main changes were:
>>> - add FIEMAP_EXTENT_PHYS_LENGTH flag to indicate if fe_phys_length was valid
>>
>> fe_phys_length will be always valid, so other the flags are set only if it's
>> not equal to the logical length.
>>
>>> - rename fe_length to fe_logi_length and #define fe_length fe_logi_length
>>> - always fill in fe_phys_length (= fe_logi_length for uncompressed files)
>>>  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
>>
>> This is my understanding and contradicts the first point.
>
> I think Dave Chinner's former point was that having fe_phys_length validity
> depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface.  It is
> not true that fe_phys_length would always be valid, since that is not the
> case for older kernels that currently always set this field to 0, so they
> need some flag to indicate if fe_phys_length is valid.  Alternately,
> userspace could do:
>
>         if (ext->fe_phys_length == 0)
>                 ext->fe_phys_length = ext->fe_logi_length;
>
> but that pre-supposes that fe_phys_length == 0 is never a valid value when
> fe_logi_length is non-zero, and this might introduce errors in some cases.
> I could imagine that some compression methods might not allocate any space
> at all if it was all zeroes, and just store a bit in the blockpointer or
> extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer
> in the long run.
zfs is an example of this.
> That opens up the question of whether a written zero
> filled space that gets compressed away is different from a hole, but I'd
> prefer to just return whatever the file mapping is than interpret it.
>
> Cheers, Andreas
>
>>> - add WARN_ONCE() in fiemap_fill_next_extent() as described below
>>>
>>> I don't know if there was any clear statement about whether there should be
>>> separate FIEMAP_EXTENT_PHYS_LENGTH and FIEMAP_EXTENT_DATA_COMPRESSED flags,
>>> or if the latter should be implicit?  Probably makes sense to have separate
>>> flags.  It should be fine to use:
>>>
>>> #define FIEMAP_EXTENT_PHYS_LENGTH    0x00000010
>>>
>>> since this flag was never used.
>>
>> I've kept only FIEMAP_EXTENT_DATA_COMPRESSED, I don't see a need for
>> FIEMAP_EXTENT_PHYS_LENGTH and this would be yet another flag because the
>> FIEMAP_EXTENT_DATA_ENCODED is also implied.
>>
>> I'll send V4, we can discuss the PHYS_LENGTH flag then.
>
>
> Cheers, Andreas
>
>
>
>
>

Regards,
Rohan
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Ocfs2-devel] [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[David Sterba]

Thanks for the feedback.

On Thu, Jul 24, 2014 at 04:34:35PM -0600, Andreas Dilger wrote:
> >> - rename fe_length to fe_logi_length and #define fe_length fe_logi_length
> >> - always fill in fe_phys_length (= fe_logi_length for uncompressed files)
> >>  and set FIEMAP_EXTENT_PHYS_LENGTH whether the extent is compressed or not
> > 
> > This is my understanding and contradicts the first point.
> 
> I think Dave Chinner's former point was that having fe_phys_length validity
> depend on FIEMAP_EXTENT_DATA_COMPRESSED is a non-intuitive interface.  It is
> not true that fe_phys_length would always be valid, since that is not the
> case for older kernels that currently always set this field to 0, so they
> need some flag to indicate if fe_phys_length is valid.

I see the backward compatibility issue now. The patches (up to v4)
updated all filesystems to fill the physical length with logical,
but this should be 0 to keep the backward compatibility and also to keep
the changes to existing fiemap users minimal.

So for v5, PHYS_LENGTH will be introduced but only used by btrfs
together with ENCODED and COMPRESSED. Though this may look too much for
my needs to represent compressed extent, it is flexible for future use.

> Alternately,
> userspace could do:
> 
> 	if (ext->fe_phys_length == 0)
> 		ext->fe_phys_length = ext->fe_logi_length;
> 
> but that pre-supposes that fe_phys_length == 0 is never a valid value when
> fe_logi_length is non-zero, and this might introduce errors in some cases.
> I could imagine that some compression methods might not allocate any space
> at all if it was all zeroes, and just store a bit in the blockpointer or
> extent, so having a separate FIEMAP_EXTENT_PHYS_LENGTH is probably safer
> in the long run.

Sounds good to me.

> That opens up the question of whether a written zero
> filled space that gets compressed away is different from a hole, but I'd
> prefer to just return whatever the file mapping is than interpret it.

It could save some bytes, but I don' see it too practical. I expect the
amount of zeroed blocks to be low on average and the current compression
methods are able to squeeze long runs of zeros to a few tens of bytes
per page.

Re: [PATCH 2/4 v3] fiemap: add EXTENT_DATA_COMPRESSED flag

[Christoph Hellwig]

On Fri, Dec 13, 2013 at 10:24:43AM +1100, Dave Chinner wrote:
> I'd prefer to just see the new physical length field always filled
> out, regardless of whether it is a compressed extent or not. In
> terms of backwards compatibility to userspace, it makes no
> difference because the value of reserved/unused fields is undefined
> by the API. Yes, the implementation zeros them, but there's nothing
> in the documentation that says "reserved fields must be zero".
> Hence I think we should just set it for every extent.
> 
> >From the point of view of the kernel API (fiemap_fill_next_extent),
> passing the physical extent size in the "len" parameter for normal
> extents, then passing 0 for the "physical length" makes absolutely
> no sense.

I tend to agree, but the additional complication here is that this is
a change to an existing API.  We'd need another HAVE_PHYS_LEN flag for
non-compressed extents so that userspace can rely on it.  Given that
it's only useful for that case I think the userspace API introduced
is the best we can get.

I think however that we should always pass the phys_len argument in the
kernel API just to make it less confusing.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 5ea0ef5..8a28f15 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4158,6 +4158,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)
@@ -4182,6 +4183,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;
 
@@ -4220,9 +4222,12 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 			if (ref_cnt > 1)
 				flags |= FIEMAP_EXTENT_SHARED;
+			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;
@@ -4244,7 +4249,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			end = 1;
 		}
 		ret = fiemap_fill_next_extent(fieinfo, em_start, disko,
-					      em_len, 0, flags);
+					      em_len, em_phys_len, flags);
 		if (ret)
 			goto out_free;
 	}
-- 
1.7.9

Re: [PATCH 3/4 v3] btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Dec 12, 2013, at 8:26 AM, David Sterba <dsterba@suse.cz> wrote:
> 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 |    9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 5ea0ef5..8a28f15 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> 
> @@ -4220,9 +4222,12 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> 
> 			if (ref_cnt > 1)
> 				flags |= FIEMAP_EXTENT_SHARED;
> +			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;

(minor nit) Could combine these (not sure it makes a difference to the compiler):

			flags |= (FIEMAP_EXTENT_ENCODED |
				  FIEMAP_EXTENT_DATA_COMPRESSED);

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/4 v3] Documentation/fiemap: Document the DATA_COMPRESSED flag

[David Sterba]

CC: linux-doc@vger.kernel.org
CC: Rob Landley <rob@landley.net>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 Documentation/filesystems/fiemap.txt |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
index 1b805a0..46bfc1c 100644
--- a/Documentation/filesystems/fiemap.txt
+++ b/Documentation/filesystems/fiemap.txt
@@ -80,10 +80,12 @@ returned in fm_extents.
 struct fiemap_extent {
 	__u64	fe_logical;  /* logical offset in bytes for the start of
 			      * the extent */
-	__u64	fe_physical; /* physical offset in bytes for the start
-			      * of the 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 the 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];
 };
@@ -93,7 +95,8 @@ for an extents logical offset to start before the request or its logical
 length to extend past the request.  Unless FIEMAP_EXTENT_NOT_ALIGNED is
 returned, fe_logical, fe_physical, and fe_length will be aligned to the
 block size of the file system.  With the exception of extents flagged as
-FIEMAP_EXTENT_MERGED, adjacent extents will not be merged.
+FIEMAP_EXTENT_MERGED, adjacent extents will not be merged.  The value of
+fe_phys_length is valid only if FIEMAP_EXTENT_DATA_COMPRESSED is set.
 
 The fe_flags field contains flags which describe the extent returned.
 A special flag, FIEMAP_EXTENT_LAST is always set on the last extent in
@@ -143,6 +146,12 @@ unmounted, and then only if the FIEMAP_EXTENT_ENCODED flag is
 clear; user applications must not try reading or writing to the
 filesystem via the block device under any other circumstances.
 
+* FIEMAP_EXTENT_DATA_COMPRESSED
+  - This will also set FIEMAP_EXTENT_ENCODED
+The data in this extent has been compressed by the file system. When
+set, fe_phys_length contains the physical extent length in bytes. The
+value may be rounded depending on the filesystem implementation.
+
 * FIEMAP_EXTENT_DATA_ENCRYPTED
   - This will also set FIEMAP_EXTENT_ENCODED
 The data in this extent has been encrypted by the file system.
-- 
1.7.9

Re: [PATCH 0/4 v3] fiemap: introduce EXTENT_DATA_COMPRESSED flag

[Andreas Dilger]

[-- Attachment #1.1: Type: text/plain, Size: 2574 bytes --]

On Dec 12, 2013, at 8:25 AM, David Sterba <dsterba@suse.cz> 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.

The whole series looks good to me (one minor nit if it needs to be resubmitted
for some reason).  You can add my:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> V3:
> Based on feedback from Andreas, implement #1 from V2, current users of
> fiemap_fill_next_extent (fs/, ext4, gfs2, ocfs2, nilfs2, xfs) updated
> accordingly, no functional change.
> 
> 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
> 
> David Sterba (4):
>  fiemap: fix comment at EXTENT_DATA_ENCRYPTED
>  fiemap: add EXTENT_DATA_COMPRESSED flag
>  btrfs: set FIEMAP_EXTENT_DATA_COMPRESSED for compressed extents
>  Documentation/fiemap: Document the DATA_COMPRESSED flag
> 
> Documentation/filesystems/fiemap.txt |   17 +++++++++++++----
> fs/btrfs/extent_io.c                 |    9 +++++++--
> fs/ext4/extents.c                    |    3 ++-
> fs/ext4/inline.c                     |    2 +-
> fs/gfs2/inode.c                      |    2 +-
> fs/ioctl.c                           |   18 ++++++++++++------
> fs/nilfs2/inode.c                    |    8 +++++---
> fs/ocfs2/extent_map.c                |    4 ++--
> fs/xfs/xfs_iops.c                    |    2 +-
> include/linux/fs.h                   |    2 +-
> include/uapi/linux/fiemap.h          |    8 ++++++--
> 11 files changed, 51 insertions(+), 24 deletions(-)
> 
> -- 
> 1.7.9
> 


Cheers, Andreas






[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs