On 5/11/20 2:21 PM, Bart Van Assche wrote:
> On 2020-05-11 11:39, Bodo Stroesser wrote:
>> On 05/10/20 23:57, Mike Christie wrote:
>>> This patch adds helpers to add/remove a dir per session. There is only 2
>>> files/dirs initially.
>>>
>>
>> ...
>>
>>> +
>>> +int target_sysfs_add_session(struct se_portal_group *se_tpg,
>>> + struct se_session *se_sess)
>>> +{
>>> + int ret;
>>> +
>>> + /*
>>> + * Copy ACL name so we don't have to worry about mixing configfs
>>> + * and sysfs refcounts.
>>> + */
>>> + if (!se_sess->se_node_acl->dynamic_node_acl) {
>>> + se_sess->acl_name = kstrdup(se_sess->se_node_acl->initiatorname,
>>> + GFP_KERNEL);
>>> + if (!se_sess->acl_name)
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + ret = kobject_add(&se_sess->kobj, se_tpg->sessions_kobj, "%s-%d",
>>> + se_sess->tpt_id->name, se_sess->sid);
>>> + if (ret) {
>>> + pr_err("Could not add session%d to sysfs. Error %d.\n",
>>> + se_sess->sid, ret);
>>> + goto free_acl_name;
>>> + }
>>> +
>>> + ret = add_transport_id_attrs(se_sess);
>>> + if (ret)
>>> + goto del_kobj;
>>> +
>>> + if (se_sess->tfo->session_attrs) {
>>> + ret = sysfs_create_group(&se_sess->kobj,
>>> + se_sess->tfo->session_attrs);
>>> + if (ret)
>>> + goto rm_tpt_id_grps;
>>> + }
>>> +
>>> + ret = sysfs_create_link(tcm_core_sessions_kobj, &se_sess->kobj,
>>> + se_sess->kobj.name);
>>
>> I would prefer to have links named "session-%d" or "%d" only, of course
>> with se_sess->sid as the value for '%d'.
Yeah for the part of your comment that got chopped I can see your point.
For the dynamic acl case (userspace did not create an ACL so the kernel
made a tmp one), then doing session-$id will be easier for userspace to
lookup a specific session since it does not know the initiator name and
only knows the session id.
>
> Isn't se_sess->sid a property that is filled in by the iSCSI target
> driver only? Is se_sess->sid zero for all other target drivers than the
> iSCSI target driver?
No, in this patch in transport_alloc_session() I added a common sid
allocator so all sessions have a unique id across all targets.
>
>> If userspace knows the session-id only, such names make it easier to
>> find the corresponding link.
>
> Personally I prefer the %s-%d naming scheme. I think that naming scheme
> has the following advantages:
> 1. No need to run cat ... to retrieve the initiator name.
> 2. It becomes possible to derive from the 'ls' output which initiators
> created multiple sessions.
> 3. All sessions created by the same initiator appear consecutively.
>
Ccing Hannes, because he was also saying that we should use generic
names like target-%X, session-$d, etc. If we change all the code to use
generic names for the target/fabric/tpgt/session, then examples like #2
or similar ones like using tree to see the topology from a SCSI'ish view
would not work.
In the end, we have this issue with SCSI on the initiator side, and it's
a pain, but not a show stopper.
