Re: [PATCH 01/23] nvmet: add context annotation for nvmet_ns::bdev

[Nilay Shroff <nilay@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

On 11/10/26 10:59 PM, Bart Van Assche wrote:
> On 11/9/26 10:38 PM, Nilay Shroff wrote:
>> I agree that passing the lock owner as a function argument is preferable whenever
>> possible. For this particular case, I tried passing subsys down the call chain to
>> nvmet_ns_dev() and annotating the function with __must_hold(&subsys-  >lock). However,
>> Clang still reports a locking violation when accessing ns->bdev:
> 
> Please take a look at the attached patches and consider to add these to
> the start of this series. These patches are needed to implement what I
> suggested in my previous email in this email thread.
> 
I reviewed the patches you attached and tried implementing your suggestion by passing
the struct nvmet_subsys pointer down the call chain to nvmet_ns_dev(), which accesses
ns->bdev protected by subsys->lock.

The relevant code looks like this:

struct nvmet_subsys {
     ...
     struct mutex lock;
     ...
}

struct nvmet_ns {
     ...
     struct block_device	*bdev __guarded_by(&subsys->lock);
     ...
     struct nvmet_subsys	*subsys;
     ...
}

static inline struct device *nvmet_ns_dev(struct nvmet_subsys *subsys,
		struct nvmet_ns *ns)
	__must_hold(&subsys->lock)
{
	return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
}

Attached the complete patch for your reference. Even after making
this change, clang still complains about locking violation:

drivers/nvme/target/nvmet.h:255:13: error: reading variable 'bdev' requires holding mutex 'ns->subsys->lock' [-Werror,-Wthread-safety-precise]
   255 |         return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
       |                    ^
drivers/nvme/target/nvmet.h:255:13: note: found near match 'subsys->lock'

 From this error, it appears that the Clang context analyzer expects accesses
to ns->bdev to be protected specifically by ns->subsys->lock. It recognizes
subsys->lock as a near match but cannot establish that both expressions refer
to the same mutex.

Therefore, passing the subsys pointer down to nvmet_ns_dev() and annotating the
function with __must_hold(&subsys->lock) does not resolve the warning in this case.

I also looked at the approach in your proposed patches for the NVMe host driver.
There, passing the struct nvme_subsystem pointer down the call chain works because
the list being protected, nsheads, is itself a member of struct nvme_subsystem:


struct nvme_subsystem {
     ....
     struct mutex lock;
     struct list_head nsheads __guarded_by(&lock);
     ....
}

In this case, the analyzer can associate accesses to subsys->nsheads directly with
subsys->lock, allowing __must_hold(&subsys->lock) to express the locking requirement
correctly.

The situation is different for nvmet_ns::bdev, where the lock protecting the field
belongs to the namespace's subsystem which is a separate struct nvmet_subsys. It seems
the analyzer cannot establish the equivalence between subsys->lock and ns->subsys->lock,
even when both pointers refer to the same subsystem.

Regarding your patches, I'm happy to include them in my series. However, my current
series focuses on adding context annotations to the NVMe target driver, whereas your
proposed patches address the NVMe host driver. So I'd suggest posting your changes as
a separate series so they can be reviewed independently.

Thanks,
--Nilay

[-- Attachment #2: nvmet-ns-bdev.diff --]
[-- Type: text/x-patch, Size: 5288 bytes --]

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index c9837263898a..982749efa6e1 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -490,8 +490,9 @@ static void nvmet_ns_dev_disable(struct nvmet_ns *ns)
 	nvmet_file_ns_disable(ns);
 }
 
-static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
-	__must_hold(&ns->subsys->lock)
+static int nvmet_p2pmem_ns_enable(struct nvmet_subsys *subsys,
+		struct nvmet_ns *ns)
+	__must_hold(&subsys->lock)
 {
 	int ret;
 	struct pci_dev *p2p_dev;
@@ -511,7 +512,8 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 	}
 
 	if (ns->p2p_dev) {
-		ret = pci_p2pdma_distance(ns->p2p_dev, nvmet_ns_dev(ns), true);
+		ret = pci_p2pdma_distance(ns->p2p_dev, nvmet_ns_dev(subsys, ns),
+				true);
 		if (ret < 0)
 			return -EINVAL;
 	} else {
@@ -522,7 +524,7 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 		 * setup the controller when the port's device is available.
 		 */
 
-		p2p_dev = pci_p2pmem_find(nvmet_ns_dev(ns));
+		p2p_dev = pci_p2pmem_find(nvmet_ns_dev(subsys, ns));
 		if (!p2p_dev) {
 			pr_err("no peer-to-peer memory is available for %s\n",
 			       ns->device_path);
@@ -535,21 +537,16 @@ static int nvmet_p2pmem_ns_enable(struct nvmet_ns *ns)
 	return 0;
 }
 
-static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
+static void nvmet_p2pmem_ns_add_p2p(struct nvmet_subsys *subsys,
+				    struct nvmet_ctrl *ctrl,
 				    struct nvmet_ns *ns)
-	__must_hold(&ctrl->subsys->lock)
+	__must_hold(&subsys->lock)
 {
 	struct device *clients[2];
 	struct pci_dev *p2p_dev;
 	int ret;
 
-	lockdep_assert_held(&ctrl->subsys->lock);
-	/*
-	 * Both ctrl->subsys->lock and ns->subsys->lock refers to the same lock.
-	 * However there's no way for us to annotate such locking semantics
-	 * using clang context annotation. So explicitly assert the capability.
-	 */
-	__assume_ctx_lock(&ns->subsys->lock);
+	lockdep_assert_held(&subsys->lock);
 
 	if (!ctrl->p2p_client || !ns->use_p2pmem)
 		return;
@@ -562,7 +559,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
 		p2p_dev = pci_dev_get(ns->p2p_dev);
 	} else {
 		clients[0] = ctrl->p2p_client;
-		clients[1] = nvmet_ns_dev(ns);
+		clients[1] = nvmet_ns_dev(subsys, ns);
 
 		p2p_dev = pci_p2pmem_find_many(clients, ARRAY_SIZE(clients));
 		if (!p2p_dev) {
@@ -620,20 +617,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 	if (ret)
 		goto out_unlock;
 
-	ret = nvmet_p2pmem_ns_enable(ns);
+	ret = nvmet_p2pmem_ns_enable(subsys, ns);
 	if (ret)
 		goto out_dev_disable;
 
-	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
-		/*
-		 * Both subsys->lock and ctrl->subsys->lock refers to the same
-		 * lock. However there's no way for us to annotate such locking
-		 * semantics using clang context annotation. So explicitly
-		 * assert the capability.
-		 */
-		__assume_ctx_lock(&ctrl->subsys->lock);
-		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
-	}
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+		nvmet_p2pmem_ns_add_p2p(subsys, ctrl, ns);
 
 	if (ns->pr.enable) {
 		ret = nvmet_pr_init_ns(ns);
@@ -1591,14 +1580,15 @@ bool nvmet_host_allowed(struct nvmet_subsys *subsys, const char *hostnqn)
 	return false;
 }
 
-static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
-		struct device *p2p_client)
-	__must_hold(&ctrl->subsys->lock)
+static void nvmet_setup_p2p_ns_map(struct nvmet_subsys *subsys,
+				   struct nvmet_ctrl *ctrl,
+				   struct device *p2p_client)
+	__must_hold(&subsys->lock)
 {
 	struct nvmet_ns *ns;
 	unsigned long idx;
 
-	lockdep_assert_held(&ctrl->subsys->lock);
+	lockdep_assert_held(&subsys->lock);
 
 	if (!p2p_client)
 		return;
@@ -1606,7 +1596,7 @@ static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
 	ctrl->p2p_client = get_device(p2p_client);
 
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns)
-		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
+		nvmet_p2pmem_ns_add_p2p(subsys, ctrl, ns);
 }
 
 static void nvmet_release_p2p_ns_map(struct nvmet_ctrl *ctrl)
@@ -1752,13 +1742,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 		goto init_pr_fail;
 	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
 
-	/*
-	 * Both subsys->lock and ctrl->subsys->lock refers to the same lock.
-	 * However there's no way for us to annotate such locking semantics
-	 * using clang context annotation. So explicitly assert the capability.
-	 */
-	__assume_ctx_lock(&ctrl->subsys->lock);
-	nvmet_setup_p2p_ns_map(ctrl, args->p2p_client);
+	nvmet_setup_p2p_ns_map(subsys, ctrl, args->p2p_client);
 	nvmet_debugfs_ctrl_setup(ctrl);
 	mutex_unlock(&subsys->lock);
 	up_read(&nvmet_config_sem);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 41912877ade3..270cb112e702 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -207,8 +207,9 @@ static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)
 	return container_of(to_config_group(item), struct nvmet_ns, group);
 }
 
-static inline struct device *nvmet_ns_dev(struct nvmet_ns *ns)
-	__must_hold(&ns->subsys->lock)
+static inline struct device *nvmet_ns_dev(struct nvmet_subsys *subsys,
+		struct nvmet_ns *ns)
+	__must_hold(&subsys->lock)
 {
 	return ns->bdev ? disk_to_dev(ns->bdev->bd_disk) : NULL;
 }