Re: [PATCH 1/3] blktests: add cgroup2 infrastructure

From: Bart Van Assche <bvanassche@acm.org>
To: Omar Sandoval <osandov@osandov.com>
Cc: Josef Bacik <josef@toxicpanda.com>,
	linux-block@vger.kernel.org, kernel-team@fb.com, osandov@fb.com
Subject: Re: [PATCH 1/3] blktests: add cgroup2 infrastructure
Date: Wed, 16 Jan 2019 18:31:51 -0800	[thread overview]
Message-ID: <781ed848-e7a1-47c7-12d1-4260f1e8924b@acm.org> (raw)
In-Reply-To: <20190117014039.GA25494@vader>

On 1/16/19 5:40 PM, Omar Sandoval wrote:
> On Tue, Jan 15, 2019 at 08:40:41AM -0800, Bart Van Assche wrote:
>> On Tue, 2019-01-01 at 19:13 -0800, Bart Van Assche wrote:
>>> On 12/4/18 9:47 AM, Josef Bacik wrote:
>>>> In order to test io.latency and other cgroup related things we need some
>>>> supporting helpers to setup and tear down cgroup2.  This adds support
>>>> for checking that we can even configure cgroup2 things, set them up if
>>>> need be, and then add the cleanup stuff to the main cleanup function so
>>>> everything is always in a clean state.
>>>
>>> Is this the patch that went in as commit ae7daae7e35a ("blktests: add
>>> cgroup2 infrastructure")? I think that commit introduced a regression.
>>> With that patch applied the SRP tests fail as follows:
>>>
>>> # ./check -q srp/001
>>> srp/001 (Create and remove LUNs)
>>>       runtime  4.067s  ...
>>> common/cgroup: line 25: CGROUP2_DIR: unbound variable
>>>
>>> Is this a known issue?
>>
>> Hi Josef,
>>
>> Had you noticed this e-mail?
>>
>> Thanks,
>>
>> Bart.
> 
> Hey, Bart, I just pushed a fix for this:
> 
> commit 8a274578e2895b9f0b66c09f3a8f63b5ff1293b2
> Author: Omar Sandoval <osandov@fb.com>
> Date:   Wed Jan 16 17:34:19 2019 -0800
> 
>      cgroup: test if CGROUP2_DIR is set with -v instead of -n
>      
>      common/multipath-over-rdma does set -u, so -n "$CGROUP2_DIR" fails with
>      an unbound variable error. Instead, use -v to test if the variable was
>      set.
>      
>      Signed-off-by: Omar Sandoval <osandov@fb.com>
> 
> diff --git a/common/cgroup b/common/cgroup
> index 48e546f..554ebf7 100644
> --- a/common/cgroup
> +++ b/common/cgroup
> @@ -22,7 +22,7 @@ _init_cgroup2()
>   
>   _exit_cgroup2()
>   {
> -	if [[ -n $CGROUP2_DIR ]]; then
> +	if [[ -v CGROUP2_DIR ]]; then
>   		find "$CGROUP2_DIR" -type d -delete
>   		unset CGROUP2_DIR
>   	fi

That change looks good to me. Thanks!

Bart.