[PATCH v2 0/2] btrfs fiemap related BUG fix.

[PATCH v2 0/2] btrfs fiemap related BUG fix.

[robbieko]

From: Robbie Ko <robbieko@synology.com>

This patchset intends to fix btrfs fiemap related bug.

The fiemap has the following problems:

1) fiemap: pass correct bytenr when fm_extent_count is zero
 When user space wants to get the number of file extents,
 set fm_extent_count to 0 to run fiemap and then read fm_mapped_extents.

 In the above example, fiemap will return with fm_mapped_extents set to 4,
 but it should be 1 since there's only one entry in the output.

 Details can refer to "[PATCH v2 1/2] Btrfs: fiemap: pass correct bytenr
 when fm_extent_count is zero"

2) fiemap extent SHARED flag error with range clone
 Currently, only the first extent is checked for shared in extent_map.

 Here we will check each extent with extent map range, if one of them
 is shared, extent map is shared.

 Details can refer to "[PATCH v2 2/2] Btrfs: fix fiemap extent SHARED
 flag error with range clone."

Robbie Ko (2):
  Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero
  Btrfs: fix fiemap extent SHARED flag error with range clone.

 fs/btrfs/extent_io.c | 150 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 132 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero

[robbieko]

From: Robbie Ko <robbieko@synology.com>

[BUG]
fm_mapped_extents is not correct when fm_extent_count is 0
Like:
   # mount /dev/vdb5 /mnt/btrfs
   # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file
   # xfs_io -c "fiemap -v" /mnt/btrfs/file
   /mnt/btrfs/file:
   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
     0: [0..127]:        25088..25215       128   0x1

When user space wants to get the number of file extents,
set fm_extent_count to 0 to run fiemap and then read fm_mapped_extents.

In the above example, fiemap will return with fm_mapped_extents set to 4,
but it should be 1 since there's only one entry in the output.

[REASON]
The problem seems to be that disko is only set if
fieinfo->fi_extents_max is set. And this member is initialized, in the
generic ioctl_fiemap function, to the value of used-passed
fm_extent_count. So when the user passes 0 then fi_extent_max is also
set to zero and this causes btrfs to not initialize disko at all.
Eventually this leads emit_fiemap_extent being called with a bogus
'phys' argument preventing proper fiemap entries merging.

[FIX]
Move the disko initialization earlier in extent_fiemap making it
independent of user-passed arguments, allowing emit_fiemap_extent to
properly handle consecutive extent entries.

Signed-off-by: Robbie Ko <robbieko@synology.com>
---
V2:
fix comments.

 fs/btrfs/extent_io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 012d638..066b6df 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4567,7 +4567,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			offset_in_extent = em_start - em->start;
 		em_end = extent_map_end(em);
 		em_len = em_end - em_start;
-		disko = 0;
+		disko = em->block_start + offset_in_extent;
 		flags = 0;
 
 		/*
@@ -4590,8 +4590,6 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			u64 bytenr = em->block_start -
 				(em->start - em->orig_start);
 
-			disko = em->block_start + offset_in_extent;
-
 			/*
 			 * As btrfs supports shared space, this information
 			 * can be exported to userspace tools via
-- 
1.9.1

[PATCH v2 2/2] Btrfs: fix fiemap extent SHARED flag error with range clone.

[robbieko]

From: Robbie Ko <robbieko@synology.com>

[BUG]
Range clone can cause fiemap to return error result.
Like:
 # mount /dev/vdb5 /mnt/btrfs
 # dd if=/dev/zero bs=16K count=2 oflag=dsync of=/mnt/btrfs/file

 # btrfs-debugfs -f file
 (276 0): ram 16384 disk 2171609088 disk_size 16384
 (276 16384): ram 16384 disk 2171625472 disk_size 16384
 (276 32768): ram 16384 disk 2171641856 disk_size 16384
 (276 49152): ram 16384 disk 2171658240 disk_size 16384
 file: file extents 4 disk size 65536 logical size 65536 ratio 1.00

 # xfs_io -c "fiemap -v" /mnt/btrfs/file
 /mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..63]:         4241424..4241487    64   0x1

 # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone
 # xfs_io -c "fiemap -v" /mnt/btrfs/file
 /mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..63]:         4241424..4241487    64   0x1
 If we clone second file extent, we will get error FLAGS,
 SHARED bit is not set.

[REASON]
Btrfs only checks if the first extent in extent map is shared,
but in fact it can correspond to many extents and each one has
the potential to be cloned so we need to examine each one individually.

[FIX]
Here we will check each extent with extent map range,
if one of them is shared, extent map is shared.

[PATCH RESULT]
 # xfs_io -c "fiemap -v" /mnt/btrfs/file
 /mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..63]:         4241424..4241487    64 0x2001

Signed-off-by: Robbie Ko <robbieko@synology.com>
---
V2:
fix comments and coding style.


 fs/btrfs/extent_io.c | 147 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 132 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 066b6df..b2267dd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4394,8 +4394,8 @@ static int emit_fiemap_extent(struct fiemap_extent_info *fieinfo,
 	 */
 	if (cache->offset + cache->len  == offset &&
 	    cache->phys + cache->len == phys  &&
-	    (cache->flags & ~FIEMAP_EXTENT_LAST) ==
-			(flags & ~FIEMAP_EXTENT_LAST)) {
+		(cache->flags & ~(FIEMAP_EXTENT_LAST|FIEMAP_EXTENT_SHARED)) ==
+			(flags & ~(FIEMAP_EXTENT_LAST|FIEMAP_EXTENT_SHARED))) {
 		cache->len += len;
 		cache->flags |= flags;
 		goto try_submit_last;
@@ -4450,6 +4450,134 @@ static int emit_last_fiemap_cache(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
+/*
+ * Helper to check the file range is shared.
+ *
+ * Fiemap extent will be combined with many extents, so we need to examine
+ * each extent, and if shared, the results are shared.
+ *
+ * Return: 0 if file range is not shared, 1 if it is shared, < 0 on error.
+ */
+static int extent_map_check_shared(struct inode *inode, u64 start, u64 end)
+{
+	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+	int ret = 0;
+	struct extent_buffer *leaf;
+	struct btrfs_path *path;
+	struct btrfs_file_extent_item *fi;
+	struct btrfs_key found_key;
+	int check_prev = 1;
+	int extent_type;
+	int shared = 0;
+	u64 cur_offset;
+	u64 extent_end;
+	u64 ino = btrfs_ino(BTRFS_I(inode));
+	u64 disk_bytenr;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	cur_offset = start;
+	while (1) {
+		ret = btrfs_lookup_file_extent(NULL, root, path, ino,
+					       cur_offset, 0);
+		if (ret < 0)
+			goto error;
+		if (ret > 0 && path->slots[0] > 0 && check_prev) {
+			leaf = path->nodes[0];
+			btrfs_item_key_to_cpu(leaf, &found_key,
+					      path->slots[0] - 1);
+			if (found_key.objectid == ino &&
+			    found_key.type == BTRFS_EXTENT_DATA_KEY)
+				path->slots[0]--;
+		}
+		check_prev = 0;
+next_slot:
+		leaf = path->nodes[0];
+		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
+			ret = btrfs_next_leaf(root, path);
+			if (ret < 0)
+				goto error;
+			if (ret > 0)
+				break;
+			leaf = path->nodes[0];
+		}
+
+		disk_bytenr = 0;
+		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
+
+		if (found_key.objectid > ino)
+			break;
+		if (WARN_ON_ONCE(found_key.objectid < ino) ||
+		    found_key.type < BTRFS_EXTENT_DATA_KEY) {
+			path->slots[0]++;
+			goto next_slot;
+		}
+		if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
+		    found_key.offset > end)
+			break;
+
+		fi = btrfs_item_ptr(leaf, path->slots[0],
+				    struct btrfs_file_extent_item);
+		extent_type = btrfs_file_extent_type(leaf, fi);
+
+		if (extent_type == BTRFS_FILE_EXTENT_REG ||
+		    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
+			disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
+			extent_end = found_key.offset +
+				btrfs_file_extent_num_bytes(leaf, fi);
+			if (extent_end <= start) {
+				path->slots[0]++;
+				goto next_slot;
+			}
+			if (disk_bytenr == 0) {
+				path->slots[0]++;
+				goto next_slot;
+			}
+
+			btrfs_release_path(path);
+
+			/*
+			 * As btrfs supports shared space, this information
+			 * can be exported to userspace tools via
+			 * flag FIEMAP_EXTENT_SHARED.  If fi_extents_max == 0
+			 * then we're just getting a count and we can skip the
+			 * lookup stuff.
+			 */
+			ret = btrfs_check_shared(root,
+						 ino, disk_bytenr);
+			if (ret < 0)
+				goto error;
+			if (ret)
+				shared = 1;
+			ret = 0;
+			if (shared)
+				break;
+		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
+			extent_end = found_key.offset +
+				btrfs_file_extent_inline_len(leaf,
+						     path->slots[0], fi);
+			extent_end = ALIGN(extent_end, fs_info->sectorsize);
+			path->slots[0]++;
+			goto next_slot;
+		} else {
+			WARN_ON(1);
+			ret = -EIO;
+			goto error;
+		}
+		cur_offset = extent_end;
+		if (cur_offset > end)
+			break;
+	}
+
+	ret = 0;
+error:
+	btrfs_free_path(path);
+	return !ret ? shared : ret;
+}
+
 int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		__u64 start, __u64 len, get_extent_t *get_extent)
 {
@@ -4587,19 +4715,8 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			flags |= (FIEMAP_EXTENT_DELALLOC |
 				  FIEMAP_EXTENT_UNKNOWN);
 		} else if (fieinfo->fi_extents_max) {
-			u64 bytenr = em->block_start -
-				(em->start - em->orig_start);
-
-			/*
-			 * As btrfs supports shared space, this information
-			 * can be exported to userspace tools via
-			 * flag FIEMAP_EXTENT_SHARED.  If fi_extents_max == 0
-			 * then we're just getting a count and we can skip the
-			 * lookup stuff.
-			 */
-			ret = btrfs_check_shared(root,
-						 btrfs_ino(BTRFS_I(inode)),
-						 bytenr);
+			ret = extent_map_check_shared(inode, em->start,
+							extent_map_end(em) - 1);
 			if (ret < 0)
 				goto out_free;
 			if (ret)
-- 
1.9.1

Re: [PATCH v2 1/2] Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero

[Nikolay Borisov]

On  7.05.2018 11:42, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
> [BUG]
> fm_mapped_extents is not correct when fm_extent_count is 0
> Like:
>    # mount /dev/vdb5 /mnt/btrfs
>    # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file
>    # xfs_io -c "fiemap -v" /mnt/btrfs/file
>    /mnt/btrfs/file:
>    EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>      0: [0..127]:        25088..25215       128   0x1
> 
> When user space wants to get the number of file extents,
> set fm_extent_count to 0 to run fiemap and then read fm_mapped_extents.
 "it sets fm_extent_count to 0.... and then reads fm_mapped_extents"

> 
> In the above example, fiemap will return with fm_mapped_extents set to 4,
> but it should be 1 since there's only one entry in the output.
> 
> [REASON]
> The problem seems to be that disko is only set if
> fieinfo->fi_extents_max is set. And this member is initialized, in the
> generic ioctl_fiemap function, to the value of used-passed
                                                  ^^^^
                                                  user-passed
> fm_extent_count. So when the user passes 0 then fi_extent_max is also
> set to zero and this causes btrfs to not initialize disko at all.
> Eventually this leads emit_fiemap_extent being called with a bogus
> 'phys' argument preventing proper fiemap entries merging.
> 
> [FIX]
> Move the disko initialization earlier in extent_fiemap making it
> independent of user-passed arguments, allowing emit_fiemap_extent to
> properly handle consecutive extent entries.
> 
> Signed-off-by: Robbie Ko <robbieko@synology.com>
Only a couple of minor nits w.r.t the changelog but I guess David can
fix this on the go. In any case :

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> V2:
> fix comments.
> 
>  fs/btrfs/extent_io.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 012d638..066b6df 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4567,7 +4567,7 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  			offset_in_extent = em_start - em->start;
>  		em_end = extent_map_end(em);
>  		em_len = em_end - em_start;
> -		disko = 0;
> +		disko = em->block_start + offset_in_extent;
>  		flags = 0;
>  
>  		/*
> @@ -4590,8 +4590,6 @@ int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  			u64 bytenr = em->block_start -
>  				(em->start - em->orig_start);
>  
> -			disko = em->block_start + offset_in_extent;
> -
>  			/*
>  			 * As btrfs supports shared space, this information
>  			 * can be exported to userspace tools via
>