Re: [PATCH blktests v2 1/2] bcache: add bcache/001

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

Hi Daniel, thank you for working on this. It's great to extend the test
coverage :)

Please find my review comments in line. I ran the new test case, and observed a
failure. I noted my findings about the failure as one of the inline comments.

On Jan 21, 2026 / 15:36, Daniel Wagner wrote:
> So far we are missing tests for bcache. Besides a relative simple
> setup/teardown tests add also the corresponding infrastructure. More
> tests are to be expected to depend on this.
> 
> _create_bcache/_remove_bcache are tracking the resources and if anything
> is missing it will complain.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  tests/bcache/001     |  32 +++++++
>  tests/bcache/001.out |   1 +
>  tests/bcache/rc      | 259 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 292 insertions(+)
> 
> diff --git a/tests/bcache/001 b/tests/bcache/001
> new file mode 100644

To be consistent with other test case files, I suggest to set the file mode 755.

> index 000000000000..4a6e01113b6b
> --- /dev/null
> +++ b/tests/bcache/001
> @@ -0,0 +1,32 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

Most of blktests files specifies GPL-3.0+. If there is no specific reason,
I suggest GPL-3.0+. Same comment for tests/bcache/rc.

> +# Copyright (C) 2026 Daniel Wagner, SUSE Labs
> +#
> +# Test bcache setup and teardown
> +
> +. tests/bcache/rc
> +
> +DESCRIPTION="test bcache setup and teardown"
> +
> +requires() {
> +	_bcache_requires
> +}

Do you foresee that all bcache/* test cases have this _bcache_requires call?
If so, I suggest to,
 - rename _bcache_requires() in tests/bcache/rc to group_requires(), and
 - drop the three lines above from bcache/001.
group_requires() is called once before start testing this bcache group.

> +
> +test_device_array() {
> +	echo "Running ${TEST_NAME}"
> +
> +	if [[ ${#TEST_DEV_ARRAY[@]} -lt 3 ]]; then
> +		SKIP_REASONS+=("requires at least 3 devices")
> +		return 1
> +	fi
> +
> +	 _create_bcache \
> +		--cache "${TEST_DEV_ARRAY[0]##*/}" \
> +		--bdev "${TEST_DEV_ARRAY[1]##*/}"
> +	 _remove_bcache
> +
> +	 _create_bcache \
> +		--cache "${TEST_DEV_ARRAY[0]##*/}" \
> +		--bdev "${TEST_DEV_ARRAY[1]##*/}" "${TEST_DEV_ARRAY[2]##*/}"
> +	 _remove_bcache
> +}
> diff --git a/tests/bcache/001.out b/tests/bcache/001.out
> new file mode 100644
> index 000000000000..f890aed2736c
> --- /dev/null
> +++ b/tests/bcache/001.out
> @@ -0,0 +1 @@
> +Running bcache/001
> diff --git a/tests/bcache/rc b/tests/bcache/rc
> new file mode 100644
> index 000000000000..3dba6c85b1ee
> --- /dev/null
> +++ b/tests/bcache/rc
> @@ -0,0 +1,259 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2026 Daniel Wagner, SUSE Labs
> +
> +. common/rc
> +
> +declare -a BCACHE_DEVS=()
> +declare -a BCACHE_BDEVS=()
> +declare -a BCACHE_CSETS=()
> +
> +BCACHE_MAX_RETRIES=5
> +
> +_bcache_requires() {
> +	_have_kernel_options MD BCACHE BCACHE_DEBUG AUTOFS_FS
> +	_have_program make-bcache
> +	_have_crypto_algorithm crc32c
> +}

As noted above, you may want to rename _bcache_requires() to group_requires().

> +
> +_bcache_wipe_devs() {

Nit: "local dev" can be added here.

> +	for dev in "${BCACHE_DEVS[@]}"; do
> +		# Attempt a clean wipe first
> +		if wipefs --all --quiet "${dev}" 2>/dev/null; then
> +			continue
> +		fi
> +
> +		# Overwrite the first 10MB to clear stubborn partition tables or metadata
> +		if ! dd if=/dev/zero of="${dev}" bs=1M count=10 conv=notrunc status=none; then
> +			echo "Error: dd failed on ${dev}" >&2
> +		fi
> +
> +		# Try wiping again after clearing the headers
> +		if ! wipefs --all --quiet --force "${dev}"; then
> +			echo "Warning: Failed to wipe ${dev} even after dd." >&2
> +		fi
> +	done
> +}
> +
> +_bcache_register() {

When I did trial run, the test case bcache/001 failed with the message below.

bcache/001 => nvme1n1 nvme2n1 nvme3n1 nvme4n1 (test bcache setup and teardown) [failed]
    runtime  8.406s  ...  12.789s
    --- tests/bcache/001.out    2026-01-22 13:55:00.510094421 +0900
    +++ /home/shin/Blktests/blktests/results/nvme1n1_nvme2n1_nvme3n1_nvme4n1/bcache/001.out.bad 2026-01-22 16:21:47.686976578 +0900
    @@ -1 +1,3 @@
     Running bcache/001
    +ERROR: bcache registration interface not found.
    +WARNING: Could not find device node for UUID b854b766-8e08-42b5-b7cc-e137fd78ca51 after 5s

I found that when the bcache driver is loadable and not yet loaded, the
make-bcache command call loads the driver. However, it takes time for
kernel to prepare /sys/fs/bcache/register. Hence the error message above.
I suggest to add a hunk below here, to wait for the register file get ready.
With this change, the failure disappeared in my test environment.

       local dev err_msg timeout=0

       while [[ ! -w /sys/fs/bcache/register ]] && (( timeout < 10 )); do
               sleep 1
               (( timeout ++ ))
       done

> +	if [[ ! -w /sys/fs/bcache/register ]]; then
> +		echo "ERROR: bcache registration interface not found." >&2
> +		return 1
> +	fi
> +
> +	for dev in "${BCACHE_DEVS[@]}"; do
> +		if ! echo "${dev}" > /sys/fs/bcache/register 2>/tmp/bcache_err; then
> +			err_msg=$(< /tmp/bcache_err)
> +
> +			if [[ "${err_msg}" != *"Device or resource busy"* ]]; then
> +				echo "ERROR: Failed to register ${dev}: ${err_msg:-"Unknown error"}" >&2
> +			fi
> +		fi
> +	done
> +}
> +
> +_create_bcache() {
> +	local -a cdevs=()
> +	local -a bdevs=()
> +	local -a ARGS=()
> +	local bucket_size="64k"
> +	local block_size="4k"
> +
> +	_register_test_cleanup _cleanup_bcache
> +
> +	while [[ $# -gt 0 ]]; do
> +		case $1 in
> +			--cache)
> +				shift
> +				# Collect arguments until the next flag or end of input
> +				while [[ $# -gt 0 && ! $1 =~ ^-- ]]; do
> +					cdevs+=("$1")
> +					shift
> +				done
> +				;;
> +			--bdev)
> +				shift
> +				# Collect arguments until the next flag or end of input
> +				while [[ $# -gt 0 && ! $1 =~ ^-- ]]; do
> +					bdevs+=("$1")
> +					shift
> +				done
> +				;;
> +			--bucket-size)
> +				bucket_size="$2"
> +				shift 2
> +				;;
> +			--block-size)
> +				block_size="$2"
> +				shift 2
> +				;;
> +			--writeback)
> +				ARGS+=(--writeback)
> +				shift 1
> +				;;
> +			--discard)
> +				ARGS+=(--discard)
> +				shift 1
> +				;;
> +			*)
> +				echo "WARNING: unknown argument: $1"
> +				shift
> +				;;
> +		esac
> +	done
> +
> +	# add /dev prefix to device names
> +	cdevs=( "${cdevs[@]/#/\/dev\/}" )
> +	bdevs=( "${bdevs[@]/#/\/dev\/}" )
> +
> +	# make-bcache expects empty/cleared devices
> +	BCACHE_DEVS+=("${cdevs[@]}" "${bdevs[@]}")
> +	_bcache_wipe_devs
> +
> +	local -a cdevs_args=()

Nit: "local dev" can be added here.

> +	for dev in "${cdevs[@]}"; do
> +		cdevs_args+=("--cache" "${dev}")
> +	done
> +
> +	local -a bdevs_args=()
> +	for dev in "${bdevs[@]}"; do
> +		bdevs_args+=("--bdev" "${dev}")
> +	done
> +
> +	local output cmd

"cmd" is array, then, I guess you meant,

        local -a cmd
	local output

> +	cmd=(make-bcache \
> +		--wipe-bcache \
> +		--bucket "${bucket_size}" \
> +		--block "${block_size}" \
> +		"${cdevs_args[@]}" \
> +		"${bdevs_args[@]}" \
> +		"${ARGS[@]}")
> +
> +	output=$("${cmd[@]}" 2>&1)
> +	local rc=$?
> +	if [[ "${rc}" -ne 0 ]]; then
> +		echo "ERROR: make-bcache failed:" >&2
> +		echo "$output" >&2
> +		return 1
> +	fi
> +
> +	local cset_uuid
> +	cset_uuid=$(echo "$output" | awk '/Set UUID:/ {print $3}' | head -n 1)
> +	if [[ -z "${cset_uuid}" ]]; then
> +		echo "ERROR: Could not extract cset UUID from make-bcache output" >&2
> +		return 1
> +	fi
> +	BCACHE_CSETS+=("${cset_uuid}")
> +
> +	local -a bdev_uuids
> +	mapfile -t bdev_uuids < <(echo "$output" | awk '
> +	  $1 == "UUID:" { last_uuid = $2 }
> +	  $1 == "version:" && $2 == "1" { print last_uuid}
> +	')
> +
> +	udevadm settle
> +
> +	_bcache_register
> +
> +	for uuid in "${bdev_uuids[@]}"; do
> +		local link found attempt
> +
> +		link=/dev/bcache/by-uuid/"${uuid}"
> +		found=false
> +		attempt=0
> +
> +		while (( attempt < BCACHE_MAX_RETRIES )); do
> +			if [[ -L "$link" ]]; then
> +				BCACHE_BDEVS+=("${uuid}")
> +				found=true
> +				break
> +			fi
> +
> +			(( attempt++ ))
> +			sleep 1
> +		done
> +
> +		if [[ "$found" == "false" ]]; then
> +		   echo "WARNING: Could not find device node for UUID ${uuid} after ${BCACHE_MAX_RETRIES}s" >&2

Two spaces are used for the indent above.

> +		fi
> +	done
> +}
> +
> +_remove_bcache() {
> +	local uuid
> +
> +	for uuid in "${BCACHE_BDEVS[@]}"; do
> +		local dev_path
> +
> +		dev_path=$(blkid -U "${uuid}")
> +		if [ -n "$dev_path" ]; then
> +			local dev_name
> +
> +			dev_name="${dev_path##*/}"
> +			if [ -f "/sys/block/${dev_name}/bcache/stop" ] ; then
> +			   echo 1 > "/sys/block/${dev_name}/bcache/stop"

Same here.

> +			fi
> +		fi
> +	done
> +
> +	for uuid in "${BCACHE_CSETS[@]}"; do
> +		if [ -f /sys/fs/bcache/"${uuid}"/unregister ] ; then
> +		   echo 1 > /sys/fs/bcache/"${uuid}"/unregister

Same here.

> +		fi
> +	done
> +
> +	udevadm settle
> +
> +	local timeout
> +	timeout=0
> +	for uuid in "${BCACHE_CSETS[@]}"; do
> +		while [[ -d "/sys/fs/bcache/${uuid}" ]] && (( timeout < 10 )); do
> +			sleep 0.5
> +			(( timeout++ ))
> +		done
> +	done
> +
> +	_bcache_wipe_devs
> +
> +	BCACHE_CSETS=()
> +	BCACHE_BDEVS=()
> +}
> +
> +_cleanup_bcache() {
> +	local cset dev
> +
> +	shopt -s nullglob
> +	for dev in /sys/block/bcache* ; do
> +		[ -e "${dev}" ] || continue
> +
> +		dev=$(basename "${dev}")
> +		echo "WARNING: bcache device ${dev} found"
> +
> +		if [[ -f /sys/block/"${dev}"/bcache/stop ]]; then
> +			echo 1 > /sys/block/"${dev}"/bcache/stop 2>/dev/null || true

Do we need "|| true" here?

> +		fi
> +	done
> +
> +	for cset in /sys/fs/bcache/*-*-*-*-*; do
> +		if [[ -d "${cset}" ]]; then
> +			echo "WARNING: Unregistering cset $(basename "${cset}")"
> +			echo 1 > "${cset}"/unregister 2>/dev/null || true

Same here.

> +		fi
> +	done
> +	shopt -u nullglob
> +
> +	udevadm settle
> +
> +	local timeout
> +	timeout=0
> +	for cset in /sys/fs/bcache/*-*-*-*-*; do
> +		while [[ -d "${cset}" ]] && (( timeout < 10 )); do
> +			sleep 0.5
> +			(( timeout++ ))
> +		done
> +	done
> +
> +	_bcache_wipe_devs
> +
> +	BCACHE_DEVS=()
> +}
> 
> -- 
> 2.52.0
>