[PATCH 0/3] qla2xxx target fixes

[PATCH 0/3] qla2xxx target fixes

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Hi Nic / Arun:

Here are a few pretty major fixes that I hope can get into the 3.5
merge of this stuff (Steve's patch that saves 128MB of memory per port
is maybe not strictly a fix but is pretty major).

Please review / apply.

Thanks!

Roland Dreier (2):
  tcm_qla2xxx: Add hook so qla_target code can shutdown sessions
  qla2xxx: Don't create duplicate target sessions

Steve Hodgson (1):
  tcm_qla2xxx: Convert FC address map from flat array to btree

 drivers/scsi/qla2xxx/Kconfig       |    1 +
 drivers/scsi/qla2xxx/qla_target.c  |   96 ++++++++---------
 drivers/scsi/qla2xxx/qla_target.h  |    1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  201 ++++++++++++++++--------------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |   22 +---
 5 files changed, 138 insertions(+), 183 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] tcm_qla2xxx: Convert FC address map from flat array to btree

[Roland Dreier]

From: Steve Hodgson <steve@purestorage.com>

tcm_qla2xxx needs a map from 24-bit FC addresses to node ACL pointers to
look up incoming commands.  Current code keeps this as a flat vmalloc()ed
array with 2^24 entries -- with 8-byte pointers, this consumes 128MB per
FC port!  Occupancy of this array is at most on the order of hundreds of
entries (and even that would be a lot of initiators on an FC fabric), so
this wastes a huge amount of memory.

Change this map to use the kernel's btree library; this reduces the memory
used to be O(number of entries) and has no measurable speed impact (in fact
since the data structure is now packed rather than sparse, the cache/TLB
effects may actually make this a net win).

Signed-off-by: Steve Hodgson <steve@purestorage.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/Kconfig       |    1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  195 ++++++++++++++++--------------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |   22 +---
 3 files changed, 90 insertions(+), 128 deletions(-)

diff --git a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig
index 940988d..0a98cea 100644
--- a/drivers/scsi/qla2xxx/Kconfig
+++ b/drivers/scsi/qla2xxx/Kconfig
@@ -30,6 +30,7 @@ config TCM_QLA2XXX
 	tristate "TCM_QLA2XXX fabric module for Qlogic 2xxx series target mode HBAs"
 	depends on SCSI_QLA_FC && TARGET_CORE 
 	select LIBFC
+	select BTREE
 	default n
 	---help---
 	Say Y here to enable the TCM_QLA2XXX fabric module for Qlogic 2xxx series target mode HBAs
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7fb4e81..7c3cbc1 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -764,11 +764,10 @@ static int tcm_qla2xxx_setup_nacl_from_rport(
 	struct Scsi_Host *sh = vha->host;
 	struct fc_host_attrs *fc_host = shost_to_fc_host(sh);
 	struct fc_rport *rport;
-	struct tcm_qla2xxx_fc_domain *d;
-	struct tcm_qla2xxx_fc_area *a;
-	struct tcm_qla2xxx_fc_al_pa *p;
 	unsigned long flags;
-	unsigned char domain, area, al_pa;
+	void *node;
+	int rc;
+
 	/*
 	 * Scan the existing rports, and create a session for the
 	 * explict NodeACL is an matching rport->node_name already
@@ -782,29 +781,31 @@ static int tcm_qla2xxx_setup_nacl_from_rport(
 		pr_debug("Located existing rport_wwpn and rport->node_name:"
 			" 0x%016LX, port_id: 0x%04x\n", rport->node_name,
 			rport->port_id);
-		domain = (rport->port_id >> 16) & 0xff;
-		area = (rport->port_id >> 8) & 0xff;
-		al_pa = rport->port_id & 0xff;
 		nacl->nport_id = rport->port_id;
 
-		pr_debug("fc_rport domain: 0x%02x area: 0x%02x al_pa: %02x\n",
-				domain, area, al_pa);
 		spin_unlock_irqrestore(sh->host_lock, flags);
 
-
 		spin_lock_irqsave(&vha->hw->hardware_lock, flags);
-		d = &((struct tcm_qla2xxx_fc_domain *)lport->lport_fcport_map)[domain];
-		pr_debug("Using d: %p for domain: 0x%02x\n", d, domain);
-		a = &d->areas[area];
-		pr_debug("Using a: %p for area: 0x%02x\n", a, area);
-		p = &a->al_pas[al_pa];
-		pr_debug("Using p: %p for al_pa: 0x%02x\n", p, al_pa);
-
-		p->se_nacl = se_nacl;
-		pr_debug("Setting p->se_nacl to se_nacl: %p for WWNN: 0x%016LX,"
+		node = btree_lookup32(&lport->lport_fcport_map, rport->port_id);
+		if (node) {
+			rc = btree_update32(&lport->lport_fcport_map,
+					    rport->port_id, se_nacl);
+		} else {
+			rc = btree_insert32(&lport->lport_fcport_map,
+					    rport->port_id, se_nacl,
+					    GFP_ATOMIC);
+		}
+		spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
+
+		if (rc) {
+			pr_err("Unable to insert se_nacl into fcport_map");
+			WARN_ON(rc > 0);
+			return rc;
+		}
+
+		pr_debug("Inserted into fcport_map: %p for WWNN: 0x%016LX,"
 			" port_id: 0x%08x\n", se_nacl, rport_wwnn,
 			nacl->nport_id);
-		spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
 
 		return 1;
 	}
@@ -825,29 +826,16 @@ void tcm_qla2xxx_clear_nacl_from_fcport_map(struct qla_tgt_sess *sess)
 				struct tcm_qla2xxx_lport, lport_wwn);
 	struct tcm_qla2xxx_nacl *nacl = container_of(se_nacl,
 				struct tcm_qla2xxx_nacl, se_node_acl);
-	struct tcm_qla2xxx_fc_domain *d;
-	struct tcm_qla2xxx_fc_area *a;
-	struct tcm_qla2xxx_fc_al_pa *p;
-	unsigned char domain, area, al_pa;
-
-	domain = (nacl->nport_id >> 16) & 0xff;
-	area = (nacl->nport_id >> 8) & 0xff;
-	al_pa = nacl->nport_id & 0xff;
+	void *node;
 
-	pr_debug("fc_rport domain: 0x%02x area: 0x%02x al_pa: %02x\n",
-			domain, area, al_pa);
+	pr_debug("fc_rport domain: port_id 0x%06x\n", nacl->nport_id);
 
-	d = &((struct tcm_qla2xxx_fc_domain *)lport->lport_fcport_map)[domain];
-	pr_debug("Using d: %p for domain: 0x%02x\n", d, domain);
-	a = &d->areas[area];
-	pr_debug("Using a: %p for area: 0x%02x\n", a, area);
-	p = &a->al_pas[al_pa];
-	pr_debug("Using p: %p for al_pa: 0x%02x\n", p, al_pa);
+	node = btree_remove32(&lport->lport_fcport_map, nacl->nport_id);
+	WARN_ON(node && (node != se_nacl));
 
-	p->se_nacl = NULL;
-	pr_debug("Clearing p->se_nacl to se_nacl: %p for WWNN: 0x%016LX,"
-		" port_id: 0x%08x\n", se_nacl, nacl->nport_wwnn,
-		nacl->nport_id);
+	pr_debug("Removed from fcport_map: %p for WWNN: 0x%016LX,"
+			       " port_id: 0x%06x\n", se_nacl, nacl->nport_wwnn,
+			       nacl->nport_id);
 }
 
 void tcm_qla2xxx_put_sess(struct qla_tgt_sess *sess)
@@ -1183,10 +1171,7 @@ static struct qla_tgt_sess *tcm_qla2xxx_find_sess_by_s_id(
 	struct tcm_qla2xxx_lport *lport;
 	struct se_node_acl *se_nacl;
 	struct tcm_qla2xxx_nacl *nacl;
-	struct tcm_qla2xxx_fc_domain *d;
-	struct tcm_qla2xxx_fc_area *a;
-	struct tcm_qla2xxx_fc_al_pa *p;
-	unsigned char domain, area, al_pa;
+	u32 key;
 
 	lport = ha->tgt.target_lport_ptr;
 	if (!lport) {
@@ -1195,24 +1180,14 @@ static struct qla_tgt_sess *tcm_qla2xxx_find_sess_by_s_id(
 		return NULL;
 	}
 
-	domain = s_id[0];
-	area = s_id[1];
-	al_pa = s_id[2];
+	key = (((unsigned long)s_id[0] << 16) |
+	       ((unsigned long)s_id[1] << 8) |
+	       (unsigned long)s_id[2]);
+	pr_debug("find_sess_by_s_id: 0x%06x\n", key);
 
-	pr_debug("find_sess_by_s_id: 0x%02x area: 0x%02x al_pa: %02x\n",
-			domain, area, al_pa);
-
-	d = &((struct tcm_qla2xxx_fc_domain *)lport->lport_fcport_map)[domain];
-	pr_debug("Using d: %p for domain: 0x%02x\n", d, domain);
-	a = &d->areas[area];
-	pr_debug("Using a: %p for area: 0x%02x\n", a, area);
-	p = &a->al_pas[al_pa];
-	pr_debug("Using p: %p for al_pa: 0x%02x\n", p, al_pa);
-
-	se_nacl = p->se_nacl;
+	se_nacl = btree_lookup32(&lport->lport_fcport_map, key);
 	if (!se_nacl) {
-		pr_debug("Unable to locate s_id: 0x%02x area: 0x%02x"
-			" al_pa: %02x\n", domain, area, al_pa);
+		pr_debug("Unable to locate s_id: 0x%06x\n", key);
 		return NULL;
 	}
 	pr_debug("find_sess_by_s_id: located se_nacl: %p,"
@@ -1238,29 +1213,29 @@ static void tcm_qla2xxx_set_sess_by_s_id(
 	struct qla_tgt_sess *qla_tgt_sess,
 	uint8_t *s_id)
 {
-	struct se_node_acl *saved_nacl;
-	struct tcm_qla2xxx_fc_domain *d;
-	struct tcm_qla2xxx_fc_area *a;
-	struct tcm_qla2xxx_fc_al_pa *p;
-	unsigned char domain, area, al_pa;
-
-	domain = s_id[0];
-	area = s_id[1];
-	al_pa = s_id[2];
-	pr_debug("set_sess_by_s_id: domain 0x%02x area: 0x%02x al_pa: %02x\n",
-			domain, area, al_pa);
-
-	d = &((struct tcm_qla2xxx_fc_domain *)lport->lport_fcport_map)[domain];
-	pr_debug("Using d: %p for domain: 0x%02x\n", d, domain);
-	a = &d->areas[area];
-	pr_debug("Using a: %p for area: 0x%02x\n", a, area);
-	p = &a->al_pas[al_pa];
-	pr_debug("Using p: %p for al_pa: 0x%02x\n", p, al_pa);
-
-	saved_nacl = p->se_nacl;
-	if (!saved_nacl) {
-		pr_debug("Setting up new p->se_nacl to new_se_nacl\n");
-		p->se_nacl = new_se_nacl;
+	u32 key;
+	void *slot;
+	int rc;
+
+	key = (((unsigned long)s_id[0] << 16) |
+	       ((unsigned long)s_id[1] << 8) |
+	       (unsigned long)s_id[2]);
+	pr_debug("set_sess_by_s_id: %06x\n", key);
+
+	slot = btree_lookup32(&lport->lport_fcport_map, key);
+	if (!slot) {
+		if (new_se_nacl) {
+			pr_debug("Setting up new fc_port entry to new_se_nacl\n");
+			nacl->nport_id = key;
+			rc = btree_insert32(&lport->lport_fcport_map, key, new_se_nacl,
+					    GFP_ATOMIC);
+			if (rc)
+				printk(KERN_ERR "Unable to insert s_id into fcport_map: %06x\n",
+				       (int)key);
+		} else {
+			pr_debug("Wiping nonexisting fc_port entry\n");
+		}
+
 		qla_tgt_sess->se_sess = se_sess;
 		nacl->qla_tgt_sess = qla_tgt_sess;
 		return;
@@ -1269,28 +1244,28 @@ static void tcm_qla2xxx_set_sess_by_s_id(
 	if (nacl->qla_tgt_sess) {
 		if (new_se_nacl == NULL) {
 			pr_debug("Clearing existing nacl->qla_tgt_sess"
-					" and p->se_nacl\n");
-			p->se_nacl = NULL;
+					" and fc_port entry\n");
+			btree_remove32(&lport->lport_fcport_map, key);
 			nacl->qla_tgt_sess = NULL;
 			return;
 		}
 		pr_debug("Replacing existing nacl->qla_tgt_sess and"
-				" p->se_nacl\n");
-		p->se_nacl = new_se_nacl;
+				       " fc_port entry\n");
+		btree_update32(&lport->lport_fcport_map, key, new_se_nacl);
 		qla_tgt_sess->se_sess = se_sess;
 		nacl->qla_tgt_sess = qla_tgt_sess;
 		return;
 	}
 
 	if (new_se_nacl == NULL) {
-		pr_debug("Clearing existing p->se_nacl\n");
-		p->se_nacl = NULL;
+		pr_debug("Clearing existing fc_port entry\n");
+		btree_remove32(&lport->lport_fcport_map, key);
 		return;
 	}
 
-	pr_debug("Replacing existing p->se_nacl w/o active"
+	pr_debug("Replacing existing fc_port entry w/o active"
 				" nacl->qla_tgt_sess\n");
-	p->se_nacl = new_se_nacl;
+	btree_update32(&lport->lport_fcport_map, key, new_se_nacl);
 	qla_tgt_sess->se_sess = se_sess;
 	nacl->qla_tgt_sess = qla_tgt_sess;
 
@@ -1321,8 +1296,7 @@ static struct qla_tgt_sess *tcm_qla2xxx_find_sess_by_loop_id(
 
 	pr_debug("find_sess_by_loop_id: Using loop_id: 0x%04x\n", loop_id);
 
-	fc_loopid = &((struct tcm_qla2xxx_fc_loopid *)lport->lport_loopid_map)[loop_id];
-
+	fc_loopid = lport->lport_loopid_map + loop_id;
 	se_nacl = fc_loopid->se_nacl;
 	if (!se_nacl) {
 		pr_debug("Unable to locate se_nacl by loop_id:"
@@ -1556,31 +1530,27 @@ static struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
 
 static int tcm_qla2xxx_init_lport(struct tcm_qla2xxx_lport *lport)
 {
-	lport->lport_fcport_map = vmalloc(
-			sizeof(struct tcm_qla2xxx_fc_domain) * 256);
-	if (!lport->lport_fcport_map) {
-		pr_err("Unable to allocate lport_fcport_map of %lu"
-			" bytes\n", sizeof(struct tcm_qla2xxx_fc_domain) * 256);
-		return -ENOMEM;
+	int rc;
+
+	rc = btree_init32(&lport->lport_fcport_map);
+	if (rc) {
+		pr_err("Unable to initialize lport->lport_fcport_map btree\n");
+		return rc;
 	}
-	memset(lport->lport_fcport_map, 0,
-			sizeof(struct tcm_qla2xxx_fc_domain) * 256);
-	pr_debug("qla2xxx: Allocated lport_fcport_map of %lu bytes\n",
-			sizeof(struct tcm_qla2xxx_fc_domain) * 256);
 
 	lport->lport_loopid_map = vmalloc(sizeof(struct tcm_qla2xxx_fc_loopid) *
 				65536);
 	if (!lport->lport_loopid_map) {
 		pr_err("Unable to allocate lport->lport_loopid_map"
-			" of %lu bytes\n", sizeof(struct tcm_qla2xxx_fc_loopid)
-			* 65536);
-		vfree(lport->lport_fcport_map);
+		       " of %lu bytes\n", sizeof(struct tcm_qla2xxx_fc_loopid)
+		       * 65536);
+		btree_destroy32(&lport->lport_fcport_map);
 		return -ENOMEM;
 	}
 	memset(lport->lport_loopid_map, 0, sizeof(struct tcm_qla2xxx_fc_loopid)
-			* 65536);
+	       * 65536);
 	pr_debug("qla2xxx: Allocated lport_loopid_map of %lu bytes\n",
-			sizeof(struct tcm_qla2xxx_fc_loopid) * 65536);
+	       sizeof(struct tcm_qla2xxx_fc_loopid) * 65536);
 	return 0;
 }
 
@@ -1630,7 +1600,7 @@ static struct se_wwn *tcm_qla2xxx_make_lport(
 	return &lport->lport_wwn;
 out_lport:
 	vfree(lport->lport_loopid_map);
-	vfree(lport->lport_fcport_map);
+	btree_destroy32(&lport->lport_fcport_map);
 out:
 	kfree(lport);
 	return ERR_PTR(ret);
@@ -1642,6 +1612,9 @@ static void tcm_qla2xxx_drop_lport(struct se_wwn *wwn)
 			struct tcm_qla2xxx_lport, lport_wwn);
 	struct scsi_qla_host *vha = lport->qla_vha;
 	struct qla_hw_data *ha = vha->hw;
+	struct se_node_acl *node;
+	u32 key;
+
 	/*
 	 * Call into qla2x_target.c LLD logic to complete the
 	 * shutdown of struct qla_tgt after the call to
@@ -1653,7 +1626,9 @@ static void tcm_qla2xxx_drop_lport(struct se_wwn *wwn)
 	qlt_lport_deregister(vha);
 
 	vfree(lport->lport_loopid_map);
-	vfree(lport->lport_fcport_map);
+	btree_for_each_safe32(&lport->lport_fcport_map, key, node)
+		btree_remove32(&lport->lport_fcport_map, key);
+	btree_destroy32(&lport->lport_fcport_map);
 	kfree(lport);
 }
 
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 41730ed..7a94f03 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -1,4 +1,5 @@
 #include <target/target_core_base.h>
+#include <linux/btree.h>
 
 #define TCM_QLA2XXX_VERSION	"v0.1"
 /* length of ASCII WWPNs including pad */
@@ -45,21 +46,6 @@ struct tcm_qla2xxx_tpg {
 
 #define QLA_TPG_ATTRIB(tpg)	(&(tpg)->tpg_attrib)
 
-/*
- * Used for the 24-bit lport->lport_fcport_map;
- */
-struct tcm_qla2xxx_fc_al_pa {
-	struct se_node_acl *se_nacl;
-};
-
-struct tcm_qla2xxx_fc_area {
-        struct tcm_qla2xxx_fc_al_pa al_pas[256];
-};
-
-struct tcm_qla2xxx_fc_domain {
-        struct tcm_qla2xxx_fc_area areas[256];
-};
-
 struct tcm_qla2xxx_fc_loopid {
 	struct se_node_acl *se_nacl;
 };
@@ -77,10 +63,10 @@ struct tcm_qla2xxx_lport {
 	char lport_name[TCM_QLA2XXX_NAMELEN];
 	/* ASCII formatted WWPN+WWNN for NPIV FC Target Lport */
 	char lport_npiv_name[TCM_QLA2XXX_NPIV_NAMELEN];
-	/* vmalloc'ed memory for fc_port pointers in 24-bit FC Port ID space */
-	char *lport_fcport_map;
+	/* map for fc_port pointers in 24-bit FC Port ID space */
+	struct btree_head32 lport_fcport_map;
 	/* vmalloc-ed memory for fc_port pointers for 16-bit FC loop ID */
-	char *lport_loopid_map;
+	struct tcm_qla2xxx_fc_loopid *lport_loopid_map;
 	/* Pointer to struct scsi_qla_host from qla2xxx LLD */
 	struct scsi_qla_host *qla_vha;
 	/* Pointer to struct scsi_qla_host for NPIV VP from qla2xxx LLD */
-- 
1.7.9.5

[PATCH 2/3] tcm_qla2xxx: Add hook so qla_target code can shutdown sessions

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

When qla_tgt_del_sess_work_fn() does a ->put_sess, that may drop the
last reference to a session and free it.  We need to make sure the
session release function waits for all pending commands, or else we may
hit use-after-free if those commands complete after the session is gone.

This won't happen unless we do target_splice_sess_cmd_list() via
tcm_qla2xxx_shutdown_session().  So add a hook in qla_tgt_func_tmpl
to let qla_target call ->shutdown_sess before ->put_sess.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c  |    1 +
 drivers/scsi/qla2xxx/qla_target.h  |    1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |    6 ++++++
 3 files changed, 8 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 4d51136..6542da4 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -691,6 +691,7 @@ static void qlt_del_sess_work_fn(struct delayed_work *work)
 				ql_dbg(ql_dbg_tgt_mgt, vha, 0xf004,
 				    "Timeout: sess %p about to be deleted\n",
 				    sess);
+				ha->tgt.tgt_ops->shutdown_sess(sess);
 				ha->tgt.tgt_ops->put_sess(sess);
 			}
 
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index a8f3c5c..bacc4e6 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -647,6 +647,7 @@ struct qla_tgt_func_tmpl {
 						const uint8_t *);
 	void (*clear_nacl_from_fcport_map)(struct qla_tgt_sess *);
 	void (*put_sess)(struct qla_tgt_sess *);
+	void (*shutdown_sess)(struct qla_tgt_sess *);
 };
 
 int qla2x00_wait_for_hba_online(struct scsi_qla_host *);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7c3cbc1..0570a8c 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -843,6 +843,11 @@ void tcm_qla2xxx_put_sess(struct qla_tgt_sess *sess)
 	target_put_session(sess->se_sess);
 }
 
+void tcm_qla2xxx_shutdown_sess(struct qla_tgt_sess *sess)
+{
+	tcm_qla2xxx_shutdown_session(sess->se_sess);
+}
+
 static struct se_node_acl *tcm_qla2xxx_make_nodeacl(
 	struct se_portal_group *se_tpg,
 	struct config_group *group,
@@ -1526,6 +1531,7 @@ static struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
 	.find_sess_by_loop_id	= tcm_qla2xxx_find_sess_by_loop_id,
 	.clear_nacl_from_fcport_map = tcm_qla2xxx_clear_nacl_from_fcport_map,
 	.put_sess		= tcm_qla2xxx_put_sess,
+	.shutdown_sess		= tcm_qla2xxx_shutdown_sess,
 };
 
 static int tcm_qla2xxx_init_lport(struct tcm_qla2xxx_lport *lport)
-- 
1.7.9.5

[PATCH 3/3] qla2xxx: Don't create duplicate target sessions

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

We run into problems when tearing down ACLs with IO in flight from the
initiator with current code.  What happens is that when removing an ACL,
we tear down the associated session; then when more commands come in, we
queue them up and create a new session in qla_tgt_do_work() for each
command.  If multiple commands get queued up, we create multiple
sessions, each with its own associated dynamic ACL.

Then when we reestablish the ACL, we find the wrong dynamic ACL and
convert it back to a normal ACL -- unfortunately the session in the
session table that points to the normal ACL has already been overwritten
and so all future commands find a session that still points to a dynamic
ACL (which rejects all commands except INQUIRY / REPORT LUNS).

Fix this by just always looking up the session in qla_tgt_do_work() and
avoiding creating duplicate sessions.  This also simplifies the code
(since we don't have two places to look up sessions) and moves more
work out of the interrupt handler and into the work queue.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c |   95 ++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 55 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 6542da4..58dfbd8 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -720,9 +720,8 @@ static struct qla_tgt_sess *qlt_create_sess(
 	unsigned char be_sid[3];
 
 	/* Check to avoid double sessions */
-#if 0
 	spin_lock_irqsave(&ha->hardware_lock, flags);
-	list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) {
+	list_for_each_entry(sess, &ha->tgt.qla_tgt->sess_list, sess_list_entry) {
 		if (!memcmp(sess->port_name, fcport->port_name, WWN_SIZE)) {
 			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf005,
 			    "Double sess %p found (s_id %x:%x:%x, "
@@ -731,12 +730,12 @@ static struct qla_tgt_sess *qlt_create_sess(
 			    sess->s_id.b.al_pa, sess->s_id.b.area,
 			    sess->loop_id, fcport->d_id.b.domain,
 			    fcport->d_id.b.al_pa, fcport->d_id.b.area,
-			    fcport->loop_id)
+			    fcport->loop_id);
 
 			if (sess->deleted)
 				qlt_undelete_sess(sess);
 
-			qlt_sess_get(sess);
+			kref_get(&sess->se_sess->sess_kref);
 			sess->s_id = fcport->d_id;
 			sess->loop_id = fcport->loop_id;
 			sess->conf_compl_supported = !!(fcport->flags &
@@ -744,12 +743,11 @@ static struct qla_tgt_sess *qlt_create_sess(
 			if (sess->local && !local)
 				sess->local = 0;
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			goto out;
+
+			return sess;
 		}
 	}
-	spin_unlock_irq_restore(&ha->hardware_lock, flags);
-#endif
-	/* We are under tgt_mutex, so a new sess can't be added behind us */
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
 	sess = kzalloc(sizeof(*sess), GFP_KERNEL);
 	if (!sess) {
@@ -2634,7 +2632,7 @@ static void qlt_do_work(struct work_struct *work)
 	scsi_qla_host_t *vha = cmd->vha;
 	struct qla_hw_data *ha = vha->hw;
 	struct qla_tgt *tgt = ha->tgt.qla_tgt;
-	struct qla_tgt_sess *sess = cmd->sess;
+	struct qla_tgt_sess *sess = NULL;
 	atio_from_isp_t *atio = &cmd->atio;
 	unsigned char *cdb;
 	unsigned long flags;
@@ -2642,25 +2640,49 @@ static void qlt_do_work(struct work_struct *work)
 	int ret, fcp_task_attr, data_dir, bidi = 0;;
 
 	if (tgt->tgt_stop)
-		goto out_term;	
+		goto out_term;
 
-	if (!sess) {
-		uint8_t *s_id = NULL;
+	spin_lock_irqsave(&ha->hardware_lock, flags);
+	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
+	    atio->u.isp24.fcp_hdr.s_id);
+	if (sess) {
+		if (unlikely(sess->tearing_down)) {
+			sess = NULL;
+			spin_unlock_irqrestore(&ha->hardware_lock, flags);
+			goto out_term;
+		} else {
+			/* Do the extra kref_get() before dropping qla_hw_data->hardware_lock. */
+			kref_get(&sess->se_sess->sess_kref);
+		}
+	}
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
+
+	if (unlikely(!sess)) {
+		uint8_t *s_id =	atio->u.isp24.fcp_hdr.s_id;
 
-		s_id = atio->u.isp24.fcp_hdr.s_id;
+		ql_dbg(ql_dbg_tgt_mgt, vha, 0xe125, "qla_target(%d):"
+		       " Unable to find wwn login (s_id %x:%x:%x),"
+		       " trying to create it manually\n", vha->vp_idx,
+		       s_id[0], s_id[1], s_id[2]);
+
+		if (atio->u.raw.entry_count > 1) {
+			ql_dbg(ql_dbg_tgt_mgt, vha, 0xe127, "Dropping multy entry"
+					" cmd %p\n", cmd);
+			goto out_term;
+		}
 
 		mutex_lock(&ha->tgt.tgt_mutex);
-		cmd->sess = sess = qlt_make_local_sess(vha, s_id);
-		/* sess has got an extra creation ref */
+		sess = qlt_make_local_sess(vha, s_id);
+		/* sess has an extra creation ref. */
 		mutex_unlock(&ha->tgt.tgt_mutex);
 
 		if (!sess)
 			goto out_term;
-		cmd->loop_id = sess->loop_id;
 	}
 
-	if (tgt->tgt_stop)
-		goto out_term;
+	cmd->sess = sess;
+	cmd->loop_id = sess->loop_id;
+	cmd->conf_compl_supported = sess->conf_compl_supported;
 
 	cdb = &atio->u.isp24.fcp_cmnd.cdb[0];
 	cmd->tag = atio->u.isp24.exchange_addr;
@@ -2716,9 +2738,7 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host *vha,
 {
 	struct qla_hw_data *ha = vha->hw;
 	struct qla_tgt *tgt = ha->tgt.qla_tgt;
-	struct qla_tgt_sess *sess;
 	struct qla_tgt_cmd *cmd;
-	int res = 0;
 
 	if (unlikely(tgt->tgt_stop)) {
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf021,
@@ -2740,45 +2760,10 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host *vha,
 	cmd->tgt = ha->tgt.qla_tgt;
 	cmd->vha = vha;
 
-	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
-	    atio->u.isp24.fcp_hdr.s_id);
-	if (unlikely(!sess)) {
-		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf022,
-		    "qla_target(%d): Unable to find wwn login (s_id %x:%x:%x),"
-		    " trying to create it manually\n", vha->vp_idx,
-		    atio->u.isp24.fcp_hdr.s_id[0],
-		    atio->u.isp24.fcp_hdr.s_id[1],
-		    atio->u.isp24.fcp_hdr.s_id[2]);
-
-		if (atio->u.raw.entry_count > 1) {
-			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf023,
-			    "Dropping multy entry cmd %p\n", cmd);
-			goto out_free_cmd;
-		}
-		goto out_sched;
-	}
-
-	if (sess->tearing_down || tgt->tgt_stop)
-		goto out_free_cmd;
-
-	cmd->sess = sess;
-	cmd->loop_id = sess->loop_id;
-	cmd->conf_compl_supported = sess->conf_compl_supported;
-	/*
-	 * Get the extra kref_get() before dropping qla_hw_data->hardware_lock,
-	 * and call kref_put() in qlt_do_work() process context to drop the
-	 * extra reference.
-	*/
-	kref_get(&sess->se_sess->sess_kref);
-
-out_sched:
 	INIT_WORK(&cmd->work, qlt_do_work);
 	queue_work(qla_tgt_wq, &cmd->work);
 	return 0;
 
-out_free_cmd:
-	qlt_free_cmd(cmd);
-	return res;
 }
 
 /* ha->hardware_lock supposed to be held on entry */
-- 
1.7.9.5

Re: [PATCH 1/3] tcm_qla2xxx: Convert FC address map from flat array to btree

[Nicholas A. Bellinger]

On Thu, 2012-05-03 at 12:42 -0700, Roland Dreier wrote:
> From: Steve Hodgson <steve@purestorage.com>
> 
> tcm_qla2xxx needs a map from 24-bit FC addresses to node ACL pointers to
> look up incoming commands.  Current code keeps this as a flat vmalloc()ed
> array with 2^24 entries -- with 8-byte pointers, this consumes 128MB per
> FC port!  Occupancy of this array is at most on the order of hundreds of
> entries (and even that would be a lot of initiators on an FC fabric), so
> this wastes a huge amount of memory.
> 
> Change this map to use the kernel's btree library; this reduces the memory
> used to be O(number of entries) and has no measurable speed impact (in fact
> since the data structure is now packed rather than sparse, the cache/TLB
> effects may actually make this a net win).
> 
> Signed-off-by: Steve Hodgson <steve@purestorage.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/Kconfig       |    1 +
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |  195 ++++++++++++++++--------------------
>  drivers/scsi/qla2xxx/tcm_qla2xxx.h |   22 +---
>  3 files changed, 90 insertions(+), 128 deletions(-)
> 

I think this patch looks like a safe enough mechanical change in
tcm_qla2xxx code for the benefit here, and definitely should be included
for the initial 3.5 merge..

Applied to lio-core.git, and including the full series into this
evenings for-next-merge re-spin.

Excellent work here by Steve Hodgson to convert the FC address <-> node
ACL pointer lookup logic to use the in-kernel btree library !!

Thanks Roland!

--nab