Skip to contentSkip to conversation

nvme_fc: fix unregistrations and stale resume

PATCH New James Smart 4 participants 6 messages Oct 27, 2017 – Nov 3, 2017

Subsystems (2)
NVMe NVMe FC transport
lore
Share & tools
lore

James Smart

author
#0 lore raw

cover nvme_fc: fix unregistrations and stale resume

These patches fix some errors in the nvme_fc_register_localport resume as well as reworking remoteport and localport unregister behavior relative to their callbacks to the lldd.

James Smart (2):
nvme_fc: fix localport resume using stale values
nvme_fc: decouple ns references from lldd references

 drivers/nvme/host/fc.c | 96 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 88 insertions(+), 8 deletions(-)
Signature · 1 line
2.13.1
author
#1 lore raw

PATCH 1/2 nvme_fc: fix localport resume using stale values

The localport resume was not updating the lldd ops structure. If the lldd is unloaded and reloaded, the ops pointers will differ.

Additionally, as there are device references taken by the localport, ensure that resume only resumes if the device matches as well.

Signed-off-by James Smart <james.smart at broadcom.com>
---
 drivers/nvme/host/fc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/nvme/host/fc.c +10 -2
index 200c1b867c0d..e2054c90b42d 100644
@@ -270,7 +270,9 @@ nvme_fc_lport_get(struct nvme_fc_lport *lport)
 
 
 static struct nvme_fc_lport *
-nvme_fc_attach_to_unreg_lport(struct nvme_fc_port_info *pinfo)
+nvme_fc_attach_to_unreg_lport(struct nvme_fc_port_info *pinfo,
+			struct nvme_fc_port_template *ops,
+			struct device *dev)
 {
 	struct nvme_fc_lport *lport;
 	unsigned long flags;

@@ -282,6 +284,11 @@ nvme_fc_attach_to_unreg_lport(struct nvme_fc_port_info *pinfo)
 		    lport->localport.port_name != pinfo->port_name)
 			continue;
 
+		if (lport->dev != dev) {
+			lport = ERR_PTR(-EXDEV);
+			goto out_done;
+		}
+
 		if (lport->localport.port_state != FC_OBJSTATE_DELETED) {
 			lport = ERR_PTR(-EEXIST);
 			goto out_done;

@@ -298,6 +305,7 @@ nvme_fc_attach_to_unreg_lport(struct nvme_fc_port_info *pinfo)
 
 		/* resume the lport */
 
+		lport->ops = ops;
 		lport->localport.port_role = pinfo->port_role;
 		lport->localport.port_id = pinfo->port_id;
 		lport->localport.port_state = FC_OBJSTATE_ONLINE;

@@ -358,7 +366,7 @@ nvme_fc_register_localport(struct nvme_fc_port_info *pinfo,
 	 * expired, we can simply re-enable the localport. Remoteports
 	 * and controller reconnections should resume naturally.
 	 */
-	newrec = nvme_fc_attach_to_unreg_lport(pinfo);
+	newrec = nvme_fc_attach_to_unreg_lport(pinfo, template, dev);
 
 	/* found an lport, but something about its state is bad */
 	if (IS_ERR(newrec)) {
Signature · 1 line
2.13.1
maintainer
#2 lore raw

PATCH 1/2 nvme_fc: fix localport resume using stale values

Quoted 3 lines

The localport resume was not updating the lldd ops structure. If the lldd is unloaded and reloaded, the ops pointers will differ.

Question, why is the lport kept around if the lldd unloaded?

#3 lore raw

PATCH 1/2 nvme_fc: fix localport resume using stale values

Quoted 5 lines · Sagi Grimberg
Quoted 3 lines

The localport resume was not updating the lldd ops structure. If the lldd is unloaded and reloaded, the ops pointers will differ.

Question, why is the lport kept around if the lldd unloaded?

It's kept around due to ref counting of the attached rports and controllers who have not timed out and torn down.? The fact that the lldd could be unloaded is an artifact of supporting the reg/unreg and the fact that the transport doesn't care what the driver does after the unreg is done, and the re-reg only cares that it's done quickly enough that there are still references so the device and it can revive the old structures.? The reg/unreg/re-reg support is mainly there to support the lldd performing an adapter reset for some type of error recovery (could come from the SCSI stack requesting it for error escalation). Thus the lldd can reset the adapter, perform it's normal init steps again an re-reg the lport and rports, and the controllers resume under their ctlr_loss_tmo/dev_loss_tmo timers and consumers of the nvme devices are not affected other than a short pause.

-- james

author
#4 lore raw

PATCH 2/2 nvme_fc: decouple ns references from lldd references

In reply to James Smart

In the lldd api, a lldd may unregister a remoteport (loss of connectivity or driver unload) or localport (driver unload). The lldd must wait for the remoteport_delete or localport_delete before completing its actions post the unregister. The xxx_deletes currently occur only when the xxxport structure is fully freed after all references are removed. Thus the lldd may be held hostage until an app or in-kernel entity that has a namespace open finally closes so the namespace can be removed, the controller removed, thus the transport objects, thus the lldd.

This patch decouples the transport and os-facing objects from the lldd and the remoteport and localport. There is a point in all deletions where the transport will no longer interact with the lldd on behalf of a controller. That point centers around the association established with the target/subsystem. It will access the lldd whenever it attempts to create an association and while the association is active. New associations may only be created if the remoteport is live (thus the localport is live). It will not access the lldd after deleting the association.

Therefore, the patch tracks the count of active controllers - those with associations being created or that are active - on a remoteport. It also tracks the number of remoteports that have active controllers, on a a localport. When a remoteport is unregistered, as soon as there are no active controllers, the lldd's remoteport_delete may be called and the lldd may continue. Similarly, when a localport is unregistered, as soon as there are no remoteports with active controllers, the localport_delete callback may be made. This significantly speeds up unregistration with the lldd.

The transport objects continue in suspended status with reconnect timers running, and upon expiration, normal ref-counting will occur and the objects will be freed. The transport object may still be held hostage by the application/kernel module, but that is acceptable.

With this change, the lldd may be fully unloaded and reloaded, and if registrations occur prior to the timeouts, the nvme controller and namespaces will resume normally as if a link bounce.

Signed-off-by James Smart <james.smart at broadcom.com>

---
NOTE: this patch was cut against nvme-4.15, but with the nvme_fc devloss patches also applied prior. This is meaningful as this patch requires the
"nvme_fc: check connectivity before initiating reconnects" patch
http://lists.infradead.org/pipermail/linux-nvme/2017-October/013664.html
which has this change, which is critical:
@@ -2464,6 +2463,9 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)

     	++ctrl->ctrl.nr_reconnects;

+ if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE)
+ return -ENODEV;
+
/*
* Create the admin queue
*/

 drivers/nvme/host/fc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 6 deletions(-)
drivers/nvme/host/fc.c +78 -6
index e2054c90b42d..9b0c6eeb6f37 100644
@@ -126,6 +126,7 @@ struct nvme_fc_lport {
 	struct device			*dev;	/* physical device for dma */
 	struct nvme_fc_port_template	*ops;
 	struct kref			ref;
+	atomic_t                        act_rport_cnt;
 } __aligned(sizeof(u64));	/* alignment for other things alloc'd with */
 
 struct nvme_fc_rport {

@@ -138,6 +139,7 @@ struct nvme_fc_rport {
 	struct nvme_fc_lport		*lport;
 	spinlock_t			lock;
 	struct kref			ref;
+	atomic_t                        act_ctrl_cnt;
 	unsigned long			dev_loss_end;
 } __aligned(sizeof(u64));	/* alignment for other things alloc'd with */
 

@@ -153,6 +155,7 @@ struct nvme_fc_ctrl {
 	struct nvme_fc_rport	*rport;
 	u32			cnum;
 
+	bool			assoc_active;
 	u64			association_id;
 
 	struct list_head	ctrl_list;	/* rport->ctrl_list */

@@ -245,9 +248,6 @@ nvme_fc_free_lport(struct kref *ref)
 	list_del(&lport->port_list);
 	spin_unlock_irqrestore(&nvme_fc_lock, flags);
 
-	/* let the LLDD know we've finished tearing it down */
-	lport->ops->localport_delete(&lport->localport);
-
 	ida_simple_remove(&nvme_fc_local_port_cnt, lport->localport.port_num);
 	ida_destroy(&lport->endp_cnt);
 

@@ -402,6 +402,7 @@ nvme_fc_register_localport(struct nvme_fc_port_info *pinfo,
 	INIT_LIST_HEAD(&newrec->port_list);
 	INIT_LIST_HEAD(&newrec->endp_list);
 	kref_init(&newrec->ref);
+	atomic_set(&newrec->act_rport_cnt, 0);
 	newrec->ops = template;
 	newrec->dev = dev;
 	ida_init(&newrec->endp_cnt);

@@ -464,6 +465,9 @@ nvme_fc_unregister_localport(struct nvme_fc_local_port *portptr)
 
 	spin_unlock_irqrestore(&nvme_fc_lock, flags);
 
+	if (atomic_read(&lport->act_rport_cnt) == 0)
+		lport->ops->localport_delete(&lport->localport);
+
 	nvme_fc_lport_put(lport);
 
 	return 0;

@@ -517,9 +521,6 @@ nvme_fc_free_rport(struct kref *ref)
 	list_del(&rport->endp_list);
 	spin_unlock_irqrestore(&nvme_fc_lock, flags);
 
-	/* let the LLDD know we've finished tearing it down */
-	lport->ops->remoteport_delete(&rport->remoteport);
-
 	ida_simple_remove(&lport->endp_cnt, rport->remoteport.port_num);
 
 	kfree(rport);

@@ -706,6 +707,7 @@ nvme_fc_register_remoteport(struct nvme_fc_local_port *localport,
 	INIT_LIST_HEAD(&newrec->ctrl_list);
 	INIT_LIST_HEAD(&newrec->ls_req_list);
 	kref_init(&newrec->ref);
+	atomic_set(&newrec->act_ctrl_cnt, 0);
 	spin_lock_init(&newrec->lock);
 	newrec->remoteport.localport = &lport->localport;
 	newrec->dev = lport->dev;

@@ -864,6 +866,9 @@ nvme_fc_unregister_remoteport(struct nvme_fc_remote_port *portptr)
 
 	nvme_fc_abort_lsops(rport);
 
+	if (atomic_read(&rport->act_ctrl_cnt) == 0)
+		rport->lport->ops->remoteport_delete(portptr);
+
 	/*
 	 * release the reference, which will allow, if all controllers
 	 * go away, which should only occur after dev_loss_tmo occurs,

@@ -2644,6 +2649,61 @@ nvme_fc_reinit_io_queues(struct nvme_fc_ctrl *ctrl)
 	return ret;
 }
 
+static void
+nvme_fc_rport_active_on_lport(struct nvme_fc_rport *rport)
+{
+	struct nvme_fc_lport *lport = rport->lport;
+
+	atomic_inc(&lport->act_rport_cnt);
+}
+
+static void
+nvme_fc_rport_inactive_on_lport(struct nvme_fc_rport *rport)
+{
+	struct nvme_fc_lport *lport = rport->lport;
+	u32 cnt;
+
+	cnt = atomic_dec_return(&lport->act_rport_cnt);
+	if (cnt == 0 && lport->localport.port_state == FC_OBJSTATE_DELETED)
+		lport->ops->localport_delete(&lport->localport);
+}
+
+static int
+nvme_fc_ctlr_active_on_rport(struct nvme_fc_ctrl *ctrl)
+{
+	struct nvme_fc_rport *rport = ctrl->rport;
+	u32 cnt;
+
+	if (ctrl->assoc_active)
+		return 1;
+
+	ctrl->assoc_active = true;
+	cnt = atomic_inc_return(&rport->act_ctrl_cnt);
+	if (cnt == 1)
+		nvme_fc_rport_active_on_lport(rport);
+
+	return 0;
+}
+
+static int
+nvme_fc_ctlr_inactive_on_rport(struct nvme_fc_ctrl *ctrl)
+{
+	struct nvme_fc_rport *rport = ctrl->rport;
+	struct nvme_fc_lport *lport = rport->lport;
+	u32 cnt;
+
+	/* ctrl->assoc_active=false will be set independently */
+
+	cnt = atomic_dec_return(&rport->act_ctrl_cnt);
+	if (cnt == 0) {
+		if (rport->remoteport.port_state == FC_OBJSTATE_DELETED)
+			lport->ops->remoteport_delete(&rport->remoteport);
+		nvme_fc_rport_inactive_on_lport(rport);
+	}
+
+	return 0;
+}
+
 /*
  * This routine restarts the controller on the host side, and
  * on the link side, recreates the controller association.

@@ -2660,6 +2720,9 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	if (ctrl->rport->remoteport.port_state != FC_OBJSTATE_ONLINE)
 		return -ENODEV;
 
+	if (nvme_fc_ctlr_active_on_rport(ctrl))
+		return -ENOTUNIQ;
+
 	/*
 	 * Create the admin queue
 	 */

@@ -2767,6 +2830,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
 out_free_queue:
 	nvme_fc_free_queue(&ctrl->queues[0]);
+	ctrl->assoc_active = false;
+	nvme_fc_ctlr_inactive_on_rport(ctrl);
 
 	return ret;
 }

@@ -2782,6 +2847,10 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 {
 	unsigned long flags;
 
+	if (!ctrl->assoc_active)
+		return;
+	ctrl->assoc_active = false;
+
 	spin_lock_irqsave(&ctrl->lock, flags);
 	ctrl->flags |= FCCTRL_TERMIO;
 	ctrl->iocnt = 0;

@@ -2854,6 +2923,8 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 
 	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
 	nvme_fc_free_queue(&ctrl->queues[0]);
+
+	nvme_fc_ctlr_inactive_on_rport(ctrl);
 }
 
 static void

@@ -3109,6 +3180,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	ctrl->dev = lport->dev;
 	ctrl->cnum = idx;
 	init_waitqueue_head(&ctrl->ioabort_wait);
+	ctrl->assoc_active = false;
 
 	get_device(ctrl->dev);
 	kref_init(&ctrl->ref);
Signature · 1 line
2.13.1