Re: [PATCH 04/10] drivers: convert fc drivers calling scsi_track_queue_full

[Mike Christie <michaelc@cs.wisc.edu>]

On 09/04/2009 04:43 PM, Vasu Dev wrote:
> On Fri, 2009-09-04 at 06:47 -0700, Alex.Iannicelli@Emulex.Com 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.

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.

You probably want to make a common fc class function which loops over 
vports and will ramp down queues. The fc LLD can then call this and that 
common fc function can make sure the common rampdown tracking is done so 
later on the ramp up code can take that into account.

Reviewing lpfc also brings up the question of why it was not doing the 
ramp up in the main IO path. It looks like it is done in a worker 
thread. The ramp down for a QUEUE_FULL is probably done in the main IO 
path because we are already getting errors and we want to prevent new 
ones. But for the ramp up, did Emulex experience performance hits when 
ramping up in the main IO path?