| Message ID | 20190402223202.30752-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix a race between tag iteration and hardware queue changes | expand |
On 4/3/19 6:32 AM, Bart Van Assche wrote: > Since the callback function called by blk_mq_queue_tag_busy_iter() > may sleep calling synchronize_rcu() from __blk_mq_update_nr_hw_queues() > is not sufficient to wait until blk_mq_queue_tag_busy_iter() has > finished. Instead of making __blk_mq_update_nr_hw_queues() wait until > q->q_usage_counter == 0 is globally visible, do not iterate over tags > if the request queue is frozen. > > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Hannes Reinecke <hare@suse.com> > Cc: James Smart <james.smart@broadcom.com> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Jianchao Wang <jianchao.w.wang@oracle.com> > Cc: Keith Busch <keith.busch@intel.com> > Cc: Dongli Zhang <dongli.zhang@oracle.com> > Cc: <stable@vger.kernel.org> > Fixes: 530ca2c9bd69 ("blk-mq: Allow blocking queue tag iter callbacks") # v4.19. > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > block/blk-mq-tag.c | 10 +++++----- > block/blk-mq.c | 5 +---- > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > index a4931fc7be8a..89f479634b4d 100644 > --- a/block/blk-mq-tag.c > +++ b/block/blk-mq-tag.c > @@ -383,14 +383,13 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > > /* > * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx > - * while the queue is frozen. So we can use q_usage_counter to avoid > - * racing with it. __blk_mq_update_nr_hw_queues() uses > - * synchronize_rcu() to ensure this function left the critical section > - * below. > + * while the queue is frozen. Hold q_usage_counter to serialize > + * __blk_mq_update_nr_hw_queues() against this function. > */ > if (!percpu_ref_tryget(&q->q_usage_counter)) > return; > - > + if (atomic_read(&q->mq_freeze_depth)) > + goto out; > queue_for_each_hw_ctx(q, hctx, i) { > struct blk_mq_tags *tags = hctx->tags; > > @@ -405,6 +404,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > bt_for_each(hctx, &tags->breserved_tags, fn, priv, true); > bt_for_each(hctx, &tags->bitmap_tags, fn, priv, false); > } > +out: > blk_queue_exit(q); > } > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 652d0c6d5945..f9fc1536408d 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -3226,10 +3226,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, > > list_for_each_entry(q, &set->tag_list, tag_set_list) > blk_mq_freeze_queue(q); > - /* > - * Sync with blk_mq_queue_tag_busy_iter. > - */ > - synchronize_rcu(); I used to see the removal of above in one of Jianchao's patchset. https://lore.kernel.org/patchwork/patch/1051047/ Dongli Zhang > + > /* > * Switch IO scheduler to 'none', cleaning up the data associated > * with the previous scheduler. We will switch back once we are done >
On 4/2/19 6:02 PM, Dongli Zhang wrote: > I used to see the removal of above in one of Jianchao's patchset. > > https://lore.kernel.org/patchwork/patch/1051047/ Thanks for the feedback. But it seems like the q->mq_freeze_depth test is missing from Jianchao's patch ... Bart.
Hi Bart, On 4/3/19 6:32 AM, Bart Van Assche wrote: > Since the callback function called by blk_mq_queue_tag_busy_iter() > may sleep calling synchronize_rcu() from __blk_mq_update_nr_hw_queues() > is not sufficient to wait until blk_mq_queue_tag_busy_iter() has > finished. Instead of making __blk_mq_update_nr_hw_queues() wait until > q->q_usage_counter == 0 is globally visible, do not iterate over tags > if the request queue is frozen.> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Hannes Reinecke <hare@suse.com> > Cc: James Smart <james.smart@broadcom.com> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Jianchao Wang <jianchao.w.wang@oracle.com> > Cc: Keith Busch <keith.busch@intel.com> > Cc: Dongli Zhang <dongli.zhang@oracle.com> > Cc: <stable@vger.kernel.org> > Fixes: 530ca2c9bd69 ("blk-mq: Allow blocking queue tag iter callbacks") # v4.19. > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > block/blk-mq-tag.c | 10 +++++----- > block/blk-mq.c | 5 +---- > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > index a4931fc7be8a..89f479634b4d 100644 > --- a/block/blk-mq-tag.c > +++ b/block/blk-mq-tag.c > @@ -383,14 +383,13 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > > /* > * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx > - * while the queue is frozen. So we can use q_usage_counter to avoid > - * racing with it. __blk_mq_update_nr_hw_queues() uses > - * synchronize_rcu() to ensure this function left the critical section > - * below. > + * while the queue is frozen. Hold q_usage_counter to serialize > + * __blk_mq_update_nr_hw_queues() against this function. > */ > if (!percpu_ref_tryget(&q->q_usage_counter)) > return; > - > + if (atomic_read(&q->mq_freeze_depth)) > + goto out; > queue_for_each_hw_ctx(q, hctx, i) { > struct blk_mq_tags *tags = hctx->tags; > > @@ -405,6 +404,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > bt_for_each(hctx, &tags->breserved_tags, fn, priv, true); > bt_for_each(hctx, &tags->bitmap_tags, fn, priv, false); > } > +out: > blk_queue_exit(q); If callback function goes to sleep, we would not be able to reach at blk_queue_exit() to dec q->q_usage_counter. On the other side of __blk_mq_update_nr_hw_queues(), once blk_mq_freeze_queue() is passed, __PERCPU_REF_DEAD is set and q->q_usage_counter should always be 0. Otherwise, blk_mq_freeze_queue_wait() would not move forward. I think we would even not be able to call the callback (which might sleep) if blk_mq_freeze_queue() is already passed? Why we need extra check of q->mq_freeze_depth? Is it an optimization? Please let me know if my understanding is incorrect. Thank you very much! > } > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 652d0c6d5945..f9fc1536408d 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -3226,10 +3226,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, > > list_for_each_entry(q, &set->tag_list, tag_set_list) > blk_mq_freeze_queue(q); > - /* > - * Sync with blk_mq_queue_tag_busy_iter. > - */ > - synchronize_rcu(); > + > /* > * Switch IO scheduler to 'none', cleaning up the data associated > * with the previous scheduler. We will switch back once we are done > Dongli Zhang
Hi Bart On 4/3/19 6:32 AM, Bart Van Assche wrote: > Since the callback function called by blk_mq_queue_tag_busy_iter() > may sleep calling synchronize_rcu() from __blk_mq_update_nr_hw_queues() > is not sufficient to wait until blk_mq_queue_tag_busy_iter() has > finished. Instead of making __blk_mq_update_nr_hw_queues() wait until > q->q_usage_counter == 0 is globally visible, do not iterate over tags > if the request queue is frozen. > > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Hannes Reinecke <hare@suse.com> > Cc: James Smart <james.smart@broadcom.com> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Jianchao Wang <jianchao.w.wang@oracle.com> > Cc: Keith Busch <keith.busch@intel.com> > Cc: Dongli Zhang <dongli.zhang@oracle.com> > Cc: <stable@vger.kernel.org> > Fixes: 530ca2c9bd69 ("blk-mq: Allow blocking queue tag iter callbacks") # v4.19. > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > block/blk-mq-tag.c | 10 +++++----- > block/blk-mq.c | 5 +---- > 2 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > index a4931fc7be8a..89f479634b4d 100644 > --- a/block/blk-mq-tag.c > +++ b/block/blk-mq-tag.c > @@ -383,14 +383,13 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > > /* > * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx > - * while the queue is frozen. So we can use q_usage_counter to avoid > - * racing with it. __blk_mq_update_nr_hw_queues() uses > - * synchronize_rcu() to ensure this function left the critical section > - * below. > + * while the queue is frozen. Hold q_usage_counter to serialize > + * __blk_mq_update_nr_hw_queues() against this function. > */ > if (!percpu_ref_tryget(&q->q_usage_counter)) > return; > - > + if (atomic_read(&q->mq_freeze_depth)) > + goto out; percpu_ref_tryget should be enough here. If the refcount is not zero, get it and the process of updating nr_hw_queues will wait, and we will be safe. If the refcount has been zeroed, it says that there is no any requests and blk_mq_queue_tag_busy_iter needn't to do anything, so quit is OK. Add a atomic_read(&q->mq_freeze_depth) here is unnecessary and may cause deadlock. For example, the recovery process want to freeze and drain the queue, but the device has been dead. We have to depend on timeout work to finish the in-flight requests with blk_mq_queue_tag_busy_iter. But now, it cannot do nothing because this mq_freeze_depth checking. The synchronize_rcu is indeed unnecessary after we introduce percpu_ref_tryget here. Thanks Jianchao > queue_for_each_hw_ctx(q, hctx, i) { > struct blk_mq_tags *tags = hctx->tags; > > @@ -405,6 +404,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, > bt_for_each(hctx, &tags->breserved_tags, fn, priv, true); > bt_for_each(hctx, &tags->bitmap_tags, fn, priv, false); > } > +out: > blk_queue_exit(q); > } > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 652d0c6d5945..f9fc1536408d 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -3226,10 +3226,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, > > list_for_each_entry(q, &set->tag_list, tag_set_list) > blk_mq_freeze_queue(q); > - /* > - * Sync with blk_mq_queue_tag_busy_iter. > - */ > - synchronize_rcu(); > + > /* > * Switch IO scheduler to 'none', cleaning up the data associated > * with the previous scheduler. We will switch back once we are done >
On Wed, 2019-04-03 at 11:26 +0800, jianchao.wang wrote:
> percpu_ref_tryget should be enough here.
Having thought further about this, I think you are right. I will drop this
patch.
Thanks,
Bart.
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index a4931fc7be8a..89f479634b4d 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -383,14 +383,13 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, /* * __blk_mq_update_nr_hw_queues() updates nr_hw_queues and queue_hw_ctx - * while the queue is frozen. So we can use q_usage_counter to avoid - * racing with it. __blk_mq_update_nr_hw_queues() uses - * synchronize_rcu() to ensure this function left the critical section - * below. + * while the queue is frozen. Hold q_usage_counter to serialize + * __blk_mq_update_nr_hw_queues() against this function. */ if (!percpu_ref_tryget(&q->q_usage_counter)) return; - + if (atomic_read(&q->mq_freeze_depth)) + goto out; queue_for_each_hw_ctx(q, hctx, i) { struct blk_mq_tags *tags = hctx->tags; @@ -405,6 +404,7 @@ void blk_mq_queue_tag_busy_iter(struct request_queue *q, busy_iter_fn *fn, bt_for_each(hctx, &tags->breserved_tags, fn, priv, true); bt_for_each(hctx, &tags->bitmap_tags, fn, priv, false); } +out: blk_queue_exit(q); } diff --git a/block/blk-mq.c b/block/blk-mq.c index 652d0c6d5945..f9fc1536408d 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -3226,10 +3226,7 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, list_for_each_entry(q, &set->tag_list, tag_set_list) blk_mq_freeze_queue(q); - /* - * Sync with blk_mq_queue_tag_busy_iter. - */ - synchronize_rcu(); + /* * Switch IO scheduler to 'none', cleaning up the data associated * with the previous scheduler. We will switch back once we are done
Since the callback function called by blk_mq_queue_tag_busy_iter() may sleep calling synchronize_rcu() from __blk_mq_update_nr_hw_queues() is not sufficient to wait until blk_mq_queue_tag_busy_iter() has finished. Instead of making __blk_mq_update_nr_hw_queues() wait until q->q_usage_counter == 0 is globally visible, do not iterate over tags if the request queue is frozen. Cc: Christoph Hellwig <hch@infradead.org> Cc: Hannes Reinecke <hare@suse.com> Cc: James Smart <james.smart@broadcom.com> Cc: Ming Lei <ming.lei@redhat.com> Cc: Jianchao Wang <jianchao.w.wang@oracle.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Dongli Zhang <dongli.zhang@oracle.com> Cc: <stable@vger.kernel.org> Fixes: 530ca2c9bd69 ("blk-mq: Allow blocking queue tag iter callbacks") # v4.19. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- block/blk-mq-tag.c | 10 +++++----- block/blk-mq.c | 5 +---- 2 files changed, 6 insertions(+), 9 deletions(-)