From: Omar Sandoval <osandov@osandov.com>
To: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: linux-block@vger.kernel.org, Omar Sandoval <osandov@fb.com>,
Masato Suzuki <masato.suzuki@wdc.com>,
Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Subject: Re: [PATCH blktests 3/5] check: Add TEST_DEV_PART_SYSFS variable
Date: Mon, 4 Mar 2019 14:34:36 -0800 [thread overview]
Message-ID: <20190304223436.GC5450@vader> (raw)
In-Reply-To: <20190220081231.15366-4-shinichiro.kawasaki@wdc.com>
On Wed, Feb 20, 2019 at 05:12:29PM +0900, Shin'ichiro Kawasaki wrote:
> When partition devices are specified in TEST_DEV, TEST_DEV_SYSFS
> variable points to the sysfs paths of holder devices of the partition
> devices (e.g., /sys/block/sda). This sysfs path is different from the
> sysfs path of the partition devices (e.g., /sys/block/sda/sda1). For
> example, size parameters exist in both the holder device sysfs and
> the partition device sysfs with different values.
>
> To allow test cases to access sysfs path of the partition devices,
> add TEST_DEV_PART_SYSFS variable. TEST_DEV_SYSFS is set as is to refer
> the sysfs path of the holder devices. If the TEST_DEV is not a partition
> device, an empty string is set to the TEST_DEV_PART_SYSFS variable.
>
> Change _find_sysfs_dir() function to return the holder device sysfs as
> well as the partition device sysfs. The function obtains the canonical
> sysfs path, and if the device is a partition device, the function cut the
> last device name in the canonical sysfs path to obtain the holder device
> sysfs path.
This makes sense. A couple of small tweaks below.
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
> check | 51 ++++++++++++++++++++++++++++++++++-----------------
> new | 16 ++++++++++++++--
> 2 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/check b/check
> index f41ecba..e45b34f 100755
> --- a/check
> +++ b/check
> @@ -442,13 +442,19 @@ _run_test() {
> _warning "$TEST_NAME: fallback_device call failure"
> return 0
> fi
> - if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
> +
> + local dirs
> + local sysfs_dir
> + local part_sysfs_dir
> + if ! dirs=$(_find_sysfs_dir "$test_dev") ; then
> _warning "$TEST_NAME: could not find sysfs directory for ${test_dev}"
> cleanup_fallback_device
> return 0
> fi
> + read -r sysfs_dir part_sysfs_dir < <(echo "${dirs}")
Let's rename _find_sysfs_dir to find_sysfs_dirs and make it set
TEST_DEV_SYSFS_DIRS["$test_dev"] and
TEST_DEV_PART_SYSFS_DIRS["$test_dev"] itself instead of returning the
string.
> TEST_DEVS=( "${test_dev}" )
> TEST_DEV_SYSFS_DIRS["$test_dev"]="$sysfs_dir"
> + TEST_DEV_PART_SYSFS_DIRS["$test_dev"]="$part_sysfs_dir"
> FALLBACK_DEVICE=1
> fi
>
> @@ -464,6 +470,7 @@ _run_test() {
> local ret=0
> for TEST_DEV in "${TEST_DEVS[@]}"; do
> TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
> + TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}"
> if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
> SKIP_REASON="${TEST_DEV} is a zoned block device"
> _output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
> @@ -483,6 +490,7 @@ _run_test() {
> if (( FALLBACK_DEVICE )); then
> cleanup_fallback_device
> unset TEST_DEV_SYSFS_DIRS["${TEST_DEVS[0]}"]
> + unset TEST_DEV_PART_SYSFS_DIRS["${TEST_DEVS[0]}"]
> TEST_DEVS=()
> fi
>
> @@ -507,6 +515,8 @@ _run_group() {
> for i in "${!TEST_DEVS[@]}"; do
> TEST_DEV="${TEST_DEVS[$i]}"
> TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
> + # shellcheck disable=SC2034
> + TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}"
> if ! group_device_requires; then
> _output_notrun "${group}/*** => $(basename "$TEST_DEV")"
> unset TEST_DEVS["$i"]
> @@ -529,28 +539,31 @@ _run_group() {
>
> _find_sysfs_dir() {
> local test_dev="$1"
> + local sysfs_path
> local major=$((0x$(stat -L -c '%t' "$test_dev")))
> local minor=$((0x$(stat -L -c '%T' "$test_dev")))
> - local dev="$major:$minor"
> + local sysdev_path="/sys/dev/block/${major}:${minor}"
>
> - local block_dir part_dir
> - for block_dir in /sys/block/*; do
> - if [[ $(cat "${block_dir}/dev") = "$dev" ]]; then
> - echo "$block_dir"
> - return
> - fi
> - for part_dir in "$block_dir"/*; do
> - if [[ -r ${part_dir}/dev && $(cat "${part_dir}/dev") = "$dev" ]]; then
> - echo "$block_dir"
> - return
> - fi
> - done
> - done
> + # Get the canonical sysfs path
> + if ! sysfs_path=/sys/dev/block/$(readlink "${sysdev_path}"); then
sysfs_path="$(realpath "/sys/dev/block/${major}:${minor}")" is a bit
shorter, does that still work?
> + return 1
> + fi
>
> - return 1
> + if [[ -r "${sysfs_path}"/partition ]]; then
> + # If the device is a partition device, cut the last device name
> + # of the canonical sysfs path to access to the sysfs of its
> + # holder device.
> + # e.g. .../block/sda/sda1 -> ...block/sda
> + # Return both the holder device sysfs path and the partition
> + # device sysfs path.
> + echo "${sysfs_path%/*}" "${sysfs_path}"
> + else
> + echo "${sysfs_path}" ""
> + fi
> }
>
> declare -A TEST_DEV_SYSFS_DIRS
> +declare -A TEST_DEV_PART_SYSFS_DIRS
> _check() {
> # shellcheck disable=SC2034
> SRCDIR="$(realpath src)"
> @@ -563,11 +576,15 @@ _check() {
> _error "${test_dev} is not a block device"
> fi
>
> + local dirs
> local sysfs_dir
> - if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
> + local part_sysfs_dir
> + if ! dirs=$(_find_sysfs_dir "$test_dev") ; then
> _error "could not find sysfs directory for ${test_dev}"
> fi
> + read -r sysfs_dir part_sysfs_dir < <(echo "${dirs}")
> TEST_DEV_SYSFS_DIRS["$test_dev"]="$sysfs_dir"
> + TEST_DEV_PART_SYSFS_DIRS["$test_dev"]="$part_sysfs_dir"
> done
>
> local test_name group prev_group
> diff --git a/new b/new
> index d7d5f7c..24c066d 100755
> --- a/new
> +++ b/new
> @@ -80,7 +80,10 @@ group_requires() {
> # should return non-zero and set the \$SKIP_REASON variable. \$TEST_DEV is the
> # full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and
> # \$TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g.,
> -# /sys/block/nvme0n1 or /sys/block/sda).
> +# /sys/block/nvme0n1 or /sys/block/sda). If the target device is a partition
> +# device, \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device
> +# (e.g., /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise,
> +# \$TEST_DEV_PART_SYSFS is an empty string.
> #
> # Usually, group_device_requires() just needs to check that the test device is
> # the right type of hardware or supports any necessary features using the
> @@ -165,7 +168,10 @@ DESCRIPTION=""
> # set the \$SKIP_REASON variable. \$TEST_DEV is the full path of the block
> # device (e.g., /dev/nvme0n1 or /dev/sda1), and \$TEST_DEV_SYSFS is the sysfs
> # path of the disk (not the partition, e.g., /sys/block/nvme0n1 or
> -# /sys/block/sda).
> +# /sys/block/sda). If the target device is a partition device,
> +# \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device (e.g.,
> +# /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise,
> +# \$TEST_DEV_PART_SYSFS is an empty string.
> #
> # Usually, device_requires() just needs to check that the test device is the
> # right type of hardware or supports any necessary features using the
> @@ -207,6 +213,12 @@ DESCRIPTION=""
> # - \$TEST_DEV_SYSFS -- the sysfs directory of the device (e.g.,
> # /sys/block/sda). In general, you should use the
> # _test_dev_queue_{get,set} helpers.
> +# If the device is a partition device, the sysfs
> +# directory of its holder device is set.
> +# - \$TEST_DEV_PART_SYSFS -- the sysfs directory of the device if the device
> +# is a partition device (e.g.,
> +# /sys/block/sda/sda1). Empty string is set if
> +# the device is not a partition device.
> test() {
> echo "Running \${TEST_NAME}"
>
> --
> 2.20.1
>
next prev parent reply other threads:[~2019-03-04 22:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-20 8:12 [PATCH blktests 0/5] Fix failures found with zoned block devices Shin'ichiro Kawasaki
2019-02-20 8:12 ` [PATCH blktests 1/5] block/024: Increase I/O time Shin'ichiro Kawasaki
2019-02-20 8:12 ` [PATCH blktests 2/5] zbd/004: Add zone condition "Closed" for sequential write required zones Shin'ichiro Kawasaki
2019-02-20 8:12 ` [PATCH blktests 3/5] check: Add TEST_DEV_PART_SYSFS variable Shin'ichiro Kawasaki
2019-03-04 22:34 ` Omar Sandoval [this message]
2019-03-05 1:47 ` Shinichiro Kawasaki
2019-02-20 8:12 ` [PATCH blktests 4/5] common: Add _test_dev_is_partition() helper function Shin'ichiro Kawasaki
2019-03-04 22:36 ` Omar Sandoval
2019-02-20 8:12 ` [PATCH blktests 5/5] zbd: Change sysfs path for partition devices Shin'ichiro Kawasaki
2019-03-04 22:36 ` Omar Sandoval
2019-02-20 18:22 ` [PATCH blktests 0/5] Fix failures found with zoned block devices Omar Sandoval
2019-02-21 2:50 ` Damien Le Moal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190304223436.GC5450@vader \
--to=osandov@osandov.com \
--cc=Chaitanya.Kulkarni@wdc.com \
--cc=linux-block@vger.kernel.org \
--cc=masato.suzuki@wdc.com \
--cc=osandov@fb.com \
--cc=shinichiro.kawasaki@wdc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.