On Thu, 2012-12-06 at 15:47 -0800, Andy Grover wrote:
> Some feedback I've gotten from users is that initiator access config
> could be easier. The way other storage vendors have addressed this is to
> support initiator groups; the admin adds initiator WWNs to the group, and
> then LUN mappings can be assigned for the entire group at once.
>
> 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.
>
> This patch adds a 'tag' parameter to nodeacl configfs for iscsi. It
> is not used internally, but allows user utilities to stick just enough
> info in configfs to simulate NodeACL aliases and groups. targetcli or
> other user tool could group nodeacls by tag, and then apply a config
> change to all nodeacls with a given tag.
>
> Code tested to work, although userspace pieces still to be implemented.
>
> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target_configfs.c | 35 ++++++++++++++++++++++++++
> drivers/target/target_core_tpg.c | 31 +++++++++++++++++++++++
> include/target/target_core_base.h | 1 +
> include/target/target_core_fabric.h | 2 +
> 4 files changed, 69 insertions(+), 0 deletions(-)
>
<SNIP>
>
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index d84cc0a..1e131a9 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -461,6 +461,7 @@ int core_tpg_del_initiator_node_acl(
> }
> list_del(&acl->acl_list);
> tpg->num_node_acls--;
> + kfree(acl->acl_tag);
> spin_unlock_irq(&tpg->acl_node_lock);
>
> spin_lock_irqsave(&acl->nacl_sess_lock, flags);
Why does the deletion of acl->acl_tag need to be protected here to
start..?
If the acl->acl_group object is being removed via configfs rmdir, it
means there are no more configfs attr stores or shows left active to
struct config_group.
> @@ -616,6 +617,36 @@ int core_tpg_set_initiator_node_queue_depth(
> }
> EXPORT_SYMBOL(core_tpg_set_initiator_node_queue_depth);
>
> +/* core_tpg_set_initiator_node_tag():
> + *
> + * Initiator nodeacl tags are not used internally, but may be used by
> + * userspace to emulate aliases or groups.
> + * Returns length of newly-set tag.
> + */
> +int core_tpg_set_initiator_node_tag(
> + struct se_portal_group *tpg,
> + struct se_node_acl *acl,
> + const char *new_tag)
> +{
> + char *tmp_tag;
> +
> + if (!strncmp("NULL", new_tag, 4)) {
> + tmp_tag = NULL;
> + } else {
> + tmp_tag = kstrndup(new_tag, PAGE_SIZE-1, GFP_KERNEL);
> + if (!tmp_tag)
> + return -ENOMEM;
> + }
> +
> + spin_lock_irq(&tpg->acl_node_lock);
> + kfree(acl->acl_tag);
> + acl->acl_tag = tmp_tag;
> + spin_unlock_irq(&tpg->acl_node_lock);
> +
> + return 0;
> +}
I still don't like the usage of a parent tpg->acl_node_lock here with
the allocation. Seriously, let configfs do what it does best for kernel
structure reference counting with a reasonable sized
acl->acl_tag[MAX_TAG_NAME_SIZE], instead of doing ugly stuff with parent
locks like this.
--nab
--
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
