Hi Chaitanya, thank you for the review comments.
On 5/29/19 1:54 AM, Chaitanya Kulkarni wrote:
> Hi Shinichiro,
>
> Thanks for the test, please see my in-line comments.
>
>
> On 05/12/2019 07:03 PM, Shin'ichiro Kawasaki wrote:
>> Check that zones sector mapping is correct 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). 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.
>>
>> To implement this test case, introduce several helper functions.
>> _find_last_sequential_zone() and _find_sequential_zone_in_middle()
>> help target zones selection. _test_dev_is_logical() checks the target
>> device type. If false is returned, the test case is skipped.
>> _get_dev_container_and_sector() helps to get the container device and
>> sector mappings. At this moment, these helper functions support
>> partition devices and dm-linear/flakey devices as the logical devices.
>> _test_dev_has_dm_map() helps to check that the dm target is linear or
>> flakey.
>>
>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
>> ---
>> tests/zbd/007 | 95 +++++++++++++++++++++++++++++++++
>> tests/zbd/007.out | 2 +
>> tests/zbd/rc | 132 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 229 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..e4723d7
>> --- /dev/null
>> +++ b/tests/zbd/007
>> @@ -0,0 +1,95 @@
>> +#!/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"
> Can this be more explanatory ?
Yes, will add more words for clarification.
>> +CAN_BE_ZONED=1
> Do we need QUICK=1 ?
Indeed. It takes less than 10 seconds.
>> +
>> +requires() {
>> + _have_program dmsetup
>> +}
>> +
>> +device_requires() {
>> + _test_dev_is_logical
>> +}
>> +
>> +test_device() {
>> + local -i bs
>> + local -i zone_idx
>> + 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
>> +
>> + # 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.
>> + _get_blkzone_report "${TEST_DEV}" || return $?
>> + zone_idx=$(_find_first_sequential_zone) || return $?
>> + test_z=( "${zone_idx}" )
>> + if zone_idx=$(_find_last_sequential_zone); then
>> + test_z+=( "${zone_idx}" )
>> + if zone_idx=$(_find_sequential_zone_in_middle \
>> + "${test_z[0]}" "${test_z[1]}"); then
>> + test_z+=( "${zone_idx}" )
>> + fi
>> + fi
>> +
>> + 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
> I think above code of building an array should be move to the same file
> in a helper function. It is just making entire test look bigger than
> it is.
OK. I will separate out test_z array building code into a function.
>> +
>> + # Reset and move write pointers of the container device
>> + for ((i=0; i < ${#test_z[@]}; i++)); do
>> + local -a arr
> nit:- add a new line after declaration.
>> + 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"
>> +
>> + blkzone reset -o "${container_start}" -c 1 "${container_dev}"
> do we need to check the return value here ?
Yes, will add the check.
>> +
>> + 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
>> diff --git a/tests/zbd/rc b/tests/zbd/rc
>> index 5f04c84..1168c4e 100644
> For rc related code changes can you please send a separate preparation
> patche ? It will be great if we can isolate the actual test from the
> rc changes.
OK. Will separate out zbd/rc part. I will post v2 as a series with two patches.
>> --- 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,99 @@ _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
> nit:- new line after declaration.
Will reflect your nit comments also in the v2 patch.
Thanks!
--
Best Regards,
Shin'ichiro Kawasaki
