[RFC PATCH v2] block: add io timeout to sysfs

[RFC PATCH v2] block: add io timeout to sysfs

[Weiping Zhang]

Give a interface to adjust io timeout by device.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---

Changes since v1:
* make sure timeout > 0

 block/blk-sysfs.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 80eef48fddc8..90a927514d30 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -417,6 +417,26 @@ static ssize_t queue_poll_store(struct request_queue *q, const char *page,
 	return ret;
 }
 
+static ssize_t queue_io_timeout_show(struct request_queue *q, char *page)
+{
+	return sprintf(page, "%u\n", q->rq_timeout);
+}
+
+static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
+				  size_t count)
+{
+	unsigned int val;
+	int err;
+
+	err = kstrtou32(page, 10, &val);
+	if (err || val == 0)
+		return -EINVAL;
+
+	blk_queue_rq_timeout(q, val);
+
+	return count;
+}
+
 static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
 {
 	if (!wbt_rq_qos(q))
@@ -685,6 +705,12 @@ static struct queue_sysfs_entry queue_dax_entry = {
 	.show = queue_dax_show,
 };
 
+static struct queue_sysfs_entry queue_io_timeout_entry = {
+	.attr = {.name = "io_timeout", .mode = 0644 },
+	.show = queue_io_timeout_show,
+	.store = queue_io_timeout_store,
+};
+
 static struct queue_sysfs_entry queue_wb_lat_entry = {
 	.attr = {.name = "wbt_lat_usec", .mode = 0644 },
 	.show = queue_wb_lat_show,
@@ -734,6 +760,7 @@ static struct attribute *default_attrs[] = {
 	&queue_dax_entry.attr,
 	&queue_wb_lat_entry.attr,
 	&queue_poll_delay_entry.attr,
+	&queue_io_timeout_entry.attr,
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	&throtl_sample_time_entry.attr,
 #endif
-- 
2.14.1

Re: [RFC PATCH v2] block: add io timeout to sysfs

[Weiping Zhang]

Hi Jens,

It's useful if user want a short timeout when a disk doesn't work normally.
Would you give some comments for this patch,

Thanks a lot
Weiping Zhang <zhangweiping@didiglobal.com> 于2018年11月19日周一 下午10:30写道：
>
> Give a interface to adjust io timeout by device.
>
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
>
> Changes since v1:
> * make sure timeout > 0
>
>  block/blk-sysfs.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 80eef48fddc8..90a927514d30 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -417,6 +417,26 @@ static ssize_t queue_poll_store(struct request_queue *q, const char *page,
>         return ret;
>  }
>
> +static ssize_t queue_io_timeout_show(struct request_queue *q, char *page)
> +{
> +       return sprintf(page, "%u\n", q->rq_timeout);
> +}
> +
> +static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
> +                                 size_t count)
> +{
> +       unsigned int val;
> +       int err;
> +
> +       err = kstrtou32(page, 10, &val);
> +       if (err || val == 0)
> +               return -EINVAL;
> +
> +       blk_queue_rq_timeout(q, val);
> +
> +       return count;
> +}
> +
>  static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
>  {
>         if (!wbt_rq_qos(q))
> @@ -685,6 +705,12 @@ static struct queue_sysfs_entry queue_dax_entry = {
>         .show = queue_dax_show,
>  };
>
> +static struct queue_sysfs_entry queue_io_timeout_entry = {
> +       .attr = {.name = "io_timeout", .mode = 0644 },
> +       .show = queue_io_timeout_show,
> +       .store = queue_io_timeout_store,
> +};
> +
>  static struct queue_sysfs_entry queue_wb_lat_entry = {
>         .attr = {.name = "wbt_lat_usec", .mode = 0644 },
>         .show = queue_wb_lat_show,
> @@ -734,6 +760,7 @@ static struct attribute *default_attrs[] = {
>         &queue_dax_entry.attr,
>         &queue_wb_lat_entry.attr,
>         &queue_poll_delay_entry.attr,
> +       &queue_io_timeout_entry.attr,
>  #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>         &throtl_sample_time_entry.attr,
>  #endif
> --
> 2.14.1
>

Re: [RFC PATCH v2] block: add io timeout to sysfs

[Jens Axboe]

On 11/28/18 7:52 AM, Weiping Zhang wrote:
> Hi Jens,
> 
> It's useful if user want a short timeout when a disk doesn't work normally.
> Would you give some comments for this patch,

I'm fine with the patch, in fact I've posted a similar/identical one in
the past but just never got around to merging it. I think we should
expose the value in seconds or msecs, however, not in jiffies. That
makes it hard to use for people.

So modify it to do a msecs_to_jiffies() on the value passed in.

-- 
Jens Axboe

Re: [RFC PATCH v2] block: add io timeout to sysfs

[Bart Van Assche]

On Mon, 2018-11-19 at 22:11 +0800, Weiping Zhang wrote:
> Give a interface to adjust io timeout by device.
> 
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
> 
> Changes since v1:
> * make sure timeout > 0
> 
>  block/blk-sysfs.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)

Documentation for new block layer sysfs attributes should be added in
Documentation/ABI/testing/sysfs-block and also in Documentation/block/queue-sysfs.txt.
Please add such documentation for this new attribute.

> +static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
> +				  size_t count)
> +{
> +	unsigned int val;
> +	int err;
> +
> +	err = kstrtou32(page, 10, &val);
> +	if (err || val == 0)
> +		return -EINVAL;
> +
> +	blk_queue_rq_timeout(q, val);
> +
> +	return count;
> +}

Setting the block layer timeout to a very high value (e.g. hours) may make it look
like a request got stuck without users having an easy way of figuring out what is
going on. I'm wondering whether this function should restrict the upper bound for
block layer timeouts. How about limiting timeout values to ten minutes?

Thanks,

Bart.

Re: [RFC PATCH v2] block: add io timeout to sysfs

[Jens Axboe]

On 11/28/18 9:13 AM, Bart Van Assche wrote:
> On Mon, 2018-11-19 at 22:11 +0800, Weiping Zhang wrote:
>> Give a interface to adjust io timeout by device.
>>
>> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
>> ---
>>
>> Changes since v1:
>> * make sure timeout > 0
>>
>>  block/blk-sysfs.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
> 
> Documentation for new block layer sysfs attributes should be added in
> Documentation/ABI/testing/sysfs-block and also in Documentation/block/queue-sysfs.txt.
> Please add such documentation for this new attribute.

Yes, please send a followup patch to add the documentation.

>> +static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
>> +				  size_t count)
>> +{
>> +	unsigned int val;
>> +	int err;
>> +
>> +	err = kstrtou32(page, 10, &val);
>> +	if (err || val == 0)
>> +		return -EINVAL;
>> +
>> +	blk_queue_rq_timeout(q, val);
>> +
>> +	return count;
>> +}
> 
> Setting the block layer timeout to a very high value (e.g. hours) may make it look
> like a request got stuck without users having an easy way of figuring out what is
> going on. I'm wondering whether this function should restrict the upper bound for
> block layer timeouts. How about limiting timeout values to ten minutes?

This is no different than folks using SG_IO/bsg and putting a high timeout
in their commands. I don't think we should impose a limit, if you set it
high, you get exactly what you asked for.

-- 
Jens Axboe