Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout

[James Bottomley <jbottomley@parallels.com>]

On Fri, 2014-06-06 at 12:18 -0500, Mike Christie wrote:
> On 6/5/14, 9:53 PM, KY Srinivasan wrote:
> >
> >
> >> -----Original Message-----
> >> From: Mike Christie [mailto:michaelc@cs.wisc.edu]
> >> Sent: Thursday, June 5, 2014 6:33 PM
> >> To: KY Srinivasan
> >> Cc: James Bottomley; linux-kernel@vger.kernel.org; apw@canonical.com;
> >> devel@linuxdriverproject.org; hch@infradead.org; linux-
> >> scsi@vger.kernel.org; ohering@suse.com; gregkh@linuxfoundation.org;
> >> jasowang@redhat.com
> >> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
> >> from the basic I/O timeout
> >>
> >> On 06/04/2014 12:15 PM, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: James Bottomley [mailto:jbottomley@parallels.com]
> >>>> Sent: Wednesday, June 4, 2014 10:02 AM
> >>>> To: KY Srinivasan
> >>>> Cc: linux-kernel@vger.kernel.org; apw@canonical.com;
> >>>> devel@linuxdriverproject.org; hch@infradead.org; linux-
> >>>> scsi@vger.kernel.org; ohering@suse.com; gregkh@linuxfoundation.org;
> >>>> jasowang@redhat.com
> >>>> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the
> >>>> FLUSH_TIMEOUT from the basic I/O timeout
> >>>>
> >>>> On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
> >>>>> Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to
> >>>>> derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this
> >>>>> patch did not use the basic I/O timeout of the device. Fix this bug.
> >>>>>
> >>>>> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> >>>>> ---
> >>>>>   drivers/scsi/sd.c |    4 +++-
> >>>>>   1 files changed, 3 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> >>>>> e9689d5..54150b1 100644
> >>>>> --- a/drivers/scsi/sd.c
> >>>>> +++ b/drivers/scsi/sd.c
> >>>>> @@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct
> >>>>> scsi_device *sdp, struct request *rq)
> >>>>>
> >>>>>   static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
> >>>>> request *rq)  {
> >>>>> -	rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> >>>>> +	int timeout = sdp->request_queue->rq_timeout;
> >>>>> +
> >>>>> +	rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
> >>>>
> >>>> Could you share where you found this to be a problem?  It looks like
> >>>> a bug in block because all inbound requests being prepared should
> >>>> have a timeout set, so block would be the place to fix it.
> >>>
> >>> Perhaps; what I found was that the value in rq->timeout was 0 coming
> >>> into this function and thus multiplying obviously has no effect.
> >>>
> >>
> >> I think you are right. We hit this problem because we are doing:
> >>
> >> scsi_request_fn -> blk_peek_request -> sd_prep_fn ->
> >> scsi_setup_flush_cmnd.
> >>
> >> At this time request->timeout is zero so the multiplication does nothing. See
> >> how sd_setup_write_same_cmnd will set the request->timeout at this time.
> >>
> >> Then in scsi_request_fn we do:
> >>
> >> scsi_request_fn -> blk_start_request -> blk_add_timer.
> >>
> >> At this time it will set the request->timeout if something like req block pc
> >> users (like scsi_execute() or block/scsi_ioctl.c) or the write same code
> >> mentioned above have not set the timeout.
> >
> > I don't think this is a recent change. Prior to this commit, we were setting the timeout
> > value in this function; it just happened to be a different constant unrelated to the I/O
> > timeout.
> >
> 
> Yeah, it looks like when 7e660100d85af860e7ad763202fff717adcdaacd was 
> merged we were supposed to initialize it like in your patch in this thread.
> 
> I guess we could do your patch in this thread, or if we want the block 
> layer to initialize the timeout before the prep_fn callout is called 
> then we would need to have the blk-flush.c code to that when it sets up 
> the request. If we do the latter, do we want the discard and write same 
> code to initialize the request's timeout before the prep_fn callout is 
> called too?

I looked through the call chain; it seems to be intentional behaviour on
the part of block.  Just from an mq point of view, it would make better
code if we unconditionally initialised rq->timeout early and allowed
prep to modify it and then dumped the if(!req->timeout) in
blk_add_timer(), but it's a marginal if condition that would compile to
a conditional store on sensible architectures, so losing the conditional
probably isn't worth worrying about.

Cc'd Jens for his opinion with the block patch

James

---

diff --git a/block/blk-core.c b/block/blk-core.c
index a0e3096..cad6b2a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -111,6 +111,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->cmd = rq->__cmd;
 	rq->cmd_len = BLK_MAX_CDB;
 	rq->tag = -1;
+	rq->timeout = q->rq_timeout;
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
 	rq->part = NULL;
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index d96f706..9063ade 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -180,13 +180,6 @@ void __blk_add_timer(struct request *req, struct list_head *timeout_list)
 
 	BUG_ON(!list_empty(&req->timeout_list));
 
-	/*
-	 * Some LLDs, like scsi, peek at the timeout to prevent a
-	 * command from being retried forever.
-	 */
-	if (!req->timeout)
-		req->timeout = q->rq_timeout;
-
 	req->deadline = jiffies + req->timeout;
 	if (timeout_list)
 		list_add_tail(&req->timeout_list, timeout_list);