Re: [PATCHv4 1/3] target/configfs: add module wide action support

[Mike Christie]

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, xiu...@redhat.com wrote:
>>> From: Xiubo Li 
>>>
>>> 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
> 
> uio19259  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.

Re: [PATCHv4 1/3] target/configfs: add module wide action support

[Xiubo Li]

On 2018/5/3 2:27, Mike Christie wrote:

On 04/19/2018 02:46 AM, xiu...@redhat.com wrote:

From: Xiubo Li 

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 
---
  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(_group,
+   tb->ops->name,
+   >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(_group, "action", 
_core_action_cit);
+   configfs_add_default_group(_group, _core_hbagroup);
+
+   /*
 * Register the target_core_mod subsystem with 

Re: [PATCHv4 1/3] target/configfs: add module wide action support

[Xiubo Li]

On 2018/5/3 2:27, Mike Christie wrote:

On 04/19/2018 02:46 AM, xiu...@redhat.com wrote:

From: Xiubo Li 

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 
---
  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(_group,
+   tb->ops->name,
+   >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(_group, "action", 
_core_action_cit);
+   configfs_add_default_group(_group, _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 

Re: [PATCHv4 1/3] target/configfs: add module wide action support

[Mike Christie]

On 04/19/2018 02:46 AM, xiu...@redhat.com wrote:
> From: Xiubo Li 
> 
> 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 
> ---
>  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(_group,
> + tb->ops->name,
> + >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(_group, "action", 
> _core_action_cit);
> + configfs_add_default_group(_group, _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)
>