Re: [PATCH 01/10] btrfs: add helper to kill background process running _btrfs_stress_balance

[Anand Jain <anand.jain@oracle.com>]

On 3/28/24 01:11, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Killing a background process running _btrfs_stress_balance() is not as
> simple as sending a signal to the process and waiting for it to die.
> Therefore we have the following logic to terminate such process:
> 
>     kill $pid
>     wait $pid
>     # Wait for the balance operation to finish.
>     while ps aux | grep "balance start" | grep -qv grep; do


>         sleep 1
>     done
> 
> Since this is repeated in several test cases, move this logic to a common
> helper and use it in all affected test cases. This will help to avoid
> repeating the same code again several times in upcoming changes.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

A few minor things below.

> ---
>   common/btrfs    | 14 ++++++++++++++
>   tests/btrfs/060 |  8 ++------
>   tests/btrfs/061 | 10 ++++------
>   tests/btrfs/062 | 10 ++++------
>   tests/btrfs/063 | 10 ++++------
>   tests/btrfs/064 | 10 ++++------
>   tests/btrfs/255 |  8 ++------
>   7 files changed, 34 insertions(+), 36 deletions(-)
> 
> diff --git a/common/btrfs b/common/btrfs
> index aa344706..e95cff7f 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -308,6 +308,20 @@ _btrfs_stress_balance()
>   	done
>   }
>   
> +# Kill a background process running _btrfs_stress_balance()
> +_btrfs_kill_stress_balance_pid()
> +{
> +	local balance_pid=$1
> +
> +	# Ignore if process already died.
> +	kill $balance_pid &> /dev/null
> +	wait $balance_pid &> /dev/null
> +	# Wait for the balance operation to finish.
> +	while ps aux | grep "balance start" | grep -qv grep; do


  We can use pgrep instead. I will make the following changes before
  merging if you are okay with it.

-       while ps aux | grep "balance start" | grep -qv grep; do
+       while pgrep -f "btrfs balance start" > /dev/null; do



> +		sleep 1
> +	done
> +}
> +
>   # stress btrfs by creating/mounting/umounting/deleting subvolume in a loop
>   _btrfs_stress_subvolume()
>   {
> diff --git a/tests/btrfs/060 b/tests/btrfs/060
> index a0184891..58167cc6 100755
> --- a/tests/btrfs/060
> +++ b/tests/btrfs/060
> @@ -57,12 +57,8 @@ run_test()
>   	wait $fsstress_pid
>   
>   	touch $stop_file
> -	kill $balance_pid
> -	wait
> -	# wait for the balance operation to finish
> -	while ps aux | grep "balance start" | grep -qv grep; do
> -		sleep 1
> -	done
> +	wait $subvol_pid
> +	_btrfs_kill_stress_balance_pid $balance_pid
>   
>   	echo "Scrub the filesystem" >>$seqres.full
>   	$BTRFS_UTIL_PROG scrub start -B $SCRATCH_MNT >>$seqres.full 2>&1
> diff --git a/tests/btrfs/061 b/tests/btrfs/061
> index c1010413..d0b55e48 100755
> --- a/tests/btrfs/061
> +++ b/tests/btrfs/061
> @@ -51,12 +51,10 @@ run_test()
>   
>   	echo "Wait for fsstress to exit and kill all background workers" >>$seqres.full
>   	wait $fsstress_pid
> -	kill $balance_pid $scrub_pid
> -	wait
> -	# wait for the balance and scrub operations to finish
> -	while ps aux | grep "balance start" | grep -qv grep; do
> -		sleep 1
> -	done
> +	_btrfs_kill_stress_balance_pid $balance_pid
> +	kill $scrub_pid
> +	wait $scrub_pid
> +	# wait for the crub operation to finish

s/crub/scrub/

Thanks, Anand

>   	while ps aux | grep "scrub start" | grep -qv grep; do
>   		sleep 1
>   	done
> diff --git a/tests/btrfs/062 b/tests/btrfs/062
> index 818a0156..a2639d6c 100755
> --- a/tests/btrfs/062
> +++ b/tests/btrfs/062
> @@ -52,12 +52,10 @@ run_test()
>   
>   	echo "Wait for fsstress to exit and kill all background workers" >>$seqres.full
>   	wait $fsstress_pid
> -	kill $balance_pid $defrag_pid
> -	wait
> -	# wait for the balance and defrag operations to finish
> -	while ps aux | grep "balance start" | grep -qv grep; do
> -		sleep 1
> -	done
> +	_btrfs_kill_stress_balance_pid $balance_pid
> +	kill $defrag_pid
> +	wait $defrag_pid
> +	# wait for the defrag operation to finish
>   	while ps aux | grep "btrfs filesystem defrag" | grep -qv grep; do
>   		sleep 1
>   	done
> diff --git a/tests/btrfs/063 b/tests/btrfs/063
> index 2f771baf..baf0c356 100755
> --- a/tests/btrfs/063
> +++ b/tests/btrfs/063
> @@ -51,12 +51,10 @@ run_test()
>   
>   	echo "Wait for fsstress to exit and kill all background workers" >>$seqres.full
>   	wait $fsstress_pid
> -	kill $balance_pid $remount_pid
> -	wait
> -	# wait for the balance and remount loop to finish
> -	while ps aux | grep "balance start" | grep -qv grep; do
> -		sleep 1
> -	done
> +	_btrfs_kill_stress_balance_pid $balance_pid
> +	kill $remount_pid
> +	wait $remount_pid
> +	# wait for the remount loop to finish
>   	while ps aux | grep "mount.*$SCRATCH_MNT" | grep -qv grep; do
>   		sleep 1
>   	done
> diff --git a/tests/btrfs/064 b/tests/btrfs/064
> index e9b46ce6..58b53afe 100755
> --- a/tests/btrfs/064
> +++ b/tests/btrfs/064
> @@ -63,12 +63,10 @@ run_test()
>   
>   	echo "Wait for fsstress to exit and kill all background workers" >>$seqres.full
>   	wait $fsstress_pid
> -	kill $balance_pid $replace_pid
> -	wait
> -	# wait for the balance and replace operations to finish
> -	while ps aux | grep "balance start" | grep -qv grep; do
> -		sleep 1
> -	done
> +	_btrfs_kill_stress_balance_pid $balance_pid
> +	kill $replace_pid
> +	wait $replace_pid
> +	# wait for the replace operation to finish
>   	while ps aux | grep "replace start" | grep -qv grep; do
>   		sleep 1
>   	done
> diff --git a/tests/btrfs/255 b/tests/btrfs/255
> index 7e70944a..aa250467 100755
> --- a/tests/btrfs/255
> +++ b/tests/btrfs/255
> @@ -41,12 +41,8 @@ for ((i = 0; i < 20; i++)); do
>   	$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT
>   	$BTRFS_UTIL_PROG quota disable $SCRATCH_MNT
>   done
> -kill $balance_pid &> /dev/null
> -wait
> -# wait for the balance operation to finish
> -while ps aux | grep "balance start" | grep -qv grep; do
> -	sleep 1
> -done
> +
> +_btrfs_kill_stress_balance_pid $balance_pid
>   
>   echo "Silence is golden"
>   status=0