[PATCH] improvement of fastfail operation

[PATCH] improvement of fastfail operation

[Masao Fukuchi]

Hi all,

We are planning to use linux for enterprise server.
Since the reliability of data is important factor, this server has RAID
or clustering system.
Also, this server needs quick response to host(< 30sec) even if device/
path error occurs.

We are planning to use fastfail flag for this purpose.
We reviewed the sequence of fastfail, but the operation is inadequate for
some error cases(mainly command timeout).

We propose the following improvements for fastfail.

1.Validate fastfail flag for command timeout.
  Currently fastfail flag is not valid for command timeout and repeats
  4 times.
2.Set timeout value to 10sec.
  Currently timeout value is set to 30sec.
3.Set wait time for bus reset/host reset to 5sec.
  Currently wait time is set to 10sec.
  (In many cases, abort task command fails for command timeout and it needs
  bus reset or host reset operation)

Each timeout values come from:
  timeout(10sec)+Abort/Bus reset(5sec+)+alt retry timeout(10sec) < 30sec

This is one idea for quick response on device/path error.
If you have any comments or idea for this improvements, please let me know.

Thanks,
Masao Fukuchi

diff -urN linux-2.6.4/drivers/scsi/scsi_error.c linux-2.6.4FF/drivers/scsi/scsi_error.c
--- linux-2.6.4/drivers/scsi/scsi_error.c       2004-02-18 12:57:12.000000000 +0900
+++ linux-2.6.4FF/drivers/scsi/scsi_error.c     2004-03-18 16:59:50.000000000 +0900
@@ -43,6 +43,8 @@
  */
 #define BUS_RESET_SETTLE_TIME   10*HZ
 #define HOST_RESET_SETTLE_TIME  10*HZ
+#define BUS_RESET_SETTLE_TIME_FAST   5*HZ
+#define HOST_RESET_SETTLE_TIME_FAST  5*HZ

 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)
@@ -909,7 +911,10 @@
        spin_unlock_irqrestore(scmd->device->host->host_lock, flags);

        if (rtn == SUCCESS) {
-               scsi_sleep(BUS_RESET_SETTLE_TIME);
+               if (blk_noretry_request(scmd->request))
+                       scsi_sleep(BUS_RESET_SETTLE_TIME_FAST);
+               else
+                       scsi_sleep(BUS_RESET_SETTLE_TIME);
                spin_lock_irqsave(scmd->device->host->host_lock, flags);
                scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
                spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
@@ -940,7 +945,10 @@
        spin_unlock_irqrestore(scmd->device->host->host_lock, flags);

        if (rtn == SUCCESS) {
-               scsi_sleep(HOST_RESET_SETTLE_TIME);
+               if (blk_noretry_request(scmd->request))
+                       scsi_sleep(HOST_RESET_SETTLE_TIME_FAST);
+               else
+                       scsi_sleep(HOST_RESET_SETTLE_TIME);
                spin_lock_irqsave(scmd->device->host->host_lock, flags);
                scsi_report_bus_reset(scmd->device->host, scmd->device->channel);
                spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
@@ -1421,7 +1429,8 @@
                scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
                list_del_init(lh);
                if (scmd->device->online &&
-                       (++scmd->retries < scmd->allowed)) {
+                       (++scmd->retries < scmd->allowed) &&
+                       (!blk_noretry_request(scmd->request))) {
                        SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
                                                          " retry cmd: %p\n",
                                                          current->comm,
diff -urN linux-2.6.4/drivers/scsi/sd.c linux-2.6.4FF/drivers/scsi/sd.c
--- linux-2.6.4/drivers/scsi/sd.c       2004-03-18 16:12:01.000000000 +0900
+++ linux-2.6.4FF/drivers/scsi/sd.c     2004-03-18 17:14:36.000000000 +0900
@@ -67,6 +67,7 @@
  * Time out in seconds for disks and Magneto-opticals (which are slower).
  */
 #define SD_TIMEOUT             (30 * HZ)
+#define SD_TIMEOUT_FAST         (10 * HZ)
 #define SD_MOD_TIMEOUT         (75 * HZ)

 /*
@@ -178,7 +179,10 @@
        sector_t block;
        struct scsi_device *sdp = SCpnt->device;

-       timeout = SD_TIMEOUT;
+        if (blk_noretry_request(SCpnt->request))
+               timeout = SD_TIMEOUT_FAST;
+        else
+               timeout = SD_TIMEOUT;
        if (SCpnt->device->type != TYPE_DISK)
                timeout = SD_MOD_TIMEOUT;

Re: [PATCH] improvement of fastfail operation

[James Bottomley]

On Tue, 2004-03-23 at 19:38, Masao Fukuchi wrote:
> We propose the following improvements for fastfail.
> 
> 1.Validate fastfail flag for command timeout.
>   Currently fastfail flag is not valid for command timeout and repeats
>   4 times.
> 2.Set timeout value to 10sec.
>   Currently timeout value is set to 30sec.
> 3.Set wait time for bus reset/host reset to 5sec.
>   Currently wait time is set to 10sec.
>   (In many cases, abort task command fails for command timeout and it needs
>   bus reset or host reset operation)
> 
> Each timeout values come from:
>   timeout(10sec)+Abort/Bus reset(5sec+)+alt retry timeout(10sec) < 30sec
> 
> This is one idea for quick response on device/path error.
> If you have any comments or idea for this improvements, please let me know.

This isn't the right thing to do.  These timeouts control transport
recovery; if it's safe to lower them in the fastfail case, then it would
also be safe to lower them in the general case.

The correct thing for what you want to do would be to return the command
with a transient transport error (which we don't actually have yet)
*before* beginning transport recovery.  This is not going to be easy
because we need to return a command we're also going to do error
recovery on (so it can't be freed as normal).  I'd suggest the best way
to do this would be to refcount the commands.

James

Re: [PATCH] improvement of fastfail operation

[Mike Christie]

James Bottomley wrote:
> On Tue, 2004-03-23 at 19:38, Masao Fukuchi wrote:
> 
>>We propose the following improvements for fastfail.
>>
>>1.Validate fastfail flag for command timeout.
>>  Currently fastfail flag is not valid for command timeout and repeats
>>  4 times.
>>2.Set timeout value to 10sec.
>>  Currently timeout value is set to 30sec.
>>3.Set wait time for bus reset/host reset to 5sec.
>>  Currently wait time is set to 10sec.
>>  (In many cases, abort task command fails for command timeout and it needs
>>  bus reset or host reset operation)
>>
>>Each timeout values come from:
>>  timeout(10sec)+Abort/Bus reset(5sec+)+alt retry timeout(10sec) < 30sec
>>
>>This is one idea for quick response on device/path error.
>>If you have any comments or idea for this improvements, please let me know.
> 
> 
> This isn't the right thing to do.  These timeouts control transport
> recovery; if it's safe to lower them in the fastfail case, then it would
> also be safe to lower them in the general case.
> 
> The correct thing for what you want to do would be to return the command
> with a transient transport error (which we don't actually have yet)
> *before* beginning transport recovery.  This is not going to be easy
> because we need to return a command we're also going to do error
> recovery on (so it can't be freed as normal).  I'd suggest the best way
> to do this would be to refcount the commands.
> 

Could this be a place to start using the transport framework? For 
something like iSCSI the timeout value should probably have the network 
load factored into it. This could be set with a transport class 
attribute (although for scanning this would probably require per host 
values as the device ones would not yet be available), which when a 
driver registers the set/get_timeout functions it could also set a 
add_timer and times_out function. I have something like this now, but 
how it works with the mid layer error handling still has kinks.

Mike

Re: [PATCH] improvement of fastfail operation

[Masao Fukuchi]

Hi James,
Thank you for response to my mail.

I'm understand what I have to do.
I'll more study about the way to return transient transport error to
upper layer and to process transport recovery using refcount.

By the way, how about item 1.
I think this is bug and we need to validate fastfail for command timeout.
If you agree with this, please put it into official patch.
(See attached patch)

Next, I tested the fastfail recovery using Kernel 2.6.4.
But the command didn't complete forever.
Then I installed following Mike Christie's patch and it worked fine.
 http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
Please put it into official patch.

Thanks,
Masao Fukuchi

James Bottomley wrote:
>On Tue, 2004-03-23 at 19:38, Masao Fukuchi wrote:
>> We propose the following improvements for fastfail.
>> 
>> 1.Validate fastfail flag for command timeout.
>>   Currently fastfail flag is not valid for command timeout and repeats
>>   4 times.
>> 2.Set timeout value to 10sec.
>>   Currently timeout value is set to 30sec.
>> 3.Set wait time for bus reset/host reset to 5sec.
>>   Currently wait time is set to 10sec.
>>   (In many cases, abort task command fails for command timeout and it needs
>>   bus reset or host reset operation)
>> 
>> Each timeout values come from:
>>   timeout(10sec)+Abort/Bus reset(5sec+)+alt retry timeout(10sec) < 30sec
>> 
>> This is one idea for quick response on device/path error.
>> If you have any comments or idea for this improvements, please let me know.
>
>This isn't the right thing to do.  These timeouts control transport
>recovery; if it's safe to lower them in the fastfail case, then it would
>also be safe to lower them in the general case.
>
>The correct thing for what you want to do would be to return the command
>with a transient transport error (which we don't actually have yet)
>*before* beginning transport recovery.  This is not going to be easy
>because we need to return a command we're also going to do error
>recovery on (so it can't be freed as normal).  I'd suggest the best way
>to do this would be to refcount the commands.
>
>James
>

diff -urN linux-2.6.4/drivers/scsi/scsi_error.c linux-2.6.4FF/drivers/scsi/scsi_error.c
--- linux-2.6.4/drivers/scsi/scsi_error.c       2004-02-18 12:57:12.000000000 +0900
+++ linux-2.6.4FF/drivers/scsi/scsi_error.c     2004-03-18 16:59:50.000000000 +0900
@@ -1421,7 +1421,8 @@
                scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
                list_del_init(lh);
                if (scmd->device->online &&
-                       (++scmd->retries < scmd->allowed)) {
+                       (++scmd->retries < scmd->allowed) &&
+                       (!blk_noretry_request(scmd->request))) {
                        SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
                                                          " retry cmd: %p\n",
                                                          current->comm,

Re: [PATCH] improvement of fastfail operation

[James Bottomley]

On Mon, 2004-03-29 at 07:17, Masao Fukuchi wrote:
> By the way, how about item 1.
> I think this is bug and we need to validate fastfail for command timeout.
> If you agree with this, please put it into official patch.
> (See attached patch)

Well, the patch doesn't apply, but I put an equivalent in the tree.

> Next, I tested the fastfail recovery using Kernel 2.6.4.
> But the command didn't complete forever.
> Then I installed following Mike Christie's patch and it worked fine.
>  http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
> Please put it into official patch.

This patch is obviously wrong on its face.  It completes the request
without regard to any leftovers.

There's clearly a problem somewhere in the completion path for commands
that run out of retries, but whatever it is, this patch only covers it
up it doesn't solve the root problem.

James

Re: [PATCH] improvement of fastfail operation

[Mike Christie]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1991 bytes --]

On 30 Mar 2004, James Bottomley wrote:

> On Mon, 2004-03-29 at 07:17, Masao Fukuchi wrote:
> > By the way, how about item 1.
> > I think this is bug and we need to validate fastfail for command timeout.
> > If you agree with this, please put it into official patch.
> > (See attached patch)
> 
> Well, the patch doesn't apply, but I put an equivalent in the tree.
> 
> > Next, I tested the fastfail recovery using Kernel 2.6.4.
> > But the command didn't complete forever.
> > Then I installed following Mike Christie's patch and it worked fine.
> >  http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
> > Please put it into official patch.
> 
> This patch is obviously wrong on its face.  It completes the request
> without regard to any leftovers.
> 
> There's clearly a problem somewhere in the completion path for commands
> that run out of retries, but whatever it is, this patch only covers it
> up it doesn't solve the root problem.
> 

This patch
http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
is correct wrt how the block layer uses hard_nr_sectors ;)

The blkdev.h comments state that hard_nr_sectors is the number
of sectors left to be completed. This is managed by the block layer
for drivers like SCSI when end_that_request_* calls
__end_that_request_first which for partial completions calls
blk_recalc_rq_sectors. This function updates hard_nr_sectors
by the completed number of bytes, so by scsi_end_request doing this:

 if (end_that_request_chunk(req, uptodate, bytes)) {
        int leftover = (req->hard_nr_sectors << 9) - bytes;

we end up with "bytes" getting subtracted twice and the final
bio not getting completed.

I know we went through that already. It is just to update
Masao/linux-scsi.

I had sent the attached patch to Jens (cc'd him)
that modifies how blk_recalc_rq_sectors uses hard_nr_sectors,
but I think it might break some other drivers that expect the
hard_nr_sectors to be updated as the request is completed.

Mike
~

[-- Attachment #2: fix blk_recalc_rq_sectors --]
[-- Type: TEXT/PLAIN, Size: 858 bytes --]

--- linux-2.6.5-rc3-orig/drivers/block/ll_rw_blk.c	2004-03-30 19:08:16.862221536 -0800
+++ linux-2.6.5-rc3/drivers/block/ll_rw_blk.c	2004-03-30 19:09:47.624270316 -0800
@@ -2579,16 +2579,14 @@ void blk_recalc_rq_sectors(struct reques
 {
 	if (blk_fs_request(rq)) {
 		rq->hard_sector += nsect;
-		rq->hard_nr_sectors -= nsect;
 
 		/*
 		 * Move the I/O submission pointers ahead if required,
 		 * i.e. for drivers not aware of rq->cbio.
 		 */
-		if ((rq->nr_sectors >= rq->hard_nr_sectors) &&
-		    (rq->sector <= rq->hard_sector)) {
+		if (rq->sector <= rq->hard_sector) {
 			rq->sector = rq->hard_sector;
-			rq->nr_sectors = rq->hard_nr_sectors;
+			rq->nr_sectors -= nsect;
 			rq->hard_cur_sectors = bio_cur_sectors(rq->bio);
 			rq->current_nr_sectors = rq->hard_cur_sectors;
 			rq->nr_cbio_segments = bio_segments(rq->bio);

Re: [PATCH] improvement of fastfail operation

[Jens Axboe]

On Tue, Mar 30 2004, Mike Christie wrote:
> On 30 Mar 2004, James Bottomley wrote:
> 
> > On Mon, 2004-03-29 at 07:17, Masao Fukuchi wrote:
> > > By the way, how about item 1.
> > > I think this is bug and we need to validate fastfail for command timeout.
> > > If you agree with this, please put it into official patch.
> > > (See attached patch)
> > 
> > Well, the patch doesn't apply, but I put an equivalent in the tree.
> > 
> > > Next, I tested the fastfail recovery using Kernel 2.6.4.
> > > But the command didn't complete forever.
> > > Then I installed following Mike Christie's patch and it worked fine.
> > >  http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
> > > Please put it into official patch.
> > 
> > This patch is obviously wrong on its face.  It completes the request
> > without regard to any leftovers.
> > 
> > There's clearly a problem somewhere in the completion path for commands
> > that run out of retries, but whatever it is, this patch only covers it
> > up it doesn't solve the root problem.
> > 
> 
> This patch
> http://marc.theaimsgroup.com/?l=linux-scsi&m=107904932710899&w=2
> is correct wrt how the block layer uses hard_nr_sectors ;)
> 
> The blkdev.h comments state that hard_nr_sectors is the number
> of sectors left to be completed. This is managed by the block layer
> for drivers like SCSI when end_that_request_* calls
> __end_that_request_first which for partial completions calls
> blk_recalc_rq_sectors. This function updates hard_nr_sectors
> by the completed number of bytes, so by scsi_end_request doing this:
> 
>  if (end_that_request_chunk(req, uptodate, bytes)) {
>         int leftover = (req->hard_nr_sectors << 9) - bytes;
> 
> we end up with "bytes" getting subtracted twice and the final
> bio not getting completed.

Yeah, that's the buggy bit. Ater calling end_that_request_chunk(), that
should just read

	int leftover = req->hard_nr_sectors << 9;

which iirc is what your patch did, it was correct.

> I know we went through that already. It is just to update
> Masao/linux-scsi.
> 
> I had sent the attached patch to Jens (cc'd him)
> that modifies how blk_recalc_rq_sectors uses hard_nr_sectors,
> but I think it might break some other drivers that expect the
> hard_nr_sectors to be updated as the request is completed.

Yeah that doesn't work... hard_* values are really just to make sure
that the non-hard values are sane after block driver messes with the
other ones. So consider hard_* internal block stuff.

So the new patch you sent with this mail is no good. Your previous one
should be adopted ;-)

-- 
Jens Axboe

Re: [PATCH] improvement of fastfail operation

[James Bottomley]

On Wed, 2004-03-31 at 17:04, Jens Axboe wrote:
> So the new patch you sent with this mail is no good. Your previous one
> should be adopted ;-)

And the:


		if (blk_pc_request(req))
			leftover = req->data_len - bytes;

Since end_that_request chunk won't muck with that, is that OK?

James

Re: [PATCH] improvement of fastfail operation

[Jens Axboe]

On Wed, Mar 31 2004, James Bottomley wrote:
> On Wed, 2004-03-31 at 17:04, Jens Axboe wrote:
> > So the new patch you sent with this mail is no good. Your previous one
> > should be adopted ;-)
> 
> And the:
> 
> 
> 		if (blk_pc_request(req))
> 			leftover = req->data_len - bytes;
> 
> Since end_that_request chunk won't muck with that, is that OK?

It's a bit messy right now really, but it's up to the drivers to modify
that one. So for partial completions, it should be updated to be

	leftover = req->data_len;

as well.

-- 
Jens Axboe

Re: [PATCH] improvement of fastfail operation

[Mike Christie]

Jens Axboe wrote:
> On Wed, Mar 31 2004, James Bottomley wrote:
> 
>>On Wed, 2004-03-31 at 17:04, Jens Axboe wrote:
>>
>>>So the new patch you sent with this mail is no good. Your previous one
>>>should be adopted ;-)
>>
>>And the:
>>
>>
>>		if (blk_pc_request(req))
>>			leftover = req->data_len - bytes;
>>
>>Since end_that_request chunk won't muck with that, is that OK?
> 
> 
> It's a bit messy right now really, but it's up to the drivers to modify
> that one. So for partial completions, it should be updated to be
> 
> 	leftover = req->data_len;
> 
> as well.
> 

Is that a typo? If drivers are to account for it, scsi should do
	leftover = req->data_len - bytes;
?

Re: [PATCH] improvement of fastfail operation

[Jens Axboe]

On Wed, Mar 31 2004, Mike Christie wrote:
> Jens Axboe wrote:
> >On Wed, Mar 31 2004, James Bottomley wrote:
> >
> >>On Wed, 2004-03-31 at 17:04, Jens Axboe wrote:
> >>
> >>>So the new patch you sent with this mail is no good. Your previous one
> >>>should be adopted ;-)
> >>
> >>And the:
> >>
> >>
> >>		if (blk_pc_request(req))
> >>			leftover = req->data_len - bytes;
> >>
> >>Since end_that_request chunk won't muck with that, is that OK?
> >
> >
> >It's a bit messy right now really, but it's up to the drivers to modify
> >that one. So for partial completions, it should be updated to be
> >
> >	leftover = req->data_len;
> >
> >as well.
> >
> 
> Is that a typo? If drivers are to account for it, scsi should do
> 	leftover = req->data_len - bytes;
> ?

No, scsi_io_completion() should already have done it.

-- 
Jens Axboe

Re: [PATCH] improvement of fastfail operation

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

Mike Christie wrote:
> On Wed, Mar 31 2004, Mike Christie wrote:
> 
>> Jens Axboe wrote:
>> >On Wed, Mar 31 2004, James Bottomley wrote:
>> >
>> >>On Wed, 2004-03-31 at 17:04, Jens Axboe wrote:
>> >>
>> >>>So the new patch you sent with this mail is no good. Your previous one
>> >>>should be adopted ;-)
>> >>
>> >>And the:
>> >>
>> >>
>> >>        if (blk_pc_request(req))
>> >>            leftover = req->data_len - bytes;
>> >>
>> >>Since end_that_request chunk won't muck with that, is that OK?
>> >
>> >
>> >It's a bit messy right now really, but it's up to the drivers to modify
>> >that one. So for partial completions, it should be updated to be
>> >
>> >    leftover = req->data_len;
>> >
>> >as well.
>> >
>>
>> Is that a typo? If drivers are to account for it, scsi should do
>>     leftover = req->data_len - bytes;
>> ?
> 
> 
> No, scsi_io_completion() should already have done it.
> 

Oops. Thanks.


James,

The attached patch fixes the leftover calculation for both
blk pc and blk fs requests per Jens's comments. It was
built against 2.6.5-rc3.

Thanks,

Mike

[-- Attachment #2: calc_leftovers.patch --]
[-- Type: text/plain, Size: 608 bytes --]

--- linux-2.6.5-rc3/drivers/scsi/scsi_lib.c	2004-03-29 19:26:15.000000000 -0800
+++ linux-2.6.5-rc3-work/drivers/scsi/scsi_lib.c	2004-03-31 23:23:20.051292415 -0800
@@ -524,10 +524,10 @@ static struct scsi_cmnd *scsi_end_reques
 	 * to queue the remainder of them.
 	 */
 	if (end_that_request_chunk(req, uptodate, bytes)) {
-		int leftover = (req->hard_nr_sectors << 9) - bytes;
+		int leftover = req->hard_nr_sectors << 9;
 
 		if (blk_pc_request(req))
-			leftover = req->data_len - bytes;
+			leftover = req->data_len;
 
 		/* kill remainder if no retrys */
 		if (!uptodate && blk_noretry_request(req))

Re: [PATCH] improvement of fastfail operation

[James Bottomley]

On Thu, 2004-04-01 at 02:53, Mike Christie wrote:
> The attached patch fixes the leftover calculation for both
> blk pc and blk fs requests per Jens's comments. It was
> built against 2.6.5-rc3.

Actually, I already put an identical patch in the misc tree yesterday.

James