[PATCH v3] block: add documentation for io_timeout

[PATCH v3] block: add documentation for io_timeout

[Weiping Zhang]

Add documentation for /sys/block/<disk>/queue/io_timeout.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 Documentation/ABI/testing/sysfs-block | 10 ++++++++++
 Documentation/block/queue-sysfs.txt   |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
index dea212db9df3..f254a374710a 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -271,3 +271,13 @@ Description:
 		size of 512B sectors of the zones of the device, with
 		the eventual exception of the last zone of the device
 		which may be smaller.
+
+What:		/sys/block/<disk>/queue/io_timeout
+Date:		November 2018
+Contact:	Weiping Zhang <zhangweiping@didiglobal.com>
+Description:
+		io_timeout is a request’s timeouts at block layer in
+		milliseconds. When the underlying driver starts processing
+		a request, the generic block layer will start a timer, if
+		this request cannot be completed in io_timeout milliseconds,
+		a timeout event will occur.
diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
index 2c1e67058fd3..f0c9bbce73fd 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -67,6 +67,13 @@ If set to a value larger than 0, the kernel will put the process issuing
 IO to sleep for this amont of microseconds before entering classic
 polling.
 
+io_timeout (RW)
+---------------
+This is a request’s timeouts at block layer in milliseconds. When the
+underlying driver starts processing a request, the generic block layer
+will start a timer, if this request cannot be completed in io_timeout
+milliseconds, a timeout event will occur.
+
 iostats (RW)
 -------------
 This file is used to control (on/off) the iostats accounting of the
-- 
2.14.1

Re: [PATCH v3] block: add documentation for io_timeout

[Christoph Hellwig]

Can you please also send a patch to not show this attribute for
drivers without a timeout handler?  Thanks!

On Wed, Dec 05, 2018 at 10:17:06PM +0800, Weiping Zhang wrote:
> Add documentation for /sys/block/<disk>/queue/io_timeout.
> 
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
>  Documentation/ABI/testing/sysfs-block | 10 ++++++++++
>  Documentation/block/queue-sysfs.txt   |  7 +++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
> index dea212db9df3..f254a374710a 100644
> --- a/Documentation/ABI/testing/sysfs-block
> +++ b/Documentation/ABI/testing/sysfs-block
> @@ -271,3 +271,13 @@ Description:
>  		size of 512B sectors of the zones of the device, with
>  		the eventual exception of the last zone of the device
>  		which may be smaller.
> +
> +What:		/sys/block/<disk>/queue/io_timeout
> +Date:		November 2018
> +Contact:	Weiping Zhang <zhangweiping@didiglobal.com>
> +Description:
> +		io_timeout is a request’s timeouts at block layer in
> +		milliseconds. When the underlying driver starts processing
> +		a request, the generic block layer will start a timer, if
> +		this request cannot be completed in io_timeout milliseconds,
> +		a timeout event will occur.
> diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
> index 2c1e67058fd3..f0c9bbce73fd 100644
> --- a/Documentation/block/queue-sysfs.txt
> +++ b/Documentation/block/queue-sysfs.txt
> @@ -67,6 +67,13 @@ If set to a value larger than 0, the kernel will put the process issuing
>  IO to sleep for this amont of microseconds before entering classic
>  polling.
>  
> +io_timeout (RW)
> +---------------
> +This is a request’s timeouts at block layer in milliseconds. When the
> +underlying driver starts processing a request, the generic block layer
> +will start a timer, if this request cannot be completed in io_timeout
> +milliseconds, a timeout event will occur.
> +
>  iostats (RW)
>  -------------
>  This file is used to control (on/off) the iostats accounting of the
> -- 
> 2.14.1
> 
---end quoted text---

Re: [PATCH v3] block: add documentation for io_timeout

[Weiping Zhang]

Christoph Hellwig <hch@infradead.org> 于2018年12月5日周三 下午10:40写道：
>
> Can you please also send a patch to not show this attribute for
> drivers without a timeout handler?  Thanks!
>
Do you means checking q->mq_ops->timeout is NULL or not ?

static void blk_mq_rq_timed_out(struct request *req, bool reserved)
{
        req->rq_flags |= RQF_TIMED_OUT;
If it's NULL, only re-start a timer for this request in blk_add_timer.
Is there some defect ? if show this attribute and the driver doesn't
support timeout handler ？

        if (req->q->mq_ops->timeout) {
                enum blk_eh_timer_return ret;

                ret = req->q->mq_ops->timeout(req, reserved);
                if (ret == BLK_EH_DONE)
                        return;
                WARN_ON_ONCE(ret != BLK_EH_RESET_TIMER);
        }

        blk_add_timer(req);
}


> On Wed, Dec 05, 2018 at 10:17:06PM +0800, Weiping Zhang wrote:
> > Add documentation for /sys/block/<disk>/queue/io_timeout.
> >
> > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > ---
> >  Documentation/ABI/testing/sysfs-block | 10 ++++++++++
> >  Documentation/block/queue-sysfs.txt   |  7 +++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
> > index dea212db9df3..f254a374710a 100644
> > --- a/Documentation/ABI/testing/sysfs-block
> > +++ b/Documentation/ABI/testing/sysfs-block
> > @@ -271,3 +271,13 @@ Description:
> >               size of 512B sectors of the zones of the device, with
> >               the eventual exception of the last zone of the device
> >               which may be smaller.
> > +
> > +What:                /sys/block/<disk>/queue/io_timeout
> > +Date:                November 2018
> > +Contact:     Weiping Zhang <zhangweiping@didiglobal.com>
> > +Description:
> > +             io_timeout is a request’s timeouts at block layer in
> > +             milliseconds. When the underlying driver starts processing
> > +             a request, the generic block layer will start a timer, if
> > +             this request cannot be completed in io_timeout milliseconds,
> > +             a timeout event will occur.
> > diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
> > index 2c1e67058fd3..f0c9bbce73fd 100644
> > --- a/Documentation/block/queue-sysfs.txt
> > +++ b/Documentation/block/queue-sysfs.txt
> > @@ -67,6 +67,13 @@ If set to a value larger than 0, the kernel will put the process issuing
> >  IO to sleep for this amont of microseconds before entering classic
> >  polling.
> >
> > +io_timeout (RW)
> > +---------------
> > +This is a request’s timeouts at block layer in milliseconds. When the
> > +underlying driver starts processing a request, the generic block layer
> > +will start a timer, if this request cannot be completed in io_timeout
> > +milliseconds, a timeout event will occur.
> > +
> >  iostats (RW)
> >  -------------
> >  This file is used to control (on/off) the iostats accounting of the
> > --
> > 2.14.1
> >
> ---end quoted text---

Re: [PATCH v3] block: add documentation for io_timeout

[Weiping Zhang]

Weiping Zhang <zwp10758@gmail.com> 于2018年12月5日周三 下午10:49写道：
>
> Christoph Hellwig <hch@infradead.org> 于2018年12月5日周三 下午10:40写道：
> >
> > Can you please also send a patch to not show this attribute for
> > drivers without a timeout handler?  Thanks!
> >
Is there a simple way do that ?

Shall we return -ENOTSUPP when user read/write this attribute when
driver has no timeout handler ?
> Do you means checking q->mq_ops->timeout is NULL or not ?
>
> static void blk_mq_rq_timed_out(struct request *req, bool reserved)
> {
>         req->rq_flags |= RQF_TIMED_OUT;
> If it's NULL, only re-start a timer for this request in blk_add_timer.
> Is there some defect ? if show this attribute and the driver doesn't
> support timeout handler ？
>
>         if (req->q->mq_ops->timeout) {
>                 enum blk_eh_timer_return ret;
>
>                 ret = req->q->mq_ops->timeout(req, reserved);
>                 if (ret == BLK_EH_DONE)
>                         return;
>                 WARN_ON_ONCE(ret != BLK_EH_RESET_TIMER);
>         }
>
>         blk_add_timer(req);
> }
>
>
> > On Wed, Dec 05, 2018 at 10:17:06PM +0800, Weiping Zhang wrote:
> > > Add documentation for /sys/block/<disk>/queue/io_timeout.
> > >
> > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-block | 10 ++++++++++
> > >  Documentation/block/queue-sysfs.txt   |  7 +++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
> > > index dea212db9df3..f254a374710a 100644
> > > --- a/Documentation/ABI/testing/sysfs-block
> > > +++ b/Documentation/ABI/testing/sysfs-block
> > > @@ -271,3 +271,13 @@ Description:
> > >               size of 512B sectors of the zones of the device, with
> > >               the eventual exception of the last zone of the device
> > >               which may be smaller.
> > > +
> > > +What:                /sys/block/<disk>/queue/io_timeout
> > > +Date:                November 2018
> > > +Contact:     Weiping Zhang <zhangweiping@didiglobal.com>
> > > +Description:
> > > +             io_timeout is a request’s timeouts at block layer in
> > > +             milliseconds. When the underlying driver starts processing
> > > +             a request, the generic block layer will start a timer, if
> > > +             this request cannot be completed in io_timeout milliseconds,
> > > +             a timeout event will occur.
> > > diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
> > > index 2c1e67058fd3..f0c9bbce73fd 100644
> > > --- a/Documentation/block/queue-sysfs.txt
> > > +++ b/Documentation/block/queue-sysfs.txt
> > > @@ -67,6 +67,13 @@ If set to a value larger than 0, the kernel will put the process issuing
> > >  IO to sleep for this amont of microseconds before entering classic
> > >  polling.
> > >
> > > +io_timeout (RW)
> > > +---------------
> > > +This is a request’s timeouts at block layer in milliseconds. When the
> > > +underlying driver starts processing a request, the generic block layer
> > > +will start a timer, if this request cannot be completed in io_timeout
> > > +milliseconds, a timeout event will occur.
> > > +
> > >  iostats (RW)
> > >  -------------
> > >  This file is used to control (on/off) the iostats accounting of the
> > > --
> > > 2.14.1
> > >
> > ---end quoted text---

Re: [PATCH v3] block: add documentation for io_timeout

[Bart Van Assche]

On Wed, 2018-12-05 at 22:59 +0800, Weiping Zhang wrote:
> Weiping Zhang <zwp10758@gmail.com> 于2018年12月5日周三 下午10:49写道：
> > Christoph Hellwig <hch@infradead.org> 于2018年12月5日周三 下午10:40写道：
> > > Can you please also send a patch to not show this attribute for
> > > drivers without a timeout handler?  Thanks!
> 
> Is there a simple way do that ?

How about checking the timeout member of struct blk_mq_ops for blk-mq and
checking the rq_timed_out_fn member in struct request_queue for the legacy
block layer?

> Shall we return -ENOTSUPP when user read/write this attribute when
> driver has no timeout handler ?

A much more elegant solution is to introduce a sysfs attribute group for the
io_timeout attribute and to make that group visible only if a timeout handler
has been defined. See e.g. disk_attr_group in block/genhd.c for an example.

Bart.

Re: [PATCH v3] block: add documentation for io_timeout

[Christoph Hellwig]

On Thu, Dec 06, 2018 at 08:06:16AM -0800, Bart Van Assche wrote:
> On Wed, 2018-12-05 at 22:59 +0800, Weiping Zhang wrote:
> > Weiping Zhang <zwp10758@gmail.com> 于2018年12月5日周三 下午10:49写道：
> > > Christoph Hellwig <hch@infradead.org> 于2018年12月5日周三 下午10:40写道：
> > > > Can you please also send a patch to not show this attribute for
> > > > drivers without a timeout handler?  Thanks!
> > 
> > Is there a simple way do that ?
> 
> How about checking the timeout member of struct blk_mq_ops for blk-mq and
> checking the rq_timed_out_fn member in struct request_queue for the legacy
> block layer?

Just the former given that the legacy code is gone in for-next.

> > Shall we return -ENOTSUPP when user read/write this attribute when
> > driver has no timeout handler ?
> 
> A much more elegant solution is to introduce a sysfs attribute group for the
> io_timeout attribute and to make that group visible only if a timeout handler
> has been defined. See e.g. disk_attr_group in block/genhd.c for an example.

Agreed, that is the way to go.

Re: [PATCH v3] block: add documentation for io_timeout

[Bart Van Assche]

On Wed, 2018-12-05 at 22:17 +0800, Weiping Zhang wrote:
> +Description:
> +		io_timeout is a request’s timeouts at block layer in
> +		milliseconds. When the underlying driver starts processing
> +		a request, the generic block layer will start a timer, if
> +		this request cannot be completed in io_timeout milliseconds,
> +		a timeout event will occur.

Sorry but I think this description is still somewhat confusing. How about
changing that description into the following?

	io_timeout is the request timeout in milliseconds. If a request does not
	complete in this time then the block driver timeout handler is invoked.
	That timeout handler can decide to retry the request, to fail it or to
	start a device recovery strategy.

Bart.

Re: [PATCH v3] block: add documentation for io_timeout

[Weiping Zhang]

Bart Van Assche <bvanassche@acm.org> 于2018年12月7日周五 上午12:22写道：
>
> On Wed, 2018-12-05 at 22:17 +0800, Weiping Zhang wrote:
> > +Description:
> > +             io_timeout is a request’s timeouts at block layer in
> > +             milliseconds. When the underlying driver starts processing
> > +             a request, the generic block layer will start a timer, if
> > +             this request cannot be completed in io_timeout milliseconds,
> > +             a timeout event will occur.
>
> Sorry but I think this description is still somewhat confusing. How about
> changing that description into the following?
>
>         io_timeout is the request timeout in milliseconds. If a request does not
>         complete in this time then the block driver timeout handler is invoked.
>         That timeout handler can decide to retry the request, to fail it or to
>         start a device recovery strategy.
>
Sorry for late reply, thanks for your suggestion I'll post it in V4.
> Bart.

>> > Is there a simple way do that ?
>>
>> How about checking the timeout member of struct blk_mq_ops for blk-mq and
>> checking the rq_timed_out_fn member in struct request_queue for the legacy
>> block layer?
>
>Just the former given that the legacy code is gone in for-next.
>
>> > Shall we return -ENOTSUPP when user read/write this attribute when
> >> driver has no timeout handler ?
> >
> >A much more elegant solution is to introduce a sysfs attribute group for the
> >io_timeout attribute and to make that group visible only if a timeout handler
> >has been defined. See e.g. disk_attr_group in block/genhd.c for an example.
>
>Agreed, that is the way to go.

OK, I'll do that.

Thanks
Weiping