| Message ID | 20170324123621.5227-3-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/24/2017 01:36 PM, Ming Lei wrote: > 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)); > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On Fri, 2017-03-24 at 20:36 +0800, Ming Lei wrote: > 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 enforced for the "dying" flag and q_usage_counter because of the order in which these are set by blk_set_queue_dying(). Thanks, Bart.
On Sat, Mar 25, 2017 at 1:24 AM, Bart Van Assche <Bart.VanAssche@sandisk.com> wrote: > On Fri, 2017-03-24 at 20:36 +0800, Ming Lei wrote: >> 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
On Sat, 2017-03-25 at 01:38 +0800, Ming Lei wrote: > As I explained, the dying flag should only be mentioned after we change > the code in blk_set_queue_dying(). Hello Ming, If patches 2 and 4 would be combined into a single patch then it wouldn't be necessary anymore to update the comment introduced in patch 2 in patch 4. I think that would make this patch series easier to review. Since the issues fixed by your patches are longstanding issues, have you considered to add a "Cc: stable" tag? Thanks, Bart.
On Sat, Mar 25, 2017 at 2:45 AM, Bart Van Assche <Bart.VanAssche@sandisk.com> wrote: > On Sat, 2017-03-25 at 01:38 +0800, Ming Lei wrote: >> As I explained, the dying flag should only be mentioned after we change >> the code in blk_set_queue_dying(). > > Hello Ming, > > If patches 2 and 4 would be combined into a single patch then it wouldn't > be necessary anymore to update the comment introduced in patch 2 in patch 4. > I think that would make this patch series easier to review. I think it is better to not do that, because we know patch 2 and patch 4 are doing two different things, and patch 2 can be thought as one fix, but patch 4 should be a improvement. And it is normal to update comment after code changes. > > Since the issues fixed by your patches are longstanding issues, have you > considered to add a "Cc: stable" tag? I don't know because there isn't real report, and maybe never, so I leave it alone. Thanks, Ming Lei
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));
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(+)