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

[Bart Van Assche <bvanassche@acm.org>]

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.