On 12/18/24 17:12, Shinichiro Kawasaki wrote:
> On Dec 18, 2024 / 15:20, Nilay Shroff wrote:
>>
>>
>> On 12/18/24 07:54, Yu Kuai wrote:
>>> Hi,
>>>
>>> 在 2024/12/17 21:14, Nilay Shroff 写道:
>>>> This commit helps fix the above race condition by touching a temp file. The
>>>> the existence of the temp file is then polled by the background process at
>>>> regular interval. Until the temp file is created, the background process
>>>> would not forward progress and starts submitting IO and from the main
>>>> thread we'd touch temp file only after we write PID of the background
>>>> process into cgroup.procs.
>>>
>>> It's right sleep 0.1 is not appropriate here, and this can work.
>>> However, I think reading cgroup.procs directly is better, something
>>> like following:
>>>
>>> _throtl_test_io() {
>>> - local pid
>>> + local pid="none"
>>>
>>> {
>>> local rw=$1
>>> local bs=$2
>>> local count=$3
>>>
>>> - sleep 0.1
>>> + while ! cat $CGROUP2_DIR/$THROTL_DIR/cgroup.procs | grep $pid; do
>>> + sleep 0.1
>>> _throtl_issue_io "$rw" "$bs" "$count"
>>> } &
>>
>> Thank you for your review and suggestion!
>>
>> However, IMO this may not work always because the issue here's that the @pid is local
>> variable and when the shell starts the background job/process, typically, all local
>> variables are copied to the new job. Henceforth, any update made to @pid in the parent
>> shell would not be visible to the background job.
>> I think, for IPC between parent shell and background job, we may use temp file,
>> named pipe or signals. As file is the easiest and simplest mechanism, for this
>> simple test I preferred using file.
>
> How about to refer to $BASHPID in the background job sub-shell? It shows the PID
> of the background job, so we don't need IPC to pass the PID.
>
> I think the hunk like below can do the trick, hopefully. If it works, it would
> be cleaner to factor out the while loop to a helper function, like
> _throtl_wait_for_cgroup_procs() or something.
>
>
> diff --git a/tests/throtl/004 b/tests/throtl/004
> index 6e28612..0a16764 100755
> --- a/tests/throtl/004
> +++ b/tests/throtl/004
> @@ -22,6 +22,10 @@ test() {
>
> {
> sleep 0.1
> + while ! grep --quiet --word-regexp "$BASHPID" \
> + "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"; do
> + sleep 0.1
> + done
> _throtl_issue_io write 10M 1
> } &
I like the idea of using $BASHPID. I just tried it and it works!!
In fact, I have now further simplified the logic such that we don't need
IPC between parent and child sub-shell:) Please find below the diff:
diff --git a/tests/throtl/004 b/tests/throtl/004
index 44b33ec..d1461b9 100755
--- a/tests/throtl/004
+++ b/tests/throtl/004
@@ -21,22 +21,13 @@ test() {
_throtl_set_limits wbps=$((1024 * 1024))
{
- while true; do
- if [[ -e "$TMPDIR/test-io" ]]; then
- break
- fi
- sleep 0.1
- done
+ echo "$BASHPID" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
_throtl_issue_io write 10M 1
} &
- local pid=$!
- echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs"
- touch "$TMPDIR/test-io"
-
sleep 0.6
echo 0 > "/sys/kernel/config/nullb/$THROTL_DEV/power"
- wait "$pid"
+ wait $!
As shown above, we could now directly write the PID of the background
job into the cgroup.procs and submit IO. So we don't require IPC between
parent and child sub-shell.
I'd spin a new patch with the above change and submit.
Thanks,
--Nilay
