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

[PATCH 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) Wrong extent count when fm_extent_count is zero.


2) SHARED bit is not correct
I have two ideas, but I do not know which one is the best.

Like:
 # dd if=/dev/zero bs=16K count=2 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..63]:         4241424..4241487    64   0x1
 # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone

1. When any extent is shared in extent map, the entire extent map is shared
 # 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

2. Split into different extent
 # xfs_io -c "fiemap -v" /mnt/btrfs/file
     /mnt/btrfs/file:
     EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
       0: [0..31]:         4241424..4241455   32  0x0
       1: [32..63]:        4241456..4241487   32  0x2001

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 1/2] Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero

[robbieko]

From: Robbie Ko <robbieko@synology.com>

 # 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

Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
instead of 1.

[REASON]
When fm_extent_count is 0, disko is not initialized correctly,
The value is 0 in this case, not the right bytenr.

[FIX]
Use correct disko.

Signed-off-by: Robbie Ko <robbieko@synology.com>
---
 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 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
 # 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 extent may merge.

[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>
---
 fs/btrfs/extent_io.c | 146 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 131 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 066b6df..5c6dca9 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 {
+			BUG_ON(1);
+		}
+		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,7 @@ 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 0/2] btrfs fiemap related BUG fix.

[Qu Wenruo]

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



On 2018年03月07日 16:20, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
> This patchset intends to fix btrfs fiemap related bug.
> 
> The fiemap has the following problems:
> 
> 1) Wrong extent count when fm_extent_count is zero.
> 
> 
> 2) SHARED bit is not correct
> I have two ideas, but I do not know which one is the best.
> 
> Like:
>  # dd if=/dev/zero bs=16K count=2 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..63]:         4241424..4241487    64   0x1
>  # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone
> 
> 1. When any extent is shared in extent map, the entire extent map is shared
>  # 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

I think this is what btrfs is doing right now.
Although I'm not sure if this is the best solution.

BTW, I just did the same operation, and just get the SHARED flag on both
source and destination.

Is there something wrong?

> 
> 2. Split into different extent
>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>      /mnt/btrfs/file:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..31]:         4241424..4241455   32  0x0
>        1: [32..63]:        4241456..4241487   32  0x2001

This is what XFS does.

Thanks,
Qu

> 
> 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[robbieko]

Qu Wenruo 於 2018-03-07 17:27 寫到:
> On 2018年03月07日 16:20, robbieko wrote:
>> From: Robbie Ko <robbieko@synology.com>
>> 
>> This patchset intends to fix btrfs fiemap related bug.
>> 
>> The fiemap has the following problems:
>> 
>> 1) Wrong extent count when fm_extent_count is zero.
>> 
>> 
>> 2) SHARED bit is not correct
>> I have two ideas, but I do not know which one is the best.
>> 
>> Like:
>>  # dd if=/dev/zero bs=16K count=2 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..63]:         4241424..4241487    64   0x1
>>  # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone
>> 
>> 1. When any extent is shared in extent map, the entire extent map is 
>> shared
>>  # 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
> 
> I think this is what btrfs is doing right now.
> Although I'm not sure if this is the best solution.
> 
> BTW, I just did the same operation, and just get the SHARED flag on 
> both
> source and destination.
> 
> Is there something wrong?
> 

Currently, only the first extent is checked for shared in extent_map.
Details can refer to "[PATCH 2/2] Btrfs: fix fiemap extent SHARED flag 
error with range clone."


>> 
>> 2. Split into different extent
>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>>      /mnt/btrfs/file:
>>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>        0: [0..31]:         4241424..4241455   32  0x0
>>        1: [32..63]:        4241456..4241487   32  0x2001
> 
> This is what XFS does.
> 
> Thanks,
> Qu
> 
>> 
>> 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
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

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

[Nikolay Borisov]

On  7.03.2018 10:20, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
>  # 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
> 
> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
> instead of 1.

Wrong value 4 instead of 1 for which exact column, the flags? State this
explicitly.

Also this seems a bit bogus since fiemap's documentation states:

If fm_extent_count is zero, then the fm_extents[] array is ignored (no
extents will be returned), and the fm_mapped_extents count will hold the
number of extents needed in fm_extents[] to hold the file's current mapping.

So when fm_extent_count we shouldn't really be returning anything from
kernel.


> 
> [REASON]
> When fm_extent_count is 0, disko is not initialized correctly,
> The value is 0 in this case, not the right bytenr.

This is too sparse, be more explicit i.e. that disko=0 is passed to
emit_fiemap_extent which then leads to issues.

> 
> [FIX]
> Use correct disko.
> 
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
>  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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[robbieko]

Nikolay Borisov 於 2018-03-07 18:19 寫到:
> On  7.03.2018 10:20, robbieko wrote:
>> From: Robbie Ko <robbieko@synology.com>
>> 
>>  # 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
>> 
>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
>> instead of 1.
> 
> Wrong value 4 instead of 1 for which exact column, the flags? State 
> this
> explicitly.
> 
> Also this seems a bit bogus since fiemap's documentation states:
> 
> If fm_extent_count is zero, then the fm_extents[] array is ignored (no
> extents will be returned), and the fm_mapped_extents count will hold 
> the
> number of extents needed in fm_extents[] to hold the file's current 
> mapping.
> 
> So when fm_extent_count we shouldn't really be returning anything from
> kernel.
> 

Sorry I did not explain clearly.
The value is fm_mapped_extents.
If fm_extent_count  is zero, the fm_mapped_extents count will hold the 
number of extents needed.


> 
>> 
>> [REASON]
>> When fm_extent_count is 0, disko is not initialized correctly,
>> The value is 0 in this case, not the right bytenr.
> 
> This is too sparse, be more explicit i.e. that disko=0 is passed to
> emit_fiemap_extent which then leads to issues.
> 
>> 
>> [FIX]
>> Use correct disko.
>> 
>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>> ---
>>  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
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

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

[Qu Wenruo]

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



On 2018年03月07日 16:20, robbieko wrote:
> 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
>  # 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 extent may merge.
> 
> [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>
> ---
>  fs/btrfs/extent_io.c | 146 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 131 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 066b6df..5c6dca9 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 {
> +			BUG_ON(1);
> +		}
> +		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,7 @@ 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);

Since we're going to use the whole extent to determine the SHARED flags,
what about just passing the extent bytenr into btrfs_check_shared?

In that case I think we could get correct shared flag without using
another helper function.
(IIRC it's em->block_start)

Thanks,
Qu

> +			ret = extent_map_check_shared(inode, em->start, extent_map_end(em) - 1);
>  			if (ret < 0)
>  				goto out_free;
>  			if (ret)
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Qu Wenruo]

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



On 2018年03月07日 18:33, Qu Wenruo wrote:
> 
> 
> On 2018年03月07日 16:20, robbieko wrote:
>> 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
>>  # 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 extent may merge.
>>
>> [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>
>> ---
>>  fs/btrfs/extent_io.c | 146 +++++++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 066b6df..5c6dca9 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 {
>> +			BUG_ON(1);
>> +		}
>> +		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,7 @@ 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);
> 
> Since we're going to use the whole extent to determine the SHARED flags,
> what about just passing the extent bytenr into btrfs_check_shared?
> 
> In that case I think we could get correct shared flag without using
> another helper function.
> (IIRC it's em->block_start)

Well, it's not em->block_start, but a little more complex.
(For compressed one it's em->block_start, but for plaintext one, it's
more complex)

em->block_start = file_extent_disk_bytenr + file_extent_file_extent_offset

We need extra calculation to determine the real extent bytenr
(file_extent_disk_bytenr).

IIRC the correct calculation would be:
file_extent_disk_bytenr = em->block_start - em->start + em->orig_start.

(Who thought out such anti-human calculation for extent map?!)

Thanks,
Qu
> 
> Thanks,
> Qu
> 
>> +			ret = extent_map_check_shared(inode, em->start, extent_map_end(em) - 1);
>>  			if (ret < 0)
>>  				goto out_free;
>>  			if (ret)
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[robbieko]

Qu Wenruo 於 2018-03-07 18:42 寫到:
> On 2018年03月07日 18:33, Qu Wenruo wrote:
>> 
>> 
>> On 2018年03月07日 16:20, robbieko wrote:
>>> 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
>>>  # 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 extent may merge.
>>> 
>>> [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>
>>> ---
>>>  fs/btrfs/extent_io.c | 146 
>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 066b6df..5c6dca9 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 {
>>> +			BUG_ON(1);
>>> +		}
>>> +		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,7 @@ 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);
>> 
>> Since we're going to use the whole extent to determine the SHARED 
>> flags,
>> what about just passing the extent bytenr into btrfs_check_shared?
>> 
>> In that case I think we could get correct shared flag without using
>> another helper function.
>> (IIRC it's em->block_start)
> 
> Well, it's not em->block_start, but a little more complex.
> (For compressed one it's em->block_start, but for plaintext one, it's
> more complex)
> 
> em->block_start = file_extent_disk_bytenr + 
> file_extent_file_extent_offset
> 
> We need extra calculation to determine the real extent bytenr
> (file_extent_disk_bytenr).
> 
> IIRC the correct calculation would be:
> file_extent_disk_bytenr = em->block_start - em->start + em->orig_start.
> 
> (Who thought out such anti-human calculation for extent map?!)
> 
> Thanks,
> Qu

dd if=/dev/zero bs=16K count=4 oflag=dsync of=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" file
file:
  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
    0: [0..127]:        4241424..4241551   128   0x1

My point is that we have only one extent_map,
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.


>> 
>> Thanks,
>> Qu
>> 
>>> +			ret = extent_map_check_shared(inode, em->start, 
>>> extent_map_end(em) - 1);
>>>  			if (ret < 0)
>>>  				goto out_free;
>>>  			if (ret)
>>> --
>>> 1.9.1
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe 
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> 
>> 

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

[Nikolay Borisov]

On  7.03.2018 12:27, robbieko wrote:
> Nikolay Borisov 於 2018-03-07 18:19 寫到:
>> On  7.03.2018 10:20, robbieko wrote:
>>> From: Robbie Ko <robbieko@synology.com>
>>>
>>>  # 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
>>>
>>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
>>> instead of 1.
>>
>> Wrong value 4 instead of 1 for which exact column, the flags? State this
>> explicitly.
>>
>> Also this seems a bit bogus since fiemap's documentation states:
>>
>> If fm_extent_count is zero, then the fm_extents[] array is ignored (no
>> extents will be returned), and the fm_mapped_extents count will hold the
>> number of extents needed in fm_extents[] to hold the file's current
>> mapping.
>>
>> So when fm_extent_count we shouldn't really be returning anything from
>> kernel.
>>
> 
> Sorry I did not explain clearly.
> The value is fm_mapped_extents.

But fm_mapped_extents is tagged as an OUT member, meaning the user has
no job writing to it.

> If fm_extent_count  is zero, the fm_mapped_extents count will hold the
> number of extents needed.
> 
> 
>>
>>>
>>> [REASON]
>>> When fm_extent_count is 0, disko is not initialized correctly,
>>> The value is 0 in this case, not the right bytenr.
>>
>> This is too sparse, be more explicit i.e. that disko=0 is passed to
>> emit_fiemap_extent which then leads to issues.
>>
>>>
>>> [FIX]
>>> Use correct disko.
>>>
>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>> ---
>>>  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
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Qu Wenruo]

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



On 2018年03月07日 19:01, robbieko wrote:
> Qu Wenruo 於 2018-03-07 18:42 寫到:
>> On 2018年03月07日 18:33, Qu Wenruo wrote:
>>>
>>>
>>> On 2018年03月07日 16:20, robbieko wrote:
>>>> 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
>>>>  # 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 extent may merge.
>>>>
>>>> [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>
>>>> ---
>>>>  fs/btrfs/extent_io.c | 146
>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>> index 066b6df..5c6dca9 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 {
>>>> +            BUG_ON(1);
>>>> +        }
>>>> +        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,7 @@ 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);
>>>
>>> Since we're going to use the whole extent to determine the SHARED flags,
>>> what about just passing the extent bytenr into btrfs_check_shared?
>>>
>>> In that case I think we could get correct shared flag without using
>>> another helper function.
>>> (IIRC it's em->block_start)
>>
>> Well, it's not em->block_start, but a little more complex.
>> (For compressed one it's em->block_start, but for plaintext one, it's
>> more complex)
>>
>> em->block_start = file_extent_disk_bytenr +
>> file_extent_file_extent_offset
>>
>> We need extra calculation to determine the real extent bytenr
>> (file_extent_disk_bytenr).
>>
>> IIRC the correct calculation would be:
>> file_extent_disk_bytenr = em->block_start - em->start + em->orig_start.
>>
>> (Who thought out such anti-human calculation for extent map?!)
>>
>> Thanks,
>> Qu
> 
> dd if=/dev/zero bs=16K count=4 oflag=dsync of=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" file
> file:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..127]:        4241424..4241551   128   0x1
> 
> My point is that we have only one extent_map,
> 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.

Right, I missed this point.

SHARED flag is determined after extent map merge, so here we can't rely
on em here.

So the current helper function looks valid, unless we have method to
access the original extent map.

Thanks,
Qu

> 
> 
>>>
>>> Thanks,
>>> Qu
>>>
>>>> +            ret = extent_map_check_shared(inode, em->start,
>>>> extent_map_end(em) - 1);
>>>>              if (ret < 0)
>>>>                  goto out_free;
>>>>              if (ret)
>>>> -- 
>>>> 1.9.1
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-btrfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Nikolay Borisov]

On  7.03.2018 13:18, Qu Wenruo wrote:
> 
> 
> On 2018年03月07日 19:01, robbieko wrote:
>> Qu Wenruo 於 2018-03-07 18:42 寫到:
>>> On 2018年03月07日 18:33, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年03月07日 16:20, robbieko wrote:
>>>>> 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
>>>>>  # 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 extent may merge.
>>>>>
>>>>> [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>
>>>>> ---
>>>>>  fs/btrfs/extent_io.c | 146
>>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>> index 066b6df..5c6dca9 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 {
>>>>> +            BUG_ON(1);
>>>>> +        }
>>>>> +        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,7 @@ 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);
>>>>
>>>> Since we're going to use the whole extent to determine the SHARED flags,
>>>> what about just passing the extent bytenr into btrfs_check_shared?
>>>>
>>>> In that case I think we could get correct shared flag without using
>>>> another helper function.
>>>> (IIRC it's em->block_start)
>>>
>>> Well, it's not em->block_start, but a little more complex.
>>> (For compressed one it's em->block_start, but for plaintext one, it's
>>> more complex)
>>>
>>> em->block_start = file_extent_disk_bytenr +
>>> file_extent_file_extent_offset
>>>
>>> We need extra calculation to determine the real extent bytenr
>>> (file_extent_disk_bytenr).
>>>
>>> IIRC the correct calculation would be:
>>> file_extent_disk_bytenr = em->block_start - em->start + em->orig_start.
>>>
>>> (Who thought out such anti-human calculation for extent map?!)
>>>
>>> Thanks,
>>> Qu
>>
>> dd if=/dev/zero bs=16K count=4 oflag=dsync of=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" file
>> file:
>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>    0: [0..127]:        4241424..4241551   128   0x1
>>
>> My point is that we have only one extent_map,
>> 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.
> 
> Right, I missed this point.
> 
> SHARED flag is determined after extent map merge, so here we can't rely
> on em here.

Shouldn't extent maps correspond to 1:1 disk-state. I.e. they are just
the memory cache of the extent state. So if we merge them, shouldn't we
also merge the on-disk extents as well ?

> 
> So the current helper function looks valid, unless we have method to
> access the original extent map.
> 
> Thanks,
> Qu
> 
>>
>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>> +            ret = extent_map_check_shared(inode, em->start,
>>>>> extent_map_end(em) - 1);
>>>>>              if (ret < 0)
>>>>>                  goto out_free;
>>>>>              if (ret)
>>>>> -- 
>>>>> 1.9.1
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-btrfs" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>
> 

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

[Qu Wenruo]

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



On 2018年03月07日 19:27, Nikolay Borisov wrote:
> 
> 
> On  7.03.2018 13:18, Qu Wenruo wrote:
>>
>>
>> On 2018年03月07日 19:01, robbieko wrote:
>>> Qu Wenruo 於 2018-03-07 18:42 寫到:
>>>> On 2018年03月07日 18:33, Qu Wenruo wrote:
>>>>>
>>>>>
>>>>> On 2018年03月07日 16:20, robbieko wrote:
>>>>>> From: Robbie Ko <robbieko@synology.com>
>>>>>>
>>>>>> [BUG]
>>>>>> Range clone can cause fiemap to return error result.
>>>>>> Like:
>>>>>>  # mount /dev/vdb5 /mnt/btrfs
>>>>>>  # dd if=ev/zero bs\x16K count=2 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..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 extent may merge.
>>>>>>
>>>>>> [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>
>>>>>> ---
>>>>>>  fs/btrfs/extent_io.c | 146
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>>> index 066b6df..5c6dca9 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 +=en;
>>>>>>          cache->flags |=lags;
>>>>>>          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 =trfs_sb(inode->i_sb);
>>>>>> +    struct btrfs_root *root =TRFS_I(inode)->root;
>>>>>> +    int ret =;
>>>>>> +    struct extent_buffer *leaf;
>>>>>> +    struct btrfs_path *path;
>>>>>> +    struct btrfs_file_extent_item *fi;
>>>>>> +    struct btrfs_key found_key;
>>>>>> +    int check_prev =;
>>>>>> +    int extent_type;
>>>>>> +    int shared =;
>>>>>> +    u64 cur_offset;
>>>>>> +    u64 extent_end;
>>>>>> +    u64 ino =trfs_ino(BTRFS_I(inode));
>>>>>> +    u64 disk_bytenr;
>>>>>> +
>>>>>> +    path =trfs_alloc_path();
>>>>>> +    if (!path) {
>>>>>> +        return -ENOMEM;
>>>>>> +    }
>>>>>> +
>>>>>> +    cur_offset =tart;
>>>>>> +    while (1) {
>>>>>> +        ret =trfs_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 =ath->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 =;
>>>>>> +next_slot:
>>>>>> +        leaf =ath->nodes[0];
>>>>>> +        if (path->slots[0] >=trfs_header_nritems(leaf)) {
>>>>>> +            ret =trfs_next_leaf(root, path);
>>>>>> +            if (ret < 0)
>>>>>> +                goto error;
>>>>>> +            if (ret > 0)
>>>>>> +                break;
>>>>>> +            leaf =ath->nodes[0];
>>>>>> +        }
>>>>>> +
>>>>>> +        disk_bytenr =;
>>>>>> +        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 =trfs_item_ptr(leaf, path->slots[0],
>>>>>> +                    struct btrfs_file_extent_item);
>>>>>> +        extent_type =trfs_file_extent_type(leaf, fi);
>>>>>> +
>>>>>> +        if (extent_type =BTRFS_FILE_EXTENT_REG ||
>>>>>> +            extent_type =BTRFS_FILE_EXTENT_PREALLOC) {
>>>>>> +            disk_bytenr =trfs_file_extent_disk_bytenr(leaf, fi);
>>>>>> +            extent_end =ound_key.offset +
>>>>>> +                btrfs_file_extent_num_bytes(leaf, fi);
>>>>>> +            if (extent_end <=tart) {
>>>>>> +                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 =trfs_check_shared(root,
>>>>>> +                         ino, disk_bytenr);
>>>>>> +            if (ret < 0)
>>>>>> +                goto error;
>>>>>> +            if (ret)
>>>>>> +                shared =;
>>>>>> +            ret =;
>>>>>> +            if (shared) {
>>>>>> +                break;
>>>>>> +            }
>>>>>> +        } else if (extent_type =BTRFS_FILE_EXTENT_INLINE) {
>>>>>> +            extent_end =ound_key.offset +
>>>>>> +                btrfs_file_extent_inline_len(leaf,
>>>>>> +                             path->slots[0], fi);
>>>>>> +            extent_end =LIGN(extent_end, fs_info->sectorsize);
>>>>>> +            path->slots[0]++;
>>>>>> +            goto next_slot;
>>>>>> +        } else {
>>>>>> +            BUG_ON(1);
>>>>>> +        }
>>>>>> +        cur_offset =xtent_end;
>>>>>> +        if (cur_offset > end)
>>>>>> +            break;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret =;
>>>>>> +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,7 @@ 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 =m->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 =trfs_check_shared(root,
>>>>>> -                         btrfs_ino(BTRFS_I(inode)),
>>>>>> -                         bytenr);
>>>>>
>>>>> Since we're going to use the whole extent to determine the SHARED flags,
>>>>> what about just passing the extent bytenr into btrfs_check_shared?
>>>>>
>>>>> In that case I think we could get correct shared flag without using
>>>>> another helper function.
>>>>> (IIRC it's em->block_start)
>>>>
>>>> Well, it's not em->block_start, but a little more complex.
>>>> (For compressed one it's em->block_start, but for plaintext one, it's
>>>> more complex)
>>>>
>>>> em->block_start =ile_extent_disk_bytenr +
>>>> file_extent_file_extent_offset
>>>>
>>>> We need extra calculation to determine the real extent bytenr
>>>> (file_extent_disk_bytenr).
>>>>
>>>> IIRC the correct calculation would be:
>>>> file_extent_disk_bytenr =m->block_start - em->start + em->orig_start.
>>>>
>>>> (Who thought out such anti-human calculation for extent map?!)
>>>>
>>>> Thanks,
>>>> Qu
>>>
>>> dd if=ev/zero bs\x16K count=4 oflag=dsync of=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" file
>>> file:
>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>>    0: [0..127]:        4241424..4241551   128   0x1
>>>
>>> My point is that we have only one extent_map,
>>> 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.
>>
>> Right, I missed this point.
>>
>> SHARED flag is determined after extent map merge, so here we can't rely
>> on em here.
> 
> Shouldn't extent maps correspond to 1:1 disk-state. I.e. they are just
> the memory cache of the extent state. So if we merge them, shouldn't we
> also merge the on-disk extents as well ?

Not 1:1.

In memory one is merged maybe to save memory.

But on-disk file extents has size limit.
For compressed one it's 128K and 128M for uncompressed one.

And for on-disk file extent size limit, it could be reduce extent
booking space waste and other reasons.


Thanks,
Qu

> 
>>
>> So the current helper function looks valid, unless we have method to
>> access the original extent map.
>>
>> Thanks,
>> Qu
>>
>>>
>>>
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>>
>>>>>> +            ret =xtent_map_check_shared(inode, em->start,
>>>>>> extent_map_end(em) - 1);
>>>>>>              if (ret < 0)
>>>>>>                  goto out_free;
>>>>>>              if (ret)
>>>>>> -- 
>>>>>> 1.9.1
>>>>>>
>>>>>> -- 
>>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>>> linux-btrfs" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[Nikolay Borisov]

On  7.03.2018 14:14, Qu Wenruo wrote:
> 
> 

<snip>

>>>
>>> SHARED flag is determined after extent map merge, so here we can't rely
>>> on em here.
>>
>> Shouldn't extent maps correspond to 1:1 disk-state. I.e. they are just
>> the memory cache of the extent state. So if we merge them, shouldn't we
>> also merge the on-disk extents as well ?
> 
> Not 1:1.
> 
> In memory one is merged maybe to save memory.
> 
> But on-disk file extents has size limit.
> For compressed one it's 128K and 128M for uncompressed one.

Fair enough, however 4 extents, 16k each should warrant merging on-disk
as well, no ?

<snip>

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

[Qu Wenruo]

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



On 2018年03月07日 20:17, Nikolay Borisov wrote:
> 
> 
> On  7.03.2018 14:14, Qu Wenruo wrote:
>>
>>
> 
> <snip>
> 
>>>>
>>>> SHARED flag is determined after extent map merge, so here we can't rely
>>>> on em here.
>>>
>>> Shouldn't extent maps correspond to 1:1 disk-state. I.e. they are just
>>> the memory cache of the extent state. So if we merge them, shouldn't we
>>> also merge the on-disk extents as well ?
>>
>> Not 1:1.
>>
>> In memory one is merged maybe to save memory.
>>
>> But on-disk file extents has size limit.
>> For compressed one it's 128K and 128M for uncompressed one.
> 
> Fair enough, however 4 extents, 16k each should warrant merging on-disk
> as well, no ?

dd oflag=dsync
That ensures we won't merge on-disk extents.

Thanks,
Qu

> 
> <snip>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

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

[robbieko]

Nikolay Borisov 於 2018-03-07 19:15 寫到:
> On  7.03.2018 12:27, robbieko wrote:
>> Nikolay Borisov 於 2018-03-07 18:19 寫到:
>>> On  7.03.2018 10:20, robbieko wrote:
>>>> From: Robbie Ko <robbieko@synology.com>
>>>> 
>>>>  # 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
>>>> 
>>>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
>>>> instead of 1.
>>> 
>>> Wrong value 4 instead of 1 for which exact column, the flags? State 
>>> this
>>> explicitly.
>>> 
>>> Also this seems a bit bogus since fiemap's documentation states:
>>> 
>>> If fm_extent_count is zero, then the fm_extents[] array is ignored 
>>> (no
>>> extents will be returned), and the fm_mapped_extents count will hold 
>>> the
>>> number of extents needed in fm_extents[] to hold the file's current
>>> mapping.
>>> 
>>> So when fm_extent_count we shouldn't really be returning anything 
>>> from
>>> kernel.
>>> 
>> 
>> Sorry I did not explain clearly.
>> The value is fm_mapped_extents.
> 
> But fm_mapped_extents is tagged as an OUT member, meaning the user has
> no job writing to it.
> 


[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]
When fm_extent_count is 0, disko is not initialized correctly,
The value is 0 in this case, not the right bytenr.
It will cause the fiemap merge mechanism to fail.

[FIX]
Use correct disko.

Thanks.
Robbie Ko

>> If fm_extent_count  is zero, the fm_mapped_extents count will hold the
>> number of extents needed.
>> 
>> 
>>> 
>>>> 
>>>> [REASON]
>>>> When fm_extent_count is 0, disko is not initialized correctly,
>>>> The value is 0 in this case, not the right bytenr.
>>> 
>>> This is too sparse, be more explicit i.e. that disko=0 is passed to
>>> emit_fiemap_extent which then leads to issues.
>>> 
>>>> 
>>>> [FIX]
>>>> Use correct disko.
>>>> 
>>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>>> ---
>>>>  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
>>>> 
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-btrfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

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

[Nikolay Borisov]

On  9.03.2018 11:01, robbieko wrote:
> Nikolay Borisov 於 2018-03-07 19:15 寫到:
>> On  7.03.2018 12:27, robbieko wrote:
>>> Nikolay Borisov 於 2018-03-07 18:19 寫到:
>>>> On  7.03.2018 10:20, robbieko wrote:
>>>>> From: Robbie Ko <robbieko@synology.com>
>>>>>
>>>>>  # 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
>>>>>
>>>>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
>>>>> instead of 1.
>>>>
>>>> Wrong value 4 instead of 1 for which exact column, the flags? State
>>>> this
>>>> explicitly.
>>>>
>>>> Also this seems a bit bogus since fiemap's documentation states:
>>>>
>>>> If fm_extent_count is zero, then the fm_extents[] array is ignored (no
>>>> extents will be returned), and the fm_mapped_extents count will hold
>>>> the
>>>> number of extents needed in fm_extents[] to hold the file's current
>>>> mapping.
>>>>
>>>> So when fm_extent_count we shouldn't really be returning anything from
>>>> kernel.
>>>>
>>>
>>> Sorry I did not explain clearly.
>>> The value is fm_mapped_extents.
>>
>> But fm_mapped_extents is tagged as an OUT member, meaning the user has
>> no job writing to it.
>>
> 
> 
> [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.

This is slightly better.

> 
> [REASON]
> When fm_extent_count is 0, disko is not initialized correctly,
> The value is 0 in this case, not the right bytenr.

The value of what, be more explicit.

"
Also 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.
"


You see how complicated in fact the issue is, and your 2 line
explanation is not really making it clear.

> It will cause the fiemap merge mechanism to fail.


> 
> [FIX]
> Use correct disko.

This is as uninformative as it gets... What about something like :

"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."

> 
> Thanks.
> Robbie Ko
> 
>>> If fm_extent_count  is zero, the fm_mapped_extents count will hold the
>>> number of extents needed.
>>>
>>>
>>>>
>>>>>
>>>>> [REASON]
>>>>> When fm_extent_count is 0, disko is not initialized correctly,
>>>>> The value is 0 in this case, not the right bytenr.
>>>>
>>>> This is too sparse, be more explicit i.e. that disko=0 is passed to
>>>> emit_fiemap_extent which then leads to issues.
>>>>
>>>>>
>>>>> [FIX]
>>>>> Use correct disko.
>>>>>
>>>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>>>> ---
>>>>>  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
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-btrfs" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
> 

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

[David Sterba]

On Wed, Mar 07, 2018 at 04:20:17PM +0800, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
> This patchset intends to fix btrfs fiemap related bug.
> 
> The fiemap has the following problems:
> 
> 1) Wrong extent count when fm_extent_count is zero.
> 
> 
> 2) SHARED bit is not correct
> I have two ideas, but I do not know which one is the best.
> 
> Like:
>  # dd if=/dev/zero bs=16K count=2 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..63]:         4241424..4241487    64   0x1
>  # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone
> 
> 1. When any extent is shared in extent map, the entire extent map is shared
>  # 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
> 
> 2. Split into different extent
>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>      /mnt/btrfs/file:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..31]:         4241424..4241455   32  0x0
>        1: [32..63]:        4241456..4241487   32  0x2001
> 
> Robbie Ko (2):
>   Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero
>   Btrfs: fix fiemap extent SHARED flag error with range clone.

There were a lot of comments, I'm not sure I have a clear picture of
what and how is addressed. Can you please update the patches and resend?
Thanks.