| Message ID | 1531696591-8558-12-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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.
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.
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.
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.
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. >
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.
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.
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.
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.
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/.
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/. >
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.
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;
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(+)