command queueing cmd_per_lun, queue_depth and tags

command queueing cmd_per_lun, queue_depth and tags

[Patrick Mansfield]

Currently cmd_per_lun is the default queue depth for both tagged and
untagged queueing. That is, if the LLDD does not modify queue_depth in its
slave_configure, queue_depth will be set to cmd_per_lun, no matter what
the command queueing/tag support. 

If we don't allow queueing in the LLDD, and cmd_per_lun is supposed to be
the default depth for untagged support, shouldn't it always be 1, and
hence go away? 

Similarily, why do some LLDD's use a cmd_per_lun of 3, or (like
ncr53c8xx_slave_configure) set queue_depth for !tagged_supported to
anything other than 1?

I know (at least) FCP can always do simple queueing, but I don't think
scsi core should allow multiple commands to be sent if the device does not
have CMDQUE (or BQUE).

It seems more sensible to have cmd_per_lun be more like it was in 2.4,
where it was (generally) the default queue_depth for all devices on an
initiator (all devices on a scsi_host).

Also we don't even check the BQUE in the INQUIRY result (byte 6, bit 7).
Should this be changed? That is set tagged_supported if BQUE is set, like
we do for CMDQUE (byte 7 bit 1). And also set simple_tags if
tagged_supported is set.

That is something like this patch to always set queue_depth to 1 by
default, then later if BQUE or CMDQUE are set, set queue_depth to
cmd_per_lun. Of course slave_configure can still override this, but this
change means queue_depth is 1 if you call scsi_deactivate_tcq().

Using this patch might also require changes in some LLDD's, like ipr.

diff -uprN -X /home/patman/dontdiff /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c linux-2.6.17-rc1/drivers/scsi/scsi.c
--- /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c	2006-04-03 11:35:58.000000000 -0700
+++ linux-2.6.17-rc1/drivers/scsi/scsi.c	2006-04-07 10:00:15.000000000 -0700
@@ -866,9 +866,7 @@ EXPORT_SYMBOL(scsi_finish_command);
  * Arguments:	sdev	- SCSI Device in question
  * 		tagged	- Do we use tagged queueing (non-0) or do we treat
  * 			  this device as an untagged device (0)
- * 		tags	- Number of tags allowed if tagged queueing enabled,
- * 			  or number of commands the low level driver can
- * 			  queue up in non-tagged mode (as per cmd_per_lun).
+ * 		tags	- Number of tags allowed if tagged queueing enabled
  *
  * Returns:	Nothing
  *
@@ -883,6 +881,8 @@ void scsi_adjust_queue_depth(struct scsi
 {
 	unsigned long flags;
 
+	if (!tagged)
+		tags = 1;
 	/*
 	 * refuse to set tagged depth to an unworkable size
 	 */
@@ -913,7 +913,6 @@ void scsi_adjust_queue_depth(struct scsi
 				    "disabled\n");
 		case 0:
 			sdev->ordered_tags = sdev->simple_tags = 0;
-			sdev->queue_depth = tags;
 			break;
 	}
  out:
@@ -935,8 +934,7 @@ EXPORT_SYMBOL(scsi_adjust_queue_depth);
  *
  * Returns:	0 - No change needed
  * 		>0 - Adjust queue depth to this new depth
- * 		-1 - Drop back to untagged operation using host->cmd_per_lun
- * 			as the untagged command depth
+ * 		-1 - Drop back to untagged operation
  *
  * Lock Status:	None held on entry
  *
@@ -960,7 +958,7 @@ int scsi_track_queue_full(struct scsi_de
 		return 0;
 	if (sdev->last_queue_full_depth < 8) {
 		/* Drop back to untagged */
-		scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
+		scsi_adjust_queue_depth(sdev, 0, 1);
 		return -1;
 	}
 	
diff -uprN -X /home/patman/dontdiff /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c linux-2.6.17-rc1/drivers/scsi/scsi_scan.c
--- /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c	2006-04-03 11:35:58.000000000 -0700
+++ linux-2.6.17-rc1/drivers/scsi/scsi_scan.c	2006-04-07 09:47:47.000000000 -0700
@@ -256,7 +256,7 @@ static struct scsi_device *scsi_alloc_sd
 	}
 
 	sdev->request_queue->queuedata = sdev;
-	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
+	scsi_adjust_queue_depth(sdev, 0, 1);
 
 	scsi_sysfs_device_initialize(sdev);
 
@@ -719,9 +719,12 @@ static int scsi_add_lun(struct scsi_devi
 	 * End sysfs code.
 	 */
 
-	if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
-	    !(*bflags & BLIST_NOTQ))
+	if ((sdev->scsi_level >= SCSI_2) && ((inq_result[6] & 0x10) ||
+	     (inq_result[7] & 2)) && !(*bflags & BLIST_NOTQ)) {
 		sdev->tagged_supported = 1;
+		scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG,
+					sdev->host->cmd_per_lun);
+	}
 	/*
 	 * Some devices (Texel CD ROM drives) have handshaking problems
 	 * when used with the Seagate controllers. borken is initialized

-- Patrick Mansfield

Re: command queueing cmd_per_lun, queue_depth and tags

[Brian King]

Patrick Mansfield wrote:
> Currently cmd_per_lun is the default queue depth for both tagged and
> untagged queueing. That is, if the LLDD does not modify queue_depth in its
> slave_configure, queue_depth will be set to cmd_per_lun, no matter what
> the command queueing/tag support. 
> 
> If we don't allow queueing in the LLDD, and cmd_per_lun is supposed to be
> the default depth for untagged support, shouldn't it always be 1, and
> hence go away? 

This seems to assume there are no SCSI devices which do command queuing, but do
not support queue tags. This is not the case.

> Similarily, why do some LLDD's use a cmd_per_lun of 3, or (like
> ncr53c8xx_slave_configure) set queue_depth for !tagged_supported to
> anything other than 1?

In the case of ipr, there are two scenarios. The first is for JBOD disks.
I use a default queue depth of 6 in ipr. When running untagged, with a queue depth
of > 1, the ipr adapter firmware then maintains a queue, guaranteeing only
one command will be outstanding to the device at once. This lower level
queue allows for a faster turnaround of commands and improved performance.
The second case is that of RAID arrays. These show up as logical scsi disks.
They support command queueing, but not tagged command queueing.

> I know (at least) FCP can always do simple queueing, but I don't think
> scsi core should allow multiple commands to be sent if the device does not
> have CMDQUE (or BQUE).

This will break both of the working scenarios I described above for ipr
and performance will suffer significantly. The inquiry data for ipr RAID
arrays does not set either CMDQUE or BQUE.


> Also we don't even check the BQUE in the INQUIRY result (byte 6, bit 7).
> Should this be changed? That is set tagged_supported if BQUE is set, like
> we do for CMDQUE (byte 7 bit 1). And also set simple_tags if
> tagged_supported is set.

I don't like the idea of always enabling TCQ in scsi core simply if
the device supports it. What if the HBA does not support it? To make
matters more interesting, what if the HBA does not support TCQ, but does
support untagged command queueing, similar to ipr as described above?
Additionally, ipr is currently relying on the fact that TCQ does not
get enabled by default. It lets userspace code enable it after verifying
the mode page settings of the drive for things like qerr, which the adapter
firmware has dependencies on.

Brian

> That is something like this patch to always set queue_depth to 1 by
> default, then later if BQUE or CMDQUE are set, set queue_depth to
> cmd_per_lun. Of course slave_configure can still override this, but this
> change means queue_depth is 1 if you call scsi_deactivate_tcq().
> 
> Using this patch might also require changes in some LLDD's, like ipr.
> 
> diff -uprN -X /home/patman/dontdiff /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c linux-2.6.17-rc1/drivers/scsi/scsi.c
> --- /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c	2006-04-03 11:35:58.000000000 -0700
> +++ linux-2.6.17-rc1/drivers/scsi/scsi.c	2006-04-07 10:00:15.000000000 -0700
> @@ -866,9 +866,7 @@ EXPORT_SYMBOL(scsi_finish_command);
>   * Arguments:	sdev	- SCSI Device in question
>   * 		tagged	- Do we use tagged queueing (non-0) or do we treat
>   * 			  this device as an untagged device (0)
> - * 		tags	- Number of tags allowed if tagged queueing enabled,
> - * 			  or number of commands the low level driver can
> - * 			  queue up in non-tagged mode (as per cmd_per_lun).
> + * 		tags	- Number of tags allowed if tagged queueing enabled
>   *
>   * Returns:	Nothing
>   *
> @@ -883,6 +881,8 @@ void scsi_adjust_queue_depth(struct scsi
>  {
>  	unsigned long flags;
>  
> +	if (!tagged)
> +		tags = 1;
>  	/*
>  	 * refuse to set tagged depth to an unworkable size
>  	 */
> @@ -913,7 +913,6 @@ void scsi_adjust_queue_depth(struct scsi
>  				    "disabled\n");
>  		case 0:
>  			sdev->ordered_tags = sdev->simple_tags = 0;
> -			sdev->queue_depth = tags;
>  			break;
>  	}
>   out:
> @@ -935,8 +934,7 @@ EXPORT_SYMBOL(scsi_adjust_queue_depth);
>   *
>   * Returns:	0 - No change needed
>   * 		>0 - Adjust queue depth to this new depth
> - * 		-1 - Drop back to untagged operation using host->cmd_per_lun
> - * 			as the untagged command depth
> + * 		-1 - Drop back to untagged operation
>   *
>   * Lock Status:	None held on entry
>   *
> @@ -960,7 +958,7 @@ int scsi_track_queue_full(struct scsi_de
>  		return 0;
>  	if (sdev->last_queue_full_depth < 8) {
>  		/* Drop back to untagged */
> -		scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
> +		scsi_adjust_queue_depth(sdev, 0, 1);
>  		return -1;
>  	}
>  	
> diff -uprN -X /home/patman/dontdiff /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c linux-2.6.17-rc1/drivers/scsi/scsi_scan.c
> --- /home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c	2006-04-03 11:35:58.000000000 -0700
> +++ linux-2.6.17-rc1/drivers/scsi/scsi_scan.c	2006-04-07 09:47:47.000000000 -0700
> @@ -256,7 +256,7 @@ static struct scsi_device *scsi_alloc_sd
>  	}
>  
>  	sdev->request_queue->queuedata = sdev;
> -	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
> +	scsi_adjust_queue_depth(sdev, 0, 1);
>  
>  	scsi_sysfs_device_initialize(sdev);
>  
> @@ -719,9 +719,12 @@ static int scsi_add_lun(struct scsi_devi
>  	 * End sysfs code.
>  	 */
>  
> -	if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
> -	    !(*bflags & BLIST_NOTQ))
> +	if ((sdev->scsi_level >= SCSI_2) && ((inq_result[6] & 0x10) ||
> +	     (inq_result[7] & 2)) && !(*bflags & BLIST_NOTQ)) {
>  		sdev->tagged_supported = 1;
> +		scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG,
> +					sdev->host->cmd_per_lun);
> +	}
>  	/*
>  	 * Some devices (Texel CD ROM drives) have handshaking problems
>  	 * when used with the Seagate controllers. borken is initialized
> 
> -- Patrick Mansfield
> -
> 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


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: command queueing cmd_per_lun, queue_depth and tags

[Patrick Mansfield]

On Fri, Apr 07, 2006 at 05:29:28PM -0500, Brian King wrote:
> Patrick Mansfield wrote:
> > Currently cmd_per_lun is the default queue depth for both tagged and
> > untagged queueing. That is, if the LLDD does not modify queue_depth in its
> > slave_configure, queue_depth will be set to cmd_per_lun, no matter what
> > the command queueing/tag support. 
> > 
> > If we don't allow queueing in the LLDD, and cmd_per_lun is supposed to be
> > the default depth for untagged support, shouldn't it always be 1, and
> > hence go away? 
> 
> This seems to assume there are no SCSI devices which do command queuing, but do
> not support queue tags. This is not the case.

I should not have used "untagged", that is misleading and a problem with
current scsi core, where we reference tcq, tags, and don't seem to mention
task attributes.

But LLDD can override anything in slave_configure.

Also, it looks like we could safely use cmd_per_lun as the default
queue_depth, rather than setting it to 1 as done in my previous
post/patch.

> > Similarily, why do some LLDD's use a cmd_per_lun of 3, or (like
> > ncr53c8xx_slave_configure) set queue_depth for !tagged_supported to
> > anything other than 1?
> 
> In the case of ipr, there are two scenarios. The first is for JBOD disks.
> I use a default queue depth of 6 in ipr. When running untagged, with a queue depth
> of > 1, the ipr adapter firmware then maintains a queue, guaranteeing only
> one command will be outstanding to the device at once. This lower level
> queue allows for a faster turnaround of commands and improved performance.
> The second case is that of RAID arrays. These show up as logical scsi disks.
> They support command queueing, but not tagged command queueing.

So you just want the task attibutes, and don't care about tag management
(i.e. you don't ever use cmd->request->tags)? That is similar to FCP.

> > I know (at least) FCP can always do simple queueing, but I don't think
> > scsi core should allow multiple commands to be sent if the device does not
> > have CMDQUE (or BQUE).
> 
> This will break both of the working scenarios I described above for ipr
> and performance will suffer significantly. The inquiry data for ipr RAID
> arrays does not set either CMDQUE or BQUE.

Sounds like they are sort of SCSI-2 compliant, that is allowed there but
mentioned as obsolete in current SCSI-3 (spc3r23.pdf).

The LLDD can override the queue_depth for these cases.

> > Also we don't even check the BQUE in the INQUIRY result (byte 6, bit 7).
> > Should this be changed? That is set tagged_supported if BQUE is set, like
> > we do for CMDQUE (byte 7 bit 1). And also set simple_tags if
> > tagged_supported is set.
> 
> I don't like the idea of always enabling TCQ in scsi core simply if
> the device supports it. What if the HBA does not support it? To make
> matters more interesting, what if the HBA does not support TCQ, but does
> support untagged command queueing, similar to ipr as described above?

They can override it in slave_configure.

It really does look like we should set tagged_supported if BQUE *or*
CMDQUE is set, independent of any queue_depth setting.

Things are messy in this area, we use scsi-2/SPI based variable names
(simple_tags, tagged_supported, tcq), that don't apply to newer transports.
We should use task attributes, not tag/untagged.

And not combine task attributes with tag management. That is FCP and ipr
don't need cmd->request->tag, they don't need the data setup by
blk_queue_start_tag().

Also for emulex/lpfc, cmd->tag is set before we have even set
request->tag, and neither of those include task attributes. cmd->tag
should be deleted.

> Additionally, ipr is currently relying on the fact that TCQ does not
> get enabled by default. It lets userspace code enable it after verifying
> the mode page settings of the drive for things like qerr, which the adapter
> firmware has dependencies on.
> 
> Brian

-- Patrick Mansfield

Re: command queueing cmd_per_lun, queue_depth and tags

[Brian King]

Patrick Mansfield wrote:
> On Fri, Apr 07, 2006 at 05:29:28PM -0500, Brian King wrote:
>> Patrick Mansfield wrote:
>>> Currently cmd_per_lun is the default queue depth for both tagged and
>>> untagged queueing. That is, if the LLDD does not modify queue_depth in its
>>> slave_configure, queue_depth will be set to cmd_per_lun, no matter what
>>> the command queueing/tag support. 
>>>
>>> If we don't allow queueing in the LLDD, and cmd_per_lun is supposed to be
>>> the default depth for untagged support, shouldn't it always be 1, and
>>> hence go away? 
>> This seems to assume there are no SCSI devices which do command queuing, but do
>> not support queue tags. This is not the case.
> 
> I should not have used "untagged", that is misleading and a problem with
> current scsi core, where we reference tcq, tags, and don't seem to mention
> task attributes.
> 
> But LLDD can override anything in slave_configure.

I guess my biggest problem with this part of the patch is that it prevents an
LLDD that wants this behavior from being able to use scsi_adjust_queue_depth
to set the queue depth, whether it be in slave_configure, change_queue_depth, etc.

> Also, it looks like we could safely use cmd_per_lun as the default
> queue_depth, rather than setting it to 1 as done in my previous
> post/patch.

Ok. If we do that and also allow scsi_adjust_queue_depth
to adjust the queue depth when tagged == 0, as is allowed today,
then I think most of my objections to the patch should disappear,
although it may require me to make a couple ipr changes.

>>> Similarily, why do some LLDD's use a cmd_per_lun of 3, or (like
>>> ncr53c8xx_slave_configure) set queue_depth for !tagged_supported to
>>> anything other than 1?
>> In the case of ipr, there are two scenarios. The first is for JBOD disks.
>> I use a default queue depth of 6 in ipr. When running untagged, with a queue depth
>> of > 1, the ipr adapter firmware then maintains a queue, guaranteeing only
>> one command will be outstanding to the device at once. This lower level
>> queue allows for a faster turnaround of commands and improved performance.
>> The second case is that of RAID arrays. These show up as logical scsi disks.
>> They support command queueing, but not tagged command queueing.
> 
> So you just want the task attibutes, and don't care about tag management
> (i.e. you don't ever use cmd->request->tags)? That is similar to FCP.

Correct. The ipr adapter firmware generates its own queue tags before sending
tagged commands to the device. 

>>> I know (at least) FCP can always do simple queueing, but I don't think
>>> scsi core should allow multiple commands to be sent if the device does not
>>> have CMDQUE (or BQUE).
>> This will break both of the working scenarios I described above for ipr
>> and performance will suffer significantly. The inquiry data for ipr RAID
>> arrays does not set either CMDQUE or BQUE.
> 
> Sounds like they are sort of SCSI-2 compliant, that is allowed there but
> mentioned as obsolete in current SCSI-3 (spc3r23.pdf).
> 
> The LLDD can override the queue_depth for these cases.

Agreed, but my first comment applies here as well.

>>> Also we don't even check the BQUE in the INQUIRY result (byte 6, bit 7).
>>> Should this be changed? That is set tagged_supported if BQUE is set, like
>>> we do for CMDQUE (byte 7 bit 1). And also set simple_tags if
>>> tagged_supported is set.
>> I don't like the idea of always enabling TCQ in scsi core simply if
>> the device supports it. What if the HBA does not support it? To make
>> matters more interesting, what if the HBA does not support TCQ, but does
>> support untagged command queueing, similar to ipr as described above?
> 
> They can override it in slave_configure.

Sure. I could add a scsi_deactivate_tcq in slave_configure.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: command queueing cmd_per_lun, queue_depth and tags

[Patrick Mansfield]

On Mon, Apr 10, 2006 at 03:04:47PM -0500, Brian King wrote:
> Patrick Mansfield wrote:

> > I should not have used "untagged", that is misleading and a problem with
> > current scsi core, where we reference tcq, tags, and don't seem to mention
> > task attributes.
> > 
> > But LLDD can override anything in slave_configure.
> 
> I guess my biggest problem with this part of the patch is that it prevents an
> LLDD that wants this behavior from being able to use scsi_adjust_queue_depth
> to set the queue depth, whether it be in slave_configure, change_queue_depth, etc.
> 
> > Also, it looks like we could safely use cmd_per_lun as the default
> > queue_depth, rather than setting it to 1 as done in my previous
> > post/patch.
> 
> Ok. If we do that and also allow scsi_adjust_queue_depth
> to adjust the queue depth when tagged == 0, as is allowed today,
> then I think most of my objections to the patch should disappear,
> although it may require me to make a couple ipr changes.

Yeh, and then there is not really anything left of my patch, small as it
was, just the check for BQUE and calling scsi_activate_tcq(). And calling
scsi_activate_tcq if tagged_supported should be a separate patch that also
includes any required driver changes (like with ipr to call
scsi_deactivate_tcq, and an audit of all other drivers ...).

Really I was confused by scsi_mid_low_api.txt and code mentioning tags
along with cmd_per_lun and LLD abilities, like this:

	The mid level invokes scsi_adjust_queue_depth() with tagged
	queuing off and "cmd_per_lun" for that host as the queue length.
	These settings can be overridden by a slave_configure() supplied
	by the LLD.

But it makes no sense to set "tagged queuing off" on transports like FCP.

So, it is fine to use cmd_per_lun as the default queue_depth, without
any adjustments (in slave_configure) via scsi_adjust_queue_depth or
scsi_activate_tcq.

-- Patrick Mansfield