[RFC PATCH 0/5] target: add sysfs support

[RFC PATCH 0/5] target: add sysfs support

[Mike Christie]

The following patches made over Linus's current tree allow lio to
export info about structs that the kernel initiates creation of
via events like initiator login where there is no user interaction
like a mkdir. These patches specificially focus on the
I_T_nexus/session but could be used for other objects if we want.

Why sysfs when we have configfs?

I started with configfs and hit bugs like:

commit cc57c07343bd071cdf1915a91a24ab7d40c9b590
Author: Mike Christie <mchristi@redhat.com>
Date:   Sun Jul 15 18:16:17 2018 -0500

    configfs: fix registered group removal

but it turns out that bug was not really a bug and was just how
configfs was meant to work. It seems it was not meant to be used
where the kernel initiates creation of dirs/files as a result of
some internal action. It's more geared to the user initiating
the creation, and my patch just lead to other bugs and was
reverted:

commit f19e4ed1e1edbfa3c9ccb9fed17759b7d6db24c6
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu Aug 29 23:13:30 2019 -0400

    configfs_register_group() shouldn't be (and isn't) called in
rmdirable parts

So to export the session info we have debugfs, sysfs, ioctl,
netlink, etc. sysfs just seemed like a decent fit since one of the
primary users is rtslib and it already has lots of file/dir
handling code.

[RFC PATCH 1/5] target: add sysfs support

[Mike Christie]

configfs does not work well when the kernel is initiating the creation
of an object we want to export info for and the objects above/below it
are created by the user. There are races/bugs like seen with this patch
and the issue the original bug was trying to fix:

commit f19e4ed1e1edbfa3c9ccb9fed17759b7d6db24c6
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu Aug 29 23:13:30 2019 -0400

    configfs_register_group() shouldn't be (and isn't) called in
rmdirable parts

The problem is that for many drivers like qla2xxx, iscsi, etc, session
creation is done by the kernel when there is a login initiated by an
initiator, but we need a common way to export the systems sessions so
tools like targetcli can report basic info like what initaitors are
logged in and daemons like tcmu-runner can track sessions for load
balancing and PGRs.

This patch begins to add a sysfs interface that will initially be used
to export LIO's sessions. The general layout will mirror the lio
configfs tree:

target_core/
`-- $fabric_driver
    `-- target_name
        |-- tpgt_1
        |   `-- sessions
        `-- tpgt_2
            `-- sessions

iscsi example:
target_core/
`-- iscsi
    `-- iqn.1999-09.com.lio:tgt1
        |-- tpgt_1
        |   `-- sessions
        `-- tpgt_2
            `-- sessions

This initial patch adds only adds the upper layer dirs above the
sessions.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/target_core_configfs.c        | 21 ++++++++++++++++++
 drivers/target/target_core_fabric_configfs.c | 32 ++++++++++++++++++++++++++++
 drivers/target/target_core_internal.h        |  1 +
 include/target/target_core_base.h            |  4 ++++
 4 files changed, 58 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index ff82b21f..3fd08c5 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -63,6 +63,8 @@
 	pr_debug("Setup generic %s\n", __stringify(_name));		\
 }
 
+static struct kobject *tcm_core_kobj;
+
 extern struct t10_alua_lu_gp *default_lu_gp;
 
 static LIST_HEAD(g_tf_list);
@@ -245,6 +247,11 @@ static struct config_group *target_core_register_fabric(
 	}
 	pr_debug("Target_Core_ConfigFS: REGISTER -> Located fabric:"
 			" %s\n", tf->tf_ops->fabric_name);
+
+	tf->kobj = kobject_create_and_add(name, tcm_core_kobj);
+	if (!tf->kobj)
+		goto dec_tf;
+
 	/*
 	 * On a successful target_core_get_fabric() look, the returned
 	 * struct target_fabric_configfs *tf will contain a usage reference.
@@ -261,6 +268,10 @@ static struct config_group *target_core_register_fabric(
 	pr_debug("Target_Core_ConfigFS: REGISTER -> Allocated Fabric: %s\n",
 		 config_item_name(&tf->tf_group.cg_item));
 	return &tf->tf_group;
+
+dec_tf:
+	atomic_dec(&tf->tf_access_cnt);
+	return ERR_PTR(-EINVAL);
 }
 
 /*
@@ -283,6 +294,9 @@ static void target_core_deregister_fabric(
 	pr_debug("Target_Core_ConfigFS: DEREGISTER -> Releasing ci"
 			" %s\n", config_item_name(item));
 
+	kobject_del(tf->kobj);
+	kobject_put(tf->kobj);
+
 	configfs_remove_default_groups(&tf->tf_group);
 	config_item_put(item);
 }
@@ -3538,6 +3552,10 @@ static int __init target_core_init_configfs(void)
 
 	target_init_dbroot();
 
+	tcm_core_kobj = kobject_create_and_add("target_core", NULL);
+	if (!tcm_core_kobj)
+		goto out;
+
 	return 0;
 
 out:
@@ -3555,6 +3573,9 @@ static int __init target_core_init_configfs(void)
 
 static void __exit target_core_exit_configfs(void)
 {
+	kobject_del(tcm_core_kobj);
+	kobject_put(tcm_core_kobj);
+
 	configfs_remove_default_groups(&alua_lu_gps_group);
 	configfs_remove_default_groups(&alua_group);
 	configfs_remove_default_groups(&target_core_hbagroup);
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index ee85602..4d208e4 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -838,6 +838,14 @@ static struct config_group *target_fabric_make_tpg(
 	if (!se_tpg || IS_ERR(se_tpg))
 		return ERR_PTR(-EINVAL);
 
+	se_tpg->kobj = kobject_create_and_add(name, wwn->kobj);
+	if (!se_tpg->kobj)
+		goto drop_tpg;
+
+	se_tpg->sessions_kobj = kobject_create_and_add("sessions", se_tpg->kobj);
+	if (!se_tpg->sessions_kobj)
+		goto del_tpg_kobj;
+
 	config_group_init_type_name(&se_tpg->tpg_group, name,
 			&tf->tf_tpg_base_cit);
 
@@ -872,6 +880,13 @@ static struct config_group *target_fabric_make_tpg(
 			&se_tpg->tpg_group);
 
 	return &se_tpg->tpg_group;
+
+del_tpg_kobj:
+	kobject_del(se_tpg->kobj);
+	kobject_put(se_tpg->kobj);
+drop_tpg:
+	tf->tf_ops->fabric_drop_tpg(se_tpg);
+	return ERR_PTR(-EINVAL);
 }
 
 static void target_fabric_drop_tpg(
@@ -881,6 +896,12 @@ static void target_fabric_drop_tpg(
 	struct se_portal_group *se_tpg = container_of(to_config_group(item),
 				struct se_portal_group, tpg_group);
 
+	kobject_del(se_tpg->sessions_kobj);
+	kobject_put(se_tpg->sessions_kobj);
+
+	kobject_del(se_tpg->kobj);
+	kobject_put(se_tpg->kobj);
+
 	configfs_remove_default_groups(&se_tpg->tpg_group);
 	config_item_put(item);
 }
@@ -927,6 +948,7 @@ static struct config_group *target_fabric_make_wwn(
 	struct target_fabric_configfs *tf = container_of(group,
 				struct target_fabric_configfs, tf_group);
 	struct se_wwn *wwn;
+	int ret;
 
 	if (!tf->tf_ops->fabric_make_wwn) {
 		pr_err("tf->tf_ops.fabric_make_wwn is NULL\n");
@@ -938,6 +960,9 @@ static struct config_group *target_fabric_make_wwn(
 		return ERR_PTR(-EINVAL);
 
 	wwn->wwn_tf = tf;
+	wwn->kobj = kobject_create_and_add(name, tf->kobj);
+	if (!wwn->kobj)
+		goto drop_wwn;
 
 	config_group_init_type_name(&wwn->wwn_group, name, &tf->tf_tpg_cit);
 
@@ -947,7 +972,12 @@ static struct config_group *target_fabric_make_wwn(
 
 	if (tf->tf_ops->add_wwn_groups)
 		tf->tf_ops->add_wwn_groups(wwn);
+
 	return &wwn->wwn_group;
+
+drop_wwn:
+	tf->tf_ops->fabric_drop_wwn(wwn);
+	return ERR_PTR(ret);
 }
 
 static void target_fabric_drop_wwn(
@@ -957,6 +987,8 @@ static void target_fabric_drop_wwn(
 	struct se_wwn *wwn = container_of(to_config_group(item),
 				struct se_wwn, wwn_group);
 
+	kobject_del(wwn->kobj);
+	kobject_put(wwn->kobj);
 	configfs_remove_default_groups(&wwn->wwn_group);
 	config_item_put(item);
 }
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 8533444..16ae020 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -27,6 +27,7 @@ struct target_backend {
 struct target_fabric_configfs {
 	atomic_t		tf_access_cnt;
 	struct list_head	tf_list;
+	struct kobject		*kobj;
 	struct config_group	tf_group;
 	struct config_group	tf_disc_group;
 	const struct target_core_fabric_ops *tf_ops;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 6d4a694..9d38b53 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -8,6 +8,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/semaphore.h>     /* struct semaphore */
 #include <linux/completion.h>
+#include <linux/kobject.h>
 
 #define TARGET_CORE_VERSION		"v5.0"
 
@@ -890,6 +891,8 @@ struct se_portal_group {
 	/* Pointer to $FABRIC_MOD dependent code */
 	const struct target_core_fabric_ops *se_tpg_tfo;
 	struct se_wwn		*se_tpg_wwn;
+	struct kobject		*kobj;
+	struct kobject		*sessions_kobj;
 	struct config_group	tpg_group;
 	struct config_group	tpg_lun_group;
 	struct config_group	tpg_np_group;
@@ -928,6 +931,7 @@ struct se_wwn {
 	void			*priv;
 	struct config_group	wwn_group;
 	struct config_group	fabric_stat_group;
+	struct kobject		*kobj;
 };
 
 static inline void atomic_inc_mb(atomic_t *v)
-- 
1.8.3.1

Re: [RFC PATCH 1/5] target: add sysfs support

[Bart Van Assche]

On 2020-04-13 22:15, Mike Christie wrote:
> target_core/
> `-- $fabric_driver
>     `-- target_name
>         |-- tpgt_1
>         |   `-- sessions
>         `-- tpgt_2
>             `-- sessions
> 
> iscsi example:
> target_core/
> `-- iscsi
>     `-- iqn.1999-09.com.lio:tgt1
>         |-- tpgt_1
>         |   `-- sessions
>         `-- tpgt_2
>             `-- sessions

After the SCSI target core was added to the kernel tree an NVMe target
core was added. How about using the name "scsi_target" for the top-level
directory instead of "target_core" to prevent confusion with the NVMe
target?

Thanks,

Bart.

Re: [RFC PATCH 1/5] target: add sysfs support

[Mike Christie]

On 04/14/2020 09:23 PM, Bart Van Assche wrote:
> On 2020-04-13 22:15, Mike Christie wrote:
>> target_core/
>> `-- $fabric_driver
>>     `-- target_name
>>         |-- tpgt_1
>>         |   `-- sessions
>>         `-- tpgt_2
>>             `-- sessions
>>
>> iscsi example:
>> target_core/
>> `-- iscsi
>>     `-- iqn.1999-09.com.lio:tgt1
>>         |-- tpgt_1
>>         |   `-- sessions
>>         `-- tpgt_2
>>             `-- sessions
> 
> After the SCSI target core was added to the kernel tree an NVMe target
> core was added. How about using the name "scsi_target" for the top-level
> directory instead of "target_core" to prevent confusion with the NVMe
> target?
> 

Will do.

[RFC PATCH 2/5] target: add sysfs session helper functions

[Mike Christie]

This patch adds helpers to add/remove a dir per session. There is
only one default file in there which points to the ACL being used
if there is one.

iSCSI example:

target_core/
└── iscsi
    └── iqn.1999-09.com.tcmu:alua
        └── tpgt_1
            └── sessions
                └── session-1
                    └── acl

cat acl 
iqn.2005-03.com.ceph:ini1

Fabric drivers like iscsi or elx can add pass in an attribute_group
to add driver specific attrs.

This patch just adds the helpers and does the initial kobject refcount
hookup. The next 2 patches will convert lio core and the fabric drivers
like iscsi to use these functions.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/Makefile                      |   1 +
 drivers/target/target_core_fabric_configfs.c |   3 +
 drivers/target/target_core_internal.h        |   4 +
 drivers/target/target_core_sysfs.c           | 143 +++++++++++++++++++++++++++
 drivers/target/target_core_transport.c       |   9 +-
 include/target/target_core_base.h            |   5 +
 include/target/target_core_fabric.h          |   5 +-
 7 files changed, 168 insertions(+), 2 deletions(-)
 create mode 100644 drivers/target/target_core_sysfs.c

diff --git a/drivers/target/Makefile b/drivers/target/Makefile
index 4563474..4a7246e 100644
--- a/drivers/target/Makefile
+++ b/drivers/target/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 target_core_mod-y		:= target_core_configfs.o \
+				   target_core_sysfs.o \
 				   target_core_device.o \
 				   target_core_fabric_configfs.o \
 				   target_core_fabric_lib.o \
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 4d208e4..b37c530 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -960,6 +960,8 @@ static struct config_group *target_fabric_make_wwn(
 		return ERR_PTR(-EINVAL);
 
 	wwn->wwn_tf = tf;
+	ida_init(&wwn->sid_ida);
+
 	wwn->kobj = kobject_create_and_add(name, tf->kobj);
 	if (!wwn->kobj)
 		goto drop_wwn;
@@ -987,6 +989,7 @@ static void target_fabric_drop_wwn(
 	struct se_wwn *wwn = container_of(to_config_group(item),
 				struct se_wwn, wwn_group);
 
+	ida_destroy(&wwn->sid_ida);
 	kobject_del(wwn->kobj);
 	kobject_put(wwn->kobj);
 	configfs_remove_default_groups(&wwn->wwn_group);
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 16ae020..1b683ce 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -106,6 +106,9 @@ int	target_get_pr_transport_id(struct se_node_acl *nacl,
 const char *target_parse_pr_out_transport_id(struct se_portal_group *tpg,
 		char *buf, u32 *out_tid_len, char **port_nexus_ptr);
 
+/* target_core_sysfs.c */
+void	target_sysfs_init_session(struct se_session *sess);
+
 /* target_core_hba.c */
 struct se_hba *core_alloc_hba(const char *, u32, u32);
 int	core_delete_hba(struct se_hba *);
@@ -153,6 +156,7 @@ void	transport_dump_dev_info(struct se_device *, struct se_lun *,
 bool	target_check_wce(struct se_device *dev);
 bool	target_check_fua(struct se_device *dev);
 void	__target_execute_cmd(struct se_cmd *, bool);
+void	__target_free_session(struct se_session *);
 
 /* target_core_stat.c */
 void	target_stat_setup_dev_default_groups(struct se_device *);
diff --git a/drivers/target/target_core_sysfs.c b/drivers/target/target_core_sysfs.c
new file mode 100644
index 0000000..d4f9d33
--- /dev/null
+++ b/drivers/target/target_core_sysfs.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/kobject.h>
+#include <linux/idr.h>
+
+#include <target/target_core_base.h>
+#include "target_core_internal.h"
+
+static void target_sysfs_session_release(struct kobject *kobj)
+{
+	struct se_session *se_sess = container_of(kobj, struct se_session, kobj);
+
+	__target_free_session(se_sess);
+}
+
+struct session_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct se_session *, char *);
+	ssize_t (*store)(struct se_session *, const char *, size_t);
+};
+
+#define to_session(atr) container_of((atr), struct session_attr, attr)
+
+static ssize_t
+session_attr_store(struct kobject *kobj, struct attribute *attr,
+		   const char *page, size_t length)
+{
+	struct se_session *se_sess = container_of(kobj, struct se_session, kobj);
+	struct session_attr *sess_attr = to_session(attr);
+
+	if (!sess_attr->store)
+		return -ENOSYS;
+
+	return sess_attr->store(se_sess, page, length);
+}
+
+static ssize_t session_acl_show(struct se_session *se_sess, char *page)
+{
+	struct se_node_acl *acl;
+	ssize_t len;
+
+	acl = se_sess->se_node_acl;
+	if (!acl)
+		return -ENOTCONN;
+
+	if (acl->dynamic_node_acl) {
+		page[0] = '\0';
+		len = 0;
+	} else {
+		len = snprintf(page, PAGE_SIZE, "%s\n", acl->initiatorname);
+	}
+
+	return len;
+}
+
+static struct session_attr session_acl_attr = {
+	.attr = { .name = "acl", .mode = 0444 },
+	.show = session_acl_show,
+};
+
+static struct attribute *session_attrs[] = {
+	&session_acl_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(session);
+
+static ssize_t
+session_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
+{
+	struct se_session *se_sess = container_of(kobj, struct se_session, kobj);
+	struct session_attr *sess_attr = to_session(attr);
+
+	if (!sess_attr->show)
+		return -ENOSYS;
+
+	return sess_attr->show(se_sess, page);
+}
+
+static const struct sysfs_ops session_sysfs_ops = {
+	.show	= session_attr_show,
+	.store	= session_attr_store,
+};
+
+static struct kobj_type session_ktype = {
+	.sysfs_ops	= &session_sysfs_ops,
+	.release	= target_sysfs_session_release,
+	.default_groups	= session_groups,
+};
+
+void target_sysfs_init_session(struct se_session *se_sess)
+{
+	kobject_init(&se_sess->kobj, &session_ktype);
+}
+
+int target_sysfs_add_session(struct se_portal_group *se_tpg,
+			     struct se_session *se_sess,
+			     struct attribute_group *fabric_attrs)
+{
+	int ret;
+
+	ret = ida_simple_get(&se_tpg->se_tpg_wwn->sid_ida, 1, 0, GFP_KERNEL);
+	if (ret < 0) {
+		pr_err("Could not allocate session id. Error %d.\n", ret);
+		return ret;
+	}
+	se_sess->sid = ret;
+
+	ret = kobject_add(&se_sess->kobj, se_tpg->sessions_kobj, "session-%d",
+			  se_sess->sid);
+	if (ret) {
+		pr_err("Could not add session%d to sysfs. Error %d.\n",
+		       se_sess->sid, ret);
+		goto remove_id;
+	}
+
+	if (fabric_attrs) {
+		ret = sysfs_create_group(&se_sess->kobj, fabric_attrs);
+		if (ret)
+			goto del_kobj;
+		se_sess->fabric_attrs = fabric_attrs;	
+	}
+
+	return 0;
+
+del_kobj:
+	kobject_del(&se_sess->kobj);
+remove_id:
+	ida_simple_remove(&se_tpg->se_tpg_wwn->sid_ida, se_sess->sid);
+	return ret;
+}
+EXPORT_SYMBOL(target_sysfs_add_session);
+
+void target_sysfs_remove_session(struct se_session *se_sess)
+{
+	/* discovery sessions are normally not added to sysfs */
+	if (!se_sess->sid)
+		return;
+
+	if (se_sess->fabric_attrs)
+		sysfs_remove_group(&se_sess->kobj, se_sess->fabric_attrs);
+	ida_simple_remove(&se_sess->se_tpg->se_tpg_wwn->sid_ida, se_sess->sid);
+	kobject_del(&se_sess->kobj);
+}
+EXPORT_SYMBOL(target_sysfs_remove_session);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 594b724..fba059c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -257,6 +257,7 @@ struct se_session *transport_alloc_session(enum target_prot_op sup_prot_ops)
 		return ERR_PTR(ret);
 	}
 	se_sess->sup_prot_ops = sup_prot_ops;
+	target_sysfs_init_session(se_sess);
 
 	return se_sess;
 }
@@ -537,8 +538,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
 }
 EXPORT_SYMBOL(transport_deregister_session_configfs);
 
+
 void transport_free_session(struct se_session *se_sess)
 {
+	kobject_put(&se_sess->kobj);
+}
+EXPORT_SYMBOL(transport_free_session);
+
+void __target_free_session(struct se_session *se_sess)
+{
 	struct se_node_acl *se_nacl = se_sess->se_node_acl;
 
 	/*
@@ -582,7 +590,6 @@ void transport_free_session(struct se_session *se_sess)
 	percpu_ref_exit(&se_sess->cmd_count);
 	kmem_cache_free(se_sess_cache, se_sess);
 }
-EXPORT_SYMBOL(transport_free_session);
 
 static int target_release_res(struct se_device *dev, void *data)
 {
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 9d38b53..3bc2498 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -9,6 +9,7 @@
 #include <linux/semaphore.h>     /* struct semaphore */
 #include <linux/completion.h>
 #include <linux/kobject.h>
+#include <linux/idr.h>
 
 #define TARGET_CORE_VERSION		"v5.0"
 
@@ -624,6 +625,9 @@ struct se_session {
 	wait_queue_head_t	cmd_list_wq;
 	void			*sess_cmd_map;
 	struct sbitmap_queue	sess_tag_pool;
+	struct kobject		kobj;
+	int			sid;
+	struct attribute_group	*fabric_attrs;
 };
 
 struct se_device;
@@ -932,6 +936,7 @@ struct se_wwn {
 	struct config_group	wwn_group;
 	struct config_group	fabric_stat_group;
 	struct kobject		*kobj;
+	struct ida		sid_ida;
 };
 
 static inline void atomic_inc_mb(atomic_t *v)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 063f133..5948b87 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -146,7 +146,10 @@ void	transport_register_session(struct se_portal_group *,
 void	target_put_nacl(struct se_node_acl *);
 void	transport_deregister_session_configfs(struct se_session *);
 void	transport_deregister_session(struct se_session *);
-
+void	target_sysfs_remove_session(struct se_session *se_sess);
+int	target_sysfs_add_session(struct se_portal_group *se_tpg,
+				 struct se_session *se_sess,
+				 struct attribute_group *fabric_attrs);
 
 void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
-- 
1.8.3.1

Re: [RFC PATCH 2/5] target: add sysfs session helper functions

[Bart Van Assche]

On 2020-04-13 22:15, Mike Christie wrote:
> @@ -537,8 +538,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
>  }
>  EXPORT_SYMBOL(transport_deregister_session_configfs);
>  
> +

A single blank line is probably sufficient here?

>  void transport_free_session(struct se_session *se_sess)
>  {
> +	kobject_put(&se_sess->kobj);
> +}
> +EXPORT_SYMBOL(transport_free_session);
> +
> +void __target_free_session(struct se_session *se_sess)
> +{
>  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
>  
>  	/*
> @@ -582,7 +590,6 @@ void transport_free_session(struct se_session *se_sess)
>  	percpu_ref_exit(&se_sess->cmd_count);
>  	kmem_cache_free(se_sess_cache, se_sess);
>  }
> -EXPORT_SYMBOL(transport_free_session);

Does this patch defer execution of the code inside
transport_free_session() from when transport_free_session() is called to
when the last reference to a session is dropped? Can that have
unintended side effects? How about keeping most of the code that occurs
in transport_free_session() in that function and only freeing the memory
associated with the session if the last reference is dropped?

Thanks,

Bart.

Re: [RFC PATCH 2/5] target: add sysfs session helper functions

[Mike Christie]

On 04/14/2020 09:30 PM, Bart Van Assche wrote:
> On 2020-04-13 22:15, Mike Christie wrote:
>> @@ -537,8 +538,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
>>  }
>>  EXPORT_SYMBOL(transport_deregister_session_configfs);
>>  
>> +
> 
> A single blank line is probably sufficient here?
> 

Yes. That was a cut and paste mistake when I was separating the code
into patches. Will fix.


>>  void transport_free_session(struct se_session *se_sess)
>>  {
>> +	kobject_put(&se_sess->kobj);
>> +}
>> +EXPORT_SYMBOL(transport_free_session);
>> +
>> +void __target_free_session(struct se_session *se_sess)
>> +{
>>  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
>>  
>>  	/*
>> @@ -582,7 +590,6 @@ void transport_free_session(struct se_session *se_sess)
>>  	percpu_ref_exit(&se_sess->cmd_count);
>>  	kmem_cache_free(se_sess_cache, se_sess);
>>  }
>> -EXPORT_SYMBOL(transport_free_session);
> 
> Does this patch defer execution of the code inside
> transport_free_session() from when transport_free_session() is called to
> when the last reference to a session is dropped? Can that have

Yes.

> unintended side effects? How about keeping most of the code that occurs

Yes. For example, we drop the refcount on the ACL in
__target_free_session so that is now not done until the last session
rerfcount is done. I did this because we reference the acl in a sysfs file.


> in transport_free_session() in that function and only freeing the memory
> associated with the session if the last reference is dropped?
> 

I tried to minimize it already.

That is why I have the new session->fabric_free_cb in the next patch.
That way we do not need refcounts on structs like the tpg and can detach
that like normal in
transport_deregister_session/transport_deregister_session_configfs.

I will double check about what I can do about the ACL ref. We can do
things like copy the acl's name to the session, so we do not have to
reference the acl in sysfs.



> Thanks,
> 
> Bart.
> 

Re: [RFC PATCH 2/5] target: add sysfs session helper functions

[Mike Christie]

On 04/15/2020 12:35 PM, Mike Christie wrote:
> On 04/14/2020 09:30 PM, Bart Van Assche wrote:
>> On 2020-04-13 22:15, Mike Christie wrote:
>>> @@ -537,8 +538,15 @@ void transport_deregister_session_configfs(struct se_session *se_sess)
>>>  }
>>>  EXPORT_SYMBOL(transport_deregister_session_configfs);
>>>  
>>> +
>>
>> A single blank line is probably sufficient here?
>>
> 
> Yes. That was a cut and paste mistake when I was separating the code
> into patches. Will fix.
> 
> 
>>>  void transport_free_session(struct se_session *se_sess)
>>>  {
>>> +	kobject_put(&se_sess->kobj);
>>> +}
>>> +EXPORT_SYMBOL(transport_free_session);
>>> +
>>> +void __target_free_session(struct se_session *se_sess)
>>> +{
>>>  	struct se_node_acl *se_nacl = se_sess->se_node_acl;
>>>  
>>>  	/*
>>> @@ -582,7 +590,6 @@ void transport_free_session(struct se_session *se_sess)
>>>  	percpu_ref_exit(&se_sess->cmd_count);
>>>  	kmem_cache_free(se_sess_cache, se_sess);
>>>  }
>>> -EXPORT_SYMBOL(transport_free_session);
>>
>> Does this patch defer execution of the code inside
>> transport_free_session() from when transport_free_session() is called to
>> when the last reference to a session is dropped? Can that have
> 
> Yes.
> 
>> unintended side effects? How about keeping most of the code that occurs
> 
> Yes. For example, we drop the refcount on the ACL in
> __target_free_session so that is now not done until the last session
> rerfcount is done. I did this because we reference the acl in a sysfs file.
> 
> 
>> in transport_free_session() in that function and only freeing the memory
>> associated with the session if the last reference is dropped?
>>
> 
> I tried to minimize it already.
> 
> That is why I have the new session->fabric_free_cb in the next patch.
> That way we do not need refcounts on structs like the tpg and can detach
> that like normal in
> transport_deregister_session/transport_deregister_session_configfs.
> 

Oh yeah, James and Bart, while investigating Bart's comment I noticed
there is a bug where in the session->fabric_free_cb the fabric module
needs the fabric_sess_ptr but that will already have been cleared in
transport_deregister_session.

So James, you will hit a bug in there if you try to adapt elx to these
patches right now.

I will resend with that fixed and Bart's comments handled.

Re: [RFC PATCH 2/5] target: add sysfs session helper functions

[Bodo Stroesser]

On 04/14/20 07:15, Mike Christie wrote:
> +static ssize_t session_acl_show(struct se_session *se_sess, char *page)
> +{
> +	struct se_node_acl *acl;
> +	ssize_t len;
> +
> +	acl = se_sess->se_node_acl;
> +	if (!acl)
> +		return -ENOTCONN;
> +
> +	if (acl->dynamic_node_acl) {
> +		page[0] = '\0';
> +		len = 0;
> +	} else {
> +		len = snprintf(page, PAGE_SIZE, "%s\n", acl->initiatorname);
> +	}
> +
> +	return len;
> +}

Would it be a good idea to provide more info about initiators using
dynamic acl?

For example the file could be named "initiatorname" instead of "acl" and
always provide the initiatorname, while a boolean file "acl" could 
return "Y" or "1" for explicit acls, but "N" or "0" for dynamic acls.

Re: [RFC PATCH 2/5] target: add sysfs session helper functions

[Mike Christie]

On 04/20/2020 12:39 PM, Bodo Stroesser wrote:
> 
> 
> On 04/14/20 07:15, Mike Christie wrote:
>> +static ssize_t session_acl_show(struct se_session *se_sess, char *page)
>> +{
>> +    struct se_node_acl *acl;
>> +    ssize_t len;
>> +
>> +    acl = se_sess->se_node_acl;
>> +    if (!acl)
>> +        return -ENOTCONN;
>> +
>> +    if (acl->dynamic_node_acl) {
>> +        page[0] = '\0';
>> +        len = 0;
>> +    } else {
>> +        len = snprintf(page, PAGE_SIZE, "%s\n", acl->initiatorname);
>> +    }
>> +
>> +    return len;
>> +}
> 
> Would it be a good idea to provide more info about initiators using
> dynamic acl?
> 
> For example the file could be named "initiatorname" instead of "acl" and

I added this info in another dir/file. I was just about to post a update.

The acl is just a way to reference the configfs dir the info would be
located in since you can symlink.

> always provide the initiatorname, while a boolean file "acl" could
> return "Y" or "1" for explicit acls, but "N" or "0" for dynamic acls.

I was trying to not duplicate what is already in configfs.

[RFC PATCH 3/5] target: add target_setup_session sysfs support

[Mike Christie]

This adds a session dir per session for users of target_setup_session.

TODO: drivers like tcm_qla2xxx allocate resources in the
target_setup_session setup callback. I added a second callback
to release those resources in the error path or when all users
have drop their references.

I have not implemented the free_cb for tcm_qlaxxx, vhost, etc.

Drivers like elx that will want to export session level attrs
can also use the new callback to release their session resources
when all references have been dropped.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c    |  8 +++--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  4 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.c       |  3 +-
 drivers/target/Makefile                  |  1 +
 drivers/target/loopback/tcm_loop.c       |  3 +-
 drivers/target/sbp/sbp_target.c          |  4 +--
 drivers/target/target_core_transport.c   | 57 ++++++++++++++++++++++++++------
 drivers/target/tcm_fc/tfc_sess.c         |  2 +-
 drivers/usb/gadget/function/f_tcm.c      |  3 +-
 drivers/vhost/scsi.c                     |  4 +--
 drivers/xen/xen-scsiback.c               |  4 ++-
 include/target/target_core_base.h        |  1 +
 include/target/target_core_fabric.h      |  7 ++--
 13 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 9855274..a4f4a55 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2320,7 +2320,8 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 			break;
 		ch->sess = target_setup_session(&stpg->tpg, tag_num,
 						tag_size, TARGET_PROT_NORMAL,
-						ch->sess_name, ch, NULL);
+						ch->sess_name, NULL, ch, NULL,
+						NULL);
 	}
 	mutex_unlock(&sport->port_guid_id.mutex);
 
@@ -2330,13 +2331,14 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 			break;
 		ch->sess = target_setup_session(&stpg->tpg, tag_num,
 					tag_size, TARGET_PROT_NORMAL, i_port_id,
-					ch, NULL);
+					NULL, ch, NULL, NULL);
 		if (!IS_ERR_OR_NULL(ch->sess))
 			break;
 		/* Retry without leading "0x" */
 		ch->sess = target_setup_session(&stpg->tpg, tag_num,
 						tag_size, TARGET_PROT_NORMAL,
-						i_port_id + 2, ch, NULL);
+						i_port_id + 2, NULL, ch, NULL,
+						NULL);
 	}
 	mutex_unlock(&sport->port_gid_id.mutex);
 
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index d9e94e8..bf5dd4c 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2223,8 +2223,8 @@ static int ibmvscsis_make_nexus(struct ibmvscsis_tport *tport)
 	}
 
 	nexus->se_sess = target_setup_session(&tport->se_tpg, 0, 0,
-					      TARGET_PROT_NORMAL, name, nexus,
-					      NULL);
+					      TARGET_PROT_NORMAL, name, NULL,
+					      nexus, NULL, NULL);
 	if (IS_ERR(nexus->se_sess)) {
 		rc = PTR_ERR(nexus->se_sess);
 		goto transport_init_fail;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 1f0a185..27f968f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1484,7 +1484,8 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 	se_sess = target_setup_session(&tpg->se_tpg, num_tags,
 				       sizeof(struct qla_tgt_cmd),
 				       TARGET_PROT_ALL, port_name,
-				       qlat_sess, tcm_qla2xxx_session_cb);
+				       NULL, qlat_sess, tcm_qla2xxx_session_cb,
+				       NULL);
 	if (IS_ERR(se_sess))
 		return PTR_ERR(se_sess);
 
diff --git a/drivers/target/Makefile b/drivers/target/Makefile
index 4a7246e..1b7949d 100644
--- a/drivers/target/Makefile
+++ b/drivers/target/Makefile
@@ -5,6 +5,7 @@ target_core_mod-y		:= target_core_configfs.o \
 				   target_core_device.o \
 				   target_core_fabric_configfs.o \
 				   target_core_fabric_lib.o \
+				   target_core_sysfs.o \
 				   target_core_hba.o \
 				   target_core_pr.o \
 				   target_core_alua.o \
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 3305b47..6e2ebe9 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -742,7 +742,8 @@ static int tcm_loop_make_nexus(
 
 	tl_nexus->se_sess = target_setup_session(&tl_tpg->tl_se_tpg, 0, 0,
 					TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS,
-					name, tl_nexus, tcm_loop_alloc_sess_cb);
+					name, NULL, tl_nexus, tcm_loop_alloc_sess_cb,
+					NULL);
 	if (IS_ERR(tl_nexus->se_sess)) {
 		ret = PTR_ERR(tl_nexus->se_sess);
 		kfree(tl_nexus);
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index e4a9b9f..43aaf35 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -198,8 +198,8 @@ static struct sbp_session *sbp_session_create(
 
 	sess->se_sess = target_setup_session(&tpg->se_tpg, 128,
 					     sizeof(struct sbp_target_request),
-					     TARGET_PROT_NORMAL, guid_str,
-					     sess, NULL);
+					     TARGET_PROT_NORMAL, guid_str, NULL,
+					     sess, NULL, NULL);
 	if (IS_ERR(sess->se_sess)) {
 		pr_err("failed to init se_session\n");
 		ret = PTR_ERR(sess->se_sess);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index fba059c..2462eca 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -414,15 +414,35 @@ void transport_register_session(
 }
 EXPORT_SYMBOL(transport_register_session);
 
+/**
+ * target_setup_session - alloc and add a session to lio core
+ * @tpg: parent tpg
+ * @tag_num: if non-zero max num in-flight commands.
+ * @tag_size: if tag_num is non-zero, fabric driver's per cmd data in bytes.
+ * @sup_prot_ops: bitmask that defines which T10-PI modes are supported.
+ * @fabric_attrs: opt fabric driver session level attrs.
+ * @private: if setup_cb is non-NULL private will be passed to setup_cb.
+ * @setup_cb: opt function called before session has been added to lio core.
+ * @free_cb: function called during removal when all user refs have dropped.
+ *
+ * If the caller passes in a setup_cb that allocates resource a free_cb is
+ * required to free those resource during session removal.
+ *
+ * If the caller passes in fabric_attrs a free_cb is required, so resources
+ * it may access are freed when all users have dropped their references.
+ */
 struct se_session *
 target_setup_session(struct se_portal_group *tpg,
 		     unsigned int tag_num, unsigned int tag_size,
 		     enum target_prot_op prot_op,
-		     const char *initiatorname, void *private,
-		     int (*callback)(struct se_portal_group *,
-				     struct se_session *, void *))
+		     const char *initiatorname,
+		     struct attribute_group *fabric_attrs, void *private,
+		     int (*setup_cb)(struct se_portal_group *,
+				     struct se_session *, void *),
+		     void (*free_cb)(struct se_session *))
 {
 	struct se_session *sess;
+	int rc;
 
 	/*
 	 * If the fabric driver is using percpu-ida based pre allocation
@@ -439,23 +459,35 @@ struct se_session *
 	sess->se_node_acl = core_tpg_check_initiator_node_acl(tpg,
 					(unsigned char *)initiatorname);
 	if (!sess->se_node_acl) {
-		transport_free_session(sess);
-		return ERR_PTR(-EACCES);
+		rc = -EACCES;
+		goto free_session;
 	}
+
 	/*
 	 * Go ahead and perform any remaining fabric setup that is
 	 * required before transport_register_session().
 	 */
-	if (callback != NULL) {
-		int rc = callback(tpg, sess, private);
-		if (rc) {
-			transport_free_session(sess);
-			return ERR_PTR(rc);
-		}
+	if (setup_cb) {
+		rc = setup_cb(tpg, sess, private);
+		if (rc)
+			goto free_session;
 	}
 
 	transport_register_session(tpg, sess->se_node_acl, sess, private);
+
+	rc = target_sysfs_add_session(tpg, sess, fabric_attrs);
+	if (rc)
+		goto fabric_free_session;
+	sess->fabric_free_cb = free_cb;
+
 	return sess;
+
+fabric_free_session:
+	if (free_cb)
+		free_cb(sess);
+free_session:
+	transport_free_session(sess);
+	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL(target_setup_session);
 
@@ -549,6 +581,8 @@ void __target_free_session(struct se_session *se_sess)
 {
 	struct se_node_acl *se_nacl = se_sess->se_node_acl;
 
+	if (se_sess->fabric_free_cb)
+		se_sess->fabric_free_cb(se_sess);
 	/*
 	 * Drop the se_node_acl->nacl_kref obtained from within
 	 * core_tpg_get_initiator_node_acl().
@@ -639,6 +673,7 @@ void transport_deregister_session(struct se_session *se_sess)
 
 void target_remove_session(struct se_session *se_sess)
 {
+	target_sysfs_remove_session(se_sess);
 	transport_deregister_session_configfs(se_sess);
 	transport_deregister_session(se_sess);
 }
diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index 4fd6a1d..4973052 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -230,7 +230,7 @@ static struct ft_sess *ft_sess_create(struct ft_tport *tport, u32 port_id,
 	sess->se_sess = target_setup_session(se_tpg, TCM_FC_DEFAULT_TAGS,
 					     sizeof(struct ft_cmd),
 					     TARGET_PROT_NORMAL, &initiatorname[0],
-					     sess, ft_sess_alloc_cb);
+					     NULL, sess, ft_sess_alloc_cb, NULL);
 	if (IS_ERR(sess->se_sess)) {
 		int rc = PTR_ERR(sess->se_sess);
 		kfree(sess);
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 3650493..9363130 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1583,7 +1583,8 @@ static int tcm_usbg_make_nexus(struct usbg_tpg *tpg, char *name)
 						     USB_G_DEFAULT_SESSION_TAGS,
 						     sizeof(struct usbg_cmd),
 						     TARGET_PROT_NORMAL, name,
-						     tv_nexus, usbg_alloc_sess_cb);
+						     NULL, tv_nexus,
+						     usbg_alloc_sess_cb, NULL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
 #define MAKE_NEXUS_MSG "core_tpg_check_initiator_node_acl() failed for %s\n"
 		pr_debug(MAKE_NEXUS_MSG, name);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 7653667..d3db74c 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1964,8 +1964,8 @@ static int vhost_scsi_make_nexus(struct vhost_scsi_tpg *tpg,
 					VHOST_SCSI_DEFAULT_TAGS,
 					sizeof(struct vhost_scsi_cmd),
 					TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS,
-					(unsigned char *)name, tv_nexus,
-					vhost_scsi_nexus_cb);
+					(unsigned char *)name, NULL, tv_nexus,
+					vhost_scsi_nexus_cb, NULL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
 		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(tv_nexus);
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 75c0a2e..057d8a1 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1531,7 +1531,9 @@ static int scsiback_make_nexus(struct scsiback_tpg *tpg,
 						     VSCSI_DEFAULT_SESSION_TAGS,
 						     sizeof(struct vscsibk_pend),
 						     TARGET_PROT_NORMAL, name,
-						     tv_nexus, scsiback_alloc_sess_cb);
+						     NULL, tv_nexus,
+						     scsiback_alloc_sess_cb,
+						     NULL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
 		kfree(tv_nexus);
 		ret = -ENOMEM;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 3bc2498..783cca2 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -628,6 +628,7 @@ struct se_session {
 	struct kobject		kobj;
 	int			sid;
 	struct attribute_group	*fabric_attrs;
+	void (*fabric_free_cb)(struct se_session *);
 };
 
 struct se_device;
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 5948b87..9815dbf 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -127,9 +127,10 @@ struct target_core_fabric_ops {
 
 struct se_session *target_setup_session(struct se_portal_group *,
 		unsigned int, unsigned int, enum target_prot_op prot_op,
-		const char *, void *,
-		int (*callback)(struct se_portal_group *,
-				struct se_session *, void *));
+		const char *, struct attribute_group *, void *,
+		int (*setup_cb)(struct se_portal_group *,
+				struct se_session *, void *),
+		void (*free_cb)(struct se_session *));
 void target_remove_session(struct se_session *);
 
 int transport_init_session(struct se_session *se_sess);
-- 
1.8.3.1

Re: [RFC PATCH 3/5] target: add target_setup_session sysfs support

[Mike Christie]

On 04/14/2020 12:15 AM, Mike Christie wrote:
> +/**
> + * target_setup_session - alloc and add a session to lio core
> + * @tpg: parent tpg
> + * @tag_num: if non-zero max num in-flight commands.
> + * @tag_size: if tag_num is non-zero, fabric driver's per cmd data in bytes.
> + * @sup_prot_ops: bitmask that defines which T10-PI modes are supported.
> + * @fabric_attrs: opt fabric driver session level attrs.
> + * @private: if setup_cb is non-NULL private will be passed to setup_cb.
> + * @setup_cb: opt function called before session has been added to lio core.
> + * @free_cb: function called during removal when all user refs have dropped.
> + *
> + * If the caller passes in a setup_cb that allocates resource a free_cb is
> + * required to free those resource during session removal.
> + *
> + * If the caller passes in fabric_attrs a free_cb is required, so resources
> + * it may access are freed when all users have dropped their references.
> + */
>  struct se_session *
>  target_setup_session(struct se_portal_group *tpg,
>  		     unsigned int tag_num, unsigned int tag_size,
>  		     enum target_prot_op prot_op,
> -		     const char *initiatorname, void *private,
> -		     int (*callback)(struct se_portal_group *,
> -				     struct se_session *, void *))
> +		     const char *initiatorname,
> +		     struct attribute_group *fabric_attrs, void *private,
> +		     int (*setup_cb)(struct se_portal_group *,
> +				     struct se_session *, void *),
> +		     void (*free_cb)(struct se_session *))
>  {
>  	struct se_session *sess;
> +	int rc;
>  

Hey James,

For this, I just added a single attribute group. For elx will you want
to add multiple dirs under the session? Would just an array of attribute
groups work or do you want multiple nested dirs?

Re: [RFC PATCH 3/5] target: add target_setup_session sysfs support

[Bart Van Assche]

On 2020-04-13 22:15, Mike Christie wrote:
>  struct se_session *target_setup_session(struct se_portal_group *,
>  		unsigned int, unsigned int, enum target_prot_op prot_op,
> -		const char *, void *,
> -		int (*callback)(struct se_portal_group *,
> -				struct se_session *, void *));
> +		const char *, struct attribute_group *, void *,
> +		int (*setup_cb)(struct se_portal_group *,
> +				struct se_session *, void *),
> +		void (*free_cb)(struct se_session *));

The argument list of target_setup_session() is getting really long. How
about moving the attribute_group, setup_cb and free_cb arguments into
struct target_core_fabric_ops? Would that make it easier to extend
session sysfs attribute support in the future?

Thanks,

Bart.

Re: [RFC PATCH 3/5] target: add target_setup_session sysfs support

[Mike Christie]

On 04/14/2020 09:37 PM, Bart Van Assche wrote:
> On 2020-04-13 22:15, Mike Christie wrote:
>>  struct se_session *target_setup_session(struct se_portal_group *,
>>  		unsigned int, unsigned int, enum target_prot_op prot_op,
>> -		const char *, void *,
>> -		int (*callback)(struct se_portal_group *,
>> -				struct se_session *, void *));
>> +		const char *, struct attribute_group *, void *,
>> +		int (*setup_cb)(struct se_portal_group *,
>> +				struct se_session *, void *),
>> +		void (*free_cb)(struct se_session *));
> 
> The argument list of target_setup_session() is getting really long. How
> about moving the attribute_group, setup_cb and free_cb arguments into
> struct target_core_fabric_ops? Would that make it easier to extend

I agree.

> session sysfs attribute support in the future?
> 

Yeah, I can move those callbacks and the attribute_group to the
target_core_fabric_ops and it then it will work more similarly to the
other callout/attr handling.

[RFC PATCH 4/5] iscsi target: use session sysfs helpers

[Mike Christie]

The iscsi target login process breaks up session allocation and
registration, so it does not use target_setup_session like every one
else. This converts iscsi to use the session sysfs helpers and drops its
session id to use the common one.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c          |  4 ++--
 drivers/target/iscsi/iscsi_target_configfs.c |  4 +---
 drivers/target/iscsi/iscsi_target_login.c    | 16 ++--------------
 drivers/target/iscsi/iscsi_target_nego.c     | 20 ++++++++++++++++----
 drivers/target/iscsi/iscsi_target_stat.c     |  3 +--
 include/target/iscsi/iscsi_target_core.h     |  2 --
 6 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 59379d6..e8ea597 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -49,7 +49,6 @@
 static DEFINE_MUTEX(np_lock);
 
 static struct idr tiqn_idr;
-DEFINE_IDA(sess_ida);
 struct mutex auth_id_lock;
 
 struct iscsit_global *iscsit_global;
@@ -4359,6 +4358,8 @@ int iscsit_close_session(struct iscsi_session *sess)
 	iscsit_stop_time2retain_timer(sess);
 	spin_unlock_bh(&se_tpg->session_lock);
 
+	target_sysfs_remove_session(sess->se_sess);
+
 	/*
 	 * transport_deregister_session_configfs() will clear the
 	 * struct se_node_acl->nacl_sess pointer now as a iscsi_np process context
@@ -4403,7 +4404,6 @@ int iscsit_close_session(struct iscsi_session *sess)
 	pr_debug("Decremented number of active iSCSI Sessions on"
 		" iSCSI TPG: %hu to %u\n", tpg->tpgt, tpg->nsessions);
 
-	ida_free(&sess_ida, sess->session_index);
 	kfree(sess->sess_ops);
 	sess->sess_ops = NULL;
 	spin_unlock_bh(&se_tpg->session_lock);
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 0fa1d57..e9e06bb 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1344,9 +1344,7 @@ static int iscsi_get_cmd_state(struct se_cmd *se_cmd)
 
 static u32 lio_sess_get_index(struct se_session *se_sess)
 {
-	struct iscsi_session *sess = se_sess->fabric_sess_ptr;
-
-	return sess->session_index;
+	return se_sess->sid;
 }
 
 static u32 lio_sess_get_initiator_sid(
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 731ee67..619ea09 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -257,7 +257,6 @@ static int iscsi_login_zero_tsih_s1(
 {
 	struct iscsi_session *sess = NULL;
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
-	int ret;
 
 	sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL);
 	if (!sess) {
@@ -291,15 +290,6 @@ static int iscsi_login_zero_tsih_s1(
 	timer_setup(&sess->time2retain_timer,
 		    iscsit_handle_time2retain_timeout, 0);
 
-	ret = ida_alloc(&sess_ida, GFP_KERNEL);
-	if (ret < 0) {
-		pr_err("Session ID allocation failed %d\n", ret);
-		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-		goto free_sess;
-	}
-
-	sess->session_index = ret;
 	sess->creation_time = get_jiffies_64();
 	/*
 	 * The FFP CmdSN window values will be allocated from the TPG's
@@ -313,7 +303,7 @@ static int iscsi_login_zero_tsih_s1(
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
 		pr_err("Unable to allocate memory for"
 				" struct iscsi_sess_ops.\n");
-		goto free_id;
+		goto free_ops;
 	}
 
 	sess->se_sess = transport_alloc_session(TARGET_PROT_NORMAL);
@@ -327,8 +317,6 @@ static int iscsi_login_zero_tsih_s1(
 
 free_ops:
 	kfree(sess->sess_ops);
-free_id:
-	ida_free(&sess_ida, sess->session_index);
 free_sess:
 	kfree(sess);
 	conn->sess = NULL;
@@ -1183,8 +1171,8 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
 	if (!zero_tsih || !conn->sess)
 		goto old_sess_out;
 
+	target_sysfs_remove_session(conn->sess->se_sess);
 	transport_free_session(conn->sess->se_sess);
-	ida_free(&sess_ida, conn->sess->session_index);
 	kfree(conn->sess->sess_ops);
 	kfree(conn->sess);
 	conn->sess = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 685d771..17e12f3 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -1260,11 +1260,23 @@ int iscsi_target_locate_portal(
 	tag_size = sizeof(struct iscsi_cmd) + conn->conn_transport->priv_size;
 
 	ret = transport_alloc_session_tags(sess->se_sess, tag_num, tag_size);
-	if (ret < 0) {
-		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-				    ISCSI_LOGIN_STATUS_NO_RESOURCES);
-		ret = -1;
+	if (ret < 0)
+		goto return_oom;
+
+	if (conn->tpg != iscsit_global->discovery_tpg) {
+		ret = target_sysfs_add_session(&conn->tpg->tpg_se_tpg,
+						sess->se_sess, NULL);
+		if (ret)
+			/* tags and nacl released when session is freed */
+			goto return_oom;
 	}
+
+	goto out;
+
+return_oom:
+	iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
+			    ISCSI_LOGIN_STATUS_NO_RESOURCES);
+	ret = -1;
 out:
 	kfree(tmpbuf);
 	return ret;
diff --git a/drivers/target/iscsi/iscsi_target_stat.c b/drivers/target/iscsi/iscsi_target_stat.c
index 35e75a3..8167fdc 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -630,8 +630,7 @@ static ssize_t iscsi_stat_sess_indx_show(struct config_item *item, char *page)
 	if (se_sess) {
 		sess = se_sess->fabric_sess_ptr;
 		if (sess)
-			ret = snprintf(page, PAGE_SIZE, "%u\n",
-					sess->session_index);
+			ret = snprintf(page, PAGE_SIZE, "%u\n", se_sess->sid);
 	}
 	spin_unlock_bh(&se_nacl->nacl_sess_lock);
 
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 591cd9e..443c5af 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -657,8 +657,6 @@ struct iscsi_session {
 	/* LIO specific session ID */
 	u32			sid;
 	char			auth_type[8];
-	/* unique within the target */
-	int			session_index;
 	/* Used for session reference counting */
 	int			session_usage_count;
 	int			session_waiting_on_uc;
-- 
1.8.3.1

[RFC PATCH 5/5] target: drop sess_get_index

[Mike Christie]

LIO now handles session id allocation so drop the callout.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c        | 15 ---------------
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c     |  6 ------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c           |  7 -------
 drivers/target/iscsi/iscsi_target_configfs.c |  6 ------
 drivers/target/loopback/tcm_loop.c           |  6 ------
 drivers/target/sbp/sbp_target.c              |  6 ------
 drivers/target/target_core_configfs.c        |  4 ----
 drivers/target/target_core_stat.c            |  5 +----
 drivers/target/tcm_fc/tfc_conf.c             |  1 -
 drivers/target/tcm_fc/tfc_sess.c             |  7 -------
 drivers/usb/gadget/function/f_tcm.c          |  6 ------
 drivers/vhost/scsi.c                         |  6 ------
 drivers/xen/xen-scsiback.c                   |  6 ------
 include/target/target_core_fabric.h          |  1 -
 14 files changed, 1 insertion(+), 81 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index a4f4a55..3c7af9a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3353,20 +3353,6 @@ static void srpt_close_session(struct se_session *se_sess)
 	srpt_disconnect_ch_sync(ch);
 }
 
-/**
- * srpt_sess_get_index - return the value of scsiAttIntrPortIndex (SCSI-MIB)
- * @se_sess: SCSI target session.
- *
- * A quote from RFC 4455 (SCSI-MIB) about this MIB object:
- * This object represents an arbitrary integer used to uniquely identify a
- * particular attached remote initiator port to a particular SCSI target port
- * within a particular SCSI target device within a particular SCSI instance.
- */
-static u32 srpt_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static void srpt_set_default_node_attrs(struct se_node_acl *nacl)
 {
 }
@@ -3843,7 +3829,6 @@ static ssize_t srpt_wwn_version_show(struct config_item *item, char *buf)
 	.release_cmd			= srpt_release_cmd,
 	.check_stop_free		= srpt_check_stop_free,
 	.close_session			= srpt_close_session,
-	.sess_get_index			= srpt_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= srpt_write_pending,
 	.set_default_node_attributes	= srpt_set_default_node_attrs,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index bf5dd4c..ac74986 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -3739,11 +3739,6 @@ static void ibmvscsis_release_cmd(struct se_cmd *se_cmd)
 	spin_unlock_bh(&vscsi->intr_lock);
 }
 
-static u32 ibmvscsis_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static int ibmvscsis_write_pending(struct se_cmd *se_cmd)
 {
 	struct ibmvscsis_cmd *cmd = container_of(se_cmd, struct ibmvscsis_cmd,
@@ -4034,7 +4029,6 @@ static ssize_t ibmvscsis_tpg_enable_store(struct config_item *item,
 	.tpg_get_inst_index		= ibmvscsis_tpg_get_inst_index,
 	.check_stop_free		= ibmvscsis_check_stop_free,
 	.release_cmd			= ibmvscsis_release_cmd,
-	.sess_get_index			= ibmvscsis_sess_get_index,
 	.write_pending			= ibmvscsis_write_pending,
 	.set_default_node_attributes	= ibmvscsis_set_default_node_attrs,
 	.get_cmd_state			= ibmvscsis_get_cmd_state,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 27f968f..80fbb55 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -377,11 +377,6 @@ static void tcm_qla2xxx_close_session(struct se_session *se_sess)
 	tcm_qla2xxx_put_sess(sess);
 }
 
-static u32 tcm_qla2xxx_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,
@@ -1852,7 +1847,6 @@ static ssize_t tcm_qla2xxx_wwn_version_show(struct config_item *item,
 	.check_stop_free		= tcm_qla2xxx_check_stop_free,
 	.release_cmd			= tcm_qla2xxx_release_cmd,
 	.close_session			= tcm_qla2xxx_close_session,
-	.sess_get_index			= tcm_qla2xxx_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= tcm_qla2xxx_write_pending,
 	.set_default_node_attributes	= tcm_qla2xxx_set_default_node_attrs,
@@ -1892,7 +1886,6 @@ static ssize_t tcm_qla2xxx_wwn_version_show(struct config_item *item,
 	.check_stop_free                = tcm_qla2xxx_check_stop_free,
 	.release_cmd			= tcm_qla2xxx_release_cmd,
 	.close_session			= tcm_qla2xxx_close_session,
-	.sess_get_index			= tcm_qla2xxx_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= tcm_qla2xxx_write_pending,
 	.set_default_node_attributes	= tcm_qla2xxx_set_default_node_attrs,
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index e9e06bb..c4cdea5 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1342,11 +1342,6 @@ static int iscsi_get_cmd_state(struct se_cmd *se_cmd)
 	return cmd->i_state;
 }
 
-static u32 lio_sess_get_index(struct se_session *se_sess)
-{
-	return se_sess->sid;
-}
-
 static u32 lio_sess_get_initiator_sid(
 	struct se_session *se_sess,
 	unsigned char *buf,
@@ -1542,7 +1537,6 @@ static void lio_release_cmd(struct se_cmd *se_cmd)
 	.check_stop_free		= lio_check_stop_free,
 	.release_cmd			= lio_release_cmd,
 	.close_session			= lio_tpg_close_session,
-	.sess_get_index			= lio_sess_get_index,
 	.sess_get_initiator_sid		= lio_sess_get_initiator_sid,
 	.write_pending			= lio_write_pending,
 	.set_default_node_attributes	= lio_set_default_node_attributes,
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 6e2ebe9..c806085 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -512,11 +512,6 @@ static u32 tcm_loop_get_inst_index(struct se_portal_group *se_tpg)
 	return 1;
 }
 
-static u32 tcm_loop_sess_get_index(struct se_session *se_sess)
-{
-	return 1;
-}
-
 static void tcm_loop_set_default_node_attributes(struct se_node_acl *se_acl)
 {
 	return;
@@ -1138,7 +1133,6 @@ static ssize_t tcm_loop_wwn_version_show(struct config_item *item, char *page)
 	.tpg_get_inst_index		= tcm_loop_get_inst_index,
 	.check_stop_free		= tcm_loop_check_stop_free,
 	.release_cmd			= tcm_loop_release_cmd,
-	.sess_get_index			= tcm_loop_sess_get_index,
 	.write_pending			= tcm_loop_write_pending,
 	.set_default_node_attributes	= tcm_loop_set_default_node_attributes,
 	.get_cmd_state			= tcm_loop_get_cmd_state,
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 43aaf35..043842f 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1708,11 +1708,6 @@ static void sbp_release_cmd(struct se_cmd *se_cmd)
 	sbp_free_request(req);
 }
 
-static u32 sbp_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static int sbp_write_pending(struct se_cmd *se_cmd)
 {
 	struct sbp_target_request *req = container_of(se_cmd,
@@ -2309,7 +2304,6 @@ static ssize_t sbp_tpg_attrib_max_logins_per_lun_store(struct config_item *item,
 	.tpg_check_prod_mode_write_protect = sbp_check_false,
 	.tpg_get_inst_index		= sbp_tpg_get_inst_index,
 	.release_cmd			= sbp_release_cmd,
-	.sess_get_index			= sbp_sess_get_index,
 	.write_pending			= sbp_write_pending,
 	.set_default_node_attributes	= sbp_set_default_node_attrs,
 	.get_cmd_state			= sbp_get_cmd_state,
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 3fd08c5..638060f 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -399,10 +399,6 @@ static int target_fabric_tf_ops_check(const struct target_core_fabric_ops *tfo)
 		pr_err("Missing tfo->release_cmd()\n");
 		return -EINVAL;
 	}
-	if (!tfo->sess_get_index) {
-		pr_err("Missing tfo->sess_get_index()\n");
-		return -EINVAL;
-	}
 	if (!tfo->write_pending) {
 		pr_err("Missing tfo->write_pending()\n");
 		return -EINVAL;
diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
index 237309d..2aeb843 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -1264,7 +1264,6 @@ static ssize_t target_stat_iport_indx_show(struct config_item *item,
 	struct se_lun_acl *lacl = iport_to_lacl(item);
 	struct se_node_acl *nacl = lacl->se_lun_nacl;
 	struct se_session *se_sess;
-	struct se_portal_group *tpg;
 	ssize_t ret;
 
 	spin_lock_irq(&nacl->nacl_sess_lock);
@@ -1274,10 +1273,8 @@ static ssize_t target_stat_iport_indx_show(struct config_item *item,
 		return -ENODEV;
 	}
 
-	tpg = nacl->se_tpg;
 	/* scsiAttIntrPortIndex */
-	ret = snprintf(page, PAGE_SIZE, "%u\n",
-			tpg->se_tpg_tfo->sess_get_index(se_sess));
+	ret = snprintf(page, PAGE_SIZE, "%u\n", se_sess->sid);
 	spin_unlock_irq(&nacl->nacl_sess_lock);
 	return ret;
 }
diff --git a/drivers/target/tcm_fc/tfc_conf.c b/drivers/target/tcm_fc/tfc_conf.c
index 1a38c98..ff18e0a 100644
--- a/drivers/target/tcm_fc/tfc_conf.c
+++ b/drivers/target/tcm_fc/tfc_conf.c
@@ -426,7 +426,6 @@ static u32 ft_tpg_get_inst_index(struct se_portal_group *se_tpg)
 	.check_stop_free =		ft_check_stop_free,
 	.release_cmd =			ft_release_cmd,
 	.close_session =		ft_sess_close,
-	.sess_get_index =		ft_sess_get_index,
 	.sess_get_initiator_sid =	NULL,
 	.write_pending =		ft_write_pending,
 	.set_default_node_attributes =	ft_set_default_node_attr,
diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index 4973052..5102be5 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -325,13 +325,6 @@ void ft_sess_close(struct se_session *se_sess)
 	synchronize_rcu();		/* let transport deregister happen */
 }
 
-u32 ft_sess_get_index(struct se_session *se_sess)
-{
-	struct ft_sess *sess = se_sess->fabric_sess_ptr;
-
-	return sess->port_id;	/* XXX TBD probably not what is needed */
-}
-
 u32 ft_sess_get_port_name(struct se_session *se_sess,
 			  unsigned char *buf, u32 len)
 {
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 9363130..7d016e2 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1287,11 +1287,6 @@ static void usbg_release_cmd(struct se_cmd *se_cmd)
 	target_free_tag(se_sess, se_cmd);
 }
 
-static u32 usbg_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static void usbg_set_default_node_attrs(struct se_node_acl *nacl)
 {
 }
@@ -1715,7 +1710,6 @@ static int usbg_check_stop_free(struct se_cmd *se_cmd)
 	.tpg_check_prod_mode_write_protect = usbg_check_false,
 	.tpg_get_inst_index		= usbg_tpg_get_inst_index,
 	.release_cmd			= usbg_release_cmd,
-	.sess_get_index			= usbg_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= usbg_send_write_request,
 	.set_default_node_attributes	= usbg_set_default_node_attrs,
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index d3db74c..e2ddbc3 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -340,11 +340,6 @@ static void vhost_scsi_release_cmd(struct se_cmd *se_cmd)
 	target_free_tag(se_sess, se_cmd);
 }
 
-static u32 vhost_scsi_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static int vhost_scsi_write_pending(struct se_cmd *se_cmd)
 {
 	/* Go ahead and process the write immediately */
@@ -2290,7 +2285,6 @@ static void vhost_scsi_drop_tport(struct se_wwn *wwn)
 	.tpg_get_inst_index		= vhost_scsi_tpg_get_inst_index,
 	.release_cmd			= vhost_scsi_release_cmd,
 	.check_stop_free		= vhost_scsi_check_stop_free,
-	.sess_get_index			= vhost_scsi_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= vhost_scsi_write_pending,
 	.set_default_node_attributes	= vhost_scsi_set_default_node_attrs,
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 057d8a1..cd37c59 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1392,11 +1392,6 @@ static void scsiback_release_cmd(struct se_cmd *se_cmd)
 	target_free_tag(se_cmd->se_sess, se_cmd);
 }
 
-static u32 scsiback_sess_get_index(struct se_session *se_sess)
-{
-	return 0;
-}
-
 static int scsiback_write_pending(struct se_cmd *se_cmd)
 {
 	/* Go ahead and process the write immediately */
@@ -1813,7 +1808,6 @@ static int scsiback_check_false(struct se_portal_group *se_tpg)
 	.tpg_get_inst_index		= scsiback_tpg_get_inst_index,
 	.check_stop_free		= scsiback_check_stop_free,
 	.release_cmd			= scsiback_release_cmd,
-	.sess_get_index			= scsiback_sess_get_index,
 	.sess_get_initiator_sid		= NULL,
 	.write_pending			= scsiback_write_pending,
 	.set_default_node_attributes	= scsiback_set_default_node_attrs,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 9815dbf..6060eb3 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -66,7 +66,6 @@ struct target_core_fabric_ops {
 	int (*check_stop_free)(struct se_cmd *);
 	void (*release_cmd)(struct se_cmd *);
 	void (*close_session)(struct se_session *);
-	u32 (*sess_get_index)(struct se_session *);
 	/*
 	 * Used only for SCSI fabrics that contain multi-value TransportIDs
 	 * (like iSCSI).  All other SCSI fabrics should set this to NULL.
-- 
1.8.3.1