[PATCH V4 0/2] Some bugfix for xfs fsmap

[PATCH V4 0/2] Some bugfix for xfs fsmap

[Zizhi Wo]

Changes since V3[1]:
 - For the first patch, simply place the modification logic in the
   xfs_fsmap_owner_to_rmap() function.
 - For the second patch, more detailed comments were added and related
   changes were made to the initialization of the end_daddr field.

This patch set contains two patches to repair fsmap. Although they are both
problems of missing query intervals, the root causes of the two are
inconsistent, so two patches are proposed.

Patch 1: The fix addresses the interval omission issue caused by the
incorrect setting of "rm_owner" in the high_key during rmap queries. In
this scenario, fsmap finds the record on the rmapbt, but due to the
incorrect setting of the "rm_owner", the key of the record is larger than
the high_key, causing the query result to be incorrect. This issue is
resolved by fixing the "rm_owner" setup logic.

Patch 2: The fix addresses the interval omission issue caused by bit
shifting during gap queries in fsmap. In this scenario, fsmap does not
find the record on the rmapbt, so it needs to locate it by the gap of the
info->next_daddr and high_key address. However, due to the shift, the two
are reduced to 0, so the query error is caused. The issue is resolved by
introducing the "end_daddr" field in the xfs_getfsmap_info structure to
store the high_key at the sector granularity.

[1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/

Zizhi Wo (2):
  xfs: Fix the owner setting issue for rmap query in xfs fsmap
  xfs: Fix missing interval for missing_owner in xfs fsmap

 fs/xfs/xfs_fsmap.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH V4 1/2] xfs: Fix the owner setting issue for rmap query in xfs fsmap

[Zizhi Wo]

I notice a rmap query bug in xfs_io fsmap:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
 EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
   0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
   1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
   2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
   3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
   4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
   5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
   6: 253:16 [104..127]:           free space                          0  (104..127)               24
   ......

Bug:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 0 3' /mnt
[root@fedora ~]#
Normally, we should be able to get one record, but we got nothing.

The root cause of this problem lies in the incorrect setting of rm_owner in
the rmap query. In the case of the initial query where the owner is not
set, __xfs_getfsmap_datadev() first sets info->high.rm_owner to ULLONG_MAX.
This is done to prevent any omissions when comparing rmap items. However,
if the current ag is detected to be the last one, the function sets info's
high_irec based on the provided key. If high->rm_owner is not specified, it
should continue to be set to ULLONG_MAX; otherwise, there will be issues
with interval omissions. For example, consider "start" and "end" within the
same block. If high->rm_owner == 0, it will be smaller than the founded
record in rmapbt, resulting in a query with no records. The main call stack
is as follows:

xfs_ioc_getfsmap
  xfs_getfsmap
    xfs_getfsmap_datadev_rmapbt
      __xfs_getfsmap_datadev
        info->high.rm_owner = ULLONG_MAX
        if (pag->pag_agno == end_ag)
	  xfs_fsmap_owner_to_rmap
	    // set info->high.rm_owner = 0 because fmr_owner == -1ULL
	    dest->rm_owner = 0
	// get nothing
	xfs_getfsmap_datadev_rmapbt_query

The problem can be resolved by simply modify the xfs_fsmap_owner_to_rmap
function internal logic to achieve.

After applying this patch, the above problem have been solved:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 0 3' /mnt
 EXT: DEV    BLOCK-RANGE      OWNER              FILE-OFFSET      AG AG-OFFSET        TOTAL
   0: 253:16 [0..7]:          static fs metadata                  0  (0..7)               8

Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/xfs/xfs_fsmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 85dbb46452ca..3a30b36779db 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -71,7 +71,7 @@ xfs_fsmap_owner_to_rmap(
 	switch (src->fmr_owner) {
 	case 0:			/* "lowest owner id possible" */
 	case -1ULL:		/* "highest owner id possible" */
-		dest->rm_owner = 0;
+		dest->rm_owner = src->fmr_owner;
 		break;
 	case XFS_FMR_OWN_FREE:
 		dest->rm_owner = XFS_RMAP_OWN_NULL;
-- 
2.39.2

Re: [PATCH V4 1/2] xfs: Fix the owner setting issue for rmap query in xfs fsmap

[Darrick J. Wong]

On Mon, Aug 19, 2024 at 08:53:19AM +0800, Zizhi Wo wrote:
> I notice a rmap query bug in xfs_io fsmap:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
>  EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
>    0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
>    1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
>    2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
>    3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
>    4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
>    5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
>    6: 253:16 [104..127]:           free space                          0  (104..127)               24
>    ......
> 
> Bug:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 0 3' /mnt
> [root@fedora ~]#
> Normally, we should be able to get one record, but we got nothing.
> 
> The root cause of this problem lies in the incorrect setting of rm_owner in
> the rmap query. In the case of the initial query where the owner is not
> set, __xfs_getfsmap_datadev() first sets info->high.rm_owner to ULLONG_MAX.
> This is done to prevent any omissions when comparing rmap items. However,
> if the current ag is detected to be the last one, the function sets info's
> high_irec based on the provided key. If high->rm_owner is not specified, it
> should continue to be set to ULLONG_MAX; otherwise, there will be issues
> with interval omissions. For example, consider "start" and "end" within the
> same block. If high->rm_owner == 0, it will be smaller than the founded
> record in rmapbt, resulting in a query with no records. The main call stack
> is as follows:
> 
> xfs_ioc_getfsmap
>   xfs_getfsmap
>     xfs_getfsmap_datadev_rmapbt
>       __xfs_getfsmap_datadev
>         info->high.rm_owner = ULLONG_MAX
>         if (pag->pag_agno == end_ag)
> 	  xfs_fsmap_owner_to_rmap
> 	    // set info->high.rm_owner = 0 because fmr_owner == -1ULL
> 	    dest->rm_owner = 0
> 	// get nothing
> 	xfs_getfsmap_datadev_rmapbt_query
> 
> The problem can be resolved by simply modify the xfs_fsmap_owner_to_rmap
> function internal logic to achieve.
> 
> After applying this patch, the above problem have been solved:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 0 3' /mnt
>  EXT: DEV    BLOCK-RANGE      OWNER              FILE-OFFSET      AG AG-OFFSET        TOTAL
>    0: 253:16 [0..7]:          static fs metadata                  0  (0..7)               8
> 
> Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_fsmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 85dbb46452ca..3a30b36779db 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -71,7 +71,7 @@ xfs_fsmap_owner_to_rmap(
>  	switch (src->fmr_owner) {
>  	case 0:			/* "lowest owner id possible" */
>  	case -1ULL:		/* "highest owner id possible" */
> -		dest->rm_owner = 0;
> +		dest->rm_owner = src->fmr_owner;
>  		break;
>  	case XFS_FMR_OWN_FREE:
>  		dest->rm_owner = XFS_RMAP_OWN_NULL;
> -- 
> 2.39.2
> 
> 

[PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Zizhi Wo]

In the fsmap query of xfs, there is an interval missing problem:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
 EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
   0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
   1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
   2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
   3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
   4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
   5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
   6: 253:16 [104..127]:           free space                          0  (104..127)               24
   ......

BUG:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
[root@fedora ~]#
Normally, we should be able to get [104, 107), but we got nothing.

The problem is caused by shifting. The query for the problem-triggered
scenario is for the missing_owner interval (e.g. freespace in rmapbt/
unknown space in bnobt), which is obtained by subtraction (gap). For this
scenario, the interval is obtained by info->last. However, rec_daddr is
calculated based on the start_block recorded in key[1], which is converted
by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
<= info->next_daddr, no records will be displayed. In the above example,
104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
are reduced to 0 and the gap is ignored:

 before calculate ----------------> after shifting
 104(st)  107(ed)		      12(st/ed)
  |---------|				  |
  sector size			      block size

Resolve this issue by introducing the "end_daddr" field in
xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
sector. If the current query is the last, the rec_daddr is end_daddr to
prevent missing interval problems caused by shifting. We only need to focus
on the last query, because xfs disks are internally aligned with disk
blocksize that are powers of two and minimum 512, so there is no problem
with shifting in previous queries.

After applying this patch, the above problem have been solved:
[root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
 EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
   0: 253:16 [104..106]:      free space                        0  (104..106)           3

Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 3a30b36779db..4734f8d6303c 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
 	xfs_daddr_t		next_daddr;	/* next daddr we expect */
 	/* daddr of low fsmap key when we're using the rtbitmap */
 	xfs_daddr_t		low_daddr;
+	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
 	u64			missing_owner;	/* owner of holes */
 	u32			dev;		/* device id */
 	/*
@@ -294,6 +295,19 @@ xfs_getfsmap_helper(
 		return 0;
 	}
 
+	/*
+	 * For an info->last query, we're looking for a gap between the
+	 * last mapping emitted and the high key specified by userspace.
+	 * If the user's query spans less than 1 fsblock, then
+	 * info->high and info->low will have the same rm_startblock,
+	 * which causes rec_daddr and next_daddr to be the same.
+	 * Therefore, use the end_daddr that we calculated from
+	 * userspace's high key to synthesize the record.  Note that if
+	 * the btree query found a mapping, there won't be a gap.
+	 */
+	if (info->last && info->end_daddr != LLONG_MAX)
+		rec_daddr = info->end_daddr;
+
 	/* Are we just counting mappings? */
 	if (info->head->fmh_count == 0) {
 		if (info->head->fmh_entries == UINT_MAX)
@@ -946,6 +960,7 @@ xfs_getfsmap(
 
 	info.next_daddr = head->fmh_keys[0].fmr_physical +
 			  head->fmh_keys[0].fmr_length;
+	info.end_daddr = LLONG_MAX;
 	info.fsmap_recs = fsmap_recs;
 	info.head = head;
 
@@ -966,8 +981,10 @@ xfs_getfsmap(
 		 * low key, zero out the low key so that we get
 		 * everything from the beginning.
 		 */
-		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
+		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
 			dkeys[1] = head->fmh_keys[1];
+			info.end_daddr = dkeys[1].fmr_physical;
+		}
 		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
 			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
 
-- 
2.39.2

Re: [PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Darrick J. Wong]

On Mon, Aug 19, 2024 at 08:53:20AM +0800, Zizhi Wo wrote:
> In the fsmap query of xfs, there is an interval missing problem:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
>  EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
>    0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
>    1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
>    2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
>    3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
>    4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
>    5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
>    6: 253:16 [104..127]:           free space                          0  (104..127)               24
>    ......
> 
> BUG:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
> [root@fedora ~]#
> Normally, we should be able to get [104, 107), but we got nothing.
> 
> The problem is caused by shifting. The query for the problem-triggered
> scenario is for the missing_owner interval (e.g. freespace in rmapbt/
> unknown space in bnobt), which is obtained by subtraction (gap). For this
> scenario, the interval is obtained by info->last. However, rec_daddr is
> calculated based on the start_block recorded in key[1], which is converted
> by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
> info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
> <= info->next_daddr, no records will be displayed. In the above example,
> 104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
> are reduced to 0 and the gap is ignored:
> 
>  before calculate ----------------> after shifting
>  104(st)  107(ed)		      12(st/ed)
>   |---------|				  |
>   sector size			      block size
> 
> Resolve this issue by introducing the "end_daddr" field in
> xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
> sector. If the current query is the last, the rec_daddr is end_daddr to
> prevent missing interval problems caused by shifting. We only need to focus
> on the last query, because xfs disks are internally aligned with disk
> blocksize that are powers of two and minimum 512, so there is no problem
> with shifting in previous queries.
> 
> After applying this patch, the above problem have been solved:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>  EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
>    0: 253:16 [104..106]:      free space                        0  (104..106)           3
> 
> Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 3a30b36779db..4734f8d6303c 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
>  	xfs_daddr_t		next_daddr;	/* next daddr we expect */
>  	/* daddr of low fsmap key when we're using the rtbitmap */
>  	xfs_daddr_t		low_daddr;
> +	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
>  	u64			missing_owner;	/* owner of holes */
>  	u32			dev;		/* device id */
>  	/*
> @@ -294,6 +295,19 @@ xfs_getfsmap_helper(
>  		return 0;
>  	}
>  
> +	/*
> +	 * For an info->last query, we're looking for a gap between the
> +	 * last mapping emitted and the high key specified by userspace.
> +	 * If the user's query spans less than 1 fsblock, then
> +	 * info->high and info->low will have the same rm_startblock,
> +	 * which causes rec_daddr and next_daddr to be the same.
> +	 * Therefore, use the end_daddr that we calculated from
> +	 * userspace's high key to synthesize the record.  Note that if
> +	 * the btree query found a mapping, there won't be a gap.
> +	 */
> +	if (info->last && info->end_daddr != LLONG_MAX)

XFS_BUF_DADDR_NULL (and yes, I know the rest of the file is wildly
inconsistent, I'll send a patch to fix that too...)

--D

> +		rec_daddr = info->end_daddr;
> +
>  	/* Are we just counting mappings? */
>  	if (info->head->fmh_count == 0) {
>  		if (info->head->fmh_entries == UINT_MAX)
> @@ -946,6 +960,7 @@ xfs_getfsmap(
>  
>  	info.next_daddr = head->fmh_keys[0].fmr_physical +
>  			  head->fmh_keys[0].fmr_length;
> +	info.end_daddr = LLONG_MAX;
>  	info.fsmap_recs = fsmap_recs;
>  	info.head = head;
>  
> @@ -966,8 +981,10 @@ xfs_getfsmap(
>  		 * low key, zero out the low key so that we get
>  		 * everything from the beginning.
>  		 */
> -		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
> +		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
>  			dkeys[1] = head->fmh_keys[1];
> +			info.end_daddr = dkeys[1].fmr_physical;
> +		}
>  		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
>  			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
>  
> -- 
> 2.39.2
> 
> 

Re: [PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Zizhi Wo]

在 2024/8/19 13:21, Darrick J. Wong 写道:
> On Mon, Aug 19, 2024 at 08:53:20AM +0800, Zizhi Wo wrote:
>> In the fsmap query of xfs, there is an interval missing problem:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
>>   EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
>>     0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
>>     1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
>>     2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
>>     3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
>>     4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
>>     5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
>>     6: 253:16 [104..127]:           free space                          0  (104..127)               24
>>     ......
>>
>> BUG:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>> [root@fedora ~]#
>> Normally, we should be able to get [104, 107), but we got nothing.
>>
>> The problem is caused by shifting. The query for the problem-triggered
>> scenario is for the missing_owner interval (e.g. freespace in rmapbt/
>> unknown space in bnobt), which is obtained by subtraction (gap). For this
>> scenario, the interval is obtained by info->last. However, rec_daddr is
>> calculated based on the start_block recorded in key[1], which is converted
>> by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
>> info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
>> <= info->next_daddr, no records will be displayed. In the above example,
>> 104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
>> are reduced to 0 and the gap is ignored:
>>
>>   before calculate ----------------> after shifting
>>   104(st)  107(ed)		      12(st/ed)
>>    |---------|				  |
>>    sector size			      block size
>>
>> Resolve this issue by introducing the "end_daddr" field in
>> xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
>> sector. If the current query is the last, the rec_daddr is end_daddr to
>> prevent missing interval problems caused by shifting. We only need to focus
>> on the last query, because xfs disks are internally aligned with disk
>> blocksize that are powers of two and minimum 512, so there is no problem
>> with shifting in previous queries.
>>
>> After applying this patch, the above problem have been solved:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>>   EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
>>     0: 253:16 [104..106]:      free space                        0  (104..106)           3
>>
>> Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
>> index 3a30b36779db..4734f8d6303c 100644
>> --- a/fs/xfs/xfs_fsmap.c
>> +++ b/fs/xfs/xfs_fsmap.c
>> @@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
>>   	xfs_daddr_t		next_daddr;	/* next daddr we expect */
>>   	/* daddr of low fsmap key when we're using the rtbitmap */
>>   	xfs_daddr_t		low_daddr;
>> +	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
>>   	u64			missing_owner;	/* owner of holes */
>>   	u32			dev;		/* device id */
>>   	/*
>> @@ -294,6 +295,19 @@ xfs_getfsmap_helper(
>>   		return 0;
>>   	}
>>   
>> +	/*
>> +	 * For an info->last query, we're looking for a gap between the
>> +	 * last mapping emitted and the high key specified by userspace.
>> +	 * If the user's query spans less than 1 fsblock, then
>> +	 * info->high and info->low will have the same rm_startblock,
>> +	 * which causes rec_daddr and next_daddr to be the same.
>> +	 * Therefore, use the end_daddr that we calculated from
>> +	 * userspace's high key to synthesize the record.  Note that if
>> +	 * the btree query found a mapping, there won't be a gap.
>> +	 */
>> +	if (info->last && info->end_daddr != LLONG_MAX)
> 
> XFS_BUF_DADDR_NULL (and yes, I know the rest of the file is wildly
> inconsistent, I'll send a patch to fix that too...)
> 
> --D

 From what I understand, you mean that info->end_daddr is initialized to
XFS_BUF_DADDR_NULL, correct?

Then, regarding this specific issue, are you going to propose a fix
patch to address it, or is the fix patch you mentioned intended to fix
other file contiguity problems in fsmap? I'm a bit unclear about that.

Thanks,
Zizhi Wo

> 
>> +		rec_daddr = info->end_daddr;
>> +
>>   	/* Are we just counting mappings? */
>>   	if (info->head->fmh_count == 0) {
>>   		if (info->head->fmh_entries == UINT_MAX)
>> @@ -946,6 +960,7 @@ xfs_getfsmap(
>>   
>>   	info.next_daddr = head->fmh_keys[0].fmr_physical +
>>   			  head->fmh_keys[0].fmr_length;
>> +	info.end_daddr = LLONG_MAX;
>>   	info.fsmap_recs = fsmap_recs;
>>   	info.head = head;
>>   
>> @@ -966,8 +981,10 @@ xfs_getfsmap(
>>   		 * low key, zero out the low key so that we get
>>   		 * everything from the beginning.
>>   		 */
>> -		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
>> +		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
>>   			dkeys[1] = head->fmh_keys[1];
>> +			info.end_daddr = dkeys[1].fmr_physical;
>> +		}
>>   		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
>>   			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
>>   
>> -- 
>> 2.39.2
>>
>>
> 

Re: [PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Darrick J. Wong]

On Mon, Aug 19, 2024 at 02:24:58PM +0800, Zizhi Wo wrote:
> 
> 
> 在 2024/8/19 13:21, Darrick J. Wong 写道:
> > On Mon, Aug 19, 2024 at 08:53:20AM +0800, Zizhi Wo wrote:
> > > In the fsmap query of xfs, there is an interval missing problem:
> > > [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
> > >   EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
> > >     0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
> > >     1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
> > >     2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
> > >     3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
> > >     4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
> > >     5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
> > >     6: 253:16 [104..127]:           free space                          0  (104..127)               24
> > >     ......
> > > 
> > > BUG:
> > > [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
> > > [root@fedora ~]#
> > > Normally, we should be able to get [104, 107), but we got nothing.
> > > 
> > > The problem is caused by shifting. The query for the problem-triggered
> > > scenario is for the missing_owner interval (e.g. freespace in rmapbt/
> > > unknown space in bnobt), which is obtained by subtraction (gap). For this
> > > scenario, the interval is obtained by info->last. However, rec_daddr is
> > > calculated based on the start_block recorded in key[1], which is converted
> > > by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
> > > info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
> > > <= info->next_daddr, no records will be displayed. In the above example,
> > > 104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
> > > are reduced to 0 and the gap is ignored:
> > > 
> > >   before calculate ----------------> after shifting
> > >   104(st)  107(ed)		      12(st/ed)
> > >    |---------|				  |
> > >    sector size			      block size
> > > 
> > > Resolve this issue by introducing the "end_daddr" field in
> > > xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
> > > sector. If the current query is the last, the rec_daddr is end_daddr to
> > > prevent missing interval problems caused by shifting. We only need to focus
> > > on the last query, because xfs disks are internally aligned with disk
> > > blocksize that are powers of two and minimum 512, so there is no problem
> > > with shifting in previous queries.
> > > 
> > > After applying this patch, the above problem have been solved:
> > > [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
> > >   EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
> > >     0: 253:16 [104..106]:      free space                        0  (104..106)           3
> > > 
> > > Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
> > > Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> > > ---
> > >   fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
> > >   1 file changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > > index 3a30b36779db..4734f8d6303c 100644
> > > --- a/fs/xfs/xfs_fsmap.c
> > > +++ b/fs/xfs/xfs_fsmap.c
> > > @@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
> > >   	xfs_daddr_t		next_daddr;	/* next daddr we expect */
> > >   	/* daddr of low fsmap key when we're using the rtbitmap */
> > >   	xfs_daddr_t		low_daddr;
> > > +	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
> > >   	u64			missing_owner;	/* owner of holes */
> > >   	u32			dev;		/* device id */
> > >   	/*
> > > @@ -294,6 +295,19 @@ xfs_getfsmap_helper(
> > >   		return 0;
> > >   	}
> > > +	/*
> > > +	 * For an info->last query, we're looking for a gap between the
> > > +	 * last mapping emitted and the high key specified by userspace.
> > > +	 * If the user's query spans less than 1 fsblock, then
> > > +	 * info->high and info->low will have the same rm_startblock,
> > > +	 * which causes rec_daddr and next_daddr to be the same.
> > > +	 * Therefore, use the end_daddr that we calculated from
> > > +	 * userspace's high key to synthesize the record.  Note that if
> > > +	 * the btree query found a mapping, there won't be a gap.
> > > +	 */
> > > +	if (info->last && info->end_daddr != LLONG_MAX)
> > 
> > XFS_BUF_DADDR_NULL (and yes, I know the rest of the file is wildly
> > inconsistent, I'll send a patch to fix that too...)
> > 
> > --D
> 
> From what I understand, you mean that info->end_daddr is initialized to
> XFS_BUF_DADDR_NULL, correct?
> 
> Then, regarding this specific issue, are you going to propose a fix
> patch to address it, or is the fix patch you mentioned intended to fix
> other file contiguity problems in fsmap? I'm a bit unclear about that.

You change this patch to use XFS_BUF_DADDR_NULL instead of LLONG_MAX,
and then I'll send a followup to fix the other users of -1ULL in
xfs_fsmap.c.

--D

> Thanks,
> Zizhi Wo
> 
> > 
> > > +		rec_daddr = info->end_daddr;
> > > +
> > >   	/* Are we just counting mappings? */
> > >   	if (info->head->fmh_count == 0) {
> > >   		if (info->head->fmh_entries == UINT_MAX)
> > > @@ -946,6 +960,7 @@ xfs_getfsmap(
> > >   	info.next_daddr = head->fmh_keys[0].fmr_physical +
> > >   			  head->fmh_keys[0].fmr_length;
> > > +	info.end_daddr = LLONG_MAX;
> > >   	info.fsmap_recs = fsmap_recs;
> > >   	info.head = head;
> > > @@ -966,8 +981,10 @@ xfs_getfsmap(
> > >   		 * low key, zero out the low key so that we get
> > >   		 * everything from the beginning.
> > >   		 */
> > > -		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
> > > +		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
> > >   			dkeys[1] = head->fmh_keys[1];
> > > +			info.end_daddr = dkeys[1].fmr_physical;
> > > +		}
> > >   		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
> > >   			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
> > > -- 
> > > 2.39.2
> > > 
> > > 
> > 
> 

Re: [PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Darrick J. Wong]

On Mon, Aug 19, 2024 at 08:53:20AM +0800, Zizhi Wo wrote:
> In the fsmap query of xfs, there is an interval missing problem:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
>  EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
>    0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
>    1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
>    2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
>    3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
>    4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
>    5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
>    6: 253:16 [104..127]:           free space                          0  (104..127)               24
>    ......
> 
> BUG:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
> [root@fedora ~]#
> Normally, we should be able to get [104, 107), but we got nothing.
> 
> The problem is caused by shifting. The query for the problem-triggered
> scenario is for the missing_owner interval (e.g. freespace in rmapbt/
> unknown space in bnobt), which is obtained by subtraction (gap). For this
> scenario, the interval is obtained by info->last. However, rec_daddr is
> calculated based on the start_block recorded in key[1], which is converted
> by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
> info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
> <= info->next_daddr, no records will be displayed. In the above example,
> 104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
> are reduced to 0 and the gap is ignored:
> 
>  before calculate ----------------> after shifting
>  104(st)  107(ed)		      12(st/ed)
>   |---------|				  |
>   sector size			      block size
> 
> Resolve this issue by introducing the "end_daddr" field in
> xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
> sector. If the current query is the last, the rec_daddr is end_daddr to
> prevent missing interval problems caused by shifting. We only need to focus
> on the last query, because xfs disks are internally aligned with disk
> blocksize that are powers of two and minimum 512, so there is no problem
> with shifting in previous queries.
> 
> After applying this patch, the above problem have been solved:
> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>  EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
>    0: 253:16 [104..106]:      free space                        0  (104..106)           3
> 
> Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 3a30b36779db..4734f8d6303c 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
>  	xfs_daddr_t		next_daddr;	/* next daddr we expect */
>  	/* daddr of low fsmap key when we're using the rtbitmap */
>  	xfs_daddr_t		low_daddr;
> +	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
>  	u64			missing_owner;	/* owner of holes */
>  	u32			dev;		/* device id */
>  	/*
> @@ -294,6 +295,19 @@ xfs_getfsmap_helper(
>  		return 0;
>  	}
>  
> +	/*
> +	 * For an info->last query, we're looking for a gap between the
> +	 * last mapping emitted and the high key specified by userspace.
> +	 * If the user's query spans less than 1 fsblock, then
> +	 * info->high and info->low will have the same rm_startblock,
> +	 * which causes rec_daddr and next_daddr to be the same.
> +	 * Therefore, use the end_daddr that we calculated from
> +	 * userspace's high key to synthesize the record.  Note that if
> +	 * the btree query found a mapping, there won't be a gap.
> +	 */
> +	if (info->last && info->end_daddr != LLONG_MAX)
> +		rec_daddr = info->end_daddr;
> +
>  	/* Are we just counting mappings? */
>  	if (info->head->fmh_count == 0) {
>  		if (info->head->fmh_entries == UINT_MAX)
> @@ -946,6 +960,7 @@ xfs_getfsmap(
>  
>  	info.next_daddr = head->fmh_keys[0].fmr_physical +
>  			  head->fmh_keys[0].fmr_length;
> +	info.end_daddr = LLONG_MAX;
>  	info.fsmap_recs = fsmap_recs;
>  	info.head = head;
>  
> @@ -966,8 +981,10 @@ xfs_getfsmap(
>  		 * low key, zero out the low key so that we get
>  		 * everything from the beginning.
>  		 */
> -		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
> +		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
>  			dkeys[1] = head->fmh_keys[1];
> +			info.end_daddr = dkeys[1].fmr_physical;

Another problem that I found while testing this out is that if
dkeys[1].fmr_physical extends a little bit beyond the end of what the
filesystem thinks is the device size, this change results in fsmap
reporting an "unknown" extent between that end point and whatever the
user specified as fmr_physical.

IOWs, let's say that the filesystem has 67G of space and 16G AGs.  This
results in 4x 16G AGs, and a runt AG 4 that is 3G long.  If you initiate
an fsmap query for [64G, 80G), it'll report "unknown" space between 67G
and 80G, whereas previously it did not report that.  I noticed this due
to a regression in xfs/566 with the rtgroups patchset applied, though it
also seems to happen with that same test if the underlying device has a
raid stripe configuration that causes runt AGs.

I think this can be fixed by constraining end_daddr to the minimum of
fmr_physical and XFS_FSB_TO_BB(dblocks/rblocks/logblocks).

--D

> +		}
>  		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
>  			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
>  
> -- 
> 2.39.2
> 
> 

Re: [PATCH V4 2/2] xfs: Fix missing interval for missing_owner in xfs fsmap

[Zizhi Wo]

在 2024/8/20 2:44, Darrick J. Wong 写道:
> On Mon, Aug 19, 2024 at 08:53:20AM +0800, Zizhi Wo wrote:
>> In the fsmap query of xfs, there is an interval missing problem:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv' /mnt
>>   EXT: DEV    BLOCK-RANGE           OWNER              FILE-OFFSET      AG AG-OFFSET             TOTAL
>>     0: 253:16 [0..7]:               static fs metadata                  0  (0..7)                    8
>>     1: 253:16 [8..23]:              per-AG metadata                     0  (8..23)                  16
>>     2: 253:16 [24..39]:             inode btree                         0  (24..39)                 16
>>     3: 253:16 [40..47]:             per-AG metadata                     0  (40..47)                  8
>>     4: 253:16 [48..55]:             refcount btree                      0  (48..55)                  8
>>     5: 253:16 [56..103]:            per-AG metadata                     0  (56..103)                48
>>     6: 253:16 [104..127]:           free space                          0  (104..127)               24
>>     ......
>>
>> BUG:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>> [root@fedora ~]#
>> Normally, we should be able to get [104, 107), but we got nothing.
>>
>> The problem is caused by shifting. The query for the problem-triggered
>> scenario is for the missing_owner interval (e.g. freespace in rmapbt/
>> unknown space in bnobt), which is obtained by subtraction (gap). For this
>> scenario, the interval is obtained by info->last. However, rec_daddr is
>> calculated based on the start_block recorded in key[1], which is converted
>> by calling XFS_BB_TO_FSBT. Then if rec_daddr does not exceed
>> info->next_daddr, which means keys[1].fmr_physical >> (mp)->m_blkbb_log
>> <= info->next_daddr, no records will be displayed. In the above example,
>> 104 >> (mp)->m_blkbb_log = 12 and 107 >> (mp)->m_blkbb_log = 12, so the two
>> are reduced to 0 and the gap is ignored:
>>
>>   before calculate ----------------> after shifting
>>   104(st)  107(ed)		      12(st/ed)
>>    |---------|				  |
>>    sector size			      block size
>>
>> Resolve this issue by introducing the "end_daddr" field in
>> xfs_getfsmap_info. This records key[1].fmr_physical at the granularity of
>> sector. If the current query is the last, the rec_daddr is end_daddr to
>> prevent missing interval problems caused by shifting. We only need to focus
>> on the last query, because xfs disks are internally aligned with disk
>> blocksize that are powers of two and minimum 512, so there is no problem
>> with shifting in previous queries.
>>
>> After applying this patch, the above problem have been solved:
>> [root@fedora ~]# xfs_io -c 'fsmap -vvvv -d 104 107' /mnt
>>   EXT: DEV    BLOCK-RANGE      OWNER            FILE-OFFSET      AG AG-OFFSET        TOTAL
>>     0: 253:16 [104..106]:      free space                        0  (104..106)           3
>>
>> Fixes: e89c041338ed ("xfs: implement the GETFSMAP ioctl")
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   fs/xfs/xfs_fsmap.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
>> index 3a30b36779db..4734f8d6303c 100644
>> --- a/fs/xfs/xfs_fsmap.c
>> +++ b/fs/xfs/xfs_fsmap.c
>> @@ -162,6 +162,7 @@ struct xfs_getfsmap_info {
>>   	xfs_daddr_t		next_daddr;	/* next daddr we expect */
>>   	/* daddr of low fsmap key when we're using the rtbitmap */
>>   	xfs_daddr_t		low_daddr;
>> +	xfs_daddr_t		end_daddr;	/* daddr of high fsmap key */
>>   	u64			missing_owner;	/* owner of holes */
>>   	u32			dev;		/* device id */
>>   	/*
>> @@ -294,6 +295,19 @@ xfs_getfsmap_helper(
>>   		return 0;
>>   	}
>>   
>> +	/*
>> +	 * For an info->last query, we're looking for a gap between the
>> +	 * last mapping emitted and the high key specified by userspace.
>> +	 * If the user's query spans less than 1 fsblock, then
>> +	 * info->high and info->low will have the same rm_startblock,
>> +	 * which causes rec_daddr and next_daddr to be the same.
>> +	 * Therefore, use the end_daddr that we calculated from
>> +	 * userspace's high key to synthesize the record.  Note that if
>> +	 * the btree query found a mapping, there won't be a gap.
>> +	 */
>> +	if (info->last && info->end_daddr != LLONG_MAX)
>> +		rec_daddr = info->end_daddr;
>> +
>>   	/* Are we just counting mappings? */
>>   	if (info->head->fmh_count == 0) {
>>   		if (info->head->fmh_entries == UINT_MAX)
>> @@ -946,6 +960,7 @@ xfs_getfsmap(
>>   
>>   	info.next_daddr = head->fmh_keys[0].fmr_physical +
>>   			  head->fmh_keys[0].fmr_length;
>> +	info.end_daddr = LLONG_MAX;
>>   	info.fsmap_recs = fsmap_recs;
>>   	info.head = head;
>>   
>> @@ -966,8 +981,10 @@ xfs_getfsmap(
>>   		 * low key, zero out the low key so that we get
>>   		 * everything from the beginning.
>>   		 */
>> -		if (handlers[i].dev == head->fmh_keys[1].fmr_device)
>> +		if (handlers[i].dev == head->fmh_keys[1].fmr_device) {
>>   			dkeys[1] = head->fmh_keys[1];
>> +			info.end_daddr = dkeys[1].fmr_physical;
> 
> Another problem that I found while testing this out is that if
> dkeys[1].fmr_physical extends a little bit beyond the end of what the
> filesystem thinks is the device size, this change results in fsmap
> reporting an "unknown" extent between that end point and whatever the
> user specified as fmr_physical.
> 
> IOWs, let's say that the filesystem has 67G of space and 16G AGs.  This
> results in 4x 16G AGs, and a runt AG 4 that is 3G long.  If you initiate
> an fsmap query for [64G, 80G), it'll report "unknown" space between 67G
> and 80G, whereas previously it did not report that.  I noticed this due
> to a regression in xfs/566 with the rtgroups patchset applied, though it
> also seems to happen with that same test if the underlying device has a
> raid stripe configuration that causes runt AGs.
> 
> I think this can be fixed by constraining end_daddr to the minimum of
> fmr_physical and XFS_FSB_TO_BB(dblocks/rblocks/logblocks).
> 
> --D
> 

Oh yeah, I missed that boundary condition. I will fix as soon as
possible, and send the next version of the patch, thanks for reminding!

Thanks,
Zizhi Wo

>> +		}
>>   		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
>>   			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
>>   
>> -- 
>> 2.39.2
>>
>>

Re: [PATCH V4 0/2] Some bugfix for xfs fsmap

[Chandan Babu R]

On Mon, Aug 19, 2024 at 08:53:18 AM +0800, Zizhi Wo wrote:
> Changes since V3[1]:
>  - For the first patch, simply place the modification logic in the
>    xfs_fsmap_owner_to_rmap() function.
>  - For the second patch, more detailed comments were added and related
>    changes were made to the initialization of the end_daddr field.
>
> This patch set contains two patches to repair fsmap. Although they are both
> problems of missing query intervals, the root causes of the two are
> inconsistent, so two patches are proposed.
>
> Patch 1: The fix addresses the interval omission issue caused by the
> incorrect setting of "rm_owner" in the high_key during rmap queries. In
> this scenario, fsmap finds the record on the rmapbt, but due to the
> incorrect setting of the "rm_owner", the key of the record is larger than
> the high_key, causing the query result to be incorrect. This issue is
> resolved by fixing the "rm_owner" setup logic.
>
> Patch 2: The fix addresses the interval omission issue caused by bit
> shifting during gap queries in fsmap. In this scenario, fsmap does not
> find the record on the rmapbt, so it needs to locate it by the gap of the
> info->next_daddr and high_key address. However, due to the shift, the two
> are reduced to 0, so the query error is caused. The issue is resolved by
> introducing the "end_daddr" field in the xfs_getfsmap_info structure to
> store the high_key at the sector granularity.
>
> [1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/
>

The two patches in this series cause xfs_scrub to execute indefinitely
immediately after xfs/556 is executed.

The fstest configuration used is provided below,

FSTYP=xfs
TEST_DIR=/media/test
SCRATCH_MNT=/media/scratch
TEST_DEV=/dev/loop16
TEST_LOGDEV=/dev/loop13
TEST_RTDEV=/dev/loop12
TEST_FS_MOUNT_OPTS="-o rtdev=/dev/loop12 -o logdev=/dev/loop13"
SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11"
MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -lsize=1g"
SCRATCH_LOGDEV=/dev/loop15
SCRATCH_RTDEV=/dev/loop14
USE_EXTERNAL=yes

-- 
Chandan

Re: [PATCH V4 0/2] Some bugfix for xfs fsmap

[Zizhi Wo]

在 2024/8/20 13:53, Chandan Babu R 写道:
> On Mon, Aug 19, 2024 at 08:53:18 AM +0800, Zizhi Wo wrote:
>> Changes since V3[1]:
>>   - For the first patch, simply place the modification logic in the
>>     xfs_fsmap_owner_to_rmap() function.
>>   - For the second patch, more detailed comments were added and related
>>     changes were made to the initialization of the end_daddr field.
>>
>> This patch set contains two patches to repair fsmap. Although they are both
>> problems of missing query intervals, the root causes of the two are
>> inconsistent, so two patches are proposed.
>>
>> Patch 1: The fix addresses the interval omission issue caused by the
>> incorrect setting of "rm_owner" in the high_key during rmap queries. In
>> this scenario, fsmap finds the record on the rmapbt, but due to the
>> incorrect setting of the "rm_owner", the key of the record is larger than
>> the high_key, causing the query result to be incorrect. This issue is
>> resolved by fixing the "rm_owner" setup logic.
>>
>> Patch 2: The fix addresses the interval omission issue caused by bit
>> shifting during gap queries in fsmap. In this scenario, fsmap does not
>> find the record on the rmapbt, so it needs to locate it by the gap of the
>> info->next_daddr and high_key address. However, due to the shift, the two
>> are reduced to 0, so the query error is caused. The issue is resolved by
>> introducing the "end_daddr" field in the xfs_getfsmap_info structure to
>> store the high_key at the sector granularity.
>>
>> [1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/
>>
> 
> The two patches in this series cause xfs_scrub to execute indefinitely
> immediately after xfs/556 is executed.
> 
> The fstest configuration used is provided below,
> 
> FSTYP=xfs
> TEST_DIR=/media/test
> SCRATCH_MNT=/media/scratch
> TEST_DEV=/dev/loop16
> TEST_LOGDEV=/dev/loop13
> TEST_RTDEV=/dev/loop12
> TEST_FS_MOUNT_OPTS="-o rtdev=/dev/loop12 -o logdev=/dev/loop13"
> SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11"
> MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -lsize=1g"
> SCRATCH_LOGDEV=/dev/loop15
> SCRATCH_RTDEV=/dev/loop14
> USE_EXTERNAL=yes
> 

Sorry, running xfs/556 with this configuration was successful in my
environment, and my mkfs.xfs version is 6.8.0:

xfs/556
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 fedora 6.11.0-rc3-00015-g1a9f212eb19f #42
SMP PREEMPT_DYNAMIC Fri Aug 16 10:19:47 CST 2024
VMIP          -- 192.168.240.11
MKFS_OPTIONS  -- -f -f -m reflink=0,rmapbt=0 -d rtinherit=1 -l size=1g
/dev/vde
MOUNT_OPTIONS -- /dev/vde /tmp/scratch

xfs/556 4s ...  5s
Ran: xfs/556
Passed all 1 tests

I am not sure if it is because of the specific user mode tools or other
environment configuration differences caused?

Thanks,
Zizhi Wo

Re: [PATCH V4 0/2] Some bugfix for xfs fsmap

[Chandan Babu R]

On Tue, Aug 20, 2024 at 03:51:23 PM +0800, Zizhi Wo wrote:
> 在 2024/8/20 13:53, Chandan Babu R 写道:
>> On Mon, Aug 19, 2024 at 08:53:18 AM +0800, Zizhi Wo wrote:
>>> Changes since V3[1]:
>>>   - For the first patch, simply place the modification logic in the
>>>     xfs_fsmap_owner_to_rmap() function.
>>>   - For the second patch, more detailed comments were added and related
>>>     changes were made to the initialization of the end_daddr field.
>>>
>>> This patch set contains two patches to repair fsmap. Although they are both
>>> problems of missing query intervals, the root causes of the two are
>>> inconsistent, so two patches are proposed.
>>>
>>> Patch 1: The fix addresses the interval omission issue caused by the
>>> incorrect setting of "rm_owner" in the high_key during rmap queries. In
>>> this scenario, fsmap finds the record on the rmapbt, but due to the
>>> incorrect setting of the "rm_owner", the key of the record is larger than
>>> the high_key, causing the query result to be incorrect. This issue is
>>> resolved by fixing the "rm_owner" setup logic.
>>>
>>> Patch 2: The fix addresses the interval omission issue caused by bit
>>> shifting during gap queries in fsmap. In this scenario, fsmap does not
>>> find the record on the rmapbt, so it needs to locate it by the gap of the
>>> info->next_daddr and high_key address. However, due to the shift, the two
>>> are reduced to 0, so the query error is caused. The issue is resolved by
>>> introducing the "end_daddr" field in the xfs_getfsmap_info structure to
>>> store the high_key at the sector granularity.
>>>
>>> [1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/
>>>
>> The two patches in this series cause xfs_scrub to execute
>> indefinitely
>> immediately after xfs/556 is executed.
>> The fstest configuration used is provided below,
>> FSTYP=xfs
>> TEST_DIR=/media/test
>> SCRATCH_MNT=/media/scratch
>> TEST_DEV=/dev/loop16
>> TEST_LOGDEV=/dev/loop13
>> TEST_RTDEV=/dev/loop12
>> TEST_FS_MOUNT_OPTS="-o rtdev=/dev/loop12 -o logdev=/dev/loop13"
>> SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8
>> /dev/loop9 /dev/loop10 /dev/loop11"
>> MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -lsize=1g"
>> SCRATCH_LOGDEV=/dev/loop15
>> SCRATCH_RTDEV=/dev/loop14
>> USE_EXTERNAL=yes
>> 
>
> Sorry, running xfs/556 with this configuration was successful in my
> environment, and my mkfs.xfs version is 6.8.0:
>
> xfs/556
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 fedora 6.11.0-rc3-00015-g1a9f212eb19f #42
> SMP PREEMPT_DYNAMIC Fri Aug 16 10:19:47 CST 2024
> VMIP          -- 192.168.240.11
> MKFS_OPTIONS  -- -f -f -m reflink=0,rmapbt=0 -d rtinherit=1 -l size=1g
> /dev/vde
> MOUNT_OPTIONS -- /dev/vde /tmp/scratch
>
> xfs/556 4s ...  5s
> Ran: xfs/556
> Passed all 1 tests
>
> I am not sure if it is because of the specific user mode tools or other
> environment configuration differences caused?
>

My Linux kernel is based on v6.11-rc4. The sources can be found at
https://github.com/chandanr/linux/commits/xfs-6.11-fixesC-without-jump-label-fixes/.

Please note that I have reverted commits modifying kernel/jump_label.c. This
is to work around
https://lore.kernel.org/linux-xfs/20240730033849.GH6352@frogsfrogsfrogs/.

Also, I am running xfsprogs v6.9.0. The sources can be found at
https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/log/?qt=range&q=v6.9.0

-- 
Chandan

Re: [PATCH V4 0/2] Some bugfix for xfs fsmap

[Zizhi Wo]

在 2024/8/20 16:27, Chandan Babu R 写道:
> On Tue, Aug 20, 2024 at 03:51:23 PM +0800, Zizhi Wo wrote:
>> 在 2024/8/20 13:53, Chandan Babu R 写道:
>>> On Mon, Aug 19, 2024 at 08:53:18 AM +0800, Zizhi Wo wrote:
>>>> Changes since V3[1]:
>>>>    - For the first patch, simply place the modification logic in the
>>>>      xfs_fsmap_owner_to_rmap() function.
>>>>    - For the second patch, more detailed comments were added and related
>>>>      changes were made to the initialization of the end_daddr field.
>>>>
>>>> This patch set contains two patches to repair fsmap. Although they are both
>>>> problems of missing query intervals, the root causes of the two are
>>>> inconsistent, so two patches are proposed.
>>>>
>>>> Patch 1: The fix addresses the interval omission issue caused by the
>>>> incorrect setting of "rm_owner" in the high_key during rmap queries. In
>>>> this scenario, fsmap finds the record on the rmapbt, but due to the
>>>> incorrect setting of the "rm_owner", the key of the record is larger than
>>>> the high_key, causing the query result to be incorrect. This issue is
>>>> resolved by fixing the "rm_owner" setup logic.
>>>>
>>>> Patch 2: The fix addresses the interval omission issue caused by bit
>>>> shifting during gap queries in fsmap. In this scenario, fsmap does not
>>>> find the record on the rmapbt, so it needs to locate it by the gap of the
>>>> info->next_daddr and high_key address. However, due to the shift, the two
>>>> are reduced to 0, so the query error is caused. The issue is resolved by
>>>> introducing the "end_daddr" field in the xfs_getfsmap_info structure to
>>>> store the high_key at the sector granularity.
>>>>
>>>> [1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/
>>>>
>>> The two patches in this series cause xfs_scrub to execute
>>> indefinitely
>>> immediately after xfs/556 is executed.
>>> The fstest configuration used is provided below,
>>> FSTYP=xfs
>>> TEST_DIR=/media/test
>>> SCRATCH_MNT=/media/scratch
>>> TEST_DEV=/dev/loop16
>>> TEST_LOGDEV=/dev/loop13
>>> TEST_RTDEV=/dev/loop12
>>> TEST_FS_MOUNT_OPTS="-o rtdev=/dev/loop12 -o logdev=/dev/loop13"
>>> SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8
>>> /dev/loop9 /dev/loop10 /dev/loop11"
>>> MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -lsize=1g"
>>> SCRATCH_LOGDEV=/dev/loop15
>>> SCRATCH_RTDEV=/dev/loop14
>>> USE_EXTERNAL=yes
>>>
>>
>> Sorry, running xfs/556 with this configuration was successful in my
>> environment, and my mkfs.xfs version is 6.8.0:
>>
>> xfs/556
>> FSTYP         -- xfs (debug)
>> PLATFORM      -- Linux/x86_64 fedora 6.11.0-rc3-00015-g1a9f212eb19f #42
>> SMP PREEMPT_DYNAMIC Fri Aug 16 10:19:47 CST 2024
>> VMIP          -- 192.168.240.11
>> MKFS_OPTIONS  -- -f -f -m reflink=0,rmapbt=0 -d rtinherit=1 -l size=1g
>> /dev/vde
>> MOUNT_OPTIONS -- /dev/vde /tmp/scratch
>>
>> xfs/556 4s ...  5s
>> Ran: xfs/556
>> Passed all 1 tests
>>
>> I am not sure if it is because of the specific user mode tools or other
>> environment configuration differences caused?
>>
> 
> My Linux kernel is based on v6.11-rc4. The sources can be found at
> https://github.com/chandanr/linux/commits/xfs-6.11-fixesC-without-jump-label-fixes/.
> 
> Please note that I have reverted commits modifying kernel/jump_label.c. This
> is to work around
> https://lore.kernel.org/linux-xfs/20240730033849.GH6352@frogsfrogsfrogs/.
> 
> Also, I am running xfsprogs v6.9.0. The sources can be found at
> https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/log/?qt=range&q=v6.9.0
> 

Hello! I upgraded xfsprogs to 6.9.0, and tested the latest code
(reverted jump_label.c related code), but still no problems with the
test, maybe there is a problem with my other environment configuration?
Here is my configuration:

export FSTYP=xfs
export TEST_DEV=/dev/vdb
export TEST_DIR=/tmp/test
mkdir /tmp/test -p
export SCRATCH_DEV=/dev/vde
export SCRATCH_MNT=/tmp/scratch
mkdir /tmp/scratch -p
export TEST_LOGDEV=/dev/vdc
export TEST_RTDEV=/dev/vdd
export TEST_FS_MOUNT_OPTS="-o rtdev=/dev/vdd -o logdev=/dev/vdc"
export MKFS_OPTIONS="-f -m reflink=0,rmapbt=0 -d rtinherit=1 -l size=1g"
export USE_EXTERNAL=yes
echo xfs/556
./check xfs/556


However, this patch is not the final version, and I found another
statistical issue with fsmap locally. In addition, Darrick also found a
boundary-case bug in the patch, which may be responsible for the scrub 
indefinitely? I'm not sure. Anyway, I'll fix it again and test it, and
hopefully pass the test next time.

Thanks,
Zizhi Wo

Re: [PATCH V4 0/2] Some bugfix for xfs fsmap

[Zizhi Wo]

在 2024/8/20 16:27, Chandan Babu R 写道:
> On Tue, Aug 20, 2024 at 03:51:23 PM +0800, Zizhi Wo wrote:
>> 在 2024/8/20 13:53, Chandan Babu R 写道:
>>> On Mon, Aug 19, 2024 at 08:53:18 AM +0800, Zizhi Wo wrote:
>>>> Changes since V3[1]:
>>>>    - For the first patch, simply place the modification logic in the
>>>>      xfs_fsmap_owner_to_rmap() function.
>>>>    - For the second patch, more detailed comments were added and related
>>>>      changes were made to the initialization of the end_daddr field.
>>>>
>>>> This patch set contains two patches to repair fsmap. Although they are both
>>>> problems of missing query intervals, the root causes of the two are
>>>> inconsistent, so two patches are proposed.
>>>>
>>>> Patch 1: The fix addresses the interval omission issue caused by the
>>>> incorrect setting of "rm_owner" in the high_key during rmap queries. In
>>>> this scenario, fsmap finds the record on the rmapbt, but due to the
>>>> incorrect setting of the "rm_owner", the key of the record is larger than
>>>> the high_key, causing the query result to be incorrect. This issue is
>>>> resolved by fixing the "rm_owner" setup logic.
>>>>
>>>> Patch 2: The fix addresses the interval omission issue caused by bit
>>>> shifting during gap queries in fsmap. In this scenario, fsmap does not
>>>> find the record on the rmapbt, so it needs to locate it by the gap of the
>>>> info->next_daddr and high_key address. However, due to the shift, the two
>>>> are reduced to 0, so the query error is caused. The issue is resolved by
>>>> introducing the "end_daddr" field in the xfs_getfsmap_info structure to
>>>> store the high_key at the sector granularity.
>>>>
>>>> [1] https://lore.kernel.org/all/20240812011505.1414130-1-wozizhi@huawei.com/
>>>>
>>> The two patches in this series cause xfs_scrub to execute
>>> indefinitely
>>> immediately after xfs/556 is executed.
>>> The fstest configuration used is provided below,
>>> FSTYP=xfs
>>> TEST_DIR=/media/test
>>> SCRATCH_MNT=/media/scratch
>>> TEST_DEV=/dev/loop16
>>> TEST_LOGDEV=/dev/loop13
>>> TEST_RTDEV=/dev/loop12
>>> TEST_FS_MOUNT_OPTS="-o rtdev=/dev/loop12 -o logdev=/dev/loop13"
>>> SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8
>>> /dev/loop9 /dev/loop10 /dev/loop11"
>>> MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -lsize=1g"
>>> SCRATCH_LOGDEV=/dev/loop15
>>> SCRATCH_RTDEV=/dev/loop14
>>> USE_EXTERNAL=yes
>>>
>>
>> Sorry, running xfs/556 with this configuration was successful in my
>> environment, and my mkfs.xfs version is 6.8.0:
>>
>> xfs/556
>> FSTYP         -- xfs (debug)
>> PLATFORM      -- Linux/x86_64 fedora 6.11.0-rc3-00015-g1a9f212eb19f #42
>> SMP PREEMPT_DYNAMIC Fri Aug 16 10:19:47 CST 2024
>> VMIP          -- 192.168.240.11
>> MKFS_OPTIONS  -- -f -f -m reflink=0,rmapbt=0 -d rtinherit=1 -l size=1g
>> /dev/vde
>> MOUNT_OPTIONS -- /dev/vde /tmp/scratch
>>
>> xfs/556 4s ...  5s
>> Ran: xfs/556
>> Passed all 1 tests
>>
>> I am not sure if it is because of the specific user mode tools or other
>> environment configuration differences caused?
>>
> 
> My Linux kernel is based on v6.11-rc4. The sources can be found at
> https://github.com/chandanr/linux/commits/xfs-6.11-fixesC-without-jump-label-fixes/.
> 
> Please note that I have reverted commits modifying kernel/jump_label.c. This
> is to work around
> https://lore.kernel.org/linux-xfs/20240730033849.GH6352@frogsfrogsfrogs/.
> 
> Also, I am running xfsprogs v6.9.0. The sources can be found at
> https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/log/?qt=range&q=v6.9.0
> 

Oh, I made a silly configuration mistake. I have already reproduced the
failure of this use case locally. Sorry for bothering you.

Thanks,
Zizhi Wo