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

[Junxiao Bi]

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: sta...@vger.kernel.org
Signed-off-by: Junxiao Bi <junxiao...@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);

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