On Mon, Oct 22 2007, malahal@xxxxxxxxxx wrote:
> Jens Axboe [jens.axboe@xxxxxxxxxx] 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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
