On Wed, 2012-12-05 at 11:36 -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.
>
This has been an long-standing useability issue in the shell, and having
groups of initiator WWNs in userspace that are mapped to existing fabric
NodeACL layout is IMHO a good thing.
> 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..?
> 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.
>
Comments below.
> 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 | 51 ++++++++++++++++++++++++++
> drivers/target/target_core_tpg.c | 43 ++++++++++++++++++++++
> include/target/target_core_base.h | 1 +
> include/target/target_core_fabric.h | 2 +
> 4 files changed, 97 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 542641c..a0a748a 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -754,9 +754,60 @@ static ssize_t lio_target_nacl_store_cmdsn_depth(
>
> TF_NACL_BASE_ATTR(lio_target, cmdsn_depth, S_IRUGO | S_IWUSR);
>
> +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..?
> + iscsit_put_tpg(tpg);
> +
> + return ret;
> +}
> +
> +static ssize_t lio_target_nacl_store_tag(
> + struct se_node_acl *se_nacl,
> + const char *page,
> + size_t count)
> +{
> + struct config_item *acl_ci;
> + 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;
> +
> + acl_ci = &se_nacl->acl_group.cg_item;
> + if (!acl_ci) {
> + pr_err("Unable to locate acl_ci\n");
> + return -EINVAL;
> + }
> +
> + if (iscsit_get_tpg(tpg) < 0)
> + return -EINVAL;
> +
Ditto overkill here as well..
> + ret = core_tpg_set_initiator_node_tag(&tpg->tpg_se_tpg,
> + config_item_name(acl_ci), page);
> +
Just pass in *se_nacl here instead of extracting initiator via
config_item_name(). See below..
> + iscsit_put_tpg(tpg);
> + return (!ret) ? count : (ssize_t)ret;
> +}
> +
This looks ugly. Please get rid of this.
> +TF_NACL_BASE_ATTR(lio_target, tag, S_IRUGO | S_IWUSR);
> +
> static struct configfs_attribute *lio_target_initiator_attrs[] = {
> &lio_target_nacl_info.attr,
> &lio_target_nacl_cmdsn_depth.attr,
> + &lio_target_nacl_tag.attr,
> NULL,
> };
>
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index d84cc0a..b47b62d 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);
> @@ -616,6 +617,48 @@ 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.
> + */
> +int core_tpg_set_initiator_node_tag(
> + struct se_portal_group *tpg,
> + char *initiatorname,
> + const char *new_tag)
> +{
> + struct se_node_acl *acl;
> + char *tmp_tag;
> +
> + spin_lock_irq(&tpg->acl_node_lock);
> + acl = __core_tpg_get_initiator_node_acl(tpg, initiatorname);
It's horribly inefficient to lookup the se_node_acl from *initiatorname
with lots of se_node_acl, especially when the fabric configfs store()
caller already has their se_node_acl pointer.
Please get rid of this code and just pass in the existing *se_node_acl
> + if (!acl) {
> + pr_err("Access Control List entry for %s Initiator"
> + " Node %s does not exist for TPG %hu, ignoring"
> + " request.\n", tpg->se_tpg_tfo->get_fabric_name(),
> + initiatorname, tpg->se_tpg_tfo->tpg_get_tag(tpg));
> + spin_unlock_irq(&tpg->acl_node_lock);
> + return -ENODEV;
> + }
> + spin_unlock_irq(&tpg->acl_node_lock);
> +
> + 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 strnlen(new_tag, PAGE_SIZE);
> +}
Also ugly to return a string value here. Please remove this.
> +EXPORT_SYMBOL(core_tpg_set_initiator_node_tag);
> +
> static int core_tpg_setup_virtual_lun0(struct se_portal_group *se_tpg)
> {
> /* Set in core_dev_setup_virtual_lun0() */
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 645d90a..7bcc090 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -507,6 +507,7 @@ struct se_node_acl {
> bool acl_stop:1;
> u32 queue_depth;
> u32 acl_index;
> + char *acl_tag;
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..?
--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
