[PATCH] tmscsim: implement change_queue_depth

[PATCH] tmscsim: implement change_queue_depth

[Guennadi Liakhovetski]

Hi

Further to my recent doubts wrt change_queue_depth semantics, I decided to 
just implement this function for tmscsim following comments in 
scsi_host.h. If this gets accepted in this form (returning -EIO if depth 
!= sdev->queue_depth after scsi_adjust_depth_queue), I'll submit patches 
to 53c700.c and a comment-modification to scsi_host.h.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

--- a/drivers/scsi/tmscsim.c	Tue Dec 14 23:20:52 2004
+++ b/drivers/scsi/tmscsim.c	Tue Dec 14 22:59:55 2004
@@ -2274,6 +2274,18 @@
 	return 0;
 }
 
+static int dc390_change_queue_depth(struct scsi_device *sdev, int depth)
+{
+	struct dc390_acb *acb = (struct dc390_acb *)sdev->host->hostdata;
+
+	if (depth > acb->TagMaxNum)
+		depth = acb->TagMaxNum;
+
+	scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, depth);
+
+	return depth == sdev->queue_depth ? depth : -EIO;
+}
+
 static struct scsi_host_template driver_template = {
 	.module			= THIS_MODULE,
 	.proc_name		= "tmscsim", 
@@ -2284,6 +2296,7 @@
 	.queuecommand		= DC390_queuecommand,
 	.eh_abort_handler	= DC390_abort,
 	.eh_bus_reset_handler	= DC390_bus_reset,
+	.change_queue_depth	= dc390_change_queue_depth,
 	.can_queue		= 1,
 	.this_id		= 7,
 	.sg_tablesize		= SG_ALL,

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: implement change_queue_depth

[James Bottomley]

On Wed, 2004-12-22 at 13:15 +0100, Guennadi Liakhovetski wrote:
> Further to my recent doubts wrt change_queue_depth semantics, I decided to 
> just implement this function for tmscsim following comments in 
> scsi_host.h. If this gets accepted in this form (returning -EIO if depth 
> != sdev->queue_depth after scsi_adjust_depth_queue), I'll submit patches 
> to 53c700.c and a comment-modification to scsi_host.h.

Actually, there's currently no way for scsi_adjust_queue_depth to fail,
so the check really isn't necessary.

Also, even if it were, the correct return would be either -EINVAL, or
the true nature of the error (most likely -ENOMEM).

The idea of the error return is more for drivers that have to allocate
internal resources to tag queues, so they can fail in those allocations.

James

Re: [PATCH] tmscsim: implement change_queue_depth

[Guennadi Liakhovetski]

On Thu, 23 Dec 2004, James Bottomley wrote:

> On Wed, 2004-12-22 at 13:15 +0100, Guennadi Liakhovetski wrote:
> > Further to my recent doubts wrt change_queue_depth semantics, I decided to 
> > just implement this function for tmscsim following comments in 
> > scsi_host.h. If this gets accepted in this form (returning -EIO if depth 
> > != sdev->queue_depth after scsi_adjust_depth_queue), I'll submit patches 
> > to 53c700.c and a comment-modification to scsi_host.h.
> 
> Actually, there's currently no way for scsi_adjust_queue_depth to fail,
> so the check really isn't necessary.

Well, I didn't look in the block layer command queue handling, I just saw 
this in scsi_adjust_queue_depth

	/* Check to see if the queue is managed by the block layer
	 * if it is, and we fail to adjust the depth, exit */
	if (blk_queue_tagged(sdev->request_queue) &&
	    blk_queue_resize_tags(sdev->request_queue, tags) != 0)
		goto out;

and thought that this failure case has to be reported back... Like, if 
somebody was trying to set queue depth for a device that doesn't support 
it. Or would this be impossible for another reason?

> Also, even if it were, the correct return would be either -EINVAL, or
> the true nature of the error (most likely -ENOMEM).

The return value from the blk_queue_resize_tags in the error case is not 
returned, so, it is either -ENXIO, in which case, I guess, the 
blk_queue_tagged test would just fail.

Am I missing anything?

> The idea of the error return is more for drivers that have to allocate
> internal resources to tag queues, so they can fail in those allocations.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: implement change_queue_depth

[James Bottomley]

On Sat, 2004-12-25 at 01:36 +0100, Guennadi Liakhovetski wrote:
> Well, I didn't look in the block layer command queue handling, I just saw 
> this in scsi_adjust_queue_depth
> 
> 	/* Check to see if the queue is managed by the block layer
> 	 * if it is, and we fail to adjust the depth, exit */
> 	if (blk_queue_tagged(sdev->request_queue) &&
> 	    blk_queue_resize_tags(sdev->request_queue, tags) != 0)
> 		goto out;
> 
> and thought that this failure case has to be reported back... Like, if 
> somebody was trying to set queue depth for a device that doesn't support 
> it. Or would this be impossible for another reason?

No, it's not impossible.  However trying to second guess what went wrong
in an API when it doesn't return an error condition to you is a bad
thing.  You can return errors from things you know about or from things
you're told about, but you shouldn't do it by using heuristics to guess
that there was an error in certain cases (well unless the heuristics are
a documented part of the API).

James

Re: [PATCH] tmscsim: implement change_queue_depth

[Guennadi Liakhovetski]

On Sun, 26 Dec 2004, James Bottomley wrote:

> On Sat, 2004-12-25 at 01:36 +0100, Guennadi Liakhovetski wrote:
> > Well, I didn't look in the block layer command queue handling, I just saw 
> > this in scsi_adjust_queue_depth
> > 
> > 	/* Check to see if the queue is managed by the block layer
> > 	 * if it is, and we fail to adjust the depth, exit */
> > 	if (blk_queue_tagged(sdev->request_queue) &&
> > 	    blk_queue_resize_tags(sdev->request_queue, tags) != 0)
> > 		goto out;
> > 
> > and thought that this failure case has to be reported back... Like, if 
> > somebody was trying to set queue depth for a device that doesn't support 
> > it. Or would this be impossible for another reason?
> 
> No, it's not impossible.  However trying to second guess what went wrong
> in an API when it doesn't return an error condition to you is a bad
> thing.

Ok. Would it make sense / be accepted to change this sitation by either 1) 
adding a new function __scsi_adjust_queue_depth() which would pass the 
return code of the block-layer, or 2) modifying the existing function to 
return an error? 1) has disadvantages, that it's a) confusing, b) even 
more confusing, because that other function also has to be exported. 2) 
has the disadvantage of changing an exported API in the "stable" series... 
I would go for 2) especially with the current extended understanding of 
the "stable" kernel series. I could roll a patch for either of these 2 
possibilities, if the latter is chosen I could try to fix the current 
users to check the return code.

As it is, the worst that can happen is that somebody tries to set 
queue_depth to a value through sysfs, we return positive return code, but 
the value is unchanged, which can be confirmed by cat'ing the same file. 
How bad is this? Does any other caller of scsi_adjust_queue_depth care if 
it really succeeded?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: implement change_queue_depth

[James Bottomley]

On Sun, 2004-12-26 at 22:49 +0100, Guennadi Liakhovetski wrote:
> Ok. Would it make sense / be accepted to change this sitation by either 1) 
> adding a new function __scsi_adjust_queue_depth() which would pass the 
> return code of the block-layer, or 2) modifying the existing function to 
> return an error? 1) has disadvantages, that it's a) confusing, b) even 
> more confusing, because that other function also has to be exported. 2) 
> has the disadvantage of changing an exported API in the "stable" series... 
> I would go for 2) especially with the current extended understanding of 
> the "stable" kernel series. I could roll a patch for either of these 2 
> possibilities, if the latter is chosen I could try to fix the current 
> users to check the return code.

To be honest, unless you can find a driver that cares about this
failure, I don't think so.

James

Re: [PATCH] tmscsim: implement change_queue_depth

[Guennadi Liakhovetski]

On Mon, 27 Dec 2004, James Bottomley wrote:

> On Sun, 2004-12-26 at 22:49 +0100, Guennadi Liakhovetski wrote:
> > Ok. Would it make sense / be accepted to change this sitation by either 1) 
> > adding a new function __scsi_adjust_queue_depth() which would pass the 
> > return code of the block-layer, or 2) modifying the existing function to 
> > return an error? 1) has disadvantages, that it's a) confusing, b) even 
> 
> To be honest, unless you can find a driver that cares about this
> failure, I don't think so.

No, I don't know any either. I thought more about a user, doing something 
like
$ echo "4" > /sys/.../queue_depth
$ echo $?
0
$ cat /sys/.../queue_depth
3
and wondering why... But, I feel I am abusing your time already with this 
minor issue, so, if you (and everybody else) think, that this possible 
confusion is unimportant, I'll shut up now:-)

Thanks
Guennadi
---
Guennadi Liakhovetski