| Message ID | 20210910084133.17956-2-d.bogdanov@yadro.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | target: make tpg/enable attribute | expand |
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
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 --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 *);