diff mbox series

[v5,1/7] target: core: add common tpg/enable attribute

Message ID 20210910084133.17956-2-d.bogdanov@yadro.com
State New
Headers show
Series target: make tpg/enable attribute | expand

Commit Message

Dmitry Bogdanov Sept. 10, 2021, 8:41 a.m. UTC
Many fabric modules provide their own implementation of enable
attribute in tpg.
The change provides a way to remove code duplication in the fabric
modules and automatically add "enable" attribute if a fabric module has
an implementation of fabric_enable_tpg() ops.

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_configfs.c        |  1 +
 drivers/target/target_core_fabric_configfs.c | 78 +++++++++++++++++++-
 include/target/target_core_base.h            |  1 +
 include/target/target_core_fabric.h          |  1 +
 4 files changed, 79 insertions(+), 2 deletions(-)

Comments

Bodo Stroesser Sept. 15, 2021, 6 p.m. UTC | #1
On 10.09.21 10:41, Dmitry Bogdanov wrote:
> Many fabric modules provide their own implementation of enable

> attribute in tpg.

> The change provides a way to remove code duplication in the fabric

> modules and automatically add "enable" attribute if a fabric module has

> an implementation of fabric_enable_tpg() ops.

> 

> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>

> ---

>   drivers/target/target_core_configfs.c        |  1 +

>   drivers/target/target_core_fabric_configfs.c | 78 +++++++++++++++++++-

>   include/target/target_core_base.h            |  1 +

>   include/target/target_core_fabric.h          |  1 +

>   4 files changed, 79 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c

> index 102ec644bc8a..3b9e50c1ccef 100644

> --- a/drivers/target/target_core_configfs.c

> +++ b/drivers/target/target_core_configfs.c

> @@ -490,6 +490,7 @@ void target_unregister_template(const struct target_core_fabric_ops *fo)

>   			 * fabric driver unload of TFO->module to proceed.

>   			 */

>   			rcu_barrier();

> +			kfree(t->tf_tpg_base_cit.ct_attrs);

>   			kfree(t);

>   			return;

>   		}

> diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c

> index fc7edc04ee09..0b65de9f2df1 100644

> --- a/drivers/target/target_core_fabric_configfs.c

> +++ b/drivers/target/target_core_fabric_configfs.c

> @@ -815,8 +815,76 @@ static struct configfs_item_operations target_fabric_tpg_base_item_ops = {

>   	.release		= target_fabric_tpg_release,

>   };

>   

> -TF_CIT_SETUP_DRV(tpg_base, &target_fabric_tpg_base_item_ops, NULL);

> +static ssize_t target_fabric_tpg_base_enable_show(struct config_item *item,

> +						  char *page)

> +{

> +	return sysfs_emit(page, "%d\n", to_tpg(item)->enabled);

> +}

> +

> +static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,

> +						   const char *page,

> +						   size_t count)

> +{

> +	struct se_portal_group *se_tpg = to_tpg(item);

> +	int ret;

> +	bool op;

> +

> +	ret = strtobool(page, &op);

> +	if (ret)

> +		return ret;

> +

> +	if (se_tpg->enabled == op)

> +		return count;


Sorry for jumping in lately.

Just one nit:
In case someone tries to enable or disable the same tpg a second time,
with the change we always do nothing and return count (--> OK).

I just checked iscsi and qla2xxx. AFAICS iscsi before the patch rejected
the second enable or disable with -EINVAL, while qla2xxx accepts the
second disable and rejects the second enable with -EEXIST.

Of course it sounds good to unify the behavior of existing enable
attributes. OTOH: even if enabling/disabling the same tpg twice can be
seen as suspicious behavior, are we sure to not confuse existing user 
space tools by changing the result?

Bodo
Dmitry Bogdanov Sept. 16, 2021, 8:30 a.m. UTC | #2
Hi Bodo,

> > +	if (se_tpg->enabled == op)

> > +		return count;


> Sorry for jumping in lately.


> Just one nit:

> In case someone tries to enable or disable the same tpg a second time, with

> the change we always do nothing and return count (--> OK).

>

> I just checked iscsi and qla2xxx. AFAICS iscsi before the patch rejected the

> second enable or disable with -EINVAL, while qla2xxx accepts the second

> disable and rejects the second enable with -EEXIST.

>

> Of course it sounds good to unify the behavior of existing enable

> attributes. OTOH: even if enabling/disabling the same tpg twice can be seen

> as suspicious behavior, are we sure to not confuse existing user space tools

> by changing the result?


targetcli tool does not check the result of disable/enable at all.
Our proprietary application checks a result but does not check the particular
return code, and the application does not expect the failure of the same second
request.

It's hardly to imagine why someone should expect the second enable and especially
the second disable to fail - the requested operation(disable or enable) is successfully
done for the caller.

BR,
 Dmitry
diff mbox series

Patch

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 102ec644bc8a..3b9e50c1ccef 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -490,6 +490,7 @@  void target_unregister_template(const struct target_core_fabric_ops *fo)
 			 * fabric driver unload of TFO->module to proceed.
 			 */
 			rcu_barrier();
+			kfree(t->tf_tpg_base_cit.ct_attrs);
 			kfree(t);
 			return;
 		}
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index fc7edc04ee09..0b65de9f2df1 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -815,8 +815,76 @@  static struct configfs_item_operations target_fabric_tpg_base_item_ops = {
 	.release		= target_fabric_tpg_release,
 };
 
-TF_CIT_SETUP_DRV(tpg_base, &target_fabric_tpg_base_item_ops, NULL);
+static ssize_t target_fabric_tpg_base_enable_show(struct config_item *item,
+						  char *page)
+{
+	return sysfs_emit(page, "%d\n", to_tpg(item)->enabled);
+}
+
+static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
+						   const char *page,
+						   size_t count)
+{
+	struct se_portal_group *se_tpg = to_tpg(item);
+	int ret;
+	bool op;
+
+	ret = strtobool(page, &op);
+	if (ret)
+		return ret;
+
+	if (se_tpg->enabled == op)
+		return count;
+
+	ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
+	if (ret)
+		return ret;
+
+	se_tpg->enabled = op;
+
+	return count;
+}
+
+CONFIGFS_ATTR(target_fabric_tpg_base_, enable);
 
+static int
+target_fabric_setup_tpg_base_cit(struct target_fabric_configfs *tf)
+{
+	struct config_item_type *cit = &tf->tf_tpg_base_cit;
+	struct configfs_attribute **attrs = NULL;
+	size_t nr_attrs = 0;
+	int i = 0;
+
+	if (tf->tf_ops->tfc_tpg_base_attrs)
+		while (tf->tf_ops->tfc_tpg_base_attrs[nr_attrs] != NULL)
+			nr_attrs++;
+
+	if (tf->tf_ops->fabric_enable_tpg)
+		nr_attrs++;
+
+	if (nr_attrs == 0)
+		goto done;
+
+	/* + 1 for final NULL in the array */
+	attrs = kcalloc(nr_attrs + 1, sizeof(*attrs), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	if (tf->tf_ops->tfc_tpg_base_attrs)
+		for (; tf->tf_ops->tfc_tpg_base_attrs[i] != NULL; i++)
+			attrs[i] = tf->tf_ops->tfc_tpg_base_attrs[i];
+
+	if (tf->tf_ops->fabric_enable_tpg)
+		attrs[i] = &target_fabric_tpg_base_attr_enable;
+
+done:
+	cit->ct_item_ops = &target_fabric_tpg_base_item_ops;
+	cit->ct_attrs = attrs;
+	cit->ct_owner = tf->tf_ops->module;
+	pr_debug("Setup generic tpg_base\n");
+
+	return 0;
+}
 /* End of tfc_tpg_base_cit */
 
 /* Start of tfc_tpg_cit */
@@ -1028,12 +1096,18 @@  TF_CIT_SETUP_DRV(discovery, NULL, NULL);
 
 int target_fabric_setup_cits(struct target_fabric_configfs *tf)
 {
+	int ret;
+
 	target_fabric_setup_discovery_cit(tf);
 	target_fabric_setup_wwn_cit(tf);
 	target_fabric_setup_wwn_fabric_stats_cit(tf);
 	target_fabric_setup_wwn_param_cit(tf);
 	target_fabric_setup_tpg_cit(tf);
-	target_fabric_setup_tpg_base_cit(tf);
+
+	ret = target_fabric_setup_tpg_base_cit(tf);
+	if (ret)
+		return ret;
+
 	target_fabric_setup_tpg_port_cit(tf);
 	target_fabric_setup_tpg_port_stat_cit(tf);
 	target_fabric_setup_tpg_lun_cit(tf);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index fb11c7693b25..2130f102798d 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -900,6 +900,7 @@  struct se_portal_group {
 	 * Negative values can be used by fabric drivers for internal use TPGs.
 	 */
 	int			proto_id;
+	bool			enabled;
 	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
 	atomic_t		tpg_pr_ref_count;
 	/* Spinlock for adding/removing ACLed Nodes */
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 3c5ade7a04a6..38f0662476d1 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -89,6 +89,7 @@  struct target_core_fabric_ops {
 	void (*add_wwn_groups)(struct se_wwn *);
 	struct se_portal_group *(*fabric_make_tpg)(struct se_wwn *,
 						   const char *);
+	int (*fabric_enable_tpg)(struct se_portal_group *se_tpg, bool enable);
 	void (*fabric_drop_tpg)(struct se_portal_group *);
 	int (*fabric_post_link)(struct se_portal_group *,
 				struct se_lun *);