[PATCH] scsi: break from queue depth adjusting loops when device found

[PATCH] scsi: break from queue depth adjusting loops when device found

[Stephen M. Cameron]

From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Don't loop through all the devices even after
finding the one we're looking for

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
Reviewed-by: Robert Elliott <elliott@hp.com>
---
 drivers/scsi/scsi_error.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cbe38e5..db8a488 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -635,6 +635,7 @@ static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 		sht->change_queue_depth(tmp_sdev, tmp_sdev->queue_depth + 1,
 					SCSI_QDEPTH_RAMP_UP);
 		sdev->last_queue_ramp_up = jiffies;
+		break;
 	}
 }
 
@@ -657,6 +658,7 @@ static void scsi_handle_queue_full(struct scsi_device *sdev)
 		 */
 		sht->change_queue_depth(tmp_sdev, tmp_sdev->queue_depth - 1,
 					SCSI_QDEPTH_QFULL);
+		break;
 	}
 }
 

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Douglas Gilbert]

On 14-07-03 11:05 AM, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>
> Don't loop through all the devices even after
> finding the one we're looking for
>
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> Reviewed-by: Robert Elliott <elliott@hp.com>

With my scsi_debug testing on Christoph's core-for-3.17
tree I can simulate infrequent TSF conditions using the
every_nth mechanism. When one is triggered, all scsi
devices (that scsi_debug is monitoring) get their
queue_depth adjusted.

So:
Acked-by: Douglas Gilbert <dgilbert@interlog.com>


> ---
>   drivers/scsi/scsi_error.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index cbe38e5..db8a488 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -635,6 +635,7 @@ static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
>   		sht->change_queue_depth(tmp_sdev, tmp_sdev->queue_depth + 1,
>   					SCSI_QDEPTH_RAMP_UP);
>   		sdev->last_queue_ramp_up = jiffies;
> +		break;
>   	}
>   }
>
> @@ -657,6 +658,7 @@ static void scsi_handle_queue_full(struct scsi_device *sdev)
>   		 */
>   		sht->change_queue_depth(tmp_sdev, tmp_sdev->queue_depth - 1,
>   					SCSI_QDEPTH_QFULL);
> +		break;
>   	}
>   }
>
>
> --
> 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
>

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Christoph Hellwig]

On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> Don't loop through all the devices even after
> finding the one we're looking for

The comments in the code seem to indicate that we want to modify
the queue depth for all LUNs on a given target.

Ccing Mike and Vasu as they wrote this code.

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Mike Christie]

On 07/03/2014 12:11 PM, Christoph Hellwig wrote:
> On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> Don't loop through all the devices even after
>> finding the one we're looking for
> 
> The comments in the code seem to indicate that we want to modify
> the queue depth for all LUNs on a given target.
> 
> Ccing Mike and Vasu as they wrote this code.
> 

Ccing James Smart for more details.

We copied this behavior from lpfc's SAM_STAT_TASK_SET_FULL handling
(look at around the 2.6.32 kernel for reference).

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Douglas Gilbert]

On 14-07-03 01:21 PM, Mike Christie wrote:
> On 07/03/2014 12:11 PM, Christoph Hellwig wrote:
>> On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>
>>> Don't loop through all the devices even after
>>> finding the one we're looking for
>>
>> The comments in the code seem to indicate that we want to modify
>> the queue depth for all LUNs on a given target.
>>
>> Ccing Mike and Vasu as they wrote this code.
>>
>
> Ccing James Smart for more details.
>
> We copied this behavior from lpfc's SAM_STAT_TASK_SET_FULL handling
> (look at around the 2.6.32 kernel for reference).

Perhaps there should be a knob to control this, maybe within
the scope of a host. I have one RAID controller that has a
JBOD mode in which real disks are presented as LUs in the same
target. Obviously the TSF (queue_type and other) capabilities
of those real disks might differ.

Doug Gilbert

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Hannes Reinecke]

On 07/03/2014 07:11 PM, Christoph Hellwig wrote:
> On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>
>> Don't loop through all the devices even after
>> finding the one we're looking for
>
> The comments in the code seem to indicate that we want to modify
> the queue depth for all LUNs on a given target.
>
> Ccing Mike and Vasu as they wrote this code.
>
Indeed, that was the idea.
This piece of code tries to keep track of the remote port queue 
depth, which isn't represented at all.
Thing is, each remote target has a target queue depth which can hold 
only so many outstanding SCSI requests. If that is full it'll return 
BUSY for _all_ LUNs served from that port.
And the very _next_ command after the one which filled the target 
queue will get the BUSY status.
So we need to decrease the queue depth on _all_ LUNs here to avoid 
starvation of individual devices.

Hence I guess this is not the correct fix.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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

RE: [PATCH] scsi: break from queue depth adjusting loops when device found

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Friday, 04 July, 2014 5:53 AM
> To: Christoph Hellwig; Stephen M. Cameron
> Cc: james.bottomley@parallels.com; Elliott, Robert (Server Storage);
> stephenmcameron@gmail.com; linux-scsi@vger.kernel.org; Vasu Dev; Mike
> Christie
> Subject: Re: [PATCH] scsi: break from queue depth adjusting loops when device
> found
> 
> On 07/03/2014 07:11 PM, Christoph Hellwig wrote:
> > On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
> >> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>
> >> Don't loop through all the devices even after
> >> finding the one we're looking for
> >
> > The comments in the code seem to indicate that we want to modify
> > the queue depth for all LUNs on a given target.
> >
> > Ccing Mike and Vasu as they wrote this code.
> >
> Indeed, that was the idea.
> This piece of code tries to keep track of the remote port queue
> depth, which isn't represented at all.
> Thing is, each remote target has a target queue depth which can hold
> only so many outstanding SCSI requests. If that is full it'll return
> BUSY for _all_ LUNs served from that port.
> And the very _next_ command after the one which filled the target
> queue will get the BUSY status.
> So we need to decrease the queue depth on _all_ LUNs here to avoid
> starvation of individual devices.
> 
> Hence I guess this is not the correct fix.

Some SCSI target device designs work that way, but others don't. 

In the original SCSI Architecture Model (SAM-1; 1995):
* BUSY meant "the logical unit is busy" 
* TASK SET FULL meant "the logical unit does not have enough 
  resources" 

It didn't say "SCSI target port" or "SCSI target device", so
they were not intended to be affected by other logical units.
That was based on the idealized notion that each I_T_L nexus was 
independent, also assumed by the hierarchical LUN addressing 
scheme and the SCSI Controller Commands (SCC) standard.

With that interpretation, the command identifier/tag (the Q in 
I_T_L_Q nexus) is independent for each I_T_L nexus - two 
logical units behind the same SCSI target port could use the 
same tag value at the same time for different commands.

However, every SCSI transport protocol after parallel SCSI
has chosen to share command tags across all logical units; 
passing the (usually) 8-byte LUN field in every IU/frame is 
not practical.  So, that idealized I_T_L nexus concept has
broken down.  Implementations really have a mix of SCSI target 
device, SCSI target port, and logical unit resources.

When we added the optional status qualifier to SAM-4 (2006), 
we added a SCOPE field indicating if the scope of the BUSY or 
TASK SET FULL is for:
* just the logical unit;
* all logical units in the SCSI target device accessible through 
  the SCSI target port; or
* all logical units in the SCSI target device.

There is no VPD page field defined to report which scope(s) are
implemented or describing the resource limits.  Some designs
are simple, others are complicated.

If TASK SET FULL means a SCSI target port limit has been 
reached, then decrementing the limit on all the logical 
units will over-correct.  If the limit is 256 and there
are 16 LUNs, nominally 16 per LUN, and you send a 257th
command, then the total will drop to 16x15=240, not 256.

mpt3sas assigns separate target numbers for target ports it
discovers, so the SCSI midlayer queue depth logic is more 
correct if those SCSI target devices are implementing a 
SCSI target port scope for BUSY and TASK SET FULL, but 
incorrect if they are implementing a logical unit scope.

[    2.711047] scsi 0:0:0:0: Direct-Access     HP       EO0400JDVFB      HPD1 PQ: 0 ANSI: 6
[    2.711746] scsi 0:0:1:0: Direct-Access     HP       EO0400JDVFB      HPD1 PQ: 0 ANSI: 6
[    2.712423] scsi 0:0:2:0: Direct-Access     HP       EO0400JDVFB      HPD1 PQ: 0 ANSI: 6

hpsa presents multiple LUNs in one target, and a TASK SET FULL 
on one logical unit doesn't mean that you'll get it on the 
others, so the SCSI midlayer queue depth logic is incorrect:
[   10.790885] scsi 2:0:0:0: Direct-Access     HP       LOGICAL VOLUME   10.0 PQ: 0 ANSI: 5
[   10.791095] scsi 2:0:0:1: Direct-Access     HP       LOGICAL VOLUME   10.0 PQ: 0 ANSI: 5
[   10.791300] scsi 2:0:0:2: Direct-Access     HP       LOGICAL VOLUME   10.0 PQ: 0 ANSI: 5

The SCSI host template .cmd_per_lun value, which is used
to set the default initial queue depth for each device 
(i.e., each LUN), implies the logical unit scope. 

Perhaps the SCSI midlayer should keep track of both
SCSI target port and logical unit queue depths, parse
the status qualifier if present, and let the host
template advise on the policy to assume if the
status qualifier is not present.

Short of that, I think it's best to assume logical unit
scope, which always the presumption in SCSI, for BUSY
and TASK SET FULL.

There is some code for a scsi_target structure that I 
don't understand and have just been ignoring: target_busy, 
target_blocked, etc.  Does that represent the SCSI target 
port over multiple logical units, or does that relate to 
target-mode where the system is acting as the SCSI target 
and presenting logical units itself?

---
Rob Elliott    HP Server Storage

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Christoph Hellwig]

On Fri, Jul 04, 2014 at 07:21:52PM +0000, Elliott, Robert (Server Storage) wrote:
> Perhaps the SCSI midlayer should keep track of both
> SCSI target port and logical unit queue depths, parse
> the status qualifier if present, and let the host
> template advise on the policy to assume if the
> status qualifier is not present.

We already keep track of the target queue depth, although with the
scsi-mq series this is not optional and only done for hosts that declare
they have a per-target queue limit by setting ->can_queue in the
scsi_target structurue.  Only a small number of iSCSI offload HBAs does
this.

I need to look at the SAM scope defintions, but it doesn't sound too hard
to implement.

> There is some code for a scsi_target structure that I 
> don't understand and have just been ignoring: target_busy, 
> target_blocked, etc.  Does that represent the SCSI target 
> port over multiple logical units, or does that relate to 
> target-mode where the system is acting as the SCSI target 
> and presenting logical units itself?

It represents the target port, and is not related to target mode support at
all.  The target mode code uses completely different data structures than
the initiator side mid layer.

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Christoph Hellwig]

I just saw this patch showing up again in the hpsa series, so let's get
the discussiong going on how to proceed again:

I think we can't simply apply the current version as it breaks the intended
semantics in the FC drivers that expect an array with target-wide ressources.

So if we want to go ahead quickly we might just need two versions of it, one
that does LUN-level ramp up/down and one that does target level.

I'd also very much like to be able to use the SAM-4 indicators, but I fear
that a lot of hardware might not be able to get this right.  If anyone
has experience with using those I'd love to hear about it.

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Christoph Hellwig]

On Sat, Jul 26, 2014 at 11:14:35AM -0500, Stephen Cameron wrote:
> Hmm, I forgot that that patch was in there, I wasn't trying to keep pushing
> it along.  From the previous discussion, I got the impression I was simply
> wrong, and that this patch wasn't needed, so I had meant to drop it, I just
> forgot to actually drop it.

It's more complicated - as Robert indicated you're tenically right, although
in practice it's not what the existing users expect.  If you have some
cycles for it I'd love to at lest get the per-LUN and per-target
ramp up/down in ASAP.  We can then start looking into doing it even
better based on the target response later on.

Re: [PATCH] scsi: break from queue depth adjusting loops when device found

[Hannes Reinecke]

On 07/26/2014 06:17 PM, Christoph Hellwig wrote:
> On Sat, Jul 26, 2014 at 11:14:35AM -0500, Stephen Cameron wrote:
>> Hmm, I forgot that that patch was in there, I wasn't trying to keep pushing
>> it along.  From the previous discussion, I got the impression I was simply
>> wrong, and that this patch wasn't needed, so I had meant to drop it, I just
>> forgot to actually drop it.
>
> It's more complicated - as Robert indicated you're tenically right, although
> in practice it's not what the existing users expect.  If you have some
> cycles for it I'd love to at lest get the per-LUN and per-target
> ramp up/down in ASAP.  We can then start looking into doing it even
> better based on the target response later on.
>
The current implementation is based on the needs of the FC HBAs, which 
would need a way to throttle I/O to avoid flooding a target port.
The reason it was done per target is (from what I can tell) due to a 
specific implementation from a large vendor which is using a 
per-target-port request queue.
And more often than not defaulting to SCSI-2 conformance to be 'legacy 
compatible'. So no way one could use any shiny new commands.

Having said that it has been quite some time since it's been 
implemented, and it _might_ be that things have changed and we can get 
away with a per-LUN throttling. As I doubt we'll get information about 
this from the various storage vendors (at least not from those we've got 
issues with even today) we probably have to bite the bullet here and 
test things out.

But hey, it's worth a shot anyway. So, storage vendors, speak up!

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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