Re: [PATCH v8 01/13] fstests: add _loop_image_create_clone() helper

[Zorro Lang <zlang@kernel.org>]

On Sat, Jul 25, 2026 at 03:38:58PM +0800, Anand Jain wrote:
> Introduce _loop_image_create_clone() and _loop_image_destroy() to mkfs an
> image file and clone it to another image file, and attach a loop device to
> them. And its destroy part.
> 
> Signed-off-by: Anand Jain <asj@kernel.org>
> ---
>  common/rc | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 106f044adc2d..e8ff5be0c547 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1520,6 +1520,71 @@ _scratch_resvblks()
>  	esac
>  }
>  
> +# Create a small loop image, run an optional tuning function ($2) on it,
> +# clone it, and attach both to loop devices, returned in ($1).
> +# Args:
> +#   $1: Nameref to return the array of allocated loop devices [base, clone].
> +#   $2: Optional callback function to tune the base filesystem before cloning.
> +_loop_image_create_clone()

To capture both the relationship and distinction between this function and
existing _create_loop_device, I'd like to rename it to *_create_cloned_loop_devs*.

> +{
> +	local -n _ret=$1
> +	local pre_clone_tune_func="$2"
> +	local img_file=$TEST_DIR/${seq}.img
> +	local img_file_clone=$TEST_DIR/${seq}_clone.img
> +	local size=$(_small_fs_size_mb 128) # Smallest possible
> +	local loop_devs=()
> +
> +	# Since we copy the block device image, we keep its size small.
> +	_require_fs_space $TEST_DIR $((size * 1024 * 2))
> +
> +	_create_file_sized $((size * 1024 * 1024)) $img_file ||
> +				_fail "Failed: Create $img_file $size"
> +
> +	loop_devs=("$(_create_loop_device $img_file)")
> +
> +	case $FSTYP in
> +	xfs)
> +		_mkfs_dev -s size=4096 "${loop_devs[0]}"

Why xfs needs a specific "-s size=4096" ? Hardcoding this parameter might cause
failures in certain incompatible environments.

> +		;;
> +	btrfs)
> +		_mkfs_dev "${loop_devs[0]}"

If btrfs is handled the same way as the default `*)` branch below, there is
no need to have a dedicated branch for it.

> +		;;
> +	*)
> +		_mkfs_dev "${loop_devs[0]}"
> +		;;
> +	esac
> +
> +	# Only execute if the function argument is not empty
> +	if [ -n "$pre_clone_tune_func" ]; then
> +		$pre_clone_tune_func "${loop_devs[0]}"
> +	fi
> +
> +	sync "${loop_devs[0]}"

Since the mkfs command above operates directly on a loop device, I wanted to
make sure, can the sync command be targeted at a block device, and does it
guarantee a cache flush or blockdev page cache sync ?

How about:
  sync -d "${loop_devs[0]}"
  blockdev --flushbufs "${loop_devs[0]}"
or any better idea?

> +	cp $img_file $img_file_clone || _fail "Failed to copy cloned image"
> +
> +	loop_devs+=("$(_create_loop_device $img_file_clone)")
> +
> +	_ret=()
> +	for i in "${loop_devs[@]}"; do
> +		_ret+=("$i")
> +	done

How about:
  _ret=("${loop_devs[@]}")
directly?

> +}
> +
> +# Teardown loop devices and delete their underlying backing image files.
> +# Accepts a list of loop device paths (e.g., /dev/loop0 /dev/loop1).
> +_loop_image_destroy()

To keep it consistent with the renamed function above, we could rename this
to *_destroy_cloned_loop_devs*. Alternatively, if we don't consider it
clone-specific, *destroy_loop_devs* would also be good to me.

Thanks,
Zorro

> +{
> +	for d in "$@"; do
> +		# Retrieve the path of the backing file
> +		local f=$(losetup --noheadings --output BACK-FILE $d)
> +
> +		# Detach the loop device from the backing file
> +		_destroy_loop_device "$d"
> +
> +		# Clean up the backing disk image file
> +		[ -n "$f" ] && rm -f "$f"
> +	done
> +}
>  
>  # Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
>  # errors found or errors were fixed) and nonzero otherwise; also spits out
> -- 
> 2.43.0
>