[PATCH] block: move SCSI timeout check into block

[PATCH] block: move SCSI timeout check into block

[James Bottomley]

We can eliminate the SCSI command timed out check entirely if the block
layer does this for us.  The way to do this in block is to check how
long the request has been outstanding if a requeue is requested and
ending it if we've gone over retries * timeout.

This will also eliminate many cases in SCSI where we evade the command
timeout for various reasons (like initial success converted to requeue)

James

---

diff --git a/block/blk-core.c b/block/blk-core.c
index c36aa98..d32af31 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -926,6 +926,8 @@ EXPORT_SYMBOL(blk_start_queueing);
  */
 void blk_requeue_request(struct request_queue *q, struct request *rq)
 {
+	unsigned long wait_for = (rq->retries + 1) * rq->timeout;
+
 	blk_delete_timer(rq);
 	blk_clear_rq_complete(rq);
 	blk_add_trace_rq(q, rq, BLK_TA_REQUEUE);
@@ -933,7 +935,17 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
 	if (blk_rq_tagged(rq))
 		blk_queue_end_tag(q, rq);
 
-	elv_requeue_request(q, rq);
+	if (time_before(rq->start_time + wait_for, jiffies)) {
+		int bytes = (rq->hard_nr_sectors << 9);
+
+		printk(KERN_ERR "%s: timing out command, waited %lus\n",
+		       rq->rq_disk ? rq->rq_disk->disk_name : "?",
+		       wait_for/HZ);
+		if (blk_pc_request(rq))
+			bytes = rq->data_len;
+		blk_end_request(rq, -EIO, bytes);
+	} else
+		elv_requeue_request(q, rq);
 }
 EXPORT_SYMBOL(blk_requeue_request);
 

Re: [PATCH] block: move SCSI timeout check into block

[Jens Axboe]

On Tue, Dec 16 2008, James Bottomley wrote:
> We can eliminate the SCSI command timed out check entirely if the block
> layer does this for us.  The way to do this in block is to check how
> long the request has been outstanding if a requeue is requested and
> ending it if we've gone over retries * timeout.
> 
> This will also eliminate many cases in SCSI where we evade the command
> timeout for various reasons (like initial success converted to requeue)
> 
> James
> 
> ---
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index c36aa98..d32af31 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -926,6 +926,8 @@ EXPORT_SYMBOL(blk_start_queueing);
>   */
>  void blk_requeue_request(struct request_queue *q, struct request *rq)
>  {
> +	unsigned long wait_for = (rq->retries + 1) * rq->timeout;
> +
>  	blk_delete_timer(rq);
>  	blk_clear_rq_complete(rq);
>  	blk_add_trace_rq(q, rq, BLK_TA_REQUEUE);
> @@ -933,7 +935,17 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
>  	if (blk_rq_tagged(rq))
>  		blk_queue_end_tag(q, rq);
>  
> -	elv_requeue_request(q, rq);
> +	if (time_before(rq->start_time + wait_for, jiffies)) {

I prefer time_after(), reads better.

> +		int bytes = (rq->hard_nr_sectors << 9);
> +
> +		printk(KERN_ERR "%s: timing out command, waited %lus\n",
> +		       rq->rq_disk ? rq->rq_disk->disk_name : "?",
> +		       wait_for/HZ);
> +		if (blk_pc_request(rq))
> +			bytes = rq->data_len;
> +		blk_end_request(rq, -EIO, bytes);

You can just do

                blk_end_request(rq, -EIO, blk_rq_bytes(rq));

and get rid of the 'bytes' variable and type check.

> +	} else
> +		elv_requeue_request(q, rq);
>  }
>  EXPORT_SYMBOL(blk_requeue_request);

But I agree with the patch. Will you rediff, or shall I just apply with
the suggest changes?

-- 
Jens Axboe

Re: [PATCH] block: move SCSI timeout check into block

[James Bottomley]

On Wed, 2008-12-17 at 09:06 +0100, Jens Axboe wrote:
> On Tue, Dec 16 2008, James Bottomley wrote:
> > We can eliminate the SCSI command timed out check entirely if the block
> > layer does this for us.  The way to do this in block is to check how
> > long the request has been outstanding if a requeue is requested and
> > ending it if we've gone over retries * timeout.
> > 
> > This will also eliminate many cases in SCSI where we evade the command
> > timeout for various reasons (like initial success converted to requeue)
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index c36aa98..d32af31 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -926,6 +926,8 @@ EXPORT_SYMBOL(blk_start_queueing);
> >   */
> >  void blk_requeue_request(struct request_queue *q, struct request *rq)
> >  {
> > +	unsigned long wait_for = (rq->retries + 1) * rq->timeout;
> > +
> >  	blk_delete_timer(rq);
> >  	blk_clear_rq_complete(rq);
> >  	blk_add_trace_rq(q, rq, BLK_TA_REQUEUE);
> > @@ -933,7 +935,17 @@ void blk_requeue_request(struct request_queue *q, struct request *rq)
> >  	if (blk_rq_tagged(rq))
> >  		blk_queue_end_tag(q, rq);
> >  
> > -	elv_requeue_request(q, rq);
> > +	if (time_before(rq->start_time + wait_for, jiffies)) {
> 
> I prefer time_after(), reads better.
> 
> > +		int bytes = (rq->hard_nr_sectors << 9);
> > +
> > +		printk(KERN_ERR "%s: timing out command, waited %lus\n",
> > +		       rq->rq_disk ? rq->rq_disk->disk_name : "?",
> > +		       wait_for/HZ);
> > +		if (blk_pc_request(rq))
> > +			bytes = rq->data_len;
> > +		blk_end_request(rq, -EIO, bytes);
> 
> You can just do
> 
>                 blk_end_request(rq, -EIO, blk_rq_bytes(rq));
> 
> and get rid of the 'bytes' variable and type check.
> 
> > +	} else
> > +		elv_requeue_request(q, rq);
> >  }
> >  EXPORT_SYMBOL(blk_requeue_request);
> 
> But I agree with the patch. Will you rediff, or shall I just apply with
> the suggest changes?

I'll rediff ... we're going to have to test it out quite a bit,
especially when I pull the SCSI timeout stuff.

James