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

From: Weiping Zhang <zwp10758@gmail.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Weiping Zhang <zhangweiping@didiglobal.com>,
	Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org
Subject: Re: [PATCH v3] block: add documentation for io_timeout
Date: Wed, 26 Dec 2018 09:51:25 +0800	[thread overview]
Message-ID: <CAA70yB4LNi=ob31MhL4Pc+uWxvnehbq-CMJjtsNLMC_BM8a2CQ@mail.gmail.com> (raw)
In-Reply-To: <1544113174.185366.240.camel@acm.org>

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