command queueing cmd_per_lun, queue_depth and tags

PATCH Patrick Mansfield 2 participants 5 messages Apr 7, 2006 – Apr 10, 2006 SCSI

Share & tools

Patrick Mansfield

author

2006-04-07 21:50 UTC #0 lore raw

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.

drivers/scsi/scsi.c renamed from home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c +5 -7

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
@@ -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;
 	}

drivers/scsi/scsi_scan.c renamed from home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c +6 -3

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
@@ -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

Brian King

2006-04-07 22:29 UTC #1 lore raw

Quoted 8 lines · 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?

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

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 90 lines

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

drivers/scsi/scsi.c renamed from home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi.c +5 -7

@@ -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

drivers/scsi/scsi_scan.c renamed from home/linux/views/linux-2.6.17-rc1/drivers/scsi/scsi_scan.c +6 -5

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Patrick Mansfield

author

2006-04-10 19:00 UTC #2 lore raw

Quoted 6 lines · Brian King

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 11 lines

Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 6 lines

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

Brian King

2006-04-10 20:04 UTC #3 lore raw

Quoted 10 lines · Patrick Mansfield

Quoted 9 lines · Brian King
Quoted 8 lines · 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?

Quoted 8 lines

Quoted 2 lines
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.

Quoted 3 lines

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.

Quoted 11 lines

Quoted 10 lines
Quoted 3 lines
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.

Quoted 2 lines

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.

Quoted 11 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 10 lines

Quoted 8 lines
Quoted 4 lines
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.

Signature · 3 lines

Brian King
eServer Storage I/O
IBM Linux Technology Center

Patrick Mansfield

author

2006-04-10 23:42 UTC #4 lore raw

Quoted 12 lines · Brian King

Quoted 5 lines · Patrick Mansfield
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.

Quoted 8 lines

Quoted 3 lines
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