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

[Junxiao Bi <junxiao.bi@xxxxxxxxxx>]

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