Re: [PATCHv2 09/11] mpt3sas: lockless command submission for scsi-mq

[Christoph Hellwig <hch@lst.de>]

On Fri, Feb 17, 2017 at 09:23:08AM +0100, Hannes Reinecke wrote:
> Enable lockless command submission for scsi-mq by moving the
> command structure into the payload for struct request.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c      | 123 ++++-----
>  drivers/scsi/mpt3sas/mpt3sas_base.h      |  13 +-
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c       |  85 ++++--
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c     | 460 ++++++++++++++-----------------
>  drivers/scsi/mpt3sas/mpt3sas_warpdrive.c |  20 +-
>  5 files changed, 330 insertions(+), 371 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index dec86c4..e9470a3 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -865,9 +865,17 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
>  struct scsiio_tracker *
>  mpt3sas_get_st_from_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid)
>  {
> +	u32 unique_tag;
> +	struct scsi_cmnd *cmd;
> +
>  	WARN_ON(!smid);
>  	WARN_ON(smid >= ioc->hi_priority_smid);
> -	return &ioc->scsi_lookup[smid - 1];
> +	unique_tag = smid - 1;
> +	cmd = scsi_host_find_tag(ioc->shost, unique_tag);
> +	if (cmd)
> +		return scsi_cmd_priv(cmd);
> +
> +	return NULL;
>  }
>  
>  /**
> @@ -2343,34 +2351,23 @@ struct scsiio_tracker *
>  mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
>  	struct scsi_cmnd *scmd)
>  {
> -	unsigned long flags;
>  	struct scsiio_tracker *request;
>  	u16 smid;
>  
> -	spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
> -	if (list_empty(&ioc->free_list)) {
> -		spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
> -		pr_err(MPT3SAS_FMT "%s: smid not available\n",
> -		    ioc->name, __func__);
> -		return 0;
> -	}
> -
>  	if (!scmd) {
> -		/* ioctl command, always use the first slot */
> -		request = ioc->lookup[0];
> -		request->scmd = NULL;
> +		smid = 1;
> +		request = mpt3sas_get_st_from_smid(ioc, smid);

How is this going to work?  mpt3sas_get_st_from_smid fails if we
don't find a block layer tag derived from smid?

>  /**
>   * mpt3sas_base_free_smid - put smid back on free_list
>   * @ioc: per adapter object
> @@ -2428,22 +2441,21 @@ struct scsiio_tracker *
>  	unsigned long flags;
>  	int i;
>  
>  	if (smid < ioc->hi_priority_smid) {
> +		struct scsiio_tracker *st;
>  
> +		st = mpt3sas_get_st_from_smid(ioc, smid);
> +		if (WARN_ON(!st)) {
> +			_base_recovery_check(ioc);
> +			return;
> +		}
> +		mpt3sas_base_clear_st(ioc, st);
>  		_base_recovery_check(ioc);

		st = mpt3sas_get_st_from_smid(ioc, smid);
		if (!WARN_ON_ONCE(!st))
			mpt3sas_base_clear_st(ioc, st);
		_base_recovery_check(ioc);
		return;
		
		
>  	/* pending command count */
> -	spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
> -	for (i = 0; i < ioc->scsiio_depth; i++)
> -		if (ioc->scsi_lookup[i].cb_idx != 0xFF)
> -			ioc->pending_io_count++;
> -	spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
> +	blk_mq_tagset_busy_iter(&ioc->shost->tag_set,
> +				_count_pending, ioc);

You can't rely on blk-mq being used, and we'd really want to avoid
layering violations like this anyway.