| Message ID | 20181205141701.GA33910@192.168.3.9 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] block: add documentation for io_timeout | expand |
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---
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---
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---
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.
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.
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.
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
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
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(+)