On Sat, Mar 25, 2017 at 1:24 AM, Bart Van Assche
[off-list ref] wrote:

On Fri, 2017-03-24 at 20:36 +0800, Ming Lei wrote:

quoted

Without the barrier, reading DEAD flag of .q_usage_counter
and reading .mq_freeze_depth may be reordered, then the
following wait_event_interruptible() may never return.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 block/blk-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index ad388d5e309a..44eed17319c0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -669,6 +669,14 @@ int blk_queue_enter(struct request_queue *q, bool nowait)
              if (nowait)
                      return -EBUSY;

+             /*
+              * read pair of barrier in blk_mq_freeze_queue_start(),
+              * we need to order reading DEAD flag of .q_usage_counter
+              * and reading .mq_freeze_depth, otherwise the following
+              * wait may never return if the two read are reordered.
+              */
+             smp_rmb();
+
              ret = wait_event_interruptible(q->mq_freeze_wq,
                              !atomic_read(&q->mq_freeze_depth) ||
                              blk_queue_dying(q));

Hello Ming,

The code looks fine to me but the comment not. You probably wanted to refer
to the "dying" flag instead of the "dead" flag? The read order has to be

No, looks you misunderstand the issue.

I mean the order between reading __PERCPU_REF_DEAD of .q_usage_counter
and reading .mq_freeze_depth should be enhanced, especially it is in
blk_queue_enter() vs. blk_mq_freeze_queue_start().

In the last patch, you will find the dying flag is mentioned in above comment
after we call blk_freeze_queue_start() just after the dying flag is set.

enforced for the "dying" flag and q_usage_counter because of the order in
which these are set by blk_set_queue_dying().

As I explained, the dying flag should only be mentioned after we change
the code in blk_set_queue_dying().


Thanks,
Ming Lei