[PATCH blktests] block/008: avoid _offline_cpu() call in sub-shell

[PATCH blktests] block/008: avoid _offline_cpu() call in sub-shell

[Shin'ichiro Kawasaki]

The helper function _offline_cpu() sets a value to RESTORE_CPUS_ONLINE.
However, the commit bd6b882b2650 ("block/008: check CPU offline failure
due to many IRQs") put _offline_cpu() call in sub-shell, then the set
value to RESTORE_CPUS_ONLINE no longer affects function caller's
environment. This resulted in offlined CPUs not restored by _cleanup()
when the test case block/008 calls only _offline_cpu() and does not call
_online_cpu().

To fix the issue, avoid _offline_cpu() call in sub-shell. Use file
redirect to get output of _offline_cpu() instead of sub-shell execution.

Fixes: bd6b882b2650 ("block/008: check CPU offline failure due to many IRQs")
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Tested-by: Yi Zhang <yi.zhang@redhat.com>
Link: https://lore.kernel.org/linux-block/20220703180956.2922025-1-yi.zhang@redhat.com/
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/block/008 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/block/008 b/tests/block/008
index 75aae65..cd09352 100755
--- a/tests/block/008
+++ b/tests/block/008
@@ -34,6 +34,7 @@ test_device() {
 	local offline_cpus=()
 	local offlining=1
 	local max_offline=${#HOTPLUGGABLE_CPUS[@]}
+	local o=$TMPDIR/offline_cpu_out
 	if [[ ${#HOTPLUGGABLE_CPUS[@]} -eq ${#ALL_CPUS[@]} ]]; then
 		(( max_offline-- ))
 	fi
@@ -60,18 +61,18 @@ test_device() {
 
 		if (( offlining )); then
 			idx=$((RANDOM % ${#online_cpus[@]}))
-			if err=$(_offline_cpu "${online_cpus[$idx]}" 2>&1); then
+			if _offline_cpu "${online_cpus[$idx]}" > "$o" 2>&1; then
 				offline_cpus+=("${online_cpus[$idx]}")
 				unset "online_cpus[$idx]"
 				online_cpus=("${online_cpus[@]}")
-			elif [[ $err =~ "No space left on device" ]]; then
+			elif [[ $(<"$o") =~ "No space left on device" ]]; then
 				# ENOSPC means CPU offline failure due to IRQ
 				# vector shortage. Keep current number of
 				# offline CPUs as maximum CPUs to offline.
 				max_offline=${#offline_cpus[@]}
 				offlining=0
 			else
-				echo "Failed to offline CPU: $err"
+				echo "Failed to offline CPU: $(<"$o")"
 				break
 			fi
 		fi
-- 
2.36.1

Re: [PATCH blktests] block/008: avoid _offline_cpu() call in sub-shell

[Bart Van Assche]

On 7/4/22 04:16, Shin'ichiro Kawasaki wrote:
> The helper function _offline_cpu() sets a value to RESTORE_CPUS_ONLINE.
> However, the commit bd6b882b2650 ("block/008: check CPU offline failure
> due to many IRQs") put _offline_cpu() call in sub-shell, then the set
> value to RESTORE_CPUS_ONLINE no longer affects function caller's
> environment. This resulted in offlined CPUs not restored by _cleanup()
> when the test case block/008 calls only _offline_cpu() and does not call
> _online_cpu().
> 
> To fix the issue, avoid _offline_cpu() call in sub-shell. Use file
> redirect to get output of _offline_cpu() instead of sub-shell execution.
> 
> Fixes: bd6b882b2650 ("block/008: check CPU offline failure due to many IRQs")
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Tested-by: Yi Zhang <yi.zhang@redhat.com>
> Link: https://lore.kernel.org/linux-block/20220703180956.2922025-1-yi.zhang@redhat.com/
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>   tests/block/008 | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/block/008 b/tests/block/008
> index 75aae65..cd09352 100755
> --- a/tests/block/008
> +++ b/tests/block/008
> @@ -34,6 +34,7 @@ test_device() {
>   	local offline_cpus=()
>   	local offlining=1
>   	local max_offline=${#HOTPLUGGABLE_CPUS[@]}
> +	local o=$TMPDIR/offline_cpu_out
>   	if [[ ${#HOTPLUGGABLE_CPUS[@]} -eq ${#ALL_CPUS[@]} ]]; then
>   		(( max_offline-- ))
>   	fi
> @@ -60,18 +61,18 @@ test_device() {
>   
>   		if (( offlining )); then
>   			idx=$((RANDOM % ${#online_cpus[@]}))
> -			if err=$(_offline_cpu "${online_cpus[$idx]}" 2>&1); then
> +			if _offline_cpu "${online_cpus[$idx]}" > "$o" 2>&1; then
>   				offline_cpus+=("${online_cpus[$idx]}")
>   				unset "online_cpus[$idx]"
>   				online_cpus=("${online_cpus[@]}")
> -			elif [[ $err =~ "No space left on device" ]]; then
> +			elif [[ $(<"$o") =~ "No space left on device" ]]; then
>   				# ENOSPC means CPU offline failure due to IRQ
>   				# vector shortage. Keep current number of
>   				# offline CPUs as maximum CPUs to offline.
>   				max_offline=${#offline_cpus[@]}
>   				offlining=0
>   			else
> -				echo "Failed to offline CPU: $err"
> +				echo "Failed to offline CPU: $(<"$o")"
>   				break
>   			fi
>   		fi

Has it been considered to move RESTORE_CPUS_ONLINE=1 from _online_cpu() 
/ _offline_cpu() into the callers of these functions instead of making 
the above change?

Thanks,

Bart.

Re: [PATCH blktests] block/008: avoid _offline_cpu() call in sub-shell

[Shinichiro Kawasaki]

On Jul 06, 2022 / 06:55, Bart Van Assche wrote:
> On 7/4/22 04:16, Shin'ichiro Kawasaki wrote:
> > The helper function _offline_cpu() sets a value to RESTORE_CPUS_ONLINE.
> > However, the commit bd6b882b2650 ("block/008: check CPU offline failure
> > due to many IRQs") put _offline_cpu() call in sub-shell, then the set
> > value to RESTORE_CPUS_ONLINE no longer affects function caller's
> > environment. This resulted in offlined CPUs not restored by _cleanup()
> > when the test case block/008 calls only _offline_cpu() and does not call
> > _online_cpu().
> > 
> > To fix the issue, avoid _offline_cpu() call in sub-shell. Use file
> > redirect to get output of _offline_cpu() instead of sub-shell execution.
> > 
> > Fixes: bd6b882b2650 ("block/008: check CPU offline failure due to many IRQs")
> > Reported-by: Yi Zhang <yi.zhang@redhat.com>
> > Tested-by: Yi Zhang <yi.zhang@redhat.com>
> > Link: https://lore.kernel.org/linux-block/20220703180956.2922025-1-yi.zhang@redhat.com/
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > ---
> >   tests/block/008 | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/block/008 b/tests/block/008
> > index 75aae65..cd09352 100755
> > --- a/tests/block/008
> > +++ b/tests/block/008
> > @@ -34,6 +34,7 @@ test_device() {
> >   	local offline_cpus=()
> >   	local offlining=1
> >   	local max_offline=${#HOTPLUGGABLE_CPUS[@]}
> > +	local o=$TMPDIR/offline_cpu_out
> >   	if [[ ${#HOTPLUGGABLE_CPUS[@]} -eq ${#ALL_CPUS[@]} ]]; then
> >   		(( max_offline-- ))
> >   	fi
> > @@ -60,18 +61,18 @@ test_device() {
> >   		if (( offlining )); then
> >   			idx=$((RANDOM % ${#online_cpus[@]}))
> > -			if err=$(_offline_cpu "${online_cpus[$idx]}" 2>&1); then
> > +			if _offline_cpu "${online_cpus[$idx]}" > "$o" 2>&1; then
> >   				offline_cpus+=("${online_cpus[$idx]}")
> >   				unset "online_cpus[$idx]"
> >   				online_cpus=("${online_cpus[@]}")
> > -			elif [[ $err =~ "No space left on device" ]]; then
> > +			elif [[ $(<"$o") =~ "No space left on device" ]]; then
> >   				# ENOSPC means CPU offline failure due to IRQ
> >   				# vector shortage. Keep current number of
> >   				# offline CPUs as maximum CPUs to offline.
> >   				max_offline=${#offline_cpus[@]}
> >   				offlining=0
> >   			else
> > -				echo "Failed to offline CPU: $err"
> > +				echo "Failed to offline CPU: $(<"$o")"
> >   				break
> >   			fi
> >   		fi
> 
> Has it been considered to move RESTORE_CPUS_ONLINE=1 from _online_cpu() /
> _offline_cpu() into the callers of these functions instead of making the
> above change?

Yes, I thought about it. Looking in the codes in "common/cpuhotplug" and
_cleanup in "check", I guess it was intended to hide RESTORE_CPUS_ONLINE from
test cases. So, I chose the fix above, but I agree the file redirection does not
look the best. And it still leaves the limitation that the _offline_cpu() can
not be called in sub-shell.

As another fix candidate, how about the fix below? It moves the
RESTORE_CPUS_ONLINE=1 from _offline_cpu() to _have_cpu_hotplug(), which is more
unlikely to be called in sub-shell. It still hides RESTORE_CPUS_ONLINE and
should fix the issue.

diff --git a/common/cpuhotplug b/common/cpuhotplug
index d3aabfa..f57a5aa 100644
--- a/common/cpuhotplug
+++ b/common/cpuhotplug
@@ -27,17 +27,21 @@ _have_cpu_hotplug() {
 		SKIP_REASON="CPU hotplugging is not supported"
 		return 1
 	fi
+
+	# shellcheck disable=SC2034
+	RESTORE_CPUS_ONLINE=1
 	return 0
 }
 
 _online_cpu() {
-	# shellcheck disable=SC2034
-	RESTORE_CPUS_ONLINE=1
 	echo 1 > "/sys/devices/system/cpu/cpu$1/online"
 }
 
 _offline_cpu() {
-	# shellcheck disable=SC2034
-	RESTORE_CPUS_ONLINE=1
+	if [[ -z ${RESTORE_CPUS_ONLINE} ]]; then
+		echo "offlining cpu but RESTORE_CPUS_ONLINE is not set"
+		return 1
+	fi
+
 	echo 0 > "/sys/devices/system/cpu/cpu$1/online"
 }


-- 
Shin'ichiro Kawasaki