From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f173.google.com (mail-pg1-f173.google.com [209.85.215.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 0EA4E2C190 for ; Sun, 16 Nov 2025 16:46:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.173 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763311572; cv=none; b=kdj1VbdLQlAt4W6ZMFieq5N7QBGrcTJkVYboCV/uPmxBmwRfRevoaV+oY/SmQapZrlBY0vXopFjYbNUbIxzuHTFrbYdFPxOGh3mUgB3Cpgbjh/O41v+xgEaCpt5XOsJWxe3ovhQhpxewpbwOmrdgTsIKWlzwq8PZq2W4dgWIoF4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763311572; c=relaxed/simple; bh=yfuoDhepLlty2cQq44Y6YuPK47EhADEcuShAQ2l0cjk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=SqPXGfezH7Spc2ewg2mzY6CGwpGR6oZo+U0k7G5LQnxebbHHtKvtV0az3Nl6ibfm1xrZnZENvegsKbDb0TWpX7HHa50m4N0lt+LaZ+zVRK35f1tdsbG23wiD26hH72GVqiKkbuBB8q2w/Fwfvyg8JR1tMgIiw11Ydv2ctpVPCv0= 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=JP4gMJd0; arc=none smtp.client-ip=209.85.215.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="JP4gMJd0" Received: by mail-pg1-f173.google.com with SMTP id 41be03b00d2f7-b553412a19bso2446875a12.1 for ; Sun, 16 Nov 2025 08:46:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1763311569; x=1763916369; 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=mzuMA0HqLv22fL5ouW8cGieDtH/YoU81WKUNJmbycJ0=; b=JP4gMJd0YI0dIQOFVkx9bo5k2J0wwsWasrBv0s25HTuMUtF12/wFwoPGboTtzoO2r+ LeEJCERv+lzi/CBwZ7lUPygLVKzQDStL/ocV3Uz4K4JjCLTqskjb6Yru9UxddYjxfh0B SATBXXooLRkgXo48dAq5lTCURUSvv0IcjZhyBRyAw9dC8BhpIOfhPxKiVgJ6LY1o5KR4 UEeIUP0VNf0PZnlPBLO+3jN9fVUmCGlSlhE1TaW/LlOHwTzO1khf3UFfSHZj+DAjGWn4 B1ESjJ4bZry44Omxb95LaO2urRctW87EADA4/uzQORG60KJEZjCALRn3BSX11mu6RZkg tvZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763311569; x=1763916369; 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=mzuMA0HqLv22fL5ouW8cGieDtH/YoU81WKUNJmbycJ0=; b=n7VZDFrXn/usoYkW14yufSjW0bGWiUhDM38djcL5Eb7B465yvf3ClQ7d4drkh0MjpG +8SWYs3LWXDqNoQDTJ1MUrtCrPJtO+w1kC0oXSDRx2QKOqbHXDGdqvZERMDanCeRZn1j gmuPCl3mUZKMlBHtPftxg/B2vk3Mc8lofSaiFk3NqQkvquPJ5UN3OnQTtyP8u0zCzDjw bkLmbezzFBE/5DChLDAxvrZjs26VgbmyedHS3VdgXaOE+vRBqHX60HqdeveGbH8CcpgF 2s/cJLX++AAmV6uFVKc54ts86xe65zrmPhzYEO00TFxwbU5MBh1FKjuFwk6eiO6v3oNm uT4w== X-Forwarded-Encrypted: i=1; AJvYcCXyAm7VV2iJV9J6cjURrFN+i6I8usWjRsyOZm+4RuESaKPNj8G+uR6mytRfDrQnU00TO5ncTKWHIYCQtG4=@vger.kernel.org X-Gm-Message-State: AOJu0YzSLtnvuk1EjvtfAeV6ZtVJ2ZxKVIqexDtc4STI4iyI3Z5H0Daj 1CkYoj27+7+ZDaP2AcuqYjBgkg9hPw0JDQ7BQrxVjo+nqZiuvIbrSZYzV0S+Am2jux1OFN3qPPS tZDC7 X-Gm-Gg: ASbGncuV8BP0NoBUp8MEJsEfBRBALDRo38dlD99Ru9zSrA3gmwR3MLONZt/CmrUjGZb ZF1kKJeIMRXjLHaygYw4EmsDoUnP6dJ2A6ClzO0McsbRo8Az4k6RJumbskMTFjS6mz99R+CnbV1 1POynxor/2pnB8tA6qxNr+MdTnQ0ivHxMeTVCDhxb7iotX9kJZFHTSqBgZ72/oR483G3E7chY3Z UcKe1DajaWBV1o7ut6CnRxrC0t2AtQCcJT6VUoGGs0DbHTbPW1LMry4m9RDfj7s5W7F0gEmg22m e4JN6whhQUbctS+1XeqtXxqphxOQjGR4y9hm0roIJ/mQp1+TZtLBw3vBK6Xl0KDSBT4q7Jf7QpD uae4qzyyvkQjefPoWcVfDZRUvXXlJ3Ci0sKTGEd0H8ZyKZrlAL+1wVlFzh8yxSPPxXubDtQsKTo OowxmsMv2gpHf6y6Q3NlMpnYbuCA8= X-Google-Smtp-Source: AGHT+IE5rrGbiwByjHLuKq7S9q5r3YznMONfkuzETCp/ON+jIpJvE4l43Yh/uyxNLb5/9bCltvKINQ== X-Received: by 2002:a05:7022:ff42:b0:11b:c1ab:bdd0 with SMTP id a92af1059eb24-11bc1abbed3mr1532024c88.35.1763311568872; Sun, 16 Nov 2025 08:46:08 -0800 (PST) Received: from medusa.lab.kspace.sh ([2601:640:8202:6fb0::f013]) by smtp.googlemail.com with UTF8SMTPSA id a92af1059eb24-11b060885e3sm37082181c88.0.2025.11.16.08.46.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 16 Nov 2025 08:46:08 -0800 (PST) Date: Sun, 16 Nov 2025 08:46:06 -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] nvme: Convert tag_list mutex to rwsemaphore to avoid deadlock Message-ID: <20251116164606.GA2376676-mkhalfella@purestorage.com> References: <20251113202320.2530531-1-mkhalfella@purestorage.com> <20251114173419.GA2197103-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 Sun 2025-11-16 23:15:38 +0800, Ming Lei wrote: > On Fri, Nov 14, 2025 at 09:34:19AM -0800, Mohamed Khalfella wrote: > > On Fri 2025-11-14 19:41:14 +0800, Ming Lei wrote: > > > On Thu, Nov 13, 2025 at 12:23:20PM -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 > > > > > > It is one AB-BA deadlock, lockdep should have complained it, but nvme doesn't > > > support owned freeze queue. > > > > > > Maybe the following change can avoid it? > > > > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > > > index c916176bd9f0..9967c4a7e72d 100644 > > > --- a/drivers/nvme/host/pci.c > > > +++ b/drivers/nvme/host/pci.c > > > @@ -3004,6 +3004,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) > > > bool dead; > > > > > > mutex_lock(&dev->shutdown_lock); > > > + nvme_quiesce_io_queues(&dev->ctrl); > > > dead = nvme_pci_ctrl_is_dead(dev); > > > if (state == NVME_CTRL_LIVE || state == NVME_CTRL_RESETTING) { > > > if (pci_is_enabled(pdev)) > > > @@ -3016,8 +3017,6 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) > > > nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT); > > > } > > > > > > - nvme_quiesce_io_queues(&dev->ctrl); > > > - > > > if (!dead && dev->ctrl.queue_count > 0) { > > > nvme_delete_io_queues(dev); > > > nvme_disable_ctrl(&dev->ctrl, shutdown); > > > > > > > > > > Interesting. Can you elaborate more on why this diff can help us avoid > > the problem? > > In nvme_dev_disable(), NS queues are frozen first, then call > nvme_quiesce_io_queues(). > > queue freeze can be thought as one lock, so q->freeze_lock -> tag_set->tag_list_lock > in timeout code path. > > However, in blk_mq_exit_queue(), the lock order becomes > tag_set->tag_list_lock -> q->freeze_lock. > > That is why I call it AB-BA lock. > > However, that looks not the reason in your deadlock, so my patch shouldn't > work here, in which nvme_dev_disable() doesn't provide forward-progress > because of ->tag_list_lock. > > > > > If the thread doing nvme_scan_work() is waiting for queue to be frozen > > while holding set->tag_list_lock, then nvme_quiesce_io_queues() moved up > > will cause the deadlock, no? > > __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 > > Here blk_mq_exit_queue() is called from disk release, and the disk is not > added actually, so question is why blk_mq_freeze_queue_wait() doesn't > return? Who holds queue usage counter here? Can you investigate a bit > and figure out the reason? The nvme controller has two namespaces. n1 was created with q1 added to the tagset. n2 was also created with q2 added to the tagset. Now the tagset is shared because it has two queues on it. Before the disk is added n2 was found to be duplicate to n1. That means the disk should be released, as noted above, and q2 should be removed from the tagset. Because n1 block device is ready to receive IO it is possible for q1 usage counter to be greater than zero. Back to q2 removal from the tagset. This requires q1 to be frozen before it can be marked as unshared. blk_mq_freeze_queue_wait() was called for q1 and it does not return because there is a request issued on the queue. nvme_timeout() was called for that request in q1. > > For one released NS/disk, no one should grab the queue usage counter, > right? Right. The disk is not visible yet. > > > Thanks, > Ming >