Re: [PATCH 08/10] mpt3sas: lockless command submission for scsi-mq

[Christoph Hellwig <hch@lst.de>]

On Tue, Jan 31, 2017 at 10:25:58AM +0100, Hannes Reinecke wrote:
> Enable lockless command submission for scsi-mq by moving the
> command structure into the payload for struct request.

We support cmd_size for both the mq and !mq code path, so this
could be simplified by always taking your new block-mq path.

>  4 files changed, 427 insertions(+), 57 deletions(-)

The amount of new code scare me.  Especially compared to the very
shport changelog.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 942fb7e..29e139f 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -867,6 +867,20 @@ struct scsiio_tracker *
>  {
>  	WARN_ON(!smid);
>  	WARN_ON(smid >= ioc->hi_priority_smid);
> +	if (shost_use_blk_mq(ioc->shost)) {
> +		u16 hwq = (smid - 1) % ioc->shost->nr_hw_queues;
> +		u16 tag = (smid - 1) / ioc->shost->nr_hw_queues;
> +		struct blk_mq_tag_set *tagset = &ioc->shost->tag_set;
> +		struct request *req;
> +
> +		req = blk_mq_tag_to_rq(tagset->tags[hwq], tag);
> +		if (req) {
> +			struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +
> +			return scsi_cmd_priv(cmd);
> +		} else
> +			return NULL;
> +	}

This looks like it basically duplicates scsi_host_find_tag().

> +	if (shost_use_blk_mq(ioc->shost)) {
> +		unsigned int unique_tag = blk_mq_unique_tag(scmd->request);
> +		u16 hwq = blk_mq_unique_tag_to_hwq(unique_tag);
> +		u16 tag = blk_mq_unique_tag_to_tag(unique_tag);
> +		u16 smid = (tag * ioc->shost->nr_hw_queues) + hwq + 1;
> +
> +		request = scsi_cmd_priv(scmd);
> +		request->scmd = scmd;

no need for a backpointer, blk_mq_rq_from_pdu gets your
back to the request, and scsi_cmd_priv back to driver private data.

> +		request->cb_idx = cb_idx;
> +		request->msix_io = hwq;
> +		request->smid = smid;

why do we need to store the smid?

> @@ -562,12 +642,7 @@ enum block_state {
>  _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,

I think this function just needs to go away instantly.
Trying to send task management requests from userspace for any
active smid is plain dangerous.