Re: [PATCH 1/2] ext4: Fix fsmap end of range reporting with bigalloc

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 05, 2025 at 02:00:30PM +0530, Ojaswin Mujoo wrote:
> With bigalloc enabled, the logic to report last extent has a bug since
> we try to use cluster units instead of block units. This can cause an issue
> where extra incorrect entries might be returned back to the user. This was
> flagged by generic/365 with 64k bs and -O bigalloc.
> 
> ** Details of issue **
> 
> The issue was noticed on 5G 64k blocksize FS with -O bigalloc which has
> only 1 bg.
> 
> $ xfs_io -c "fsmap -d" /mnt/scratch
> 
>   0: 253:48 [0..127]: static fs metadata 128   /* sb */
>   1: 253:48 [128..255]: special 102:1 128   /* gdt */
>   3: 253:48 [256..383]: special 102:3 128   /* block bitmap */
>   4: 253:48 [384..2303]: unknown 1920       /* flex bg empty space */
>   5: 253:48 [2304..2431]: special 102:4 128   /* inode bitmap */
>   6: 253:48 [2432..4351]: unknown 1920      /* flex bg empty space */
>   7: 253:48 [4352..6911]: inodes 2560
>   8: 253:48 [6912..538623]: unknown 531712
>   9: 253:48 [538624..10485759]: free space 9947136
> 
> The issue can be seen with:
> 
> $ xfs_io -c "fsmap -d 0 3" /mnt/scratch
> 
>   0: 253:48 [0..127]: static fs metadata 128
>   1: 253:48 [384..2047]: unknown 1664
> 
> Only the first entry was expected to be returned but we get 2. This is
> because:
> 
> ext4_getfsmap_datadev()
>   first_cluster, last_cluster = 0
>   ...
>   info->gfi_last = true;
>   ext4_getfsmap_datadev_helper(sb, end_ag, last_cluster + 1, 0, info);
>     fsb = C2B(1) = 16
>     fslen = 0
>     ...
>     /* Merge in any relevant extents from the meta_list */
>     list_for_each_entry_safe(p, tmp, &info->gfi_meta_list, fmr_list) {
>       ...
>       // since fsb = 16, considers all metadata which starts before 16 blockno
>       iter 1: error = ext4_getfsmap_helper(sb, info, p);  // p = sb (0,1), nop
>         info->gfi_next_fsblk = 1
>       iter 2: error = ext4_getfsmap_helper(sb, info, p);  // p = gdt (1,2), nop
>         info->gfi_next_fsblk = 2
>       iter 3: error = ext4_getfsmap_helper(sb, info, p);  // p = blk bitmap (2,3), nop
>         info->gfi_next_fsblk = 3
>       iter 4: error = ext4_getfsmap_helper(sb, info, p);  // p = ino bitmap (18,19)
>         if (rec_blk > info->gfi_next_fsblk) { // (18 > 3)
>           // emits an extra entry ** BUG **
>         }
>     }
> 
> Fix this by directly calling ext4_getfsmap_datadev() with a dummy record
> that has fmr_physical set to (end_fsb + 1) instead of last_cluster + 1. By
> using the block instead of cluster we get the correct behavior.
> 
> Replacing ext4_getfsmap_datadev_helper() with ext4_getfsmap_helper() is
> okay since the gfi_lastfree and metadata checks in
> ext4_getfsmap_datadev_helper() are anyways redundant when we only want to
> emit the last allocated block of the range, as we have already taken care
> of emitting metadata and any last free blocks.
> 
> Reported-by: Disha Goel <disgoel@linux.ibm.com>
> Fixes: 4a622e4d477b ("ext4: fix FS_IOC_GETFSMAP handling")
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Looks fine to me
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/ext4/fsmap.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
> index 383c6edea6dd..9d63c39f6077 100644
> --- a/fs/ext4/fsmap.c
> +++ b/fs/ext4/fsmap.c
> @@ -526,6 +526,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
>  	ext4_group_t end_ag;
>  	ext4_grpblk_t first_cluster;
>  	ext4_grpblk_t last_cluster;
> +	struct ext4_fsmap irec;
>  	int error = 0;
>  
>  	bofs = le32_to_cpu(sbi->s_es->s_first_data_block);
> @@ -609,10 +610,18 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
>  			goto err;
>  	}
>  
> -	/* Report any gaps at the end of the bg */
> +	/*
> +	 * The dummy record below will cause ext4_getfsmap_helper() to report
> +	 * any allocated blocks at the end of the range.
> +	 */
> +	irec.fmr_device = 0;
> +	irec.fmr_physical = end_fsb + 1;
> +	irec.fmr_length = 0;
> +	irec.fmr_owner = EXT4_FMR_OWN_FREE;
> +	irec.fmr_flags = 0;
> +
>  	info->gfi_last = true;
> -	error = ext4_getfsmap_datadev_helper(sb, end_ag, last_cluster + 1,
> -					     0, info);
> +	error = ext4_getfsmap_helper(sb, info, &irec);
>  	if (error)
>  		goto err;
>  
> -- 
> 2.49.0
> 
>