[11/15] target: export initiator port values for all sessions
diff mbox

Message ID 1531696591-8558-12-git-send-email-mchristi@redhat.com
State New, archived
Headers show

Commit Message

Mike Christie July 15, 2018, 11:16 p.m. UTC
Export the initiator port info in configfs

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/target_core_fabric_configfs.c | 48 ++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Bart Van Assche July 18, 2018, 10:41 p.m. UTC | #1
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> Export the initiator port info in configfs

Does configfs support soft links? Can this information be exported as a
soft link from the session directory to the struct se_portal_group configfs
object?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie July 18, 2018, 11:04 p.m. UTC | #2
On 07/18/2018 05:41 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> Export the initiator port info in configfs
> 
> Does configfs support soft links? Can this information be exported as a
> soft link from the session directory to the struct se_portal_group configfs
> object?
> 

If you just needed to export the initiator name or if a single session
per initiator can be connected to a tpg then it would work ok.

The problem is for iscsi the scsi initiator port / transport id, is the
initiator name and isid. The isid is just a 48 bit number and the
initiator will allocate a new value for every session. So on the
initiator side if there are multiple nics, then it is common to create a
session through nic and each session will have the same initiator name
but different isids. So at some place you need to put multiple files to
export the different isids or indicate to userspace tools that there is
more than one session connected to that tpg.


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie July 19, 2018, 2:15 a.m. UTC | #3
On 07/18/2018 06:04 PM, Mike Christie wrote:
> On 07/18/2018 05:41 PM, Bart Van Assche wrote:
>> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>>> Export the initiator port info in configfs
>>
>> Does configfs support soft links? Can this information be exported as a
>> soft link from the session directory to the struct se_portal_group configfs
>> object?
>>
> 
> If you just needed to export the initiator name or if a single session
> per initiator can be connected to a tpg then it would work ok.
> 
> The problem is for iscsi the scsi initiator port / transport id, is the
> initiator name and isid. The isid is just a 48 bit number and the
> initiator will allocate a new value for every session. So on the
> initiator side if there are multiple nics, then it is common to create a
> session through nic and each session will have the same initiator name
> but different isids. So at some place you need to put multiple files to
> export the different isids or indicate to userspace tools that there is
> more than one session connected to that tpg.
>

Oh wait, I think I know what you mean. Did you want something like this
where the symlink name is the info in the initiator_port file like this:

[tpgt_1]# tree -L 2
.
`-- sessions
    `-- 1
    |   `-- iqn.2005-03.com.ceph:ini1,i,0x00023d000001 -> ../../tpgt_1
     `--2
.       `-- iqn.2005-03.com.ceph:ini1,i,0x00023d000002 -> ../../tpgt_1

If that is what you are asking about, I did not get why we want to link
to the tpg object, because we already know the tpg since it is the
parent of the session dir.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche July 19, 2018, 3:37 p.m. UTC | #4
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> +	if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
> +		len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
> +			       se_sess->se_node_acl->initiatorname,
> +			       &se_sess->sess_bin_isid);
> +	} else {
> +		len = snprintf(page, PAGE_SIZE, "%s\n",
> +			       se_sess->se_node_acl->initiatorname);
> +	}

Hello Mike,

The general recommendation for configfs is that each attribute contains a
single value, just like for sysfs. Patch 11/15 exports two values through
a single attribute. Have you considered to split the above into two
attributes, namely the initiator name and the ISID? Can the initiator name
be changed into a soft link to the se_node_acl configfs directory to make
it easy for shell scripts to retrieve additional initiator configuration
information?

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche July 19, 2018, 3:38 p.m. UTC | #5
On Wed, 2018-07-18 at 21:15 -0500, Mike Christie wrote:
> Oh wait, I think I know what you mean.

I should have written "se_node_acl" instead of "se_portal_group".

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie July 19, 2018, 4:38 p.m. UTC | #6
On 07/19/2018 10:37 AM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> +	if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
>> +		len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
>> +			       se_sess->se_node_acl->initiatorname,
>> +			       &se_sess->sess_bin_isid);
>> +	} else {
>> +		len = snprintf(page, PAGE_SIZE, "%s\n",
>> +			       se_sess->se_node_acl->initiatorname);
>> +	}
> 
> Hello Mike,
> 
> The general recommendation for configfs is that each attribute contains a
> single value, just like for sysfs. Patch 11/15 exports two values through
> a single attribute. Have you considered to split the above into two

What about just making it the initiator port transport id so it aligns
with the get_initiator_port_transport_id() comment for the other patch.
For iscsi it would be 1 value with the format from SPC/SAM
"target_name,i,0x,isid"?


> attributes, namely the initiator name and the ISID? Can the initiator name
> be changed into a soft link to the se_node_acl configfs directory to make
> it easy for shell scripts to retrieve additional initiator configuration
> information?

Because the kernel is creating the session instead of it being driven
from a mkdir, there are no existing functions for this. I do not know
configfs code well, but I think making a function to do this is possible
though.

What about the dynamic_acl case? Just make those a normal file?

Just to make sure we are on the same page too. The initiator name is
always the name of the acl right? It looked like that from
target_fabric_make_nodeacl but I was wondering if you are asking for the
symlink because there are some fabric module quirks where that is not
the case. If it's the same names, then you know the acl already from the
initiator name file.

> 
> Thanks,
> 
> Bart.
> 

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche July 19, 2018, 5:07 p.m. UTC | #7
On Thu, 2018-07-19 at 11:38 -0500, Mike Christie wrote:
> On 07/19/2018 10:37 AM, Bart Van Assche wrote:
> > The general recommendation for configfs is that each attribute contains a
> > single value, just like for sysfs. Patch 11/15 exports two values through
> > a single attribute. Have you considered to split the above into two
> 
> What about just making it the initiator port transport id so it aligns
> with the get_initiator_port_transport_id() comment for the other patch.
> For iscsi it would be 1 value with the format from SPC/SAM
> "target_name,i,0x,isid"?

That sounds fine to me.

> > attributes, namely the initiator name and the ISID? Can the initiator name
> > be changed into a soft link to the se_node_acl configfs directory to make
> > it easy for shell scripts to retrieve additional initiator configuration
> > information?
> 
> Because the kernel is creating the session instead of it being driven
> from a mkdir, there are no existing functions for this. I do not know
> configfs code well, but I think making a function to do this is possible
> though.

Initially configfs did not support creation of a directory from the kernel
side. Last time I brought this up with Christoph he replied that this
functionality has been added to configfs (if I understood Christoph
correctly).

> What about the dynamic_acl case? Just make those a normal file?

I'm not that familiar with dynamic ACLs. Is there a difference between
"dynamic ACL" generation and "demo mode"? How does this interact with the
generate_node_acls mode?
 
> Just to make sure we are on the same page too. The initiator name is
> always the name of the acl right? It looked like that from
> target_fabric_make_nodeacl but I was wondering if you are asking for the
> symlink because there are some fabric module quirks where that is not
> the case. If it's the same names, then you know the acl already from the
> initiator name file.

It depends on what is called the "initiator name". E.g. the SRP target
driver supports multiple formats to refer to a single initiator port. The
first version of the ib_srpt driver supported only 128-bit GIDs as initiator
name. Since the 64-bit prefix of a GID may change due to a reboot, later
on support for 64-bit GUIDs was added. During login three formats for
the initiator name are verified: GID, GUID without "0x" prefix and GUID
with "0x" prefix. In any case, target_alloc_session() will store a
pointer to the first struct se_node_acl that matches in sess->se_node_acl.
I think using the name stored in struct se_node_acl is fine in all cases.

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig July 19, 2018, 8:47 p.m. UTC | #8
On Thu, Jul 19, 2018 at 05:07:59PM +0000, Bart Van Assche wrote:
> Initially configfs did not support creation of a directory from the kernel
> side. Last time I brought this up with Christoph he replied that this
> functionality has been added to configfs (if I understood Christoph
> correctly).

I don't think the functionality was ever missing, but I might be
mistaken.  You can always call config_group_init_type_name() +
configfs_add_default_group to add a directory.  nvmet makes heavy
use of that.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie July 19, 2018, 9:30 p.m. UTC | #9
On 07/19/2018 03:47 PM, Christoph Hellwig wrote:
> On Thu, Jul 19, 2018 at 05:07:59PM +0000, Bart Van Assche wrote:
>> Initially configfs did not support creation of a directory from the kernel
>> side. Last time I brought this up with Christoph he replied that this
>> functionality has been added to configfs (if I understood Christoph
>> correctly).
> 
> I don't think the functionality was ever missing, but I might be
> mistaken.  You can always call config_group_init_type_name() +
> configfs_add_default_group to add a directory.  nvmet makes heavy
> use of that.

Just to clarify. We can create a dir from the kernel already. It is no
problem. I am doing that in this patchset with configfs_register_group.

What Bart was requesting originally and what is missing is being able to
add a symlink from the kernel.

I have not fully looked into it, but I think it would be something like
taking part of configfs_symlink and making it so we can call it with
config_items for the 2 items to be symlinked.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig July 20, 2018, 3:10 p.m. UTC | #10
On Thu, Jul 19, 2018 at 04:30:36PM -0500, Mike Christie wrote:
> Just to clarify. We can create a dir from the kernel already. It is no
> problem. I am doing that in this patchset with configfs_register_group.
> 
> What Bart was requesting originally and what is missing is being able to
> add a symlink from the kernel.
> 
> I have not fully looked into it, but I think it would be something like
> taking part of configfs_symlink and making it so we can call it with
> config_items for the 2 items to be symlinked.

Yes, it shouldn't be too hard.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie Aug. 1, 2018, 4:44 p.m. UTC | #11
On 07/19/2018 12:07 PM, Bart Van Assche wrote:
> On Thu, 2018-07-19 at 11:38 -0500, Mike Christie wrote:
>> On 07/19/2018 10:37 AM, Bart Van Assche wrote:
>>> The general recommendation for configfs is that each attribute contains a
>>> single value, just like for sysfs. Patch 11/15 exports two values through
>>> a single attribute. Have you considered to split the above into two
>>
>> What about just making it the initiator port transport id so it aligns
>> with the get_initiator_port_transport_id() comment for the other patch.
>> For iscsi it would be 1 value with the format from SPC/SAM
>> "target_name,i,0x,isid"?
> 
> That sounds fine to me.
> 
>>> attributes, namely the initiator name and the ISID? Can the initiator name
>>> be changed into a soft link to the se_node_acl configfs directory to make
>>> it easy for shell scripts to retrieve additional initiator configuration
>>> information?
>>
>> Because the kernel is creating the session instead of it being driven
>> from a mkdir, there are no existing functions for this. I do not know
>> configfs code well, but I think making a function to do this is possible
>> though.
> 
> Initially configfs did not support creation of a directory from the kernel
> side. Last time I brought this up with Christoph he replied that this
> functionality has been added to configfs (if I understood Christoph
> correctly).
> 
>> What about the dynamic_acl case? Just make those a normal file?
> 
> I'm not that familiar with dynamic ACLs. Is there a difference between
> "dynamic ACL" generation and "demo mode"? How does this interact with the
> generate_node_acls mode?

Ah sorry, I think I made up that term. I was referring to when
se_node_acl->dynamic_node_acl=1, so generate_node_acls=1 and demo mode
and dynamic_node_acl=1 is the same state.

>  
>> Just to make sure we are on the same page too. The initiator name is
>> always the name of the acl right? It looked like that from
>> target_fabric_make_nodeacl but I was wondering if you are asking for the
>> symlink because there are some fabric module quirks where that is not
>> the case. If it's the same names, then you know the acl already from the
>> initiator name file.
> 
> It depends on what is called the "initiator name". E.g. the SRP target
> driver supports multiple formats to refer to a single initiator port. The
> first version of the ib_srpt driver supported only 128-bit GIDs as initiator
> name. Since the 64-bit prefix of a GID may change due to a reboot, later
> on support for 64-bit GUIDs was added. During login three formats for
> the initiator name are verified: GID, GUID without "0x" prefix and GUID
> with "0x" prefix. In any case, target_alloc_session() will store a
> pointer to the first struct se_node_acl that matches in sess->se_node_acl.
> I think using the name stored in struct se_node_acl is fine in all cases.
> 

Hey Bart,

I did patches to add symlinks. There is one problem that this will break
userspace though.

The userspace tools assume that they can tear down a tpgt while sessions
are running because currently a rmdir on the acl will force running
sessions to be shutdown. For example, a FC or iscsi initiator can be
logged into the target and userspace currently does something like this
simplified sequence:

for each object under a tpgt
    ulink luns
    rmdir luns
    rmdir acls
    rmdir tpt

The problem is that because the session has a symlink to the acl and
configfs has done a config_item_get on the acl config_item to make sure
it does not get freed while in use the "rmdir acl" will now fail.

I agree knowing the acl info is useful, so how about the following:

1. Create a file named "acl" in the session's dir.
2. For dynamic_node_acl=0 the acl file will return a empty string or
"generate_node_acls=1" or "demo mode enabled".
3. For dynamic_node_acl=1 the acl file will return
se_node_acl->initiatorname which is the name of the acl in
..../tpgt_$N/acls/.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Christie Aug. 1, 2018, 5:11 p.m. UTC | #12
On 08/01/2018 11:44 AM, Mike Christie wrote:
> 1. Create a file named "acl" in the session's dir.
> 2. For dynamic_node_acl=0 the acl file will return a empty string or

Miswrote this.

Above should be dynamic_node_acl=1

> "generate_node_acls=1" or "demo mode enabled".
> 3. For dynamic_node_acl=1 the acl file will return

This should be dynamic_node_acl=0

> se_node_acl->initiatorname which is the name of the acl in
> ..../tpgt_$N/acls/.
> 

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Aug. 1, 2018, 5:38 p.m. UTC | #13
On Wed, 2018-08-01 at 11:44 -0500, Mike Christie wrote:
> I agree knowing the acl info is useful, so how about the following:
> 
> 1. Create a file named "acl" in the session's dir.
> 2. For dynamic_node_acl=0 the acl file will return a empty string or
> "generate_node_acls=1" or "demo mode enabled".
> 3. For dynamic_node_acl=1 the acl file will return
> se_node_acl->initiatorname which is the name of the acl in
> ..../tpgt_$N/acls/.

Hello Mike,

That sounds fine to me. My personal preference is that the "acl" file is
empty if demo mode is enabled.

Thanks,

Bart.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 497fa01..2deda28 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -897,6 +897,53 @@  static void target_fabric_drop_tpg(
 	config_item_put(item);
 }
 
+static ssize_t target_fabric_session_initiator_port_show(
+	struct config_item *item, char *page)
+{
+	struct se_session *se_sess = container_of(to_config_group(item),
+						  struct se_session, group);
+	ssize_t len;
+	int isid_len = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&se_sess->configfs_lock, flags);
+	if (!se_sess->se_tpg) {
+		len = -ENOTCONN;
+		goto unlock;
+	}
+
+	if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid)
+		isid_len = 15;
+
+	if (strlen(se_sess->se_node_acl->initiatorname) + isid_len + 1 >
+	    PAGE_SIZE) {
+		len = -EINVAL;
+		goto unlock;
+	}
+
+	if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
+		len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
+			       se_sess->se_node_acl->initiatorname,
+			       &se_sess->sess_bin_isid);
+	} else {
+		len = snprintf(page, PAGE_SIZE, "%s\n",
+			       se_sess->se_node_acl->initiatorname);
+	}
+	len += 1; /* Include NULL terminator */
+
+unlock:
+	spin_unlock_irqrestore(&se_sess->configfs_lock, flags);
+
+	return len;
+}
+
+CONFIGFS_ATTR_RO(target_fabric_session_, initiator_port);
+
+static struct configfs_attribute *target_fabric_session_attrs[] = {
+	&target_fabric_session_attr_initiator_port,
+	NULL,
+};
+
 static void target_fabric_session_release(struct config_item *item)
 {
 	struct se_session *se_sess = container_of(to_config_group(item),
@@ -918,6 +965,7 @@  int target_fabric_init_session(struct se_session *se_sess)
 
 	se_sess->cit.ct_owner = THIS_MODULE;
 	se_sess->cit.ct_item_ops = &target_session_item_ops;
+	se_sess->cit.ct_attrs = target_fabric_session_attrs;
 	config_group_init_type_name(&se_sess->group, name, &se_sess->cit);
 	kfree(name);
 	return 0;