Sorry I had missed those comments. Will address those and resubmit. Regards, Sumant -----Original Message----- From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxx] Sent: Thursday, May 10, 2007 12:24 PM To: Patro, Sumant Cc: akpm@xxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Kolli, Neela; Yang, Bo Subject: Re: [PATCH] scsi: megaraid_sas - intercepts cmd timeout andthrottle io On Thu, 2007-05-10 at 07:01 -0700, Sumant Patro wrote: > eh_timed_out call back (megasas_reset_timer) is used to throttle io to > the adapter when it is called the first time for a scmd. > The MEGASAS_FW_BUSY flag is set and can_queue reduced to 16. The > can_queue is restored from completion routine in following two > conditions : 5 seconds has elapsed and the # of outstanding cmds in FW is < 17. > Also removed reserved fields from struct megasas_instance. And the rest of the comments? This is the cut out of all the ones I made, I think: > > +static enum > > +scsi_eh_timer_return megasas_reset_timer(struct scsi_cmnd *scmd) { > > + struct megasas_cmd *cmd = (struct megasas_cmd *)scmd->SCp.ptr; > > + struct megasas_instance *instance; > > + unsigned long flags; > > + > > + if (cmd) { > > I don't believe we can ever get a command timeout with no command, can > we? > > > + if (time_after(jiffies, scmd->jiffies_at_alloc + 170 * > HZ)) > > + return EH_NOT_HANDLED; > > This 170s is a bit arbitrary ... surely you want it to be related to a > multiple of scmd->timeout_per_command? > > + if (!(instance->flag & MEGASAS_FW_BUSY)) { > > + /* FW is busy, throttle IO */ > > + spin_lock_irqsave(&instance->throttle_io_lock, > flags); > > + > > + instance->host->can_queue = 16; > > can_queue is protected by the host lock ... I think you need to dump > the throttle_io_lock and simply use the host lock for all of this. > > - if (cmd->scmd) { > > + if (cmd->scmd) > > cmd->scmd->SCp.ptr = (char *)0; > > That's NULL, ordinarily ... James - 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
