On Thu, Oct 30 2008, FUJITA Tomonori wrote:
> On Tue, 28 Oct 2008 23:06:25 -0700
> malahal@xxxxxxxxxx wrote:
>
> > FUJITA Tomonori [fujita.tomonori@xxxxxxxxxxxxx] wrote:
> > > On Tue, 28 Oct 2008 11:22:43 -0700
> > > malahal@xxxxxxxxxx wrote:
> > >
> > > > Now the rq->deadline can't be zero if the request is in the
> > > > timeout_list, so there is no need to have next_set. There is no need to
> > > > access a request's deadline field if blk_rq_timed_out is called on it.
> > > >
> > > > Signed-off-by: Malahal Naineni <malahal@xxxxxxxxxx>
> > > >
> > > > diff -r a6ae42397ede block/blk-timeout.c
> > > > --- a/block/blk-timeout.c Thu Oct 23 11:48:45 2008 -0700
> > > > +++ b/block/blk-timeout.c Fri Oct 24 17:08:24 2008 -0700
> > > > @@ -118,7 +118,7 @@
> > > > void blk_rq_timed_out_timer(unsigned long data)
> > > > {
> > > > struct request_queue *q = (struct request_queue *) data;
> > > > - unsigned long flags, uninitialized_var(next), next_set = 0;
> > > > + unsigned long flags, next = 0;
> > > > struct request *rq, *tmp;
> > > >
> > > > spin_lock_irqsave(q->queue_lock, flags);
> > > > @@ -133,15 +133,13 @@
> > > > if (blk_mark_rq_complete(rq))
> > > > continue;
> > > > blk_rq_timed_out(rq);
> > > > + } else {
> > > > + if (!next || time_after(next, rq->deadline))
> > > > + next = rq->deadline;
> > >
> > > Hmm, blk_rq_timed_out(rq) could put the rq back to q->timeout_list. We
> > > need to take care of the rq->deadline like this?
> >
> > If it is put back on the list, it would be placed at the end of the list
> > so we will get to work on it again in the list traversal. There is no
> > need to access it now.
>
> Ah, I see.
>
>
> > > > if (blk_mark_rq_complete(rq))
> > > > continue;
> > > > blk_rq_timed_out(rq);
> > > > + }
> > > >
> > > > + if (!next || time_after(next, rq->deadline))
> > > > + next = rq->deadline;
> > >
> > > But if blk_rq_timed_out calls __blk_complete_request, we don't want to
> > > touch the rq here, I guess.
> >
> > Yes, we should not access it although I don't think the request can be
> > freed while the existing code is accessing the deadline field.
>
> I don't think so too.
>
> > What
> > likely can happen is that we may call mod_timer with jiffies that is
> > older than current which would call the timer immediately...
>
> Yeah, I think that the timer is called immediately here. It's
> unnecessary.
Hmm, just checked the code, and indeed it does. Have the timers always
behaved like that?
> The patch looks good to me.
No further gried from me either, I've applied it. It does need a comment
on why 'next' cannot be 0 validly, because ->deadline is always rounded
one up if that happens.
--
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
