On 12/05/2012 01:34 PM, Nicholas A. Bellinger wrote:
>> Instead of changing ktarget's configfs interface, I propose we keep
>> the configfs interface per-wwn but modify targetcli to support
>> modifying multiple initiators at once. targetcli could certainly keep
>> its own list of which hosts are in its "groups", but one nice feature
>> of the current implementation is *all* info is tracked in configfs.
>>
>
> Are you intending this to just be in targetcli, or in rtslib as well..?
rtslib would need a minor change to expose the new configfs data via its
API, but beyond that I was thinking everything would be in targetcli
(initially at least. see below.)
>> +static ssize_t lio_target_nacl_show_tag(
>> + struct se_node_acl *se_nacl,
>> + char *page)
>> +{
>> + struct se_portal_group *se_tpg = se_nacl->se_tpg;
>> + struct iscsi_portal_group *tpg = container_of(se_tpg,
>> + struct iscsi_portal_group, tpg_se_tpg);
>> + int ret;
>> +
>> + if (iscsit_get_tpg(tpg) < 0)
>> + return -EINVAL;
>> +
>
> Using iscsit_get_tpg() here (and elsewhere) is overkill. I don't see a
> reason why we need to sychronize across the entire se_portal_group when
> only doing a se_node_acl specific operation.
>
>> + spin_lock_irq(&se_tpg->acl_node_lock);
>> + ret = snprintf(page, PAGE_SIZE, "%s", se_nacl->acl_tag);
>> + spin_unlock_irq(&se_tpg->acl_node_lock);
>> +
>
> Ditto here, using ->acl_node_lock is overkill.
>
> Why shouldn't independent se_node_acl tags allowed to be updated in
> parallel from userspace code..?
Tend to agree about get_tpg(), but afaict se_tpg->acl_node_lock is the
only lock protecting members of struct se_node_acl, and we need some
lock around updating acl_tag or there's a memory leak, and some lock
around using acl_tag or we might use-after-free.
[snip straightforward comments]
OK will clean these up.
> Is using a string here really the cleanest way to the end result..?
>
> Why not just use a u32 acl_group_id, and map the numerical value to
> human readable strings in userspace code..?
Yes I think we want the actual string in there. While initially a
targetcli thing, we shouldn't rule out other rtslib clients using it, or
even refactoring initiator group support into rtslib down the line. Both
of these things are options if configfs stays the go-to place for all
names and config values.
Regards -- Andy
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
