Re: [PATCH] block: fix io hung by block throttle

From: Junxiao Bi
Date: Thu Apr 15 2021 - 01:02:24 EST


On 4/14/21 9:11 PM, Hillf Danton wrote:

On Wed, 14 Apr 2021 14:18:30 Junxiao Bi wrote:
There is a race bug which can cause io hung when multiple processes
run parallel in rq_qos_wait().
Let assume there were 4 processes P1/P2/P3/P4, P1/P2 were at the entry
of rq_qos_wait, and P3/P4 were waiting for io done, 2 io were inflight,
the inflight io limit was 2. See race below.

void rq_qos_wait()
{
...
bool has_sleeper;

>>>> P3/P4 were in sleeper list, has_sleeper was true for both P1 and P2.
has_sleeper = wq_has_sleeper(&rqw->wait);
if (!has_sleeper && acquire_inflight_cb(rqw, private_data))
return;

>>>> 2 inflight io done, P3/P4 were waken up to issue 2 new io.
>>>> 2 new io done, no inflight io.

>>>> P1/P2 were added to the sleeper list, 2 entry in the list
prepare_to_wait_exclusive(&rqw->wait, &data.wq, TASK_UNINTERRUPTIBLE);

>>>> P1/P2 were in the sleeper list, has_sleeper was true for P1/P2.
has_sleeper = !wq_has_single_sleeper(&rqw->wait);
do {
/* The memory barrier in set_task_state saves us here. */
if (data.got_token)
break;
if (!has_sleeper && acquire_inflight_cb(rqw, private_data)) {
finish_wait(&rqw->wait, &data.wq);

/*
* We raced with wbt_wake_function() getting a token,
* which means we now have two. Put our local token
* and wake anyone else potentially waiting for one.
*/
smp_rmb();
if (data.got_token)
cleanup_cb(rqw, private_data);
break;
}

>>>> P1/P2 hung here forever. New io requests will also hung here.
io_schedule();
has_sleeper = true;
set_current_state(TASK_UNINTERRUPTIBLE);
} while (1);
finish_wait(&rqw->wait, &data.wq);
}

Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
---
block/blk-rq-qos.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index 656460636ad3..04d888c99bc0 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -260,19 +260,17 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data,
.cb = acquire_inflight_cb,
.private_data = private_data,
};
- bool has_sleeper;
- has_sleeper = wq_has_sleeper(&rqw->wait);
- if (!has_sleeper && acquire_inflight_cb(rqw, private_data))
+ if (!wq_has_sleeper(&rqw->wait)
+ && acquire_inflight_cb(rqw, private_data))
return;
prepare_to_wait_exclusive(&rqw->wait, &data.wq, TASK_UNINTERRUPTIBLE);
- has_sleeper = !wq_has_single_sleeper(&rqw->wait);
do {
/* The memory barrier in set_task_state saves us here. */
if (data.got_token)
break;
- if (!has_sleeper && acquire_inflight_cb(rqw, private_data)) {
+ if (acquire_inflight_cb(rqw, private_data)) {
finish_wait(&rqw->wait, &data.wq);
/*
@@ -286,7 +284,6 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data,
break;
}
io_schedule();
- has_sleeper = true;
set_current_state(TASK_UNINTERRUPTIBLE);
} while (1);
finish_wait(&rqw->wait, &data.wq);
--
2.24.3 (Apple Git-128)

No wakeup may cause the hang.

--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -287,7 +287,8 @@ void rq_qos_wait(struct rq_wait *rqw, vo
}
io_schedule();
has_sleeper = true;
- set_current_state(TASK_UNINTERRUPTIBLE);
+ prepare_to_wait_exclusive(&rqw->wait, &data.wq,
+ TASK_UNINTERRUPTIBLE);

From rq_qos_wake_function(), the process can be waken up and removed from the sleeper list only when it get the budget. Looks not necessary to re-add it to sleeper list again.

Thanks,

Junxiao.

} while (1);
finish_wait(&rqw->wait, &data.wq);
}