[PATCH blktests v2 0/2] Test zone mapping of logical devices

[PATCH blktests v2 0/2] Test zone mapping of logical devices

[Shin'ichiro Kawasaki]

Add a test case to zbd group to check that zones sector mapping correctness for
zoned block devices that are not an entire full device (null_block device or
physical device). These logical devices are for now partition devices or
device-mapper devices (dm-linear or dm-flakey).

Changes from v1:
* Reflected Chaitanya's review comments on the list
* Separated helper functions in zbd/rc into a preparation patch

Shin'ichiro Kawasaki (2):
  zbd/rc: Introduce helper functions for zone mapping test
  zbd/007: Add zone mapping test for logical devices

 tests/zbd/007     | 110 ++++++++++++++++++++++++++++++++++++++
 tests/zbd/007.out |   2 +
 tests/zbd/rc      | 133 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+)
 create mode 100755 tests/zbd/007
 create mode 100644 tests/zbd/007.out

-- 
2.21.0

[PATCH blktests v2 1/2] zbd/rc: Introduce helper functions for zone mapping test

[Shin'ichiro Kawasaki]

As a preparation for the zone mapping test case, add several helper
functions. _find_last_sequential_zone() and
_find_sequential_zone_in_middle() help to select test target zones.
_test_dev_is_logical() checks TEST_DEV is the valid test target.
_test_dev_has_dm_map() helps to check that the dm target is linear or
flakey. _get_dev_container_and_sector() helps to get the container device
and sector mappings.
---
 tests/zbd/rc | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/tests/zbd/rc b/tests/zbd/rc
index 5f04c84..792b83d 100644
--- a/tests/zbd/rc
+++ b/tests/zbd/rc
@@ -193,6 +193,42 @@ _find_first_sequential_zone() {
 	return 1
 }
 
+_find_last_sequential_zone() {
+	for ((idx = REPORTED_COUNT - 1; idx > 0; idx--)); do
+		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
+			echo "${idx}"
+			return 0
+		fi
+	done
+
+	echo "-1"
+	return 1
+}
+
+# Try to find a sequential required zone between given two zone indices
+_find_sequential_zone_in_middle() {
+	local -i s=${1}
+	local -i e=${2}
+	local -i idx=$(((s + e) / 2))
+	local -i i=1
+
+	while ((idx != s && idx != e)); do
+		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
+			echo "${idx}"
+			return 0
+		fi
+		if ((i%2 == 0)); then
+			$((idx += i))
+		else
+			$((idx -= i))
+		fi
+		$((i++))
+	done
+
+	echo "-1"
+	return 1
+}
+
 # Search zones and find two contiguous sequential required zones.
 # Return index of the first zone of the found two zones.
 # Call _get_blkzone_report() beforehand.
@@ -210,3 +246,100 @@ _find_two_contiguous_seq_zones() {
 	echo "Contiguous sequential write required zones not found"
 	return 1
 }
+
+_test_dev_is_dm() {
+	if [[ ! -r "${TEST_DEV_SYSFS}/dm/name" ]]; then
+		SKIP_REASON="$TEST_DEV is not device-mapper"
+		return 1
+	fi
+	return 0
+}
+
+_test_dev_is_logical() {
+	if ! _test_dev_is_partition && ! _test_dev_is_dm; then
+		SKIP_REASON="$TEST_DEV is not a logical device"
+		return 1
+	fi
+	return 0
+}
+
+_test_dev_has_dm_map() {
+	local target_type=${1}
+	local dm_name
+
+	dm_name=$(<"${TEST_DEV_SYSFS}/dm/name")
+	if ! dmsetup status "${dm_name}" | grep -qe "${target_type}"; then
+		SKIP_REASON="$TEST_DEV does not have ${target_type} map"
+		return 1
+	fi
+	if dmsetup status "${dm_name}" | grep -v "${target_type}"; then
+		SKIP_REASON="$TEST_DEV has map other than ${target_type}"
+		return 1
+	fi
+	return 0
+}
+
+# Get device file path from the device ID "major:minor".
+_get_dev_path_by_id() {
+	for d in /sys/block/*; do
+		if [[ ! -r "${d}/dev" ]]; then
+			continue
+		fi
+		if [[ "${1}" == "$(<"${d}/dev")" ]]; then
+			echo "/dev/${d##*/}"
+			return 0
+		fi
+	done
+	return 1
+}
+
+# Given sector of TEST_DEV, return the device which contain the sector and
+# corresponding sector of the container device.
+_get_dev_container_and_sector() {
+	local -i sector=${1}
+	local cont_dev
+	local -i offset
+	local -a tbl_line
+
+	if _test_dev_is_partition; then
+		offset=$(<"${TEST_DEV_PART_SYSFS}/start")
+		cont_dev=$(_get_dev_path_by_id "$(<"${TEST_DEV_SYSFS}/dev")")
+		echo "${cont_dev}" "$((offset + sector))"
+		return 0
+	fi
+
+	if ! _test_dev_is_dm; then
+		echo "${TEST_DEV} is not a logical device"
+		return 1
+	fi
+	if ! _test_dev_has_dm_map linear &&
+			! _test_dev_has_dm_map flakey; then
+		echo -n "dm mapping test other than linear/flakey is"
+		echo "not implemented"
+		return 1
+	fi
+
+	# Parse dm table lines for dm-linear or dm-flakey target
+	while read -r -a tbl_line; do
+		local -i map_start=${tbl_line[0]}
+		local -i map_end=$((tbl_line[0] + tbl_line[1]))
+
+		if ((sector < map_start)) || (((map_end) <= sector)); then
+			continue
+		fi
+
+		offset=${tbl_line[4]}
+		if ! cont_dev=$(_get_dev_path_by_id "${tbl_line[3]}"); then
+			echo -n "Cannot access to container device: "
+			echo "${tbl_line[3]}"
+			return 1
+		fi
+
+		echo "${cont_dev}" "$((offset + sector - map_start))"
+		return 0
+
+	done < <(dmsetup table "$(<"${TEST_DEV_SYSFS}/dm/name")")
+
+	echo -n "Cannot find container device of ${TEST_DEV}"
+	return 1
+}
-- 
2.21.0

[PATCH blktests v2 2/2] zbd/007: Add zone mapping test for logical devices

[Shin'ichiro Kawasaki]

Add the test case to check zones sector mapping of logical devices. This
test case requires that such a logical device be specified in TEST_DEVS
in config. The test is skipped for devices that are identified as not
logically created.

To test that the zone mapping is correct, select a few sequential write
required zones of the logical device and move the write pointers of
these zones through the container device of the logical device, using
the physical sector mapping of the zones. The write pointers position of
the selected zones is then checked through a zone report of the logical
device using the logical sector mapping of the zones. The test reports a
success if the position of the zone write pointers relative to the zone
start sector must be identical for both the logical and physical
locations of the zones.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/zbd/007     | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/zbd/007.out |   2 +
 2 files changed, 112 insertions(+)
 create mode 100755 tests/zbd/007
 create mode 100644 tests/zbd/007.out

diff --git a/tests/zbd/007 b/tests/zbd/007
new file mode 100755
index 0000000..b4dcbd8
--- /dev/null
+++ b/tests/zbd/007
@@ -0,0 +1,110 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2019 Western Digital Corporation or its affiliates.
+#
+# Test zones are mapped correctly between a logical device and its container
+# device. Move write pointers of sequential write required zones on the
+# container devices, and confirm same write pointer positions of zones on the
+# logical devices.
+
+. tests/zbd/rc
+
+DESCRIPTION="zone mapping between logical and container devices"
+CAN_BE_ZONED=1
+QUICK=1
+
+requires() {
+	_have_program dmsetup
+}
+
+device_requires() {
+	_test_dev_is_logical
+}
+
+# Select test target zones. Pick up the first sequential required zones. If
+# available, add one or two more sequential required zones. One is at the last
+# end of TEST_DEV. The other is in middle between the first and the last zones.
+select_zones() {
+	local -i zone_idx
+	local -a zones
+
+	zone_idx=$(_find_first_sequential_zone) || return $?
+	zones=( "${zone_idx}" )
+	if zone_idx=$(_find_last_sequential_zone); then
+		zones+=( "${zone_idx}" )
+		if zone_idx=$(_find_sequential_zone_in_middle \
+				      "${zones[0]}" "${zones[1]}"); then
+			zones+=( "${zone_idx}" )
+		fi
+	fi
+	echo "${zones[@]}"
+}
+
+test_device() {
+	local -i bs
+	local -a test_z # test target zones
+	local -a test_z_start
+
+	echo "Running ${TEST_NAME}"
+
+	# Get physical block size to meet zoned block device I/O requirement
+	_get_sysfs_variable "${TEST_DEV}" || return $?
+	bs=${SYSFS_VARS[SV_PHYS_BLK_SIZE]}
+	_put_sysfs_variable
+
+	# Get test target zones
+	_get_blkzone_report "${TEST_DEV}" || return $?
+	read -r -a test_z < <(select_zones)
+	for ((i = 0; i < ${#test_z[@]}; i++)); do
+		test_z_start+=("${ZONE_STARTS[test_z[i]]}")
+	done
+	echo "${test_z[*]}" >> "$FULL"
+	echo "${test_z_start[*]}" >> "$FULL"
+	_put_blkzone_report
+	if ((!${#test_z[@]})); then
+		echo "Test target zones not available on ${TEST_DEV}"
+		return 1
+	fi
+
+	# Reset and move write pointers of the container device
+	for ((i=0; i < ${#test_z[@]}; i++)); do
+		local -a arr
+
+		read -r -a arr < <(_get_dev_container_and_sector \
+					   "${test_z_start[i]}")
+		container_dev="${arr[0]}"
+		container_start="${arr[1]}"
+
+		echo "${container_dev}" "${container_start}" >> "$FULL"
+
+		if ! blkzone reset -o "${container_start}" -c 1 \
+		     "${container_dev}"; then
+			echo "Reset zone failed"
+			return 1
+		fi
+
+		if ! dd if=/dev/zero of="${container_dev}" bs="${bs}" \
+		     count=$((4096 * (i + 1) / bs)) oflag=direct \
+		     seek=$((container_start * 512 / bs)) \
+		     >> "$FULL" 2>&1 ; then
+			echo "dd failed"
+		fi
+
+		# Wait for partition table re-read event settles
+		udevadm settle
+	done
+
+	# Check write pointer positions on the logical device
+	_get_blkzone_report "${TEST_DEV}" || return $?
+	for ((i=0; i < ${#test_z[@]}; i++)); do
+		if ((ZONE_WPTRS[test_z[i]] != 8 * (i + 1))); then
+			echo "Unexpected write pointer position"
+			echo -n "zone=${i}, wp=${ZONE_WPTRS[i]}, "
+			echo "dev=${TEST_DEV}"
+		fi
+		echo "${ZONE_WPTRS[${test_z[i]}]}" >> "$FULL"
+	done
+	_put_blkzone_report
+
+	echo "Test complete"
+}
diff --git a/tests/zbd/007.out b/tests/zbd/007.out
new file mode 100644
index 0000000..28a1395
--- /dev/null
+++ b/tests/zbd/007.out
@@ -0,0 +1,2 @@
+Running zbd/007
+Test complete
-- 
2.21.0

Re: [PATCH blktests v2 1/2] zbd/rc: Introduce helper functions for zone mapping test

[Chaitanya Kulkarni]

Overall it looks good to me, couple of nits, can be ignored for now.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 5/30/19 6:59 PM, Shin'ichiro Kawasaki wrote:
> As a preparation for the zone mapping test case, add several helper
> functions. _find_last_sequential_zone() and
> _find_sequential_zone_in_middle() help to select test target zones.
> _test_dev_is_logical() checks TEST_DEV is the valid test target.
> _test_dev_has_dm_map() helps to check that the dm target is linear or
> flakey. _get_dev_container_and_sector() helps to get the container device
> and sector mappings.
> ---
>  tests/zbd/rc | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>
> diff --git a/tests/zbd/rc b/tests/zbd/rc
> index 5f04c84..792b83d 100644
> --- a/tests/zbd/rc
> +++ b/tests/zbd/rc
> @@ -193,6 +193,42 @@ _find_first_sequential_zone() {
>  	return 1
>  }
>  
> +_find_last_sequential_zone() {
> +	for ((idx = REPORTED_COUNT - 1; idx > 0; idx--)); do
> +		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
> +			echo "${idx}"
> +			return 0
> +		fi
> +	done
> +
> +	echo "-1"
> +	return 1
> +}
> +
> +# Try to find a sequential required zone between given two zone indices
> +_find_sequential_zone_in_middle() {
> +	local -i s=${1}
> +	local -i e=${2}
nit:- do we need to validate the s and e before we use ?
> +	local -i idx=$(((s + e) / 2))
> +	local -i i=1
> +
nit:- Is there a reason for while ? we can also get away with for loop
right ?
> +	while ((idx != s && idx != e)); do
> +		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
> +			echo "${idx}"
> +			return 0
> +		fi
> +		if ((i%2 == 0)); then
> +			$((idx += i))
> +		else
> +			$((idx -= i))
> +		fi
> +		$((i++))
> +	done
> +
> +	echo "-1"
> +	return 1
> +}
> +
>  # Search zones and find two contiguous sequential required zones.
>  # Return index of the first zone of the found two zones.
>  # Call _get_blkzone_report() beforehand.
> @@ -210,3 +246,100 @@ _find_two_contiguous_seq_zones() {
>  	echo "Contiguous sequential write required zones not found"
>  	return 1
>  }
> +
> +_test_dev_is_dm() {
> +	if [[ ! -r "${TEST_DEV_SYSFS}/dm/name" ]]; then
> +		SKIP_REASON="$TEST_DEV is not device-mapper"
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +_test_dev_is_logical() {
> +	if ! _test_dev_is_partition && ! _test_dev_is_dm; then
> +		SKIP_REASON="$TEST_DEV is not a logical device"
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +_test_dev_has_dm_map() {
> +	local target_type=${1}
> +	local dm_name
> +
> +	dm_name=$(<"${TEST_DEV_SYSFS}/dm/name")
> +	if ! dmsetup status "${dm_name}" | grep -qe "${target_type}"; then
> +		SKIP_REASON="$TEST_DEV does not have ${target_type} map"
> +		return 1
> +	fi
> +	if dmsetup status "${dm_name}" | grep -v "${target_type}"; then
> +		SKIP_REASON="$TEST_DEV has map other than ${target_type}"
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +# Get device file path from the device ID "major:minor".
> +_get_dev_path_by_id() {
> +	for d in /sys/block/*; do
> +		if [[ ! -r "${d}/dev" ]]; then
> +			continue
> +		fi
> +		if [[ "${1}" == "$(<"${d}/dev")" ]]; then
> +			echo "/dev/${d##*/}"
> +			return 0
> +		fi
> +	done
> +	return 1
> +}
> +
> +# Given sector of TEST_DEV, return the device which contain the sector and
> +# corresponding sector of the container device.
> +_get_dev_container_and_sector() {
> +	local -i sector=${1}
> +	local cont_dev
> +	local -i offset
> +	local -a tbl_line
> +
> +	if _test_dev_is_partition; then
> +		offset=$(<"${TEST_DEV_PART_SYSFS}/start")
> +		cont_dev=$(_get_dev_path_by_id "$(<"${TEST_DEV_SYSFS}/dev")")
> +		echo "${cont_dev}" "$((offset + sector))"
> +		return 0
> +	fi
> +
> +	if ! _test_dev_is_dm; then
> +		echo "${TEST_DEV} is not a logical device"
> +		return 1
> +	fi
> +	if ! _test_dev_has_dm_map linear &&
> +			! _test_dev_has_dm_map flakey; then
> +		echo -n "dm mapping test other than linear/flakey is"
> +		echo "not implemented"
> +		return 1
> +	fi
> +
> +	# Parse dm table lines for dm-linear or dm-flakey target
> +	while read -r -a tbl_line; do
> +		local -i map_start=${tbl_line[0]}
> +		local -i map_end=$((tbl_line[0] + tbl_line[1]))
> +
> +		if ((sector < map_start)) || (((map_end) <= sector)); then
> +			continue
> +		fi
> +
> +		offset=${tbl_line[4]}
> +		if ! cont_dev=$(_get_dev_path_by_id "${tbl_line[3]}"); then
> +			echo -n "Cannot access to container device: "
> +			echo "${tbl_line[3]}"
> +			return 1
> +		fi
> +
> +		echo "${cont_dev}" "$((offset + sector - map_start))"
> +		return 0
> +
> +	done < <(dmsetup table "$(<"${TEST_DEV_SYSFS}/dm/name")")
> +
> +	echo -n "Cannot find container device of ${TEST_DEV}"
> +	return 1
> +}

Re: [PATCH blktests v2 2/2] zbd/007: Add zone mapping test for logical devices

[Chaitanya Kulkarni]

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 5/30/19 6:59 PM, Shin'ichiro Kawasaki wrote:
> Add the test case to check zones sector mapping of logical devices. This
> test case requires that such a logical device be specified in TEST_DEVS
> in config. The test is skipped for devices that are identified as not
> logically created.
>
> To test that the zone mapping is correct, select a few sequential write
> required zones of the logical device and move the write pointers of
> these zones through the container device of the logical device, using
> the physical sector mapping of the zones. The write pointers position of
> the selected zones is then checked through a zone report of the logical
> device using the logical sector mapping of the zones. The test reports a
> success if the position of the zone write pointers relative to the zone
> start sector must be identical for both the logical and physical
> locations of the zones.
>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  tests/zbd/007     | 110 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/zbd/007.out |   2 +
>  2 files changed, 112 insertions(+)
>  create mode 100755 tests/zbd/007
>  create mode 100644 tests/zbd/007.out
>
> diff --git a/tests/zbd/007 b/tests/zbd/007
> new file mode 100755
> index 0000000..b4dcbd8
> --- /dev/null
> +++ b/tests/zbd/007
> @@ -0,0 +1,110 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2019 Western Digital Corporation or its affiliates.
> +#
> +# Test zones are mapped correctly between a logical device and its container
> +# device. Move write pointers of sequential write required zones on the
> +# container devices, and confirm same write pointer positions of zones on the
> +# logical devices.
> +
> +. tests/zbd/rc
> +
> +DESCRIPTION="zone mapping between logical and container devices"
> +CAN_BE_ZONED=1
> +QUICK=1
> +
> +requires() {
> +	_have_program dmsetup
> +}
> +
> +device_requires() {
> +	_test_dev_is_logical
> +}
> +
> +# Select test target zones. Pick up the first sequential required zones. If
> +# available, add one or two more sequential required zones. One is at the last
> +# end of TEST_DEV. The other is in middle between the first and the last zones.
> +select_zones() {
> +	local -i zone_idx
> +	local -a zones
> +
> +	zone_idx=$(_find_first_sequential_zone) || return $?
> +	zones=( "${zone_idx}" )
> +	if zone_idx=$(_find_last_sequential_zone); then
> +		zones+=( "${zone_idx}" )
> +		if zone_idx=$(_find_sequential_zone_in_middle \
> +				      "${zones[0]}" "${zones[1]}"); then
> +			zones+=( "${zone_idx}" )
> +		fi
> +	fi
> +	echo "${zones[@]}"
> +}
> +
> +test_device() {
> +	local -i bs
> +	local -a test_z # test target zones
> +	local -a test_z_start
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	# Get physical block size to meet zoned block device I/O requirement
> +	_get_sysfs_variable "${TEST_DEV}" || return $?
> +	bs=${SYSFS_VARS[SV_PHYS_BLK_SIZE]}
> +	_put_sysfs_variable
> +
> +	# Get test target zones
> +	_get_blkzone_report "${TEST_DEV}" || return $?
> +	read -r -a test_z < <(select_zones)
> +	for ((i = 0; i < ${#test_z[@]}; i++)); do
> +		test_z_start+=("${ZONE_STARTS[test_z[i]]}")
> +	done
> +	echo "${test_z[*]}" >> "$FULL"
> +	echo "${test_z_start[*]}" >> "$FULL"
> +	_put_blkzone_report
> +	if ((!${#test_z[@]})); then
> +		echo "Test target zones not available on ${TEST_DEV}"
> +		return 1
> +	fi
> +
> +	# Reset and move write pointers of the container device
> +	for ((i=0; i < ${#test_z[@]}; i++)); do
> +		local -a arr
> +
> +		read -r -a arr < <(_get_dev_container_and_sector \
> +					   "${test_z_start[i]}")
> +		container_dev="${arr[0]}"
> +		container_start="${arr[1]}"
> +
> +		echo "${container_dev}" "${container_start}" >> "$FULL"
> +
> +		if ! blkzone reset -o "${container_start}" -c 1 \
> +		     "${container_dev}"; then
> +			echo "Reset zone failed"
> +			return 1
> +		fi
> +
> +		if ! dd if=/dev/zero of="${container_dev}" bs="${bs}" \
> +		     count=$((4096 * (i + 1) / bs)) oflag=direct \
> +		     seek=$((container_start * 512 / bs)) \
> +		     >> "$FULL" 2>&1 ; then
> +			echo "dd failed"
> +		fi
> +
> +		# Wait for partition table re-read event settles
> +		udevadm settle
> +	done
> +
> +	# Check write pointer positions on the logical device
> +	_get_blkzone_report "${TEST_DEV}" || return $?
> +	for ((i=0; i < ${#test_z[@]}; i++)); do
> +		if ((ZONE_WPTRS[test_z[i]] != 8 * (i + 1))); then
> +			echo "Unexpected write pointer position"
> +			echo -n "zone=${i}, wp=${ZONE_WPTRS[i]}, "
> +			echo "dev=${TEST_DEV}"
> +		fi
> +		echo "${ZONE_WPTRS[${test_z[i]}]}" >> "$FULL"
> +	done
> +	_put_blkzone_report
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/zbd/007.out b/tests/zbd/007.out
> new file mode 100644
> index 0000000..28a1395
> --- /dev/null
> +++ b/tests/zbd/007.out
> @@ -0,0 +1,2 @@
> +Running zbd/007
> +Test complete

Re: [PATCH blktests v2 1/2] zbd/rc: Introduce helper functions for zone mapping test

[Omar Sandoval]

On Fri, May 31, 2019 at 10:59:12AM +0900, Shin'ichiro Kawasaki wrote:
> As a preparation for the zone mapping test case, add several helper
> functions. _find_last_sequential_zone() and
> _find_sequential_zone_in_middle() help to select test target zones.
> _test_dev_is_logical() checks TEST_DEV is the valid test target.
> _test_dev_has_dm_map() helps to check that the dm target is linear or
> flakey. _get_dev_container_and_sector() helps to get the container device
> and sector mappings.

This has a few shellcheck warnings:

tests/zbd/rc:221:4: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]
tests/zbd/rc:223:4: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]
tests/zbd/rc:225:3: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]

And it's missing your Signed-off-by.

Thanks!

Re: [PATCH blktests v2 2/2] zbd/007: Add zone mapping test for logical devices

[Omar Sandoval]

On Fri, May 31, 2019 at 10:59:13AM +0900, Shin'ichiro Kawasaki wrote:
> Add the test case to check zones sector mapping of logical devices. This
> test case requires that such a logical device be specified in TEST_DEVS
> in config. The test is skipped for devices that are identified as not
> logically created.
> 
> To test that the zone mapping is correct, select a few sequential write
> required zones of the logical device and move the write pointers of
> these zones through the container device of the logical device, using
> the physical sector mapping of the zones. The write pointers position of
> the selected zones is then checked through a zone report of the logical
> device using the logical sector mapping of the zones. The test reports a
> success if the position of the zone write pointers relative to the zone
> start sector must be identical for both the logical and physical
> locations of the zones.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  tests/zbd/007     | 110 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/zbd/007.out |   2 +
>  2 files changed, 112 insertions(+)
>  create mode 100755 tests/zbd/007
>  create mode 100644 tests/zbd/007.out
> 
> diff --git a/tests/zbd/007 b/tests/zbd/007
> new file mode 100755
> index 0000000..b4dcbd8
> --- /dev/null
> +++ b/tests/zbd/007
> @@ -0,0 +1,110 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2019 Western Digital Corporation or its affiliates.
> +#
> +# Test zones are mapped correctly between a logical device and its container
> +# device. Move write pointers of sequential write required zones on the
> +# container devices, and confirm same write pointer positions of zones on the
> +# logical devices.
> +
> +. tests/zbd/rc
> +
> +DESCRIPTION="zone mapping between logical and container devices"
> +CAN_BE_ZONED=1
> +QUICK=1
> +
> +requires() {
> +	_have_program dmsetup
> +}

Looks like this test doesn't have a fallback device, so I can't run it
here. Is that intentional, or was it just overlooked?

> +device_requires() {
> +	_test_dev_is_logical
> +}

Re: [PATCH blktests v2 1/2] zbd/rc: Introduce helper functions for zone mapping test

[Shinichiro Kawasaki]

On 6/5/19 7:12 AM, Chaitanya Kulkarni wrote:
> Overall it looks good to me, couple of nits, can be ignored for now.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 
> On 5/30/19 6:59 PM, Shin'ichiro Kawasaki wrote:
>> As a preparation for the zone mapping test case, add several helper
>> functions. _find_last_sequential_zone() and
>> _find_sequential_zone_in_middle() help to select test target zones.
>> _test_dev_is_logical() checks TEST_DEV is the valid test target.
>> _test_dev_has_dm_map() helps to check that the dm target is linear or
>> flakey. _get_dev_container_and_sector() helps to get the container device
>> and sector mappings.
>> ---
>>   tests/zbd/rc | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 133 insertions(+)
>>
>> diff --git a/tests/zbd/rc b/tests/zbd/rc
>> index 5f04c84..792b83d 100644
>> --- a/tests/zbd/rc
>> +++ b/tests/zbd/rc
>> @@ -193,6 +193,42 @@ _find_first_sequential_zone() {
>>   	return 1
>>   }
>>   
>> +_find_last_sequential_zone() {
>> +	for ((idx = REPORTED_COUNT - 1; idx > 0; idx--)); do
>> +		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
>> +			echo "${idx}"
>> +			return 0
>> +		fi
>> +	done
>> +
>> +	echo "-1"
>> +	return 1
>> +}
>> +
>> +# Try to find a sequential required zone between given two zone indices
>> +_find_sequential_zone_in_middle() {
>> +	local -i s=${1}
>> +	local -i e=${2}
> nit:- do we need to validate the s and e before we use ?

Yes. Will add the checks in the v3 patch. Thanks.

>> +	local -i idx=$(((s + e) / 2))
>> +	local -i i=1
>> +
> nit:- Is there a reason for while ? we can also get away with for loop
> right ?

My understanding is that ZBC and ZAC allow to place conventional zones between 
the sequential required zones 's' and 'e'. The loop is required to check zone 
types to find out a sequential required zone, not a conventional zone in case 
such devices get available in the future.

>> +	while ((idx != s && idx != e)); do
>> +		if ((ZONE_TYPES[idx] == ZONE_TYPE_SEQ_WRITE_REQUIRED)); then
>> +			echo "${idx}"
>> +			return 0
>> +		fi
>> +		if ((i%2 == 0)); then
>> +			$((idx += i))
>> +		else
>> +			$((idx -= i))
>> +		fi
>> +		$((i++))
>> +	done
>> +
>> +	echo "-1"
>> +	return 1
>> +}
>> +
>>   # Search zones and find two contiguous sequential required zones.
>>   # Return index of the first zone of the found two zones.
>>   # Call _get_blkzone_report() beforehand.
>> @@ -210,3 +246,100 @@ _find_two_contiguous_seq_zones() {
>>   	echo "Contiguous sequential write required zones not found"
>>   	return 1
>>   }
>> +
>> +_test_dev_is_dm() {
>> +	if [[ ! -r "${TEST_DEV_SYSFS}/dm/name" ]]; then
>> +		SKIP_REASON="$TEST_DEV is not device-mapper"
>> +		return 1
>> +	fi
>> +	return 0
>> +}
>> +
>> +_test_dev_is_logical() {
>> +	if ! _test_dev_is_partition && ! _test_dev_is_dm; then
>> +		SKIP_REASON="$TEST_DEV is not a logical device"
>> +		return 1
>> +	fi
>> +	return 0
>> +}
>> +
>> +_test_dev_has_dm_map() {
>> +	local target_type=${1}
>> +	local dm_name
>> +
>> +	dm_name=$(<"${TEST_DEV_SYSFS}/dm/name")
>> +	if ! dmsetup status "${dm_name}" | grep -qe "${target_type}"; then
>> +		SKIP_REASON="$TEST_DEV does not have ${target_type} map"
>> +		return 1
>> +	fi
>> +	if dmsetup status "${dm_name}" | grep -v "${target_type}"; then
>> +		SKIP_REASON="$TEST_DEV has map other than ${target_type}"
>> +		return 1
>> +	fi
>> +	return 0
>> +}
>> +
>> +# Get device file path from the device ID "major:minor".
>> +_get_dev_path_by_id() {
>> +	for d in /sys/block/*; do
>> +		if [[ ! -r "${d}/dev" ]]; then
>> +			continue
>> +		fi
>> +		if [[ "${1}" == "$(<"${d}/dev")" ]]; then
>> +			echo "/dev/${d##*/}"
>> +			return 0
>> +		fi
>> +	done
>> +	return 1
>> +}
>> +
>> +# Given sector of TEST_DEV, return the device which contain the sector and
>> +# corresponding sector of the container device.
>> +_get_dev_container_and_sector() {
>> +	local -i sector=${1}
>> +	local cont_dev
>> +	local -i offset
>> +	local -a tbl_line
>> +
>> +	if _test_dev_is_partition; then
>> +		offset=$(<"${TEST_DEV_PART_SYSFS}/start")
>> +		cont_dev=$(_get_dev_path_by_id "$(<"${TEST_DEV_SYSFS}/dev")")
>> +		echo "${cont_dev}" "$((offset + sector))"
>> +		return 0
>> +	fi
>> +
>> +	if ! _test_dev_is_dm; then
>> +		echo "${TEST_DEV} is not a logical device"
>> +		return 1
>> +	fi
>> +	if ! _test_dev_has_dm_map linear &&
>> +			! _test_dev_has_dm_map flakey; then
>> +		echo -n "dm mapping test other than linear/flakey is"
>> +		echo "not implemented"
>> +		return 1
>> +	fi
>> +
>> +	# Parse dm table lines for dm-linear or dm-flakey target
>> +	while read -r -a tbl_line; do
>> +		local -i map_start=${tbl_line[0]}
>> +		local -i map_end=$((tbl_line[0] + tbl_line[1]))
>> +
>> +		if ((sector < map_start)) || (((map_end) <= sector)); then
>> +			continue
>> +		fi
>> +
>> +		offset=${tbl_line[4]}
>> +		if ! cont_dev=$(_get_dev_path_by_id "${tbl_line[3]}"); then
>> +			echo -n "Cannot access to container device: "
>> +			echo "${tbl_line[3]}"
>> +			return 1
>> +		fi
>> +
>> +		echo "${cont_dev}" "$((offset + sector - map_start))"
>> +		return 0
>> +
>> +	done < <(dmsetup table "$(<"${TEST_DEV_SYSFS}/dm/name")")
>> +
>> +	echo -n "Cannot find container device of ${TEST_DEV}"
>> +	return 1
>> +}
> 
> 
> 


-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 1/2] zbd/rc: Introduce helper functions for zone mapping test

[Shinichiro Kawasaki]

On 6/6/19 6:52 AM, Omar Sandoval wrote:
> On Fri, May 31, 2019 at 10:59:12AM +0900, Shin'ichiro Kawasaki wrote:
>> As a preparation for the zone mapping test case, add several helper
>> functions. _find_last_sequential_zone() and
>> _find_sequential_zone_in_middle() help to select test target zones.
>> _test_dev_is_logical() checks TEST_DEV is the valid test target.
>> _test_dev_has_dm_map() helps to check that the dm target is linear or
>> flakey. _get_dev_container_and_sector() helps to get the container device
>> and sector mappings.
> 
> This has a few shellcheck warnings:
> 
> tests/zbd/rc:221:4: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]
> tests/zbd/rc:223:4: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]
> tests/zbd/rc:225:3: error: Remove '$' or use '_=$((expr))' to avoid executing output. [SC2084]
> 
> And it's missing your Signed-off-by.
> 
> Thanks!

Thank you for the review and sorry about the mistakes. Will fix with v3 patch.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 2/2] zbd/007: Add zone mapping test for logical devices

[Shinichiro Kawasaki]

On 6/6/19 6:53 AM, Omar Sandoval wrote:
> On Fri, May 31, 2019 at 10:59:13AM +0900, Shin'ichiro Kawasaki wrote:
>> Add the test case to check zones sector mapping of logical devices. This
>> test case requires that such a logical device be specified in TEST_DEVS
>> in config. The test is skipped for devices that are identified as not
>> logically created.
>>
>> To test that the zone mapping is correct, select a few sequential write
>> required zones of the logical device and move the write pointers of
>> these zones through the container device of the logical device, using
>> the physical sector mapping of the zones. The write pointers position of
>> the selected zones is then checked through a zone report of the logical
>> device using the logical sector mapping of the zones. The test reports a
>> success if the position of the zone write pointers relative to the zone
>> start sector must be identical for both the logical and physical
>> locations of the zones.
>>
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>> ---
>>   tests/zbd/007     | 110 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/zbd/007.out |   2 +
>>   2 files changed, 112 insertions(+)
>>   create mode 100755 tests/zbd/007
>>   create mode 100644 tests/zbd/007.out
>>
>> diff --git a/tests/zbd/007 b/tests/zbd/007
>> new file mode 100755
>> index 0000000..b4dcbd8
>> --- /dev/null
>> +++ b/tests/zbd/007
>> @@ -0,0 +1,110 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2019 Western Digital Corporation or its affiliates.
>> +#
>> +# Test zones are mapped correctly between a logical device and its container
>> +# device. Move write pointers of sequential write required zones on the
>> +# container devices, and confirm same write pointer positions of zones on the
>> +# logical devices.
>> +
>> +. tests/zbd/rc
>> +
>> +DESCRIPTION="zone mapping between logical and container devices"
>> +CAN_BE_ZONED=1
>> +QUICK=1
>> +
>> +requires() {
>> +	_have_program dmsetup
>> +}
> 
> Looks like this test doesn't have a fallback device, so I can't run it
> here. Is that intentional, or was it just overlooked?
> 
>> +device_requires() {
>> +	_test_dev_is_logical
>> +}

Zone logical remapping is necessary only and only if the target device is not
the raw entire block device, that is, if the target is a partition block device,
a DM device mapping only a portion of the backend disk, or a combination of
both. When the target device is an entire raw block device, zone remapping
(shifting of zone start and zone write pointer position) is not necessary at
all. If we add a fallback device, the test will still run, but will end up being
a test for a straight report zones, which is already covered by test zbd/002. So 
we thought it not necessary to add here.

This test is rather intended at catching complex zone remapping problems like we
had in the past already: see commit 9864cd5dc54c "dm: fix report zone remapping 
to account for partition offset".

-- 
Best Regards,
Shin'ichiro Kawasaki