Re: [PATCH] shared/298: call fs commands on the loop device

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

On Tue, Mar 05, 2024 at 06:22:46PM -0700, Christoph Hellwig wrote:
> In general calling fs tools is best done on the block device used for
> the file system and not the backing device of a loop file.  Thus switch
> shared/298 to call all fs commands on the loop device.  Also add a
> common on why the xfs_io fiemap command is called on the backing file,
> and to have a good place for the comment stop passing the backing file
> as the argument to get_holes function and just use it implicitly as
> the other helpers to with the loop device.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  tests/shared/298 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/shared/298 b/tests/shared/298
> index a6e368143..1d4e8d943 100755
> --- a/tests/shared/298
> +++ b/tests/shared/298
> @@ -46,7 +46,11 @@ get_holes()
>  	# to established convention which requires the filesystem to be
>  	# unmounted while we probe the underlying file.
>  	$UMOUNT_PROG $loop_mnt
> -	$XFS_IO_PROG -F -c fiemap $1 | grep hole | $SED_PROG 's/.*\[\(.*\)\.\.\(.*\)\].*/\1 \2/'
> +
> +	# FIEMAP only works on regular files, so call it on the backing file
> +	# and not the loop device like everything else
> +	$XFS_IO_PROG -F -c fiemap $img_file | grep hole | \
> +		$SED_PROG 's/.*\[\(.*\)\.\.\(.*\)\].*/\1 \2/'
>  	_mount $loop_dev $loop_mnt
>  }
>  
> @@ -55,7 +59,7 @@ get_free_sectors()
>  	case $FSTYP in
>  	ext4)
>  	$UMOUNT_PROG $loop_mnt
> -	$DUMPE2FS_PROG $img_file  2>&1 | grep " Free blocks" | cut -d ":" -f2- | \
> +	$DUMPE2FS_PROG $loop_dev  2>&1 | grep " Free blocks" | cut -d ":" -f2- | \
>  		tr ',' '\n' | $SED_PROG 's/^ //' | \
>  		$AWK_PROG -v spb=$sectors_per_block 'BEGIN{FS="-"};
>  		     NF {
> @@ -77,15 +81,15 @@ get_free_sectors()
>  		local device_size=$($BTRFS_UTIL_PROG filesystem show --raw $loop_mnt 2>&1 \
>  			| sed -n "s/^.*size \([0-9]*\).*$/\1/p")
>  
> -		local nodesize=$($BTRFS_UTIL_PROG inspect-internal dump-super $img_file  \
> +		local nodesize=$($BTRFS_UTIL_PROG inspect-internal dump-super $loop_dev \
>  			| sed -n 's/nodesize\s*\(.*\)/\1/p')
>  
>  		# Get holes within block groups
> -		$BTRFS_UTIL_PROG inspect-internal dump-tree -t extent $img_file \
> +		$BTRFS_UTIL_PROG inspect-internal dump-tree -t extent $loop_dev \
>  			| $AWK_PROG -v sectorsize=512 -v nodesize=$nodesize -f $here/src/parse-extent-tree.awk
>  
>  		# Get holes within unallocated space on disk
> -		$BTRFS_UTIL_PROG inspect-internal dump-tree -t dev $img_file \
> +		$BTRFS_UTIL_PROG inspect-internal dump-tree -t dev $loop_dev \
>  			| $AWK_PROG -v sectorsize=512 -v devsize=$device_size -f $here/src/parse-dev-tree.awk
>  
>  	;;
> @@ -159,7 +163,7 @@ done
>  
>  # Get reference fiemap, this can contain i.e. uninitialized inode table
>  sync
> -get_holes $img_file > $fiemap_ref
> +get_holes > $fiemap_ref
>  
>  # Delete some files
>  find $loop_mnt -type f -print | $AWK_PROG \
> @@ -173,7 +177,7 @@ echo "done."
>  echo -n "Detecting interesting holes in image..."
>  # Get after-trim fiemap
>  sync
> -get_holes $img_file > $fiemap_after
> +get_holes > $fiemap_after
>  echo "done."
>  
>  echo -n "Comparing holes to the reported space from FS..."
> -- 
> 2.39.2
> 
>