On Sun, 2012-12-09 at 17:04 -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.
>
This is looking better, but still a few more comments below.
> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target_configfs.c | 31 ++++++++++++++++++++++++++
> drivers/target/target_core_tpg.c | 22 ++++++++++++++++++
> include/target/target_core_base.h | 2 +
> include/target/target_core_fabric.h | 2 +
> 4 files changed, 57 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 542641c..19e39ad 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -754,9 +754,40 @@ 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)
> +{
> + if (se_nacl->acl_tag[0])
> + return snprintf(page, PAGE_SIZE, "%s", se_nacl->acl_tag);
> +
No need for the extra se_nacl->acl_tag check here..
> + return 0;
> +}
> +
> +static ssize_t lio_target_nacl_store_tag(
> + struct se_node_acl *se_nacl,
> + const char *page,
> + size_t count)
> +{
> + 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;
> +
No need for the extra *se_tpg + *tpg dereferences here..
> + ret = core_tpg_set_initiator_node_tag(&tpg->tpg_se_tpg,
> + se_nacl, page);
> +
Just pass se_nacl->se_tpg here..
> + if (ret < 0)
> + return ret;
> + return count;
> +}
> +
> +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..945da8e 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -616,6 +616,28 @@ 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)
> +{
There needs to be a MAX_ACL_TAG_SIZE check here, and return -EINVAL if
*new_tag exceeds this value.
> + if (!strncmp("NULL", new_tag, 4)) {
> + acl->acl_tag[0] = '\0';
> + } else {
> + strncpy(acl->acl_tag, new_tag, MAX_ACL_TAG_SIZE);
> + acl->acl_tag[MAX_ACL_TAG_SIZE-1] = '\0';
> + }
> +
Please use snprintf() here instead.
> + return strlen(acl->acl_tag);
> +}
As mentioned before, returning strlen() here is ugly. Please don't do
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..54609ec 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -507,6 +507,8 @@ struct se_node_acl {
> bool acl_stop:1;
> u32 queue_depth;
> u32 acl_index;
> +#define MAX_ACL_TAG_SIZE 64
> + char acl_tag[MAX_ACL_TAG_SIZE];
> u64 num_cmds;
> u64 read_bytes;
> u64 write_bytes;
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 9087b20..aaa1ee6 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -142,6 +142,8 @@ int core_tpg_del_initiator_node_acl(struct se_portal_group *,
> struct se_node_acl *, int);
> int core_tpg_set_initiator_node_queue_depth(struct se_portal_group *,
> unsigned char *, u32, int);
> +int core_tpg_set_initiator_node_tag(struct se_portal_group *,
> + struct se_node_acl *, const char *);
> int core_tpg_register(struct target_core_fabric_ops *, struct se_wwn *,
> struct se_portal_group *, void *, int);
> int core_tpg_deregister(struct se_portal_group *);
--
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
