Re: [PATCH 6/7] mpt2sas: store scsi io tracker data in the scsi command / request

[Jens Axboe <axboe@kernel.dk>]

On 04/05/2015 10:03 AM, Christoph Hellwig wrote:
> On Fri, Apr 03, 2015 at 09:58:22AM -0600, Jens Axboe wrote:
>> +struct scsiio_tracker *
>> +mpt2sas_get_st_from_smid(struct MPT2SAS_ADAPTER *ioc, u16 smid)
>> +{
>> +	if (shost_use_blk_mq(ioc->shost)) {
>> +		struct scsi_cmnd *scmd;
>> +
>> +		scmd = scsi_mq_find_tag(ioc->shost, smid - 1);
>> +		if (!scmd)
>> +			return NULL;
>> +		return scsi_mq_scmd_to_pdu(scmd);
>> +	} else
>> +		return &ioc->scsi_lookup[smid - 1];
>> +}
>
> The mq case will also work for the !mq case when you call
> scsi_host_find_tag and scsi_cmd_priv.   In general all the mq-specific
> codepathes you add should become the default and only one, even if this
> requires a lit bit of additional core work.

For the core code, I definitely agree. But for this case, in scsi-mq 
mode, we know that tag == smid - 1. That's not the case if we are not 
using scsi-mq.

In general, it'd be great if we could "convert" drivers and not have to 
support both scsi-mq and legacy mode. Then I could just rip the old code.

>> @@ -1724,6 +1739,18 @@ mpt2sas_base_get_smid_scsiio(struct MPT2SAS_ADAPTER *ioc, u8 cb_idx,
>>   	struct scsiio_tracker *request;
>>   	u16 smid;
>>
>> +	if (shost_use_blk_mq(ioc->shost)) {
>> +		/*
>> +		 * If we don't have a SCSI command associated with this smid,
>> +		 * bump it to high-prio
>> +		 */
>> +		if (!scmd)
>> +			return mpt2sas_base_get_smid_hpr(ioc, cb_idx);
>
> Seems like _ctl_do_mpt_command should be changed to just
> call mpt2sas_base_get_smid_hpr unconditionally instead of adding this
> hack  Preferably as a standalone preparatory patch.

Sounds reasonable, I'll do that.

>>   	unsigned long flags;
>>   	int i;
>> -	struct chain_tracker *chain_req, *next;
>> +
>> +	if (shost_use_blk_mq(ioc->shost) && smid < ioc->hi_priority_smid) {
>> +		struct scsiio_tracker *st;
>> +
>> +		st = mpt2sas_get_st_from_smid(ioc, smid);
>> +		if (!st)
>> +			return;
>> +
>> +		st->direct_io = 0;
>> +
>> +		if (!list_empty(&st->chain_list)) {
>> +			spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
>> +			_dechain_st(ioc, st);
>> +			spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
>> +		}
>
> This whole chain list thing looks bonkers to me.  We always allocated
> a fixed multiple of the queue depth in ->chain_lookup, but then do this
> required list manipulation at least once per I/O submission and completion.

It is completely crazy, and very suboptimal. The only thing that "saves" 
it is that we only need to do it multiple times for larger IOs, where a 
larger per-IO hit can be accepted. But yes, it really should just die a 
horrible death.

> Seems like we should instead add an array of (cpu address, dma address)
> tuples to the scsiio_tracker and avoid all the chain_lookup / chain_list
> lookups entirely.

Agree.

>> +			if (shost_use_blk_mq(ioc->shost)) {
>> +				scmd = scsi_mq_find_tag(ioc->shost,  i);
>> +				if (scsi_mq_scmd_started(scmd))
>> +					pending++;
>
> Ok, I guess we should move the request_started check into the _find_tag
> helpers, as tags that aren't started aren't something that driver
> should ever lookup.

I'll move it in there.

>> +static bool
>> +_scmd_match(struct scsi_cmnd *scmd, u16 handle, u32 lun)
>> +{
>> +	struct MPT2SAS_DEVICE *priv_data;
>> +
>> +	if (scmd == NULL || scmd->device == NULL ||
>> +	    scmd->device->hostdata == NULL)
>> +		return false;
>
> If the queue is started this can't ever happen.
>
>> +	if (lun != scmd->device->lun)
>> +		return false;
>
> If you pass in a specific scsi_device and thus request_queue  this
> can't happen.
>
>> +static u16
>> +_ctl_find_smid(struct MPT2SAS_ADAPTER *ioc, u16 handle, u32 lun)
>> +{
>> +	if (shost_use_blk_mq(ioc->shost))
>> +		return _ctl_find_smid_mq(ioc, handle, lun);
>> +	else
>> +		return _ctl_find_smid_legacy(ioc, handle, lun);
>> +}
>
> The caller of this looks entirely broken.  It's a driver specific API
> to submit task management commands, duplicating the mid level code,
> and it doesn't even allow which task to target.  I think we should
> just return a error when invoking MPI2_FUNCTION_SCSI_TASK_MGMT instead
> of digging us an even deeper grave here.  If someone complains we'll
> have to find a way to redirect it to the generic EH ioctls.

Sounds fine to me, will make my life a lot easier and we can kill this 
horrible lookup mess.

-- 
Jens Axboe