Re: [PATCH] megaraid_sas: Enable shared tag map

[Hannes Reinecke <hare@suse.de>]

On 09/30/2014 09:43 AM, Christoph Hellwig wrote:
> On Mon, Sep 29, 2014 at 01:47:52PM +0200, Hannes Reinecke wrote:
>> Megaraid_sas uses a shared pool of commands per HBA, so we
>> should be enabling a shared tag map.
>> This will allow the I/O scheduler to make better scheduling
>> decisions and will avoid BUSY states in the driver.
> 
> What exact problem did you see?  Do you have a link to a bugzilla entry
> or similar?  Was this observed on real hardware or your qemu emulation?
> 
Well, _actually_ I just did it so that I could get the tag number
for my printk patchset :-)

But there is an underlying reason:
megaraid_sas uses an internal frame pool of a fixed size.
Once that's exhausted it cannot queue any more commands, and need to
return busy.
But as this is a per-host command pool all LUNs have to shared the
same frame pool, and the driver uses heuristics (ie 1/4 of the pool
size) to set cmd_per_lun. So if you have more than 4 LUNs you'll run
into issues as the pool can become exhausted.
Hence I thought it would be good to expose this to the block layer
so that we can avoid BUSY states in the driver.


>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/megaraid/megaraid_sas_base.c | 34 ++++++++++++++++++++++++++++++-
>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
>> index f6a69a3..996fa9a 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>> @@ -1659,7 +1659,7 @@ static int megasas_slave_configure(struct scsi_device *sdev)
>>  	*/
>>  	blk_queue_rq_timeout(sdev->request_queue,
>>  		MEGASAS_DEFAULT_CMD_TIMEOUT * HZ);
>> -
>> +	sdev->tagged_supported = 1;
> 
>> +	/* We have a shared tag map, so TCQ is always supported */
>> +	sdev->tagged_supported = 1;
>> +
> 
> Why doesn't the device return the proper data in the INQUIRY response?
> 
> And more importantly why do you want to do this twice?
> 
Bah, Typo.

>>  	instance = megasas_lookup_instance(sdev->host->host_no);
>>  	if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
>>  		/*
>> @@ -1677,13 +1681,20 @@ static int megasas_slave_alloc(struct scsi_device *sdev)
>>  			sdev->id;
>>  		if (instance->pd_list[pd_index].driveState ==
>>  					MR_PD_STATE_SYSTEM) {
>> +			scsi_activate_tcq(sdev, MEGASAS_DEFAULT_CMD_PER_LUN);
>>  			return 0;
>>  		}
>>  		return -ENXIO;
>>  	}
>> +	scsi_activate_tcq(sdev, MEGASAS_DEFAULT_CMD_PER_LUN);
>>  	return 0;
> 
> Please refactor the code so that the first case falls through to the
> second, something like:
> 
> 
> 		if (instance->pd_list[pd_index].driveState !=
> 					MR_PD_STATE_SYSTEM)
> 			return -ENXIO;
> 	}
> 
> 	scsi_activate_tcq(sdev, MEGASAS_DEFAULT_CMD_PER_LUN);
> 	return 0;
> 
Okay, will be doing it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html