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.
The patch looks good to me.
--
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
