* Re: [PATCHv4 1/3] target/configfs: add module wide action support
2018-04-19 7:46 [PATCHv4 1/3] target/configfs: add module wide action support xiubli
@ 2018-05-02 18:27 ` Mike Christie
2018-05-03 0:54 ` Xiubo Li
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Mike Christie @ 2018-05-02 18:27 UTC (permalink / raw)
To: target-devel
On 04/19/2018 02:46 AM, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
>
> For some case we need some module wide configfs to contol some
> attributes of the whole transport module.
When I suggested to move it module wide I just meant to add another mod
param like the global max data area param. I like the approach below
though, because rtslib can work similar to how it does for other
objects. However for tcmu we will have a mix of types, so I am not sure
how you are going to deal with compat. Maybe add a module level attrs
attr and add a max data area there that calls the same mod param code.
There would still be a kernel where it is not supported though.
Add some comments below if we go this route.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
> drivers/target/target_core_configfs.c | 46 +++++++++++++++++++++++++++++++++++
> drivers/target/target_core_hba.c | 11 +++++++++
> drivers/target/target_core_internal.h | 5 ++++
> include/target/target_core_backend.h | 1 +
> 4 files changed, 63 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 3f4bf12..a1ee716 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -79,6 +79,7 @@
> static struct config_group target_core_hbagroup;
> static struct config_group alua_group;
> static struct config_group alua_lu_gps_group;
> +static struct config_group action_group;
>
> static inline struct se_hba *
> item_to_hba(struct config_item *item)
> @@ -1198,6 +1199,7 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>
> TB_CIT_SETUP_DRV(dev_attrib, NULL, NULL);
> TB_CIT_SETUP_DRV(dev_action, NULL, NULL);
> +TB_CIT_SETUP_DRV(mod_action, NULL, NULL);
>
> /* End functions for struct config_item_type tb_dev_attrib_cit */
>
> @@ -2893,6 +2895,20 @@ static void target_core_alua_drop_tg_pt_gp(
>
> /* End functions for struct config_item_type target_core_alua_cit */
>
> +/* Start functions for struct config_item_type target_core_action_cit */
> +
> +/*
> + * target_core_action_cit is a ConfigFS group that lives under
> + * /sys/kernel/config/target/core/action.
> + */
> +static const struct config_item_type target_core_action_cit = {
> + .ct_item_ops = NULL,
> + .ct_attrs = NULL,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* End functions for struct config_item_type target_core_action_cit */
> +
> /* Start functions for struct config_item_type tb_dev_stat_cit */
>
> static struct config_group *target_core_stat_mkdir(
> @@ -3211,6 +3227,30 @@ void target_setup_backend_cits(struct target_backend *tb)
> target_core_setup_dev_wwn_cit(tb);
> target_core_setup_dev_alua_tg_pt_gps_cit(tb);
> target_core_setup_dev_stat_cit(tb);
> +
> + target_core_setup_mod_action_cit(tb);
> +}
> +
> +int target_setup_backend_action(struct target_backend *tb)
I think it might be best to rename these to:
target_register_backend_action_group
target_unregister_backend_action_group
It seems we are doing the setup and un/registration and unset is not a
common name type in the existing code.
> +{
> + if (!tb->ops->tb_mod_action_attrs)
> + return 0;
> +
> + tb->action_group = configfs_register_default_group(&action_group,
> + tb->ops->name,
> + &tb->tb_mod_action_cit);
> + if (!tb->action_group)
I think you need to do
if (IS_ERR(tb->action_group))
> + return PTR_ERR(tb->action_group);
> +
> + return 0;
> +}
> +
> +void target_unset_backend_action(struct target_backend *tb)
> +{
> + if (!tb->ops->tb_mod_action_attrs)
> + return;
> +
> + configfs_unregister_default_group(tb->action_group);
> }
>
> static int __init target_core_init_configfs(void)
> @@ -3267,6 +3307,12 @@ static int __init target_core_init_configfs(void)
> default_lu_gp = lu_gp;
>
> /*
> + * Create ALUA infrastructure under /sys/kernel/config/target/core/action/
Fix up the ALUA reference in the comment.
> + */
> + config_group_init_type_name(&action_group, "action", &target_core_action_cit);
> + configfs_add_default_group(&action_group, &target_core_hbagroup);
> +
> + /*
> * Register the target_core_mod subsystem with configfs.
> */
> ret = configfs_register_subsystem(subsys);
> diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
> index 22390e0..6903087 100644
> --- a/drivers/target/target_core_hba.c
> +++ b/drivers/target/target_core_hba.c
> @@ -51,6 +51,7 @@
> int transport_backend_register(const struct target_backend_ops *ops)
> {
> struct target_backend *tb, *old;
> + int ret;
>
> tb = kzalloc(sizeof(*tb), GFP_KERNEL);
> if (!tb)
> @@ -67,11 +68,20 @@ int transport_backend_register(const struct target_backend_ops *ops)
> }
> }
> target_setup_backend_cits(tb);
> +
> + ret = target_setup_backend_action(tb);
> + if (ret) {
> + mutex_unlock(&backend_mutex);
> + kfree(tb);
Just do a goto since we have multiple places doing the same cleanup with
this addition.
> + return ret;
> + }
> +
> list_add_tail(&tb->list, &backend_list);
> mutex_unlock(&backend_mutex);
>
> pr_debug("TCM: Registered subsystem plugin: %s struct module: %p\n",
> ops->name, ops->owner);
> +
Don't add extra formatting.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv4 1/3] target/configfs: add module wide action support
2018-04-19 7:46 [PATCHv4 1/3] target/configfs: add module wide action support xiubli
2018-05-02 18:27 ` Mike Christie
@ 2018-05-03 0:54 ` Xiubo Li
2018-05-03 1:03 ` Xiubo Li
2018-05-03 15:31 ` Mike Christie
3 siblings, 0 replies; 5+ messages in thread
From: Xiubo Li @ 2018-05-03 0:54 UTC (permalink / raw)
To: target-devel
On 2018/5/3 2:27, Mike Christie wrote:
> On 04/19/2018 02:46 AM, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> For some case we need some module wide configfs to contol some
>> attributes of the whole transport module.
> When I suggested to move it module wide I just meant to add another mod
> param like the global max data area param. I like the approach below
> though, because rtslib can work similar to how it does for other
> objects. However for tcmu we will have a mix of types, so I am not sure
> how you are going to deal with compat. Maybe add a module level attrs
> attr and add a max data area there that calls the same mod param code.
> There would still be a kernel where it is not supported though.
Hey Mike,
I just thought currently approach will be more flexible.
Yeah, for the compat issue it should be a little painful to deal with.
It's up to you and I am okay for both approaches.
BRs
Thanks,
> Add some comments below if we go this route.
>
>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> drivers/target/target_core_configfs.c | 46 +++++++++++++++++++++++++++++++++++
>> drivers/target/target_core_hba.c | 11 +++++++++
>> drivers/target/target_core_internal.h | 5 ++++
>> include/target/target_core_backend.h | 1 +
>> 4 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index 3f4bf12..a1ee716 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -79,6 +79,7 @@
>> static struct config_group target_core_hbagroup;
>> static struct config_group alua_group;
>> static struct config_group alua_lu_gps_group;
>> +static struct config_group action_group;
>>
>> static inline struct se_hba *
>> item_to_hba(struct config_item *item)
>> @@ -1198,6 +1199,7 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>>
>> TB_CIT_SETUP_DRV(dev_attrib, NULL, NULL);
>> TB_CIT_SETUP_DRV(dev_action, NULL, NULL);
>> +TB_CIT_SETUP_DRV(mod_action, NULL, NULL);
>>
>> /* End functions for struct config_item_type tb_dev_attrib_cit */
>>
>> @@ -2893,6 +2895,20 @@ static void target_core_alua_drop_tg_pt_gp(
>>
>> /* End functions for struct config_item_type target_core_alua_cit */
>>
>> +/* Start functions for struct config_item_type target_core_action_cit */
>> +
>> +/*
>> + * target_core_action_cit is a ConfigFS group that lives under
>> + * /sys/kernel/config/target/core/action.
>> + */
>> +static const struct config_item_type target_core_action_cit = {
>> + .ct_item_ops = NULL,
>> + .ct_attrs = NULL,
>> + .ct_owner = THIS_MODULE,
>> +};
>> +
>> +/* End functions for struct config_item_type target_core_action_cit */
>> +
>> /* Start functions for struct config_item_type tb_dev_stat_cit */
>>
>> static struct config_group *target_core_stat_mkdir(
>> @@ -3211,6 +3227,30 @@ void target_setup_backend_cits(struct target_backend *tb)
>> target_core_setup_dev_wwn_cit(tb);
>> target_core_setup_dev_alua_tg_pt_gps_cit(tb);
>> target_core_setup_dev_stat_cit(tb);
>> +
>> + target_core_setup_mod_action_cit(tb);
>> +}
>> +
>> +int target_setup_backend_action(struct target_backend *tb)
>
> I think it might be best to rename these to:
>
> target_register_backend_action_group
> target_unregister_backend_action_group
>
> It seems we are doing the setup and un/registration and unset is not a
> common name type in the existing code.
>
>
>
>> +{
>> + if (!tb->ops->tb_mod_action_attrs)
>> + return 0;
>> +
>> + tb->action_group = configfs_register_default_group(&action_group,
>> + tb->ops->name,
>> + &tb->tb_mod_action_cit);
>> + if (!tb->action_group)
> I think you need to do
>
> if (IS_ERR(tb->action_group))
>
>
>> + return PTR_ERR(tb->action_group);
>> +
>> + return 0;
>> +}
>> +
>> +void target_unset_backend_action(struct target_backend *tb)
>> +{
>> + if (!tb->ops->tb_mod_action_attrs)
>> + return;
>> +
>> + configfs_unregister_default_group(tb->action_group);
>> }
>>
>> static int __init target_core_init_configfs(void)
>> @@ -3267,6 +3307,12 @@ static int __init target_core_init_configfs(void)
>> default_lu_gp = lu_gp;
>>
>> /*
>> + * Create ALUA infrastructure under /sys/kernel/config/target/core/action/
> Fix up the ALUA reference in the comment.
>
>> + */
>> + config_group_init_type_name(&action_group, "action", &target_core_action_cit);
>> + configfs_add_default_group(&action_group, &target_core_hbagroup);
>> +
>> + /*
>> * Register the target_core_mod subsystem with configfs.
>> */
>> ret = configfs_register_subsystem(subsys);
>> diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
>> index 22390e0..6903087 100644
>> --- a/drivers/target/target_core_hba.c
>> +++ b/drivers/target/target_core_hba.c
>> @@ -51,6 +51,7 @@
>> int transport_backend_register(const struct target_backend_ops *ops)
>> {
>> struct target_backend *tb, *old;
>> + int ret;
>>
>> tb = kzalloc(sizeof(*tb), GFP_KERNEL);
>> if (!tb)
>> @@ -67,11 +68,20 @@ int transport_backend_register(const struct target_backend_ops *ops)
>> }
>> }
>> target_setup_backend_cits(tb);
>> +
>> + ret = target_setup_backend_action(tb);
>> + if (ret) {
>> + mutex_unlock(&backend_mutex);
>> + kfree(tb);
> Just do a goto since we have multiple places doing the same cleanup with
> this addition.
>
>> + return ret;
>> + }
>> +
>> list_add_tail(&tb->list, &backend_list);
>> mutex_unlock(&backend_mutex);
>>
>> pr_debug("TCM: Registered subsystem plugin: %s struct module: %p\n",
>> ops->name, ops->owner);
>> +
> Don't add extra formatting.
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv4 1/3] target/configfs: add module wide action support
2018-04-19 7:46 [PATCHv4 1/3] target/configfs: add module wide action support xiubli
2018-05-02 18:27 ` Mike Christie
2018-05-03 0:54 ` Xiubo Li
@ 2018-05-03 1:03 ` Xiubo Li
2018-05-03 15:31 ` Mike Christie
3 siblings, 0 replies; 5+ messages in thread
From: Xiubo Li @ 2018-05-03 1:03 UTC (permalink / raw)
To: target-devel
On 2018/5/3 2:27, Mike Christie wrote:
> On 04/19/2018 02:46 AM, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> For some case we need some module wide configfs to contol some
>> attributes of the whole transport module.
> When I suggested to move it module wide I just meant to add another mod
> param like the global max data area param. I like the approach below
> though, because rtslib can work similar to how it does for other
> objects. However for tcmu we will have a mix of types, so I am not sure
> how you are going to deal with compat. Maybe add a module level attrs
> attr and add a max data area there that calls the same mod param code.
> There would still be a kernel where it is not supported though.
But from currently after the tcmu-runner is crashed the
target_core_user.ko will still be kept inserted, something like:
[root@gblock2 ~]# lsmod |grep target
target_core_pscsi 18799 0
target_core_file 18217 0
target_core_iblock 18282 0
iscsi_target_mod 291661 8
target_core_user 24557 2
target_core_mod 340729 18
target_core_iblock,target_core_pscsi,iscsi_target_mod,target_core_file,target_core_user
uio 19259 1 target_core_user
crc_t10dif 12912 2 target_core_mod,sd_mod
If make it a mod param, like this issue how could it be work when the
tcmu-runner is crashed and try to start again?
BRs
> Add some comments below if we go this route.
>
>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> drivers/target/target_core_configfs.c | 46 +++++++++++++++++++++++++++++++++++
>> drivers/target/target_core_hba.c | 11 +++++++++
>> drivers/target/target_core_internal.h | 5 ++++
>> include/target/target_core_backend.h | 1 +
>> 4 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index 3f4bf12..a1ee716 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -79,6 +79,7 @@
>> static struct config_group target_core_hbagroup;
>> static struct config_group alua_group;
>> static struct config_group alua_lu_gps_group;
>> +static struct config_group action_group;
>>
>> static inline struct se_hba *
>> item_to_hba(struct config_item *item)
>> @@ -1198,6 +1199,7 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>>
>> TB_CIT_SETUP_DRV(dev_attrib, NULL, NULL);
>> TB_CIT_SETUP_DRV(dev_action, NULL, NULL);
>> +TB_CIT_SETUP_DRV(mod_action, NULL, NULL);
>>
>> /* End functions for struct config_item_type tb_dev_attrib_cit */
>>
>> @@ -2893,6 +2895,20 @@ static void target_core_alua_drop_tg_pt_gp(
>>
>> /* End functions for struct config_item_type target_core_alua_cit */
>>
>> +/* Start functions for struct config_item_type target_core_action_cit */
>> +
>> +/*
>> + * target_core_action_cit is a ConfigFS group that lives under
>> + * /sys/kernel/config/target/core/action.
>> + */
>> +static const struct config_item_type target_core_action_cit = {
>> + .ct_item_ops = NULL,
>> + .ct_attrs = NULL,
>> + .ct_owner = THIS_MODULE,
>> +};
>> +
>> +/* End functions for struct config_item_type target_core_action_cit */
>> +
>> /* Start functions for struct config_item_type tb_dev_stat_cit */
>>
>> static struct config_group *target_core_stat_mkdir(
>> @@ -3211,6 +3227,30 @@ void target_setup_backend_cits(struct target_backend *tb)
>> target_core_setup_dev_wwn_cit(tb);
>> target_core_setup_dev_alua_tg_pt_gps_cit(tb);
>> target_core_setup_dev_stat_cit(tb);
>> +
>> + target_core_setup_mod_action_cit(tb);
>> +}
>> +
>> +int target_setup_backend_action(struct target_backend *tb)
>
> I think it might be best to rename these to:
>
> target_register_backend_action_group
> target_unregister_backend_action_group
>
> It seems we are doing the setup and un/registration and unset is not a
> common name type in the existing code.
>
>
>
>> +{
>> + if (!tb->ops->tb_mod_action_attrs)
>> + return 0;
>> +
>> + tb->action_group = configfs_register_default_group(&action_group,
>> + tb->ops->name,
>> + &tb->tb_mod_action_cit);
>> + if (!tb->action_group)
> I think you need to do
>
> if (IS_ERR(tb->action_group))
>
>
>> + return PTR_ERR(tb->action_group);
>> +
>> + return 0;
>> +}
>> +
>> +void target_unset_backend_action(struct target_backend *tb)
>> +{
>> + if (!tb->ops->tb_mod_action_attrs)
>> + return;
>> +
>> + configfs_unregister_default_group(tb->action_group);
>> }
>>
>> static int __init target_core_init_configfs(void)
>> @@ -3267,6 +3307,12 @@ static int __init target_core_init_configfs(void)
>> default_lu_gp = lu_gp;
>>
>> /*
>> + * Create ALUA infrastructure under /sys/kernel/config/target/core/action/
> Fix up the ALUA reference in the comment.
>
>> + */
>> + config_group_init_type_name(&action_group, "action", &target_core_action_cit);
>> + configfs_add_default_group(&action_group, &target_core_hbagroup);
>> +
>> + /*
>> * Register the target_core_mod subsystem with configfs.
>> */
>> ret = configfs_register_subsystem(subsys);
>> diff --git a/drivers/target/target_core_hba.c b/drivers/target/target_core_hba.c
>> index 22390e0..6903087 100644
>> --- a/drivers/target/target_core_hba.c
>> +++ b/drivers/target/target_core_hba.c
>> @@ -51,6 +51,7 @@
>> int transport_backend_register(const struct target_backend_ops *ops)
>> {
>> struct target_backend *tb, *old;
>> + int ret;
>>
>> tb = kzalloc(sizeof(*tb), GFP_KERNEL);
>> if (!tb)
>> @@ -67,11 +68,20 @@ int transport_backend_register(const struct target_backend_ops *ops)
>> }
>> }
>> target_setup_backend_cits(tb);
>> +
>> + ret = target_setup_backend_action(tb);
>> + if (ret) {
>> + mutex_unlock(&backend_mutex);
>> + kfree(tb);
> Just do a goto since we have multiple places doing the same cleanup with
> this addition.
>
>> + return ret;
>> + }
>> +
>> list_add_tail(&tb->list, &backend_list);
>> mutex_unlock(&backend_mutex);
>>
>> pr_debug("TCM: Registered subsystem plugin: %s struct module: %p\n",
>> ops->name, ops->owner);
>> +
> Don't add extra formatting.
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCHv4 1/3] target/configfs: add module wide action support
2018-04-19 7:46 [PATCHv4 1/3] target/configfs: add module wide action support xiubli
` (2 preceding siblings ...)
2018-05-03 1:03 ` Xiubo Li
@ 2018-05-03 15:31 ` Mike Christie
3 siblings, 0 replies; 5+ messages in thread
From: Mike Christie @ 2018-05-03 15:31 UTC (permalink / raw)
To: target-devel
On 05/02/2018 08:03 PM, Xiubo Li wrote:
> On 2018/5/3 2:27, Mike Christie wrote:
>> On 04/19/2018 02:46 AM, xiubli@redhat.com wrote:
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> For some case we need some module wide configfs to contol some
>>> attributes of the whole transport module.
>> When I suggested to move it module wide I just meant to add another mod
>> param like the global max data area param. I like the approach below
>> though, because rtslib can work similar to how it does for other
>> objects. However for tcmu we will have a mix of types, so I am not sure
>> how you are going to deal with compat. Maybe add a module level attrs
>> attr and add a max data area there that calls the same mod param code.
>> There would still be a kernel where it is not supported though.
> But from currently after the tcmu-runner is crashed the
> target_core_user.ko will still be kept inserted, something like:
>
> [root@gblock2 ~]# lsmod |grep target
> target_core_pscsi 18799 0
> target_core_file 18217 0
> target_core_iblock 18282 0
> iscsi_target_mod 291661 8
> target_core_user 24557 2
> target_core_mod 340729 18
> target_core_iblock,target_core_pscsi,iscsi_target_mod,target_core_file,target_core_user
>
> uio 19259 1 target_core_user
> crc_t10dif 12912 2 target_core_mod,sd_mod
>
> If make it a mod param, like this issue how could it be work when the
> tcmu-runner is crashed and try to start again?
When you do a module_param_cb it creates a sysfs file in
/sys/module/target_core_user/parameters that you can read/write to like
any other sysfs file.
^ permalink raw reply [flat|nested] 5+ messages in thread