| Message ID | 20241217131440.1980151-3-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | throtl: fix IO block-size and race while submitting IO | expand |
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" } & Thanks, Kuai
On 12/17/24 6:24 PM, Yu Kuai wrote: > - sleep 0.1 > + while ! cat $CGROUP2_DIR/$THROTL_DIR/cgroup.procs | grep > $pid; do > + sleep 0.1 > _throtl_issue_io "$rw" "$bs" "$count" > } & If this approach is used, please add the options -q (quiet) and -w (match whole words only) to the grep command. Thanks, Bart.
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. Thanks, --Nilay
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 } &
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
diff --git a/tests/throtl/004 b/tests/throtl/004 index 6e28612..44b33ec 100755 --- a/tests/throtl/004 +++ b/tests/throtl/004 @@ -21,12 +21,18 @@ test() { _throtl_set_limits wbps=$((1024 * 1024)) { - sleep 0.1 + while true; do + if [[ -e "$TMPDIR/test-io" ]]; then + break + fi + sleep 0.1 + done _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" diff --git a/tests/throtl/005 b/tests/throtl/005 index 0778258..4797ea3 100755 --- a/tests/throtl/005 +++ b/tests/throtl/005 @@ -20,12 +20,19 @@ test() { _throtl_set_limits wbps=$((512 * 1024)) { - sleep 0.1 + while true; do + if [[ -e "$TMPDIR/test-io" ]]; then + break + fi + sleep 0.1 + done + _throtl_issue_io write 1M 1 } & local pid=$! echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs" + touch "$TMPDIR/test-io" sleep 1 _throtl_set_limits wbps=$((256 * 1024)) diff --git a/tests/throtl/rc b/tests/throtl/rc index 330e6b9..a9c1889 100644 --- a/tests/throtl/rc +++ b/tests/throtl/rc @@ -104,11 +104,18 @@ _throtl_test_io() { local bs=$2 local count=$3 - sleep 0.1 + while true; do + if [[ -e "$TMPDIR/test-io" ]]; then + break + fi + sleep 0.1 + done _throtl_issue_io "$rw" "$bs" "$count" } & pid=$! echo "$pid" > "$CGROUP2_DIR/$THROTL_DIR/cgroup.procs" + touch "$TMPDIR/test-io" wait $pid + rm "$TMPDIR/test-io" }
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. 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. Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> --- tests/throtl/004 | 8 +++++++- tests/throtl/005 | 9 ++++++++- tests/throtl/rc | 9 ++++++++- 3 files changed, 23 insertions(+), 3 deletions(-)