Messages in this thread |  | | Date | Fri, 5 Dec 2025 08:39:26 -0800 | | From | Mohamed Khalfella <> | | Subject | Re: [PATCH 1/1] block: Use RCU in blk_mq_[un]quiesce_tagset() instead of set->tag_list_lock |
| |
On Thu 2025-12-04 18:32:31 -0700, Keith Busch wrote: > On Thu, Dec 04, 2025 at 01:22:49PM -1000, Bart Van Assche wrote: > > > > On 12/4/25 11:26 AM, Keith Busch wrote: > > > On Thu, Dec 04, 2025 at 10:24:03AM -1000, Bart Van Assche wrote:>> Hence, the deadlock can be > > > > solved by removing the blk_mq_quiesce_tagset() call from nvme_timeout() > > > > and by failing I/O from inside nvme_timeout(). If nvme_timeout() fails > > > > I/O and does not call blk_mq_quiesce_tagset() then the > > > > blk_mq_freeze_queue_wait() call will finish instead of triggering a > > > > deadlock. However, I do not know whether this proposal seems acceptable > > > > to the NVMe maintainers. > > > > > > You periodically make this suggestion, but there's never a reason > > > offered to introduce yet another work queue for the driver to > > > synchronize with at various points. The whole point of making blk-mq > > > timeout handler in a work queue (it used to be a timer) was so that we > > > could do blocking actions like this. > > Hi Keith, > > > > The blk_mq_quiesce_tagset() call from the NVMe timeout handler is > > unfortunate because it triggers a deadlock with > > blk_mq_update_tag_set_shared(). > > So in this scenario, the driver is modifying a tagset list from two > queues to one, which causes blk-mq to clear the "shared" flags. The > remaining one just so happens to have hit a timeout at the same time, > which runs in a context with an elevated "q_usage_counter". The current > rule, then, is you can not take the tag_list_lock from any context using > any queue in the tag list. > > > I proposed to modify the NVMe driver because I think that's a better > > approach than introducing a new synchronize_rcu() call in the block > > layer core. > > I'm not interested in introducing rcu synchronize here either. I guess I > would make it so you can quiesce a tagset from a context that entered > the queue. So quick shot at that here:
Why sychronize_rcu() is intolerable in this blk_mq_del_queue_tag_set()? This code is not performance sensitive, right?
Looking at the code again, I _think_ synchronize_rcu() along with INIT_LIST_HEAD(&q->tag_set_list) can be deleted. I do not see usecase where "q" is re-added to a tagset after it is deleted from one. Also, "q" is freed in blk_free_queue() after end of RCU grace period.
Are there any other concerns with this approach other than synchronize_rcu()? If not, I will delete it and submit v3.
> > --- > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 4e96bb2462475..20450017b9512 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -4262,11 +4262,16 @@ static void blk_mq_map_swqueue(struct request_queue *q) > * Caller needs to ensure that we're either frozen/quiesced, or that > * the queue isn't live yet. > */ > -static void queue_set_hctx_shared(struct request_queue *q, bool shared) > +static void queue_set_hctx_shared_locked(struct request_queue *q) > { > + struct blk_mq_tag_set *set = q->tag_set; > struct blk_mq_hw_ctx *hctx; > unsigned long i; > + bool shared; > + > + lockdep_assert_held(&set->tag_list_lock); > > + shared = set->flags & BLK_MQ_F_TAG_QUEUE_SHARED; > queue_for_each_hw_ctx(q, hctx, i) { > if (shared) { > hctx->flags |= BLK_MQ_F_TAG_QUEUE_SHARED; > @@ -4277,24 +4282,22 @@ static void queue_set_hctx_shared(struct request_queue *q, bool shared) > } > } > > -static void blk_mq_update_tag_set_shared(struct blk_mq_tag_set *set, > - bool shared) > +static void queue_set_hctx_shared(struct request_queue *q) > { > - struct request_queue *q; > + struct blk_mq_tag_set *set = q->tag_set; > unsigned int memflags; > > - lockdep_assert_held(&set->tag_list_lock); > - > - list_for_each_entry(q, &set->tag_list, tag_set_list) { > - memflags = blk_mq_freeze_queue(q); > - queue_set_hctx_shared(q, shared); > - blk_mq_unfreeze_queue(q, memflags); > - } > + memflags = blk_mq_freeze_queue(q); > + mutex_lock(&set->tag_list_lock); > + queue_set_hctx_shared_locked(q); > + mutex_unlock(&set->tag_list_lock); > + blk_mq_unfreeze_queue(q, memflags); > } > > static void blk_mq_del_queue_tag_set(struct request_queue *q) > { > struct blk_mq_tag_set *set = q->tag_set; > + struct request_queue *shared = NULL; > > mutex_lock(&set->tag_list_lock); > list_del(&q->tag_set_list); > @@ -4302,15 +4305,25 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q) > /* just transitioned to unshared */ > set->flags &= ~BLK_MQ_F_TAG_QUEUE_SHARED; > /* update existing queue */ > - blk_mq_update_tag_set_shared(set, false); > + shared = list_first_entry(&set->tag_list, struct request_queue, > + tag_set_list); > + if (!blk_get_queue(shared)) > + shared = NULL; > } > mutex_unlock(&set->tag_list_lock); > INIT_LIST_HEAD(&q->tag_set_list); > + > + if (shared) { > + queue_set_hctx_shared(shared); > + blk_put_queue(shared); > + } > } > > static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set, > struct request_queue *q) > { > + struct request_queue *shared = NULL; > + > mutex_lock(&set->tag_list_lock); > > /* > @@ -4318,15 +4331,24 @@ static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set, > */ > if (!list_empty(&set->tag_list) && > !(set->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) { > - set->flags |= BLK_MQ_F_TAG_QUEUE_SHARED; > /* update existing queue */ > - blk_mq_update_tag_set_shared(set, true); > + shared = list_first_entry(&set->tag_list, struct request_queue, > + tag_set_list); > + if (!blk_get_queue(shared)) > + shared = NULL; > + else > + set->flags |= BLK_MQ_F_TAG_QUEUE_SHARED; > } > if (set->flags & BLK_MQ_F_TAG_QUEUE_SHARED) > - queue_set_hctx_shared(q, true); > + queue_set_hctx_shared_locked(q); > list_add_tail(&q->tag_set_list, &set->tag_list); > > mutex_unlock(&set->tag_list_lock); > + > + if (shared) { > + queue_set_hctx_shared(shared); > + blk_put_queue(shared); > + } > } > > /* All allocations will be freed in release handler of q->mq_kobj */ > --
__blk_mq_update_nr_hw_queues() freezes the queues while holding set->tag_list_lock. Can this cause the same deadlock?
|  |