[PATCHv2 blktests 0/2] throtl: fix IO block-size and race while submitting IO

[PATCHv2 blktests 0/2] throtl: fix IO block-size and race while submitting IO

[Nilay Shroff]

Hi,

The first patch fixes the IO block-size used for submitting IO depending
on the throttle device max-sectors settings.

The second patch fixes the potential race condition between submitting IO
and setting PID of the background IO process to cgroup.procs.

Changes from v1:
    - Use $BASHPID to write the child sub-shell PID into cgroup.procs
      (Shinichiro Kawasaki)
    - Link to v1: https://lore.kernel.org/all/20241217131440.1980151-1-nilay@linux.ibm.com/

Nilay Shroff (2):
  throtl/002: calculate block-size based on device max-sectors setting
  throtl: fix the race between submitting IO and setting cgroup.procs

 tests/throtl/002 | 14 ++++++++++----
 tests/throtl/004 |  7 ++-----
 tests/throtl/005 |  7 ++-----
 tests/throtl/rc  | 11 ++++++-----
 4 files changed, 20 insertions(+), 19 deletions(-)

-- 
2.45.2

[PATCHv2 blktests 1/2] throtl/002: calculate block-size based on device max-sectors setting

[Nilay Shroff]

The commit 60fa2e3ff3ab ("update max_sectors setting") added max-sectors
while setting up throttle device. So now we should also calculate block-
size which matches the wiops. Typically, size of each IO which is submitted
to the block device depends on the max-sectors setting of the block device.
For example setting max-sectors to 128 results into 64kb of max. IO size
which should be used for sending read/write command to the device. So take
into account the max-sectors-kb and wiops settings and calculate the
appropriate block-size which is then used to issue IO to the block device.
This change would result in always submitting 256 I/O read/write commands
to block device.

Without this change on a system with 64k PAGE SIZE, using block-size of 1M
would result in 16 I/O being submitted to the device and this operation may
finish in a fraction of a section and result in test failure. However the
intent of this test case is that we want to test submitting 256 I/O after
setting wiops limit to 256.

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 tests/throtl/002 | 14 ++++++++++----
 tests/throtl/rc  |  4 ++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tests/throtl/002 b/tests/throtl/002
index 185e66b..02b0969 100755
--- a/tests/throtl/002
+++ b/tests/throtl/002
@@ -14,6 +14,9 @@ test() {
 	echo "Running ${TEST_NAME}"
 
 	local page_size max_secs
+	local io_size_kb block_size
+	local iops=256
+
 	page_size=$(getconf PAGE_SIZE)
 	max_secs=$((page_size / 512))
 
@@ -21,12 +24,15 @@ test() {
 		return 1;
 	fi
 
-	_throtl_set_limits wiops=256
-	_throtl_test_io write 1M 1
+	io_size_kb=$(($(_throtl_get_max_io_size) * 1024))
+	block_size=$((iops * io_size_kb))
+
+	_throtl_set_limits wiops="${iops}"
+	_throtl_test_io write "${block_size}" 1
 	_throtl_remove_limits
 
-	_throtl_set_limits riops=256
-	_throtl_test_io read 1M 1
+	_throtl_set_limits riops="${iops}"
+	_throtl_test_io read "${block_size}" 1
 	_throtl_remove_limits
 
 	_clean_up_throtl
diff --git a/tests/throtl/rc b/tests/throtl/rc
index 9c264bd..330e6b9 100644
--- a/tests/throtl/rc
+++ b/tests/throtl/rc
@@ -71,6 +71,10 @@ _throtl_remove_limits() {
 		"$CGROUP2_DIR/$THROTL_DIR/io.max"
 }
 
+_throtl_get_max_io_size() {
+	cat "/sys/block/$THROTL_DEV/queue/max_sectors_kb"
+}
+
 _throtl_issue_io() {
 	local start_time
 	local end_time
-- 
2.45.2

Re: [PATCHv2 blktests 1/2] throtl/002: calculate block-size based on device max-sectors setting

[Yi Zhang]

Tested-by: Yi Zhang <yi.zhang@redhat.com>

Confirmed the patch fixed the throtl/002 failure on
ppc64le/aarch64+64k on 6.13-rc4.

On Wed, Dec 18, 2024 at 9:49 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>
> The commit 60fa2e3ff3ab ("update max_sectors setting") added max-sectors
> while setting up throttle device. So now we should also calculate block-
> size which matches the wiops. Typically, size of each IO which is submitted
> to the block device depends on the max-sectors setting of the block device.
> For example setting max-sectors to 128 results into 64kb of max. IO size
> which should be used for sending read/write command to the device. So take
> into account the max-sectors-kb and wiops settings and calculate the
> appropriate block-size which is then used to issue IO to the block device.
> This change would result in always submitting 256 I/O read/write commands
> to block device.
>
> Without this change on a system with 64k PAGE SIZE, using block-size of 1M
> would result in 16 I/O being submitted to the device and this operation may
> finish in a fraction of a section and result in test failure. However the
> intent of this test case is that we want to test submitting 256 I/O after
> setting wiops limit to 256.
>
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  tests/throtl/002 | 14 ++++++++++----
>  tests/throtl/rc  |  4 ++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/tests/throtl/002 b/tests/throtl/002
> index 185e66b..02b0969 100755
> --- a/tests/throtl/002
> +++ b/tests/throtl/002
> @@ -14,6 +14,9 @@ test() {
>         echo "Running ${TEST_NAME}"
>
>         local page_size max_secs
> +       local io_size_kb block_size
> +       local iops=256
> +
>         page_size=$(getconf PAGE_SIZE)
>         max_secs=$((page_size / 512))
>
> @@ -21,12 +24,15 @@ test() {
>                 return 1;
>         fi
>
> -       _throtl_set_limits wiops=256
> -       _throtl_test_io write 1M 1
> +       io_size_kb=$(($(_throtl_get_max_io_size) * 1024))
> +       block_size=$((iops * io_size_kb))
> +
> +       _throtl_set_limits wiops="${iops}"
> +       _throtl_test_io write "${block_size}" 1
>         _throtl_remove_limits
>
> -       _throtl_set_limits riops=256
> -       _throtl_test_io read 1M 1
> +       _throtl_set_limits riops="${iops}"
> +       _throtl_test_io read "${block_size}" 1
>         _throtl_remove_limits
>
>         _clean_up_throtl
> diff --git a/tests/throtl/rc b/tests/throtl/rc
> index 9c264bd..330e6b9 100644
> --- a/tests/throtl/rc
> +++ b/tests/throtl/rc
> @@ -71,6 +71,10 @@ _throtl_remove_limits() {
>                 "$CGROUP2_DIR/$THROTL_DIR/io.max"
>  }
>
> +_throtl_get_max_io_size() {
> +       cat "/sys/block/$THROTL_DEV/queue/max_sectors_kb"
> +}
> +
>  _throtl_issue_io() {
>         local start_time
>         local end_time
> --
> 2.45.2
>


--
Best Regards,
  Yi Zhang

[PATCHv2 blktests 2/2] throtl: fix the race between submitting IO and setting cgroup.procs

[Nilay Shroff]

The throttle test cases uses _throtl_issue_io function to submit IO
to the device. This function typically runs in the background process
however before this function starts execution and submit IO, we need
to set the PID of the background process into cgroup.procs. The current
implementation adds sleep 0.1 before _throtl_issue_io and it's assumed
that during this sleep time of 0.1 second, we shall be able to write the
PID of the background process to cgroup.procs. However this may not be
always true as background process might starts running after sleep of 0.1
seconds (and hence start submitting IO) before we could actually write
the PID of background process into cgroup.procs from the parent shell.

This commit helps fix the above race condition by writing pid of the
background/child process using $BASHPID into cgroup.procs. The $BASHPID
returns the pid of the current bash process. So we leverage $BASHPID to
first write the pid of the background/child job/process into cgroup.procs
from within the child sub-shell and then start submitting IO. This way we
eliminate the need of any communication between parent shell and the 
background/child shell process and that helps avoid the race.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 tests/throtl/004 | 7 ++-----
 tests/throtl/005 | 7 ++-----
 tests/throtl/rc  | 7 ++-----
 3 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/tests/throtl/004 b/tests/throtl/004
index 6e28612..d1461b9 100755
--- a/tests/throtl/004
+++ b/tests/throtl/004
@@ -21,16 +21,13 @@ test() {
 	_throtl_set_limits wbps=$((1024 * 1024))
 
 	{
-		sleep 0.1
+		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
 		_throtl_issue_io write 10M 1
 	} &
 
-	local pid=$!
-	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
-
 	sleep 0.6
 	echo 0 > "/sys/kernel/config/nullb/$THROTL_DEV/power"
-	wait "$pid"
+	wait $!
 
 	_clean_up_throtl
 	echo "Test complete"
diff --git a/tests/throtl/005 b/tests/throtl/005
index 0778258..86e52b3 100755
--- a/tests/throtl/005
+++ b/tests/throtl/005
@@ -20,16 +20,13 @@ test() {
 	_throtl_set_limits wbps=$((512 * 1024))
 
 	{
-		sleep 0.1
+		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
 		_throtl_issue_io write 1M 1
 	} &
 
-	local pid=$!
-	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
-
 	sleep 1
 	_throtl_set_limits wbps=$((256 * 1024))
-	wait $pid
+	wait $!
 	_throtl_remove_limits
 
 	_clean_up_throtl
diff --git a/tests/throtl/rc b/tests/throtl/rc
index 330e6b9..df54cb9 100644
--- a/tests/throtl/rc
+++ b/tests/throtl/rc
@@ -97,18 +97,15 @@ _throtl_issue_io() {
 # IO and then print time elapsed to the second, blk-throttle limits should be
 # set before this function.
 _throtl_test_io() {
-	local pid
 
 	{
 		local rw=$1
 		local bs=$2
 		local count=$3
 
-		sleep 0.1
+		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
 		_throtl_issue_io "$rw" "$bs" "$count"
 	} &
 
-	pid=$!
-	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
-	wait $pid
+	wait $!
 }
-- 
2.45.2

Re: [PATCHv2 blktests 2/2] throtl: fix the race between submitting IO and setting cgroup.procs

[Yu Kuai]

在 2024/12/18 21:43, Nilay Shroff 写道:
> The throttle test cases uses _throtl_issue_io function to submit IO
> to the device. This function typically runs in the background process
> however before this function starts execution and submit IO, we need
> to set the PID of the background process into cgroup.procs. The current
> implementation adds sleep 0.1 before _throtl_issue_io and it's assumed
> that during this sleep time of 0.1 second, we shall be able to write the
> PID of the background process to cgroup.procs. However this may not be
> always true as background process might starts running after sleep of 0.1
> seconds (and hence start submitting IO) before we could actually write
> the PID of background process into cgroup.procs from the parent shell.
> 
> This commit helps fix the above race condition by writing pid of the
> background/child process using $BASHPID into cgroup.procs. The $BASHPID
> returns the pid of the current bash process. So we leverage $BASHPID to
> first write the pid of the background/child job/process into cgroup.procs
> from within the child sub-shell and then start submitting IO. This way we
> eliminate the need of any communication between parent shell and the
> background/child shell process and that helps avoid the race.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>   tests/throtl/004 | 7 ++-----
>   tests/throtl/005 | 7 ++-----
>   tests/throtl/rc  | 7 ++-----
>   3 files changed, 6 insertions(+), 15 deletions(-)
> 
Thanks for the patch!
Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/tests/throtl/004 b/tests/throtl/004
> index 6e28612..d1461b9 100755
> --- a/tests/throtl/004
> +++ b/tests/throtl/004
> @@ -21,16 +21,13 @@ test() {
>   	_throtl_set_limits wbps=$((1024 * 1024))
>   
>   	{
> -		sleep 0.1
> +		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
>   		_throtl_issue_io write 10M 1
>   	} &
>   
> -	local pid=$!
> -	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
> -
>   	sleep 0.6
>   	echo 0 > "/sys/kernel/config/nullb/$THROTL_DEV/power"
> -	wait "$pid"
> +	wait $!
>   
>   	_clean_up_throtl
>   	echo "Test complete"
> diff --git a/tests/throtl/005 b/tests/throtl/005
> index 0778258..86e52b3 100755
> --- a/tests/throtl/005
> +++ b/tests/throtl/005
> @@ -20,16 +20,13 @@ test() {
>   	_throtl_set_limits wbps=$((512 * 1024))
>   
>   	{
> -		sleep 0.1
> +		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
>   		_throtl_issue_io write 1M 1
>   	} &
>   
> -	local pid=$!
> -	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
> -
>   	sleep 1
>   	_throtl_set_limits wbps=$((256 * 1024))
> -	wait $pid
> +	wait $!
>   	_throtl_remove_limits
>   
>   	_clean_up_throtl
> diff --git a/tests/throtl/rc b/tests/throtl/rc
> index 330e6b9..df54cb9 100644
> --- a/tests/throtl/rc
> +++ b/tests/throtl/rc
> @@ -97,18 +97,15 @@ _throtl_issue_io() {
>   # IO and then print time elapsed to the second, blk-throttle limits should be
>   # set before this function.
>   _throtl_test_io() {
> -	local pid
>   
>   	{
>   		local rw=$1
>   		local bs=$2
>   		local count=$3
>   
> -		sleep 0.1
> +		echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
>   		_throtl_issue_io "$rw" "$bs" "$count"
>   	} &
>   
> -	pid=$!
> -	echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
> -	wait $pid
> +	wait $!
>   }
> 

Re: [PATCHv2 blktests 0/2] throtl: fix IO block-size and race while submitting IO

[Shinichiro Kawasaki]

On Dec 18, 2024 / 19:13, Nilay Shroff wrote:
> Hi,
> 
> The first patch fixes the IO block-size used for submitting IO depending
> on the throttle device max-sectors settings.
> 
> The second patch fixes the potential race condition between submitting IO
> and setting PID of the background IO process to cgroup.procs.
> 
> Changes from v1:
>     - Use $BASHPID to write the child sub-shell PID into cgroup.procs
>       (Shinichiro Kawasaki)
>     - Link to v1: https://lore.kernel.org/all/20241217131440.1980151-1-nilay@linux.ibm.com/

Thanks for this v2 series. The patches looks good to me. In case anyone has
some more comments after this vacation season, I will wait several days before
applying them.

Re: [PATCHv2 blktests 0/2] throtl: fix IO block-size and race while submitting IO

[Nilay Shroff]

Hi Shinichiro,

On 12/24/24 1:56 PM, Shinichiro Kawasaki wrote:
> On Dec 18, 2024 / 19:13, Nilay Shroff wrote:
>> Hi,
>>
>> The first patch fixes the IO block-size used for submitting IO depending
>> on the throttle device max-sectors settings.
>>
>> The second patch fixes the potential race condition between submitting IO
>> and setting PID of the background IO process to cgroup.procs.
>>
>> Changes from v1:
>>     - Use $BASHPID to write the child sub-shell PID into cgroup.procs
>>       (Shinichiro Kawasaki)
>>     - Link to v1: https://lore.kernel.org/all/20241217131440.1980151-1-nilay@linux.ibm.com/
> 
> Thanks for this v2 series. The patches looks good to me. In case anyone has
> some more comments after this vacation season, I will wait several days before
> applying them.

A gentle ping...

Just wanted to check with you whether did you get a chance to merge this change?

Thanks,
--Nilay

Re: [PATCHv2 blktests 0/2] throtl: fix IO block-size and race while submitting IO

[Shinichiro Kawasaki]

On Jan 16, 2025 / 16:52, Nilay Shroff wrote:
> Hi Shinichiro,
> 
> On 12/24/24 1:56 PM, Shinichiro Kawasaki wrote:
> > On Dec 18, 2024 / 19:13, Nilay Shroff wrote:
> >> Hi,
> >>
> >> The first patch fixes the IO block-size used for submitting IO depending
> >> on the throttle device max-sectors settings.
> >>
> >> The second patch fixes the potential race condition between submitting IO
> >> and setting PID of the background IO process to cgroup.procs.
> >>
> >> Changes from v1:
> >>     - Use $BASHPID to write the child sub-shell PID into cgroup.procs
> >>       (Shinichiro Kawasaki)
> >>     - Link to v1: https://lore.kernel.org/all/20241217131440.1980151-1-nilay@linux.ibm.com/
> > 
> > Thanks for this v2 series. The patches looks good to me. In case anyone has
> > some more comments after this vacation season, I will wait several days before
> > applying them.
> 
> A gentle ping...
> 
> Just wanted to check with you whether did you get a chance to merge this change?

Sorry, I overlooked it after the vacation. I have applied the series. Thanks!