Re: [RFC] [PATCH 1/1] blk request timeout handler patches

[Jens Axboe <jens.axboe@oracle.com>]

On Mon, Oct 22 2007, malahal@us.ibm.com wrote:
> Jens Axboe [jens.axboe@oracle.com] wrote:
> > Current code is below, btw. Not a lot of changes, iirc it's just the
> > delete-always, a missing export, delete timer on empty list.
> > 
> > +void blk_add_timer(struct request *req)
> > +{
> > +	struct request_queue *q = req->q;
> > +	unsigned long expiry;
> > +
> > +	BUG_ON(!list_empty(&req->timeout_list));
> > +
> > +	if (req->timeout)
> > +		req->timeout += jiffies;
> > +	else
> > +		req->timeout = jiffies + q->rq_timeout;
> > +
> 
> The meaning of req->timeout is changed here. It was a timeout value, now
> it became an expiry time. If we happen to requeue the request, its
> timeout would be incorrect. Found using scsi_debug! Maybe, make it a
> bitfield to avoid another field...

Yes, I did that deliberately, did expect some fallout... Note how I
changed ->timeout to be an unsigned long as well.

> > +	list_add_tail(&req->timeout_list, &q->timeout_list);
> > +
> > +	/*
> > +	 * If the timer isn't already pending or this timeout is earlier
> > +	 * than an existing one, modify the timer. Round to next nearest
> > +	 * second.
> > +	 */
> > +	expiry = round_jiffies(req->timeout);
> > +	if (!timer_pending(&q->timeout) ||
> > +	    time_before(expiry, q->timeout.expires))
> > +		mod_timer(&q->timeout, expiry);
> 
> How about changing q->timeout to q->timer?

There are more timers in the queue, ->timer doesn't tell you what it's
for. I considered ->timeout_timer, but it's a bit long.

> > diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
> > index 32982eb..22a9013 100644
> > --- a/drivers/scsi/gdth_proc.c
> > +++ b/drivers/scsi/gdth_proc.c
> > @@ -846,19 +846,19 @@ static int gdth_update_timeout(int hanum, Scsi_Cmnd *scp, int timeout)
> >  {
> >      int oldto;
> > 
> > -    oldto = scp->timeout_per_command;
> > -    scp->timeout_per_command = timeout;
> > +    oldto = scp->request->timeout;
> > +    scp->request->timeout = timeout;
> > 
> >      if (timeout == 0) {
> > -        del_timer(&scp->eh_timeout);
> > -        scp->eh_timeout.data = (unsigned long) NULL;
> > -        scp->eh_timeout.expires = 0;
> > +        del_timer(&scp->request->timer);
> > +        scp->request->timer.data = (unsigned long) NULL;
> > +        scp->request->timer.expires = 0;
> 
> request doesn't have "timer" field, code compilation fails for this
> file.

Thanks, will fix that up.

-- 
Jens Axboe