[4/4] tcmu: make cmd timeout configurable

Commit Message

Nicholas A. Bellinger March 18, 2017, 11:26 p.m. UTC

On Thu, 2017-03-09 at 02:42 -0600, Mike Christie wrote:
> A single daemon could implement multiple types of devices
> using multuple types of real devices that may not support
> restarting from crashes and/or handling tcmu timeouts. This
> makes the cmd timeout configurable, so handlers that do not
> support it can turn if off for now.
> 
> Signed-off-by: Mike Christie <[e-mail]>
> ---
>  drivers/target/target_core_user.c | 41 +++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 892b311..10cc15f 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c

<SNIP>

> @@ -1037,7 +1046,7 @@ static void tcmu_free_device(struct se_device *dev)
>  
>  enum {
>  	Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors,
> -	Opt_err,
> +	Opt_cmd_time_out, Opt_err,
>  };
>  
>  static match_table_t tokens = {
> @@ -1045,6 +1054,7 @@ enum {
>  	{Opt_dev_size, "dev_size=%u"},
>  	{Opt_hw_block_size, "hw_block_size=%u"},
>  	{Opt_hw_max_sectors, "hw_max_sectors=%u"},
> +	{Opt_cmd_time_out, "cmd_time_out=%u"},
>  	{Opt_err, NULL}
>  };
>  
> @@ -1111,6 +1121,23 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev,
>  			if (ret < 0)
>  				pr_err("kstrtoul() failed for dev_size=\n");
>  			break;
> +		case Opt_cmd_time_out:
> +			if (tcmu_dev_configured(udev)) {
> +				pr_err("Can not update cmd_time_out after device has been configured.\n");
> +				ret = -EINVAL;
> +				break;
> +			}
> +			arg_p = match_strdup(&args[0]);
> +			if (!arg_p) {
> +				ret = -ENOMEM;
> +				break;
> +			}
> +			ret = kstrtouint(arg_p, 0, &udev->cmd_time_out);
> +			kfree(arg_p);
> +			if (ret < 0)
> +				pr_err("kstrtouint() failed for cmd_time_out=\n");
> +			udev->cmd_time_out *= MSEC_PER_SEC;
> +			break;
>  		case Opt_hw_block_size:
>  			ret = tcmu_set_dev_attrib(&args[0],
>  					&(dev->dev_attrib.hw_block_size));
> @@ -1138,7 +1165,9 @@ static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
>  
>  	bl = sprintf(b + bl, "Config: %s ",
>  		     udev->dev_config[0] ? udev->dev_config : "NULL");
> -	bl += sprintf(b + bl, "Size: %zu\n", udev->dev_size);
> +	bl += sprintf(b + bl, "Size: %zu ", udev->dev_size);
> +	bl += sprintf(b + bl, "Cmd Time Out: %lu\n",
> +		      udev->cmd_time_out / MSEC_PER_SEC);
>  
>  	return bl;
>  }

Historically /sys/kernel/config/target/core/user_0/foo/control has been
reserved for parameters needed to configure the device.

So I think cmd_time_out is better served by appearing under 
/sys/kernel/config/target/core/user_0/foo/attrib/, so that it's
accessible via targetcli + friends.

Here is a patch to do this for TCMU, that adds cmd_time_out as a backend
specific attribute along with the existing passthrough_attrib_attrs[].

Note it uses a ->dev_export check in tcmu_cmd_time_out_store() to ensure
the value cannot be changed with active fabric exports, but can still be
changed after the device is configured.

I'll go ahead and include this as a incremental patch to your original
series.

Comments

Mike Christie March 20, 2017, 5:05 a.m. UTC | #1

On 03/18/2017 06:26 PM, Nicholas A. Bellinger wrote:
> +static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *page,
> +				       size_t count)
> +{


...


> +
> +	if (!val) {
> +		pr_err("Illegal value for cmd_time_out\n");
> +		return -EINVAL;
> +	}
> +

Thanks for the patch! I tested with this chunk removed so you can
disable the timer (0 == disabled), and it all works as expected.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 10cc15f..c6874c3 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -28,6 +28,7 @@ 
 #include <linux/stringify.h>
 #include <linux/bitops.h>
 #include <linux/highmem.h>
+#include <linux/configfs.h>
 #include <net/genetlink.h>
 #include <scsi/scsi_common.h>
 #include <scsi/scsi_proto.h>
@@ -1046,7 +1047,7 @@  static void tcmu_free_device(struct se_device *dev)
 
 enum {
 	Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors,
-	Opt_cmd_time_out, Opt_err,
+	Opt_err,
 };
 
 static match_table_t tokens = {
@@ -1054,7 +1055,6 @@  enum {
 	{Opt_dev_size, "dev_size=%u"},
 	{Opt_hw_block_size, "hw_block_size=%u"},
 	{Opt_hw_max_sectors, "hw_max_sectors=%u"},
-	{Opt_cmd_time_out, "cmd_time_out=%u"},
 	{Opt_err, NULL}
 };
 
@@ -1121,23 +1121,6 @@  static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev,
 			if (ret < 0)
 				pr_err("kstrtoul() failed for dev_size=\n");
 			break;
-		case Opt_cmd_time_out:
-			if (tcmu_dev_configured(udev)) {
-				pr_err("Can not update cmd_time_out after device has been configured.\n");
-				ret = -EINVAL;
-				break;
-			}
-			arg_p = match_strdup(&args[0]);
-			if (!arg_p) {
-				ret = -ENOMEM;
-				break;
-			}
-			ret = kstrtouint(arg_p, 0, &udev->cmd_time_out);
-			kfree(arg_p);
-			if (ret < 0)
-				pr_err("kstrtouint() failed for cmd_time_out=\n");
-			udev->cmd_time_out *= MSEC_PER_SEC;
-			break;
 		case Opt_hw_block_size:
 			ret = tcmu_set_dev_attrib(&args[0],
 					&(dev->dev_attrib.hw_block_size));
@@ -1165,9 +1148,7 @@  static ssize_t tcmu_show_configfs_dev_params(struct se_device *dev, char *b)
 
 	bl = sprintf(b + bl, "Config: %s ",
 		     udev->dev_config[0] ? udev->dev_config : "NULL");
-	bl += sprintf(b + bl, "Size: %zu ", udev->dev_size);
-	bl += sprintf(b + bl, "Cmd Time Out: %lu\n",
-		      udev->cmd_time_out / MSEC_PER_SEC);
+	bl += sprintf(b + bl, "Size: %zu\n", udev->dev_size);
 
 	return bl;
 }
@@ -1186,7 +1167,48 @@  static sector_t tcmu_get_blocks(struct se_device *dev)
 	return passthrough_parse_cdb(cmd, tcmu_queue_cmd);
 }
 
-static const struct target_backend_ops tcmu_ops = {
+static ssize_t tcmu_cmd_time_out_show(struct config_item *item, char *page)
+{
+	struct se_dev_attrib *da = container_of(to_config_group(item),
+					struct se_dev_attrib, da_group);
+	struct tcmu_dev *udev = container_of(da->da_dev,
+					struct tcmu_dev, se_dev);
+
+	return snprintf(page, PAGE_SIZE, "%lu\n", udev->cmd_time_out / MSEC_PER_SEC);
+}
+
+static ssize_t tcmu_cmd_time_out_store(struct config_item *item, const char *page,
+				       size_t count)
+{
+	struct se_dev_attrib *da = container_of(to_config_group(item),
+					struct se_dev_attrib, da_group);
+	struct tcmu_dev *udev = container_of(da->da_dev,
+					struct tcmu_dev, se_dev);
+	u32 val;
+	int ret;
+
+	if (da->da_dev->export_count) {
+		pr_err("Unable to set tcmu cmd_time_out while exports exist\n");
+		return -EINVAL;
+	}
+
+	ret = kstrtou32(page, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	if (!val) {
+		pr_err("Illegal value for cmd_time_out\n");
+		return -EINVAL;
+	}
+
+	udev->cmd_time_out = val * MSEC_PER_SEC;
+	return count;
+}
+CONFIGFS_ATTR(tcmu_, cmd_time_out);
+
+static struct configfs_attribute **tcmu_attrs;
+
+static struct target_backend_ops tcmu_ops = {
 	.name			= "user",
 	.owner			= THIS_MODULE,
 	.transport_flags	= TRANSPORT_FLAG_PASSTHROUGH,
@@ -1200,12 +1222,12 @@  static sector_t tcmu_get_blocks(struct se_device *dev)
 	.show_configfs_dev_params = tcmu_show_configfs_dev_params,
 	.get_device_type	= sbc_get_device_type,
 	.get_blocks		= tcmu_get_blocks,
-	.tb_dev_attrib_attrs	= passthrough_attrib_attrs,
+	.tb_dev_attrib_attrs	= NULL,
 };
 
 static int __init tcmu_module_init(void)
 {
-	int ret;
+	int ret, i, len = 0;
 
 	BUILD_BUG_ON((sizeof(struct tcmu_cmd_entry) % TCMU_OP_ALIGN_SIZE) != 0);
 
@@ -1227,12 +1249,31 @@  static int __init tcmu_module_init(void)
 		goto out_unreg_device;
 	}
 
+	for (i = 0; passthrough_attrib_attrs[i] != NULL; i++) {
+		len += sizeof(struct configfs_attribute *);
+	}
+	len += sizeof(struct configfs_attribute *) * 2;
+
+	tcmu_attrs = kzalloc(len, GFP_KERNEL);
+	if (!tcmu_attrs) {
+		ret = -ENOMEM;
+		goto out_unreg_genl;
+	}
+
+	for (i = 0; passthrough_attrib_attrs[i] != NULL; i++) {
+		tcmu_attrs[i] = passthrough_attrib_attrs[i];
+	}
+	tcmu_attrs[i] = &tcmu_attr_cmd_time_out;
+	tcmu_ops.tb_dev_attrib_attrs = tcmu_attrs;
+
 	ret = transport_backend_register(&tcmu_ops);
 	if (ret)
-		goto out_unreg_genl;
+		goto out_attrs;
 
 	return 0;
 
+out_attrs:
+	kfree(tcmu_attrs);
 out_unreg_genl:
 	genl_unregister_family(&tcmu_genl_family);
 out_unreg_device:
@@ -1246,6 +1287,7 @@  static int __init tcmu_module_init(void)
 static void __exit tcmu_module_exit(void)
 {
 	target_backend_unregister(&tcmu_ops);
+	kfree(tcmu_attrs);
 	genl_unregister_family(&tcmu_genl_family);
 	root_device_unregister(tcmu_root_device);
 	kmem_cache_destroy(tcmu_cmd_cache);