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

[Junxiao Bi <junxiao.bi@oracle.com>]

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);
>   }