On Mon, 2009-09-14 at 12:09 -0500, Mike Christie wrote: > Vasu Dev wrote: > > On Fri, 2009-09-11 at 11:18 -0500, Mike Christie wrote: > >> On 09/04/2009 04:43 PM, Vasu Dev wrote: > >>> On Fri, 2009-09-04 at 06:47 -0700, Alex.Iannicelli@xxxxxxxxxx wrote: > >>>> It looks like you moved the ramp up functionality into the scsi layer, > >>>> but did not move the ramp up code from the lpfc driver in the > >>> Correct. > >>> > >>>> lpfc_scsi_cmd_iocb_cmpl routine (just above the code that was removed > >>>> for the ramp down in this patch) to the new lpfc_change_queue_depth > >>>> routine. I think that this new routine should handle both ramp up and > >>>> ramp down but you have it only handling the ramp down case. > >>>> > >>> I agree all FC HBA should handle both ramp down and up as per added new > >>> change_queue_depth interface by this series. I did this for libfc/fcoe > >>> and Chrirstof did this for zfcp driver but lpfc& qla2xxx got only ramp > >>> down changes from Mike, now that Mike is busy with other stuff I don't > >>> know how to complete them in this series since I don't understand lpfc > >>> and qla2xxx enough and neither I have way to test changes to these > >>> drivers. > >>> > >>> So I'm going to update this series to have just libfc and zfcp driver > >>> changes for now and lpfc and qla2xxx can be updated later by someone > >>> familiar lpfc and qla2xxx, their ramp down changes can be collect from > >>> this series post. > >>> > >> I think it is fine not to convert a driver immediately and let the > >> driver maintainer handle it. I normally like to take a stab at it to try > >> and give the driver maintainer some more info on the how I think it > >> should work. > >> > >> I think at the very least you want to make sure your code will work for > >> other drivers, so sometimes doing a pseudo patch is useful for another > >> reason. > >> > > > > I'll try to come up with a compile tested code for lpfc and you already > > did that for qla2xx ramp up today. As you said "it is fine not to > > convert a driver immediately", so I'll provide this separately and will > > try to do it earliest possible. > > > > Modified change_queue_depth interface changes by this series should be > > sufficient to later do lpfc and qla2xx changes. > > > >> For the case of lpfc and rampup I think we need a little more code. It > >> looks like lpfc will ramp down queues if it gets a reject on its port > >> (when we get a IOSTAT_LOCAL_REJECT we call lpfc_rampdown_queue_depth). > >> When it then tries to ramp up, it also takes that rampdown event into > >> account. The common code being added by Vasu, only tracks rampdown > >> events from QUEUE_FULLs. > >> > > > > The qla2xx ramps down only on QUEUE_FULL beside added zfcp and lpfc > > doing same only on QUEUE_FULL condition, but still a HBAs could call > > their own change_queue_depth function for other conditions to ramp down > > e.g. lpfc for IOSTAT_LOCAL_REJECT. > > If they do this will they have to duplicate the rampdown/up time > tracking done by the common scsi_error code? > lpfc could still use same added common ramp up and ramp down code for lpfc specific IOSTAT_LOCAL_REJECT condition. This would just require exporting added scsi_handle_queue_full() and then have lpc call this for IOSTAT_LOCAL_REJECT condition. The lpfc change_queue_depth handler could make additional checks for IOSTAT_LOCAL_REJECT condition before final qdepth adjustment which would get called by added common code. So I don't see any duplicate code in that case while also limiting IOSTAT_LOCAL_REJECT code to only lpfc. > > > > The lpfc and qla2xxx both are calling ramp up code only after specified > > time interval since last ramp down/up on a successful IO completion, so > > does the added code does the same with tunable time interval. I chose > > least 120HZ from qla2xxx as default. > > > > So added common code for ramp down/up is sufficient for majority of FC > > HBAs and lpfc specific additional criteria should be limited to only > > lpfc HBA code. > > Why should it only be limited to lpfc? Are you saying other drivers or > hw do not have something like a local reject or out of hba port > resources? Do you know this by just looking at the driver? A lot of > times lpfc will add something then other drivers (myself included) will > add it later when they have seen lpfc do it. Or are you saying you think > other drivers are going to want to handle the problem in a different way? Yes, I think adjusting can_queue will be more helpful in case of resource allocation failures instead queue_depth adjustment on all luns. In case of rport is gone, no use of queue_depth ramp down on luns of rport. Regards Vasu > -- > 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 -- 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
