> On Jul 3, 2020, at 11:57 AM, Bodo Stroesser <bstroesser@xxxxxxxxxxxxxx> wrote:
>
> On 2020-07-03 18:27, Mike Christie wrote:
>> On 7/3/20 7:51 AM, Bodo Stroesser wrote:
>>> On 2020-06-27 06:35, Mike Christie wrote:
>>>> This patch exports the LIO sessions via configfs. If userspace makes
>>>> a "sessions" dir on the ACL or TPG dir to indicate to the kernel it
>>>> supports the new interface on that TPG, then the kernel will make a
>>>> dir per session in the tpg/sessions or tpg/acls/alc/sessions dir
>>>
>>> I someone creates a new ACL on a running tpg, can it happen that a
>>> session already is created before user can create the sessions folder?
>>
>> Right now yes. In the patch I started to try to support a per tpg mix. If a session exists then you do mkdir sessions, then before you can delete the tpg you have to delete the sessions that were created after the mkdir.
>>
>> But to handle all the side cases, it becomes a pain, and I don't think anyone will ever use that feature, so I plan to make it either on or off for all sessions on the tpg and no mixing. I think normally we see different tools at the per target or per fabric level, so we should be ok.
>
> Maybe I don't get it. What I meant is:
>
> Creation of the "sessions" folders in tpg and acl has to be done by
> user.
> If user creates a new acl while tpg is active, the acl immediately
> becomes active, I think.
> If user then creates the "sessions" folder, it could happen that
> a session already is started meanwhile. That session would be
> invisible then.
> Maybe an "enable" attr for acl would help?
I see what you are saying. See below.
>> No. It works the same as today. When you do a tpg removal like when you do
>>
>> targetlci clearconfig
>>
>> rtslib disables the tpg which prevents the target from creating new sessions. We then bring down the objects under it like luns, portals, etc. When we get to sessions, if the target has not yet brought them down (some targets do this on tpg disablement and some do not), then with that github patch rtslib will kill them like it does for the other objects.
>>
>
> Ahh, I see.
>
>
>
>
> Let me resend another part of my response, that was far down:
>
>> }
>> +int target_cfgfs_register_session(struct se_portal_group *se_tpg,
>> + struct se_session *se_sess)
>> +{
>> + struct se_node_acl *se_nacl;
>> + int ret;
>> +
>> + /*
>> + * If the fabric doesn't support close_session, there's no way for
>> + * userspace to clean up the session during nacl/tpg deletion.
>> + */
>> + if (!se_tpg->cfgfs_sess_supp || !se_tpg->se_tpg_tfo->close_session)
>> + return 0;
>
> Why is the cfgfs_sess_supp flag per tpg? It seems to be set if either
> tpg/sessions or any acl/sessions folder is created.
> So what will happen here if e.g session for an acl is created while
> only tpg/sessions exists?
Ah yeah, that is bogus. I am still working on an issue in this code. I wasn’t expecting a line by line and just a general review of cfgfs vs sysfs :) Sorry about that.
I originally made it so when we make the tpg and before it is enabled you had to do mkdir sessions on the tpg to signal the kernel that the app supports the new interface. The kernel would then make the acl sessions dir for you.
I was tracking down a bug in that though, and for the posting I made it so userspace had to create the acl sessions dir. While cutting and pasting the code I forgot to fix up that code.
> Do we need an similar flag per acl also?
> And if we have a per acl and the tpg flag I think they should be removed
> when user removes an empty sessions folder.
It’s just a lot easier to leave it set if userspace has enabled it once. You actually need to take into account if the tpg is enabled, and then you have some issues with the drivers that have their own nexus interface, and then there are different userspace code paths that handle this.
I don’t think it’s going to be common to mix and match updated and non-updated tools, so once its set, it’s set. If you disagree let me know. It can be done.
>
>> +
>> + se_nacl = se_sess->se_node_acl;
>> + if (se_nacl->dynamic_node_acl) {
>> + ret = configfs_register_group(&se_tpg->tpg_sess_group,
>> + &se_sess->group);
>> + } else {
>> + ret = configfs_register_group(&se_nacl->acl_sess_group,
>> + &se_sess->group);
>> + }
>> + if (ret)
>> + goto fail;
>
> Do we have a possible race here? I think it would be better to first
> call target_depend_item() and then register the new session's group.
Yes. Will fix.
>
>> +
>> + /*
>> + * The session is not created via a mkdir like other objects. A
>> + * transport event like a login or userspace used the nexus file to
>> + * initiate creation. However, we want the same behavior as other
>> + * objects where we must remove the children before removing the
>> + * parent dir, so do a depend on the parent that is released when the
>> + * session is removed.
>> + */
>> + if (se_nacl->dynamic_node_acl) {
>> + ret = target_depend_item(&se_tpg->tpg_sess_group.cg_item);
>> + } else {
>> + ret = target_depend_item(&se_nacl->acl_sess_group.cg_item);
>> + }
>> + if (ret)
>> + goto unreg_cfgfs;
>> +
>> + se_sess->added_to_cfgfs = true;
>> + return 0;
