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

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

On Sun, Oct 07 2007, malahal@us.ibm.com wrote:
> Jens Axboe [jens.axboe@oracle.com] wrote:
> > On Thu, Oct 04 2007, malahal@us.ibm.com wrote:
> > > Mike Christie's patches refreshed to 2.6.23-rc8-mm1.
> > > 
> > > Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> > > Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> > > 
> > > 
> > > diff -r 3697367c6e4d block/ll_rw_blk.c
> > > --- a/block/ll_rw_blk.c	Thu Sep 27 00:12:13 2007 -0700
> > > +++ b/block/ll_rw_blk.c	Thu Sep 27 00:13:07 2007 -0700
> > > @@ -181,6 +181,19 @@ void blk_queue_softirq_done(struct reque
> > >  
> > >  EXPORT_SYMBOL(blk_queue_softirq_done);
> > >  
> > > +void blk_queue_rq_timeout(struct request_queue *q, unsigned int timeout)
> > > +{
> > > +	q->rq_timeout = timeout;
> > > +}
> > > +EXPORT_SYMBOL_GPL(blk_queue_rq_timeout);
> > > +
> > > +void blk_queue_rq_timed_out(struct request_queue *q, rq_timed_out_fn *fn)
> > > +{
> > > +	q->rq_timed_out_fn = fn;
> > > +}
> > > +
> > > +EXPORT_SYMBOL_GPL(blk_queue_rq_timed_out);
> > > +
> > >  /**
> > >   * blk_queue_make_request - define an alternate make_request function for a device
> > >   * @q:  the request queue for the device to be affected
> > > @@ -243,7 +256,9 @@ static void rq_init(struct request_queue
> > >  {
> > >  	INIT_LIST_HEAD(&rq->queuelist);
> > >  	INIT_LIST_HEAD(&rq->donelist);
> > > -
> > > +	init_timer(&rq->timer);
> > > +
> > > +	rq->timeout = 0;
> > >  	rq->errors = 0;
> > >  	rq->bio = rq->biotail = NULL;
> > >  	INIT_HLIST_NODE(&rq->hash);
> > > @@ -2305,6 +2320,7 @@ EXPORT_SYMBOL(blk_start_queueing);
> > >   */
> > >  void blk_requeue_request(struct request_queue *q, struct request *rq)
> > >  {
> > > +	blk_delete_timer(rq);
> > >  	blk_add_trace_rq(q, rq, BLK_TA_REQUEUE);
> > >  
> > >  	if (blk_rq_tagged(rq))
> > > @@ -3647,8 +3663,121 @@ static struct notifier_block blk_cpu_not
> > >  };
> > >  
> > >  /**
> > > + * blk_delete_timer - Delete/cancel timer for a given function.
> > > + * @req:	request that we are canceling timer for
> > > + *
> > > + * Return value:
> > > + *     1 if we were able to detach the timer.  0 if we blew it, and the
> > > + *     timer function has already started to run.
> > > + **/
> > > +int blk_delete_timer(struct request *req)
> > > +{
> > > +	int rtn;
> > > +
> > > +	if (!req->q->rq_timed_out_fn)
> > > +		return 1;
> > > +
> > > +	rtn = del_timer(&req->timer);
> > > +	req->timer.data = (unsigned long)NULL;
> > > +	req->timer.function = NULL;
> > > +
> > > +	return rtn;
> > > +}
> > 
> > Hmm, that looks fishy. What if the timer _just_ fired, yet it hasn't
> > retrieved ->data yet?
> > 
> 
> Looks like it is just copied from the SCSI code. More over _run_timers()
> copies data and function on to stack variables before calling the timer
> function. So, I don't think there is a problem here but assuming that
> implementation detail is bad! I would like modify the above function to
> reinitialize data and function fields only if del_timer() returns TRUE.
> Please let me know if that is OK.

I guess it could be OK, but it then deserves a big fat comment
describing why it is ok.

> > > +
> > > +EXPORT_SYMBOL_GPL(blk_delete_timer);
> > > +
> > > +static void blk_rq_timed_out(struct request *req)
> > > +{
> > > +	struct request_queue *q = req->q;
> > > +
> > > +	switch (q->rq_timed_out_fn(req)) {
> > > +	case BLK_EH_HANDLED:
> > > +		__blk_complete_request(req);
> > > +		return;
> > > +	case BLK_EH_RESET_TIMER:
> > > +		blk_add_timer(req);
> > > +		return;
> > > +	case BLK_EH_NOT_HANDLED:
> > > +		/*
> > > +		 * LLD handles this for now but in the future
> > > +		 * we can send a request msg to abort the command
> > > +		 * and we can move more of the generic scsi eh code to
> > > +		 * the blk layer.
> > > +		 */
> > > +		return;
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * blk_abort_req -- Request request recovery for the specified command
> > > + * req:		pointer to the request of interest
> > > + *
> > > + * This function requests that the block layer start recovery for the
> > > + * request by deleting the timer and calling the q's timeout function.
> > > + * LLDDs who implement their own error recovery MAY ignore the timeout
> > > + * event if they generated blk_abort_req.
> > > + */
> > > +void blk_abort_req(struct request *req)
> > > +{
> > > +        if (!blk_delete_timer(req))
> > > +                return;
> > > +        blk_rq_timed_out(req);
> > > +}
> > > +
> > > +EXPORT_SYMBOL_GPL(blk_abort_req);
> > > +
> > > +/**
> > > + * blk_add_timer - Start timeout timer for a single request
> > > + * @req:	request that is about to start running.
> > > + *
> > > + * Notes:
> > > + *    Each request has its own timer, and as it is added to the queue, we
> > > + *    set up the timer.  When the request completes, we cancel the timer.
> > > + **/
> > > +void blk_add_timer(struct request *req)
> > > +{
> > > +	struct request_queue *q = req->q;
> > > +
> > > +	/*
> > > +	 * If the clock was already running for this command, then
> > > +	 * first delete the timer.  The timer handling code gets rather
> > > +	 * confused if we don't do this.
> > > +	 */
> > > +	if (req->timer.function)
> > > +		del_timer(&req->timer);
> > > +
> > > +	req->timer.data = (unsigned long)req;
> > > +	if (req->timeout)
> > > +		req->timer.expires = jiffies + req->timeout;
> > > +	else
> > > +		req->timer.expires = jiffies + q->rq_timeout;
> > > +	req->timer.function = (void (*)(unsigned long))blk_rq_timed_out;
> > 
> > Why the cast?
> > 
> 
> Because blk_rq_timed_out() doesn't take "unsigned long" as its input
> parameter. Again, copied from SCSI.

Don't copy stuff from SCSI just because you can, it's allowed to fix
things up along the way! I'd propose adding blk_rq_timed_out_timer()
ala:

static void blk_rq_timed_out_timer(unsigned long data)
{
        struct request *rq = (struct request *) data;

        blk_rq_timed_out(rq);
}

and using that to get rid of the cast.

> 
> > > +        add_timer(&req->timer);
> > 
> > You have space vs tab issues.
> > 
> > > +}
> > 
> > That's an ugly interface. I'd say it's a bug to call blk_add_timer()
> > with it already pending, so fix the API.
> 
> blk_add_timer is also called from the timer function (blk_rq_timed_out) if
> BLK_EH_RESET_TIMER is retuned. This is also copied from SCSI! Please let me
> know if you want to do it some other way.

I thought I just did :-)

Delete the timer first, the API you are proposing isn't very nice.

-- 
Jens Axboe