Checks added in megasas_queue_command to know if FW is able to process
commands within timeout period. If number of retries is 2 or greater,
the driver stops sending cmd to FW. IO is resumed if pending cmd count
reduces to 16 or 5 seconds has elapsed from the time cmds were last sent
to FW.
Signed-off-by: Sumant Patro <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas.c | 27 +++++++++++++++++++++++++
drivers/scsi/megaraid/megaraid_sas.h | 3 ++
2 files changed, 30 insertions(+)
diff -uprN 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c
--- 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06 08:43:40.000000000 -0800
+++ 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06 08:50:40.000000000 -0800
@@ -839,6 +839,7 @@ megasas_queue_command(struct scsi_cmnd *
u32 frame_count;
struct megasas_cmd *cmd;
struct megasas_instance *instance;
+ unsigned long sec;
instance = (struct megasas_instance *)
scmd->device->host->hostdata;
@@ -856,6 +857,23 @@ megasas_queue_command(struct scsi_cmnd *
goto out_done;
}
+ /* Check if we can process cmds */
+ if(instance->is_busy){
+ sec = (jiffies - instance->last_time) / HZ;
+ if(sec<5)
+ return SCSI_MLQUEUE_HOST_BUSY;
+ else{
+ instance->is_busy=0;
+ instance->last_time=0;
+ }
+ }
+
+ if(scmd->retries>1){
+ /* FW is busy */
+ instance->is_busy=1;
+ instance->last_time=jiffies;
+ }
+
cmd = megasas_get_cmd(instance);
if (!cmd)
return SCSI_MLQUEUE_HOST_BUSY;
@@ -1200,6 +1218,7 @@ megasas_complete_cmd(struct megasas_inst
{
int exception = 0;
struct megasas_header *hdr = &cmd->frame->hdr;
+ int outstanding;
if (cmd->scmd) {
cmd->scmd->SCp.ptr = (char *)0;
@@ -1313,6 +1332,12 @@ megasas_complete_cmd(struct megasas_inst
hdr->cmd);
break;
}
+
+ if(instance->is_busy){
+ outstanding = atomic_read(&instance->fw_outstanding);
+ if(outstanding<17)
+ instance->is_busy=0;
+ }
}
/**
@@ -2384,6 +2409,8 @@ megasas_probe_one(struct pci_dev *pdev,
instance->init_id = MEGASAS_DEFAULT_INIT_ID;
megasas_dbg_lvl = 0;
+ instance->is_busy = 0;
+ instance->last_time = 0;
/*
* Initialize MFI Firmware
diff -uprN 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.h 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.h
--- 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.h 2007-02-06 06:56:27.000000000 -0800
+++ 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.h 2007-02-06 08:51:23.000000000 -0800
@@ -1102,6 +1102,9 @@ struct megasas_instance {
atomic_t fw_outstanding;
u32 hw_crit_error;
+ u8 is_busy;
+ unsigned long last_time;
+
struct megasas_instance_template *instancet;
struct tasklet_struct isr_tasklet;
};
On Tue, 2007-02-06 at 14:11 -0800, Sumant Patro wrote:
> Checks added in megasas_queue_command to know if FW is able to process
> commands within timeout period. If number of retries is 2 or greater,
> the driver stops sending cmd to FW. IO is resumed if pending cmd count
> reduces to 16 or 5 seconds has elapsed from the time cmds were last sent
> to FW.
>
> Signed-off-by: Sumant Patro <[email protected]>
> ---
>
> drivers/scsi/megaraid/megaraid_sas.c | 27 +++++++++++++++++++++++++
> drivers/scsi/megaraid/megaraid_sas.h | 3 ++
> 2 files changed, 30 insertions(+)
>
> diff -uprN 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c
> --- 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06 08:43:40.000000000 -0800
> +++ 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06 08:50:40.000000000 -0800
> @@ -839,6 +839,7 @@ megasas_queue_command(struct scsi_cmnd *
> u32 frame_count;
> struct megasas_cmd *cmd;
> struct megasas_instance *instance;
> + unsigned long sec;
>
> instance = (struct megasas_instance *)
> scmd->device->host->hostdata;
> @@ -856,6 +857,23 @@ megasas_queue_command(struct scsi_cmnd *
> goto out_done;
> }
>
> + /* Check if we can process cmds */
> + if(instance->is_busy){
^ ^
space needed per linux coding style (and the rest of the file
> + sec = (jiffies - instance->last_time) / HZ;
please don't do this. You want to be using time_before() and
jiffies_to_msecs(). The space problems apply to the rest of the code
> + if(sec<5)
> + return SCSI_MLQUEUE_HOST_BUSY;
> + else{
> + instance->is_busy=0;
> + instance->last_time=0;
> + }
> + }
> +
> + if(scmd->retries>1){
I really don't think this is a good indicator of your firmware
necessarily having problems; I really think you might want to look at
other indicators ... jiffies_at_alloc might be better, or even
throttling from the abort handler, which must have been called before
you get to here if the command is actually timing out.
Timeout and abort has it's own throttle anyway, since we quiesce the
host before beginning error recovery ... are you sure this scheme
actually solves anything for your device?
James
Hello James,
I re-submitted the patch yesterday with the "space" issue fixed
(adhering to coding guideline).
I will check for alternative to calculate the time driver have
been sending host busy to OS. Will check with time_before() as you have
suggested.
Throttling from megasas_generic_reset() handler did not help.
megaraid does not have feature to abort cmds. So, in the generic reset
routine, the driver just waits for cmd completion by FW. These timed-out
cmds gets retried by mid-layer with "retries" incremented by 1.
Eventually we see retries equals max_allowed followed by SCSI error with
"DRIVER_TIMEOUT".
By throttling from the megasas_queue_command we do not hit the
issue. In our test with this code, retries did not exceed 2.
Regards,
Sumant
-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Thursday, February 15, 2007 4:11 PM
To: Patro, Sumant
Cc: [email protected]; [email protected];
[email protected]; Kolli, Neela; Yang, Bo; Patro, Sumant
Subject: Re: [PATCH 3/5] scsi: megaraid_sas - throttle io if FW is busy
On Tue, 2007-02-06 at 14:11 -0800, Sumant Patro wrote:
> Checks added in megasas_queue_command to know if FW is able to process
> commands within timeout period. If number of retries is 2 or greater,
> the driver stops sending cmd to FW. IO is resumed if pending cmd count
> reduces to 16 or 5 seconds has elapsed from the time cmds were last
> sent to FW.
>
> Signed-off-by: Sumant Patro <[email protected]>
> ---
>
> drivers/scsi/megaraid/megaraid_sas.c | 27 +++++++++++++++++++++++++
> drivers/scsi/megaraid/megaraid_sas.h | 3 ++
> 2 files changed, 30 insertions(+)
>
> diff -uprN 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c
2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c
> --- 2.6.new-p2/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06
08:43:40.000000000 -0800
> +++ 2.6.new-p3/drivers/scsi/megaraid/megaraid_sas.c 2007-02-06
08:50:40.000000000 -0800
> @@ -839,6 +839,7 @@ megasas_queue_command(struct scsi_cmnd *
> u32 frame_count;
> struct megasas_cmd *cmd;
> struct megasas_instance *instance;
> + unsigned long sec;
>
> instance = (struct megasas_instance *)
> scmd->device->host->hostdata;
> @@ -856,6 +857,23 @@ megasas_queue_command(struct scsi_cmnd *
> goto out_done;
> }
>
> + /* Check if we can process cmds */
> + if(instance->is_busy){
^ ^
space needed per linux coding style (and the rest of the file
> + sec = (jiffies - instance->last_time) / HZ;
please don't do this. You want to be using time_before() and
jiffies_to_msecs(). The space problems apply to the rest of the code
> + if(sec<5)
> + return SCSI_MLQUEUE_HOST_BUSY;
> + else{
> + instance->is_busy=0;
> + instance->last_time=0;
> + }
> + }
> +
> + if(scmd->retries>1){
I really don't think this is a good indicator of your firmware
necessarily having problems; I really think you might want to look at
other indicators ... jiffies_at_alloc might be better, or even
throttling from the abort handler, which must have been called before
you get to here if the command is actually timing out.
Timeout and abort has it's own throttle anyway, since we quiesce the
host before beginning error recovery ... are you sure this scheme
actually solves anything for your device?
James
On Thu, 2007-02-15 at 19:53 -0700, Patro, Sumant wrote:
> Hello James,
>
> I re-submitted the patch yesterday with the "space" issue fixed
> (adhering to coding guideline).
>
> I will check for alternative to calculate the time driver have
> been sending host busy to OS. Will check with time_before() as you have
> suggested.
>
> Throttling from megasas_generic_reset() handler did not help.
> megaraid does not have feature to abort cmds. So, in the generic reset
> routine, the driver just waits for cmd completion by FW. These timed-out
> cmds gets retried by mid-layer with "retries" incremented by 1.
> Eventually we see retries equals max_allowed followed by SCSI error with
> "DRIVER_TIMEOUT".
That's rather what worries me. When the error handler activates (which
it will on the first timeout), it waits for all commands to complete or
time out before running. Your reset handler does nothing other than
wait for the firmware to complete the commands (now uselessly), so we
now wait for the entire firmware command queue to drain, then you tell
the mid layer everything is OK, so it loads you up again with all the
commands plus a few test unit readies for good measure, then you
throttle.
You really want to catch the device going into this condition and do
something at that point ... prime candidate would be lowering the
can_queue depth to get fewer commands transiting the firmware.
James
Hello James,
It is difficult to know when the device will go into a state
where it is not able to process cmds within timeout period consistently.
Many factors may be contributing for the device to get into this state.
Reducing can_queue may help but the difficult part is when does
the can_queue value be restored to the original value. Because, the
device may come back to normal functioning after some time. Also
reducing it to a optimum value that will work for all possible cases is
a challenge.
In a typical setup the throttle io code may not get executed. It
is required only when it has not been able to complete cmds more than 2
times within timeout period.
Regards,
Sumant
-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Friday, February 16, 2007 9:51 AM
To: Patro, Sumant
Cc: [email protected]; [email protected];
[email protected]; Kolli, Neela; Yang, Bo
Subject: RE: [PATCH 3/5] scsi: megaraid_sas - throttle io if FW is busy
On Thu, 2007-02-15 at 19:53 -0700, Patro, Sumant wrote:
> Hello James,
>
> I re-submitted the patch yesterday with the "space" issue fixed
> (adhering to coding guideline).
>
> I will check for alternative to calculate the time driver have
been
> sending host busy to OS. Will check with time_before() as you have
> suggested.
>
> Throttling from megasas_generic_reset() handler did not help.
> megaraid does not have feature to abort cmds. So, in the generic reset
> routine, the driver just waits for cmd completion by FW. These
> timed-out cmds gets retried by mid-layer with "retries" incremented by
1.
> Eventually we see retries equals max_allowed followed by SCSI error
> with "DRIVER_TIMEOUT".
That's rather what worries me. When the error handler activates (which
it will on the first timeout), it waits for all commands to complete or
time out before running. Your reset handler does nothing other than
wait for the firmware to complete the commands (now uselessly), so we
now wait for the entire firmware command queue to drain, then you tell
the mid layer everything is OK, so it loads you up again with all the
commands plus a few test unit readies for good measure, then you
throttle.
You really want to catch the device going into this condition and do
something at that point ... prime candidate would be lowering the
can_queue depth to get fewer commands transiting the firmware.
James