[PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[amit.engel]

From: Amit Engel <amit.engel@dell.com>

Alloc newhost only if no host has been found in host_list
Add a missing tgtport release reference

Signed-off-by: Amit Engel <amit.engel@dell.com>
---
 drivers/nvme/target/fc.c | 45 +++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index d375745fc4ed..debee48080d8 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1034,28 +1034,6 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 	if (!nvmet_fc_tgtport_get(tgtport))
 		return ERR_PTR(-EINVAL);
 
-	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
-	if (!newhost) {
-		spin_lock_irqsave(&tgtport->lock, flags);
-		list_for_each_entry(host, &tgtport->host_list, host_list) {
-			if (host->hosthandle == hosthandle && !host->invalid) {
-				if (nvmet_fc_hostport_get(host)) {
-					match = host;
-					break;
-				}
-			}
-		}
-		spin_unlock_irqrestore(&tgtport->lock, flags);
-		/* no allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
-		return (match) ? match : ERR_PTR(-ENOMEM);
-	}
-
-	newhost->tgtport = tgtport;
-	newhost->hosthandle = hosthandle;
-	INIT_LIST_HEAD(&newhost->host_list);
-	kref_init(&newhost->ref);
-
 	spin_lock_irqsave(&tgtport->lock, flags);
 	list_for_each_entry(host, &tgtport->host_list, host_list) {
 		if (host->hosthandle == hosthandle && !host->invalid) {
@@ -1065,13 +1043,24 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 			}
 		}
 	}
-	if (match) {
-		kfree(newhost);
-		newhost = NULL;
-		/* releasing allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
-	} else
+	if (!match) {
+		newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
+		if (!newhost) {
+			/* no allocation - release reference */
+			nvmet_fc_tgtport_put(tgtport);
+			spin_unlock_irqrestore(&tgtport->lock, flags);
+			return ERR_PTR(-ENOMEM);
+		}
+
+		newhost->tgtport = tgtport;
+		newhost->hosthandle = hosthandle;
+		INIT_LIST_HEAD(&newhost->host_list);
+		kref_init(&newhost->ref);
+
 		list_add_tail(&newhost->host_list, &tgtport->host_list);
+	}
+	/* release reference */
+	nvmet_fc_tgtport_put(tgtport);
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 
 	return (match) ? match : newhost;
-- 
2.18.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[Chaitanya Kulkarni]

On 3/3/21 08:43, amit.engel@dell.com wrote:
> From: Amit Engel <amit.engel@dell.com>
>
> Alloc newhost only if no host has been found in host_list
> Add a missing tgtport release reference
>
> Signed-off-by: Amit Engel <amit.engel@dell.com>
You need to CC James on this patch.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[James Smart]

On 3/6/2021 11:26 AM, James Smart wrote:
> On 3/4/2021 12:12 AM, amit.engel@dell.com wrote:
>> From: Amit Engel <amit.engel@dell.com>
>>
>> Alloc newhost only if no host has been found in host_list
>> Add a missing tgtport release reference
>>
>> Signed-off-by: Amit Engel <amit.engel@dell.com>
> 
...

I looked at this again for the missing reference and as to why you may 
have wanted to rework this.

I didn't see anything relative to tgtport reference.  There is to be a 
tgtport reference relative to the hostport struct, which is released 
when the hostport is freed.

If the issue is many allocations that are then thrown away (the normal 
case once created), then I recommend you rework your patch to release 
the lock after not finding a match, doing the allocation, then relock 
and re-search, if not found add, if found free the alloc. I'm ok with that.

-- james


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[James Smart]

On 3/4/2021 12:12 AM, amit.engel@dell.com wrote:
> From: Amit Engel <amit.engel@dell.com>
> 
> Alloc newhost only if no host has been found in host_list
> Add a missing tgtport release reference
> 
> Signed-off-by: Amit Engel <amit.engel@dell.com>

I don't see a reason for this change...   The whole point of allocating 
first was to ensure you had memory if there was dynamic change of the 
host_list while waiting for memory to be alloc'd. Also not nice to be 
holding the lock while allocating.

It's very possible for there to be other requests for possibly the same 
remote port as well as other remote ports on the hostport as LS requests 
are receive and then posted as work items, allowing them to be run in 
parallel.

Is there a real issue to be resolved ?

-- james


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvmet-fc: simplify and fix nvmet_fc_alloc_hostport

[amit.engel]

From: Amit Engel <amit.engel@dell.com>

Alloc newhost only if no host has been found in host_list
Add a missing tgtport release reference

Signed-off-by: Amit Engel <amit.engel@dell.com>
---
 drivers/nvme/target/fc.c | 45 +++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index d375745fc4ed..debee48080d8 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1034,28 +1034,6 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 	if (!nvmet_fc_tgtport_get(tgtport))
 		return ERR_PTR(-EINVAL);
 
-	newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
-	if (!newhost) {
-		spin_lock_irqsave(&tgtport->lock, flags);
-		list_for_each_entry(host, &tgtport->host_list, host_list) {
-			if (host->hosthandle == hosthandle && !host->invalid) {
-				if (nvmet_fc_hostport_get(host)) {
-					match = host;
-					break;
-				}
-			}
-		}
-		spin_unlock_irqrestore(&tgtport->lock, flags);
-		/* no allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
-		return (match) ? match : ERR_PTR(-ENOMEM);
-	}
-
-	newhost->tgtport = tgtport;
-	newhost->hosthandle = hosthandle;
-	INIT_LIST_HEAD(&newhost->host_list);
-	kref_init(&newhost->ref);
-
 	spin_lock_irqsave(&tgtport->lock, flags);
 	list_for_each_entry(host, &tgtport->host_list, host_list) {
 		if (host->hosthandle == hosthandle && !host->invalid) {
@@ -1065,13 +1043,24 @@ nvmet_fc_alloc_hostport(struct nvmet_fc_tgtport *tgtport, void *hosthandle)
 			}
 		}
 	}
-	if (match) {
-		kfree(newhost);
-		newhost = NULL;
-		/* releasing allocation - release reference */
-		nvmet_fc_tgtport_put(tgtport);
-	} else
+	if (!match) {
+		newhost = kzalloc(sizeof(*newhost), GFP_KERNEL);
+		if (!newhost) {
+			/* no allocation - release reference */
+			nvmet_fc_tgtport_put(tgtport);
+			spin_unlock_irqrestore(&tgtport->lock, flags);
+			return ERR_PTR(-ENOMEM);
+		}
+
+		newhost->tgtport = tgtport;
+		newhost->hosthandle = hosthandle;
+		INIT_LIST_HEAD(&newhost->host_list);
+		kref_init(&newhost->ref);
+
 		list_add_tail(&newhost->host_list, &tgtport->host_list);
+	}
+	/* release reference */
+	nvmet_fc_tgtport_put(tgtport);
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 
 	return (match) ? match : newhost;
-- 
2.18.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme