[PATCH blktests 0/3] Fix partition device sysfs access

[PATCH blktests 0/3] Fix partition device sysfs access

[Shin'ichiro Kawasaki]

This patch series fixes test failures found in the zbd test group when run on
partition devices. The failure cause is a block device sysfs attributes access
problem if the target device is a partition.

This series resends patches 3, 4 and 5 posted as a part of another series.

  https://www.spinics.net/lists/linux-block/msg37431.html

The patches reflect the review comments by Omar made on the list.

Shin'ichiro Kawasaki (3):
  check: Add TEST_DEV_PART_SYSFS variable
  common: Add _test_dev_is_partition() helper function
  zbd: Change sysfs path for partition devices

 check        | 46 +++++++++++++++++++++++++---------------------
 common/rc    |  8 ++++++++
 new          | 16 ++++++++++++++--
 tests/zbd/rc |  8 ++++++--
 4 files changed, 53 insertions(+), 25 deletions(-)

-- 
2.20.1

[PATCH blktests 1/3] check: Add TEST_DEV_PART_SYSFS variable

[Shin'ichiro Kawasaki]

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.

Rename _find_sysfs_dir() function to _find_sysfs_dirs() and make it find
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 cuts the last device name in the canonical sysfs
path to obtain the holder device sysfs path.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check | 46 +++++++++++++++++++++++++---------------------
 new   | 16 ++++++++++++++--
 2 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/check b/check
index 819d8f3..6d44bfb 100755
--- a/check
+++ b/check
@@ -443,13 +443,13 @@ _run_test() {
 				_warning "$TEST_NAME: fallback_device call failure"
 				return 0
 			fi
-			if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
+
+			if ! _find_sysfs_dirs "$test_dev"; then
 				_warning "$TEST_NAME: could not find sysfs directory for ${test_dev}"
 				cleanup_fallback_device
 				return 0
 			fi
 			TEST_DEVS=( "${test_dev}" )
-			TEST_DEV_SYSFS_DIRS["$test_dev"]="$sysfs_dir"
 			FALLBACK_DEVICE=1
 		fi
 
@@ -465,6 +465,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")"
@@ -484,6 +485,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
 
@@ -508,6 +510,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"]
@@ -528,30 +532,32 @@ _run_group() {
 	return $ret
 }
 
-_find_sysfs_dir() {
+_find_sysfs_dirs() {
 	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 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="$(realpath "/sys/dev/block/${major}:${minor}")"; then
+		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
+		TEST_DEV_SYSFS_DIRS["$test_dev"]="${sysfs_path%/*}"
+		TEST_DEV_PART_SYSFS_DIRS["$test_dev"]="${sysfs_path}"
+	else
+		TEST_DEV_SYSFS_DIRS["$test_dev"]="${sysfs_path}"
+		TEST_DEV_PART_SYSFS_DIRS["$test_dev"]=""
+	fi
 }
 
 declare -A TEST_DEV_SYSFS_DIRS
+declare -A TEST_DEV_PART_SYSFS_DIRS
 _check() {
 	# shellcheck disable=SC2034
 	SRCDIR="$(realpath src)"
@@ -564,11 +570,9 @@ _check() {
 			_error "${test_dev} is not a block device"
 		fi
 
-		local sysfs_dir
-		if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
+		if ! _find_sysfs_dirs "$test_dev"; then
 			_error "could not find sysfs directory for ${test_dev}"
 		fi
-		TEST_DEV_SYSFS_DIRS["$test_dev"]="$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

[PATCH blktests 2/3] common: Add _test_dev_is_partition() helper function

[Shin'ichiro Kawasaki]

To control test conditions unique for partition devices, introduce the
_test_dev_is_partition() helper function. Refer TEST_DEV_PART_SYSFS
variable to tell if the TEST_DEV is a partition device or not.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
---
 common/rc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/rc b/common/rc
index 153a323..0c354a2 100644
--- a/common/rc
+++ b/common/rc
@@ -208,6 +208,14 @@ _test_dev_in_hotplug_slot() {
 	return 0
 }
 
+_test_dev_is_partition() {
+	if [[ -z ${TEST_DEV_PART_SYSFS} ]]; then
+		SKIP_REASON="${TEST_DEV} is not a partition device"
+		return 1
+	fi
+	return 0
+}
+
 # Older versions of xfs_io use pwrite64 and such, so the error messages won't
 # match current versions of xfs_io. See c52086226bc6 ("filter: xfs_io output
 # has dropped "64" from error messages") in xfstests.
-- 
2.20.1

[PATCH blktests 3/3] zbd: Change sysfs path for partition devices

[Shin'ichiro Kawasaki]

zbd/001 and zbd/002 test cases fail for partition devices because of
sysfs path difference between partition devices and their holder
devices. The size parameter in sysfs path is different between the
partition devices and their holder devices. The holder devices have
nr_zones parameter in sysfs but the partition devices do not.

Utilize _test_dev_is_partition() helper function and TEST_DEV_PART_SYSFS
variable to refer correct sysfs size parameter for the partition devices.
Do not refer sysfs nr_zones parameter for the partition devices. Instead,
calculate the expected nr_zones from device capacity and zone size.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
---
 tests/zbd/rc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/zbd/rc b/tests/zbd/rc
index c32bf31..88538d0 100644
--- a/tests/zbd/rc
+++ b/tests/zbd/rc
@@ -75,7 +75,11 @@ export SV_NR_ZONES=4
 _get_sysfs_variable() {
 	unset SYSFS_VARS
 	local _dir=${TEST_DEV_SYSFS}
-	SYSFS_VARS[$SV_CAPACITY]=$(<"${_dir}"/size)
+	if _test_dev_is_partition; then
+		SYSFS_VARS[$SV_CAPACITY]=$(<"${TEST_DEV_PART_SYSFS}"/size)
+	else
+		SYSFS_VARS[$SV_CAPACITY]=$(<"${_dir}"/size)
+	fi
 	SYSFS_VARS[$SV_CHUNK_SECTORS]=$(<"${_dir}"/queue/chunk_sectors)
 	SYSFS_VARS[$SV_PHYS_BLK_SIZE]=$(<"${_dir}"/queue/physical_block_size)
 	SYSFS_VARS[$SV_PHYS_BLK_SECTORS]=$((SYSFS_VARS[SV_PHYS_BLK_SIZE] / 512))
@@ -83,7 +87,7 @@ _get_sysfs_variable() {
 	# If the nr_zones sysfs attribute exists, get its value. Otherwise,
 	# calculate its value based on the total capacity and zone size, taking
 	# into account that the last zone can be smaller than other zones.
-	if [[ -e ${TEST_DEV_SYSFS}/queue/nr_zones ]]; then
+	if [[ -e "${_dir}"/queue/nr_zones ]] && ! _test_dev_is_partition; then
 		SYSFS_VARS[$SV_NR_ZONES]=$(<"${_dir}"/queue/nr_zones)
 	else
 		SYSFS_VARS[$SV_NR_ZONES]=$(( (SYSFS_VARS[SV_CAPACITY] - 1) \
-- 
2.20.1

Re: [PATCH blktests 0/3] Fix partition device sysfs access

[Omar Sandoval]

On Tue, Mar 05, 2019 at 02:24:44PM +0900, Shin'ichiro Kawasaki wrote:
> This patch series fixes test failures found in the zbd test group when run on
> partition devices. The failure cause is a block device sysfs attributes access
> problem if the target device is a partition.
> 
> This series resends patches 3, 4 and 5 posted as a part of another series.
> 
>   https://www.spinics.net/lists/linux-block/msg37431.html
> 
> The patches reflect the review comments by Omar made on the list.
> 
> Shin'ichiro Kawasaki (3):
>   check: Add TEST_DEV_PART_SYSFS variable
>   common: Add _test_dev_is_partition() helper function
>   zbd: Change sysfs path for partition devices
> 
>  check        | 46 +++++++++++++++++++++++++---------------------
>  common/rc    |  8 ++++++++
>  new          | 16 ++++++++++++++--
>  tests/zbd/rc |  8 ++++++--
>  4 files changed, 53 insertions(+), 25 deletions(-)

Applied, thanks.