Re: [PATCH, RFC] scsi: use host wide tags by default

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Fri, 2015-04-17 at 22:11 +0200, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
> index 82abfce..941a424 100644
> --- a/drivers/scsi/53c700.c
> +++ b/drivers/scsi/53c700.c
> @@ -325,7 +325,6 @@ NCR_700_detect(struct scsi_host_template *tpnt,
>  	tpnt->slave_destroy = NCR_700_slave_destroy;
>  	tpnt->slave_alloc = NCR_700_slave_alloc;
>  	tpnt->change_queue_depth = NCR_700_change_queue_depth;
> -	tpnt->use_blk_tags = 1;
>  
>  	if(tpnt->name == NULL)
>  		tpnt->name = "53c700";
> @@ -1107,7 +1106,9 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp,
>  			BUG();
>  		}
>  		if(hostdata->msgin[1] == A_SIMPLE_TAG_MSG) {
> -			struct scsi_cmnd *SCp = scsi_find_tag(SDp, hostdata->msgin[2]);
> +			struct scsi_cmnd *SCp;
> +
> +			SCp = scsi_find_tag(SDp->host, hostdata->msgin[2]);

This (and the following) is a stylistic modification and doesn't really
belong here.

>  			if(unlikely(SCp == NULL)) {
>  				printk(KERN_ERR "scsi%d: (%d:%d) no saved request for tag %d\n", 
>  				       host->host_no, reselection_id, lun, hostdata->msgin[2]);
> @@ -1119,7 +1120,9 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp,
>  				"reselection is tag %d, slot %p(%d)\n",
>  				hostdata->msgin[2], slot, slot->tag);
>  		} else {
> -			struct scsi_cmnd *SCp = scsi_find_tag(SDp, SCSI_NO_TAG);
> +			struct scsi_cmnd *SCp;
> +
> +			SCp = scsi_find_tag(SDp->host, SCSI_NO_TAG);
>  			if(unlikely(SCp == NULL)) {
>  				sdev_printk(KERN_ERR, SDp,
>  					"no saved request for untagged cmd\n");
[...]
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3833bf5..72a72f9 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -662,32 +662,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>   */
>  int scsi_change_queue_depth(struct scsi_device *sdev, int depth)
>  {
> -	unsigned long flags;
> -
> -	if (depth <= 0)
> -		goto out;
> -
> -	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
> +	if (depth > 0) {
> +		unsigned long flags;
>  
> -	/*
> -	 * Check to see if the queue is managed by the block layer.
> -	 * If it is, and we fail to adjust the depth, exit.
> -	 *
> -	 * Do not resize the tag map if it is a host wide share bqt,
> -	 * because the size should be the hosts's can_queue. If there
> -	 * is more IO than the LLD's can_queue (so there are not enuogh
> -	 * tags) request_fn's host queue ready check will handle it.
> -	 */
> -	if (!shost_use_blk_mq(sdev->host) && !sdev->host->bqt) {
> -		if (blk_queue_tagged(sdev->request_queue) &&
> -		    blk_queue_resize_tags(sdev->request_queue, depth) != 0)
> -			goto out_unlock;
> +		spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
> +		sdev->queue_depth = depth;
> +		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);

This lock/unlock is a nasty global sync point which can be eliminated:
we can rely on the architectural atomicity of 32 bit writes (might need
to make sdev->queue_depth a u32 because I seem to remember 16 bit writes
had to be done as two byte stores on some architectures).

James