From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 39D4C2BEC2E for ; Mon, 24 Nov 2025 17:42:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.173 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764006136; cv=none; b=ModFA+lwiBzNmcpxy+Y3L/UqtHEcSN0eqokBzWYR0NFWFQICqd2Pngcdtd+H7RJqElHvdPoIPKNL2YgvD49oYWpw7LLj3vAQXASO1GGlrOJRLpToiXqhuzYqtFI7eRHLfiQrGsu79N3C2Gcp1i5uKqkNrRG+dKZ+CdhG38wORkM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764006136; c=relaxed/simple; bh=Ba1wp77lCda3xImzlfWQFaJinBMlzcqwOuM1hd6v0v0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HCwWH26MKQlyw3uWfppUnxHjC20KJ18lVjoJH/HxHTBf+ai3RfP1F40n02uTTLJ+dT91Q7H5yyyAT2fDC4BP3NsMl/rpxhY/uCrjnp9VcZ8wb7vp64IDjKR/a3fz+ifsKCO6Ou2VVlX8JYq4srqNA5uXp9Qs8kDxRqUCt3IP9HM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com; spf=fail smtp.mailfrom=purestorage.com; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b=d3yOvq+y; arc=none smtp.client-ip=209.85.214.173 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=purestorage.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b="d3yOvq+y" Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-29ba9249e9dso594445ad.3 for ; Mon, 24 Nov 2025 09:42:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1764006133; x=1764610933; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=WppdyCSciz9HWrs/hR0oDV7k3a6Trk+vQz3os79KrFw=; b=d3yOvq+ySrZNxOT10H1UZj6Oc46Gm8+AIposbjuhRjdAHwlO4qavYZYjCkPe1Al1d1 zH1vguTuqnjcysQC09PgxfgX3B6HFY2BPJhVi0r36g8CvC+up9XUPq/U2Hw5bsM4rfsm 6VDTjUADT9TtjmsEcfxlJ4WD+vM50kuVU9OyOhmNEmfoIPGy39j3/kHt6oiBsGm0/xi/ YYGdKRl9enfiSsWj9F0LbppsFJKOemZK7x3yQ5g6wM3VpEt+1G/iK1WZBYQF4olhb1Cl yO2hTsEYGT464UUbUYNTRILc0NFPjbhxoR+uggpbqbVhw5MS1MFCES64vJhmdce5N87z jqSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764006133; x=1764610933; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WppdyCSciz9HWrs/hR0oDV7k3a6Trk+vQz3os79KrFw=; b=mJ2yJkDU3zQ8UshwfGUHkTkMTayVH6r/DA3KmuViczLe/3haAXaZpnCsbLfKYAzfUB 3iHbqQPmTv4iq8tdYpiKwugAFlN7JyEDxKZ4evdhg5Lkf7WxtiFHOhYtjDNh3ikroMvI mMFyYLezW3k5YqcgW3vJNBsVqHg6iiVx2jqs8h3+Ig9+M+hqIqGROSJU0t/GtmRmYgmm U/4Jz6mHnEEuJqWoLKuRaJVqx+4tcP6RCFnNQdyLGq2NFpb56140DLdkA4ud9nKVi31g bFbaaUz5o6w/WIbY7INwtehaQHCTaP+vRcAHM2eS/22wz30JvRkt0tdD9b+IJ3Z/palg tcnQ== X-Forwarded-Encrypted: i=1; AJvYcCWBfZeWdoMUXdcKfzY7hu7OTFYfRwYxC8os3KfdtAfLiIrj5mlyZPyDV3rO8gmkjP6gmH6wU38NFNEmWv4=@vger.kernel.org X-Gm-Message-State: AOJu0YwLt8k0DcjWAiFWlISW0J5TTUrn6+NFKD3BpzZCEttG0Jphliuu mAURlsVYkRYfsP+LVzneloB0UPLBsjHVXD+84PbSHfUnckQlxY3IOVH2q/FddCpxnos= X-Gm-Gg: ASbGncvJ/0ZPfgq4GQYCld7duwibwDFtR5xzlv21cWWjjyZknhmVWY78ptIiUB+wDjL ERJYWvLrH4gyAQCWIjBhIV7ybOwqDqsHsoK9uX2X9B0NnaDg18MzuX05PhQ21xv48BRYtCumEd0 p+WhZHKYqadyPUrwPwpKF9ZkQgcfcduILxY8jMQq7S4zkxPrBkb4qExTN8TUywxP6yJNqBWEEvA T8Jkjppq8YyXF5AoXX3fjayzZ62H1ZUaFxBpQ5eI7CMILZFbwAem46tW9dTYOgEmZlfasZVLwX7 tq9OoxM8z09/LaknQPV+aA90HaTbKjgY/3sEpmV7X22/dmzraKmytHh4gpdzqLIPAm30QHpN/Te DIwp6EW2e1/HbxK8h0FbypjZSaSmBiuJGN0fU3imLVusKfELwct2UqRWjIgBC6VUHIx3LAFonvx 3Qb2h8hh97ENRl3c6wykopkbEVwRuY2eg= X-Google-Smtp-Source: AGHT+IG5QYOBjAVVFMK45Nvn6OfeThSH9HuJ61Oz7/6lURBggYPzYHBApxq+nzxrdnDicnCGqIRYGA== X-Received: by 2002:a05:7300:320b:b0:2a4:5028:3433 with SMTP id 5a478bee46e88-2a719fb6c72mr8386858eec.34.1764006133064; Mon, 24 Nov 2025 09:42:13 -0800 (PST) Received: from medusa.lab.kspace.sh ([208.88.152.253]) by smtp.googlemail.com with UTF8SMTPSA id 5a478bee46e88-2a6fc3d0bb6sm75866424eec.2.2025.11.24.09.42.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Nov 2025 09:42:12 -0800 (PST) Date: Mon, 24 Nov 2025 09:42:11 -0800 From: Mohamed Khalfella To: Ming Lei Cc: Jens Axboe , Keith Busch , Sagi Grimberg , Chaitanya Kulkarni , Casey Chen , Vikas Manocha , Yuanyuan Zhong , Hannes Reinecke , linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/1] nvme: Convert tag_list mutex to rwsemaphore to avoid deadlock Message-ID: <20251124174211.GQ337106-mkhalfella@purestorage.com> References: <20251117202414.4071380-1-mkhalfella@purestorage.com> <20251117202414.4071380-2-mkhalfella@purestorage.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Mon 2025-11-24 12:00:15 +0800, Ming Lei wrote: > On Mon, Nov 17, 2025 at 12:23:53PM -0800, Mohamed Khalfella wrote: > > blk_mq_{add,del}_queue_tag_set() functions add and remove queues from > > tagset, the functions make sure that tagset and queues are marked as > > shared when two or more queues are attached to the same tagset. > > Initially a tagset starts as unshared and when the number of added > > queues reaches two, blk_mq_add_queue_tag_set() marks it as shared along > > with all the queues attached to it. When the number of attached queues > > drops to 1 blk_mq_del_queue_tag_set() need to mark both the tagset and > > the remaining queues as unshared. > > > > Both functions need to freeze current queues in tagset before setting on > > unsetting BLK_MQ_F_TAG_QUEUE_SHARED flag. While doing so, both functions > > hold set->tag_list_lock mutex, which makes sense as we do not want > > queues to be added or deleted in the process. This used to work fine > > until commit 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset") > > made the nvme driver quiesce tagset instead of quiscing individual > > queues. blk_mq_quiesce_tagset() does the job and quiesce the queues in > > set->tag_list while holding set->tag_list_lock also. > > > > This results in deadlock between two threads with these stacktraces: > > > > __schedule+0x48e/0xed0 > > schedule+0x5a/0xc0 > > schedule_preempt_disabled+0x11/0x20 > > __mutex_lock.constprop.0+0x3cc/0x760 > > blk_mq_quiesce_tagset+0x26/0xd0 > > nvme_dev_disable_locked+0x77/0x280 [nvme] > > nvme_timeout+0x268/0x320 [nvme] > > blk_mq_handle_expired+0x5d/0x90 > > bt_iter+0x7e/0x90 > > blk_mq_queue_tag_busy_iter+0x2b2/0x590 > > ? __blk_mq_complete_request_remote+0x10/0x10 > > ? __blk_mq_complete_request_remote+0x10/0x10 > > blk_mq_timeout_work+0x15b/0x1a0 > > process_one_work+0x133/0x2f0 > > ? mod_delayed_work_on+0x90/0x90 > > worker_thread+0x2ec/0x400 > > ? mod_delayed_work_on+0x90/0x90 > > kthread+0xe2/0x110 > > ? kthread_complete_and_exit+0x20/0x20 > > ret_from_fork+0x2d/0x50 > > ? kthread_complete_and_exit+0x20/0x20 > > ret_from_fork_asm+0x11/0x20 > > > > __schedule+0x48e/0xed0 > > schedule+0x5a/0xc0 > > blk_mq_freeze_queue_wait+0x62/0x90 > > ? destroy_sched_domains_rcu+0x30/0x30 > > blk_mq_exit_queue+0x151/0x180 > > disk_release+0xe3/0xf0 > > device_release+0x31/0x90 > > kobject_put+0x6d/0x180 > > nvme_scan_ns+0x858/0xc90 [nvme_core] > > ? nvme_scan_work+0x281/0x560 [nvme_core] > > nvme_scan_work+0x281/0x560 [nvme_core] > > process_one_work+0x133/0x2f0 > > ? mod_delayed_work_on+0x90/0x90 > > worker_thread+0x2ec/0x400 > > ? mod_delayed_work_on+0x90/0x90 > > kthread+0xe2/0x110 > > ? kthread_complete_and_exit+0x20/0x20 > > ret_from_fork+0x2d/0x50 > > ? kthread_complete_and_exit+0x20/0x20 > > ret_from_fork_asm+0x11/0x20 > > > > The top stacktrace is showing nvme_timeout() called to handle nvme > > command timeout. timeout handler is trying to disable the controller and > > as a first step, it needs to blk_mq_quiesce_tagset() to tell blk-mq not > > to call queue callback handlers. The thread is stuck waiting for > > set->tag_list_lock as it tires to walk the queues in set->tag_list. > > > > The lock is held by the second thread in the bottom stack which is > > waiting for one of queues to be frozen. The queue usage counter will > > drop to zero after nvme_timeout() finishes, and this will not happen > > because the thread will wait for this mutex forever. > > > > Convert set->tag_list_lock mutex to set->tag_list_rwsem rwsemaphore to > > avoid the deadlock. Update blk_mq_[un]quiesce_tagset() to take the > > semaphore for read since this is enough to guarantee no queues will be > > added or removed. Update blk_mq_{add,del}_queue_tag_set() to take the > > semaphore for write while updating set->tag_list and downgrade it to > > read while freezing the queues. It should be safe to update set->flags > > and hctx->flags while holding the semaphore for read since the queues > > are already frozen. > > > > Fixes: 98d81f0df70c ("nvme: use blk_mq_[un]quiesce_tagset") > > Signed-off-by: Mohamed Khalfella > > Reviewed-by: Ming Lei > Sorry, I was supposed to reply to this thread eariler. The concern raised about potential deadlock in set->tag_list_rwsem caused by writer blocking readers makes this approach buggy. The way I understood it is that rw_semaphore have this writer starvation prevention mechanism. If a writer is waiting for the semaphore to be available then readers that come after the waiting writer will not be able to take the semphore. Even if it is available for reader. If we rely on the readers to do something to make the semaphore available for the waiting writer then this is a deadlock. This change relies on the reader to cancel inflight requests so that queue usage counter drops to zero and queue is fully frozen. Only then semphore will be available for the waiting writer. This results in a deadlock between three threads. To put it in another way blk_mq_del_queue_tag_set() downgrades the semaphore and waits for the queue to be frozen. If another call to blk_mq_del_queue_tag_set() happens from another thread, before blk_mq_quiesce_tagset() comes in, it will cause a deadlock. The second call to blk_mq_del_queue_tag_set() is a writer and it will wait until the semaphore is available. blk_mq_quiesce_tagset() is a reader that comes after a waiting writer. Eventhough the semaphore is available for readers blk_mq_quiesce_tagset() will not be able to take it because of the writer starvation prevention mechanism. The first thread that is waiting for queue to be frozen in blk_mq_del_queue_tag_set() will not be able to make progress because of inflight requests. The second writer thread waiting for the semphore on blk_mq_del_queue_tag_set() will not be able to make progress because the semaphore is not availble. The thread calling blk_mq_quiesce_tagset() will not be able to make progress because it is blocked behind the writer (second thread). Commit 4e893ca81170 ("nvme_core: scan namespaces asynchronously") makes this scenario more likely to happen. If a controller has a namespace that is duplicate three times then it is possible to hit this deadlock. I was thinking about use RCU to protect set->tag_list but never had a chance to write the code and test it. I hope I will find time in the coming few days. Thanks, Mohamed Khalfella