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

Junxiao Bi
2021-04-21T21:30:02Z

Ping? This can cause io hung.

Thanks,

Junxiao.

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@vger.kernel.org
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>> ---
>> 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);
> } while (1);
> finish_wait(&rqw->wait, &data.wq);
> }