[PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Patrick Mansfield]

On Thu, Nov 20, 2003 at 08:57:15AM -0700, Pat LaVarre wrote:

> This also could matter.  In our kernel code who wrongly thinks we gave
> permission to retry SG_IO?  This evidence echoes someone else's - may

sg.c looks OK, Dmitri must be using /dev/sdN. We need this fix, patch
is for the current scsi-bugfixes-2.6:

Don't retry SG_IO (REQ_BLOCK_PC) commands.

===== drivers/scsi/sd.c 1.138 vs edited =====
--- 1.138/drivers/scsi/sd.c	Fri Oct 17 16:14:06 2003
+++ edited/drivers/scsi/sd.c	Thu Nov 20 08:18:34 2003
@@ -347,9 +347,9 @@
 	 */
 	SCpnt->transfersize = sdp->sector_size;
 	SCpnt->underflow = this_count << 9;
+	SCpnt->allowed = SD_MAX_RETRIES;
 
 queue:
-	SCpnt->allowed = SD_MAX_RETRIES;
 	SCpnt->timeout_per_command = timeout;
 
 	/*

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Mike Anderson]

Patrick Mansfield [patmans@us.ibm.com] wrote:
> Don't retry SG_IO (REQ_BLOCK_PC) commands.

Should we also have the REQ_BLOCK_PC requests set REQ_FAILFAST? The
patch to sd will imply the same behavior.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Jens Axboe]

On Thu, Nov 20 2003, Mike Anderson wrote:
> Patrick Mansfield [patmans@us.ibm.com] wrote:
> > Don't retry SG_IO (REQ_BLOCK_PC) commands.
> 
> Should we also have the REQ_BLOCK_PC requests set REQ_FAILFAST? The
> patch to sd will imply the same behavior.

Definitely!

-- 
Jens Axboe

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Patrick Mansfield]

On Thu, Nov 20, 2003 at 08:16:51PM +0100, Jens Axboe wrote:
> On Thu, Nov 20 2003, Mike Anderson wrote:
> > Patrick Mansfield [patmans@us.ibm.com] wrote:
> > > Don't retry SG_IO (REQ_BLOCK_PC) commands.
> > 
> > Should we also have the REQ_BLOCK_PC requests set REQ_FAILFAST? The
> > patch to sd will imply the same behavior.
> 
> Definitely!

Should the setting of REQ_FAILFAST be expanded to prevent requeueing after
a device or host reports queue full?

There are probably many SG_IO users that would want them retried, others
(multimedia?) might not want a retry. Multi-path would *not* want them
retried (well at least the host queue full).

For IO that cannot be sent to the device (if we have !scsi_dev_queue_ready()
or !scsi_host_queue_ready()), it is unclear if REQ_FAILFAST should
complete the IO without ever having sent it to the device.

If we expand the checks for REQ_FAILFAST, the SG_IO user interface should
allow the conditional setting of it.

-- Patrick Mansfield

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Jens Axboe]

On Thu, Nov 20 2003, Patrick Mansfield wrote:
> On Thu, Nov 20, 2003 at 08:16:51PM +0100, Jens Axboe wrote:
> > On Thu, Nov 20 2003, Mike Anderson wrote:
> > > Patrick Mansfield [patmans@us.ibm.com] wrote:
> > > > Don't retry SG_IO (REQ_BLOCK_PC) commands.
> > > 
> > > Should we also have the REQ_BLOCK_PC requests set REQ_FAILFAST? The
> > > patch to sd will imply the same behavior.
> > 
> > Definitely!
> 
> Should the setting of REQ_FAILFAST be expanded to prevent requeueing after
> a device or host reports queue full?

Not without extra hints.

> There are probably many SG_IO users that would want them retried, others
> (multimedia?) might not want a retry. Multi-path would *not* want them
> retried (well at least the host queue full).
> 
> For IO that cannot be sent to the device (if we have !scsi_dev_queue_ready()
> or !scsi_host_queue_ready()), it is unclear if REQ_FAILFAST should
> complete the IO without ever having sent it to the device.
> 
> If we expand the checks for REQ_FAILFAST, the SG_IO user interface should
> allow the conditional setting of it.

Yes, we would need to be able to pass that hint in from user space. A
SG_FLAG_NO_RETRY would work.

-- 
Jens Axboe

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[James Bottomley]

On Fri, 2003-11-21 at 04:17, Jens Axboe wrote:
> On Thu, Nov 20 2003, Patrick Mansfield wrote:
> > Should the setting of REQ_FAILFAST be expanded to prevent requeueing after
> > a device or host reports queue full?
> 
> Not without extra hints.

So the code to add the hints is in the works?

;-)

James

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[James Bottomley]

On Fri, 2003-11-21 at 06:48, James Bottomley wrote:
> So the code to add the hints is in the works?

To be more specific:

I think we need to divide the error types up into

1. retryable
2. fatal

and to have at least two, possibly three sources

1. the transport
2. the device
(3. the driver)

so a medium error (say a read failure) is a fatal device error;
something like a no connect would be a fatal transport error, etc.

QUEUE_FULL is a retryable transport error under these terms, but we
might add a third category, flow control, to the error types which it
would more properly fall under.

the entity setting REQ_FASTFAIL should be able to indicate what type of
errors it is interested in (allowing others to be handled
automatically).

James

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Patrick Mansfield]

James -

I don't think we are going to resolve the setting and use of the fast fail
flag in the immediate future.

Can you push the patch?

Jens are you OK with the patch?

-- Patrick Mansfield

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Jens Axboe]

On Fri, Nov 21 2003, Patrick Mansfield wrote:
> James -
> 
> I don't think we are going to resolve the setting and use of the fast fail
> flag in the immediate future.
> 
> Can you push the patch?

Agree, can't be done these days at all. Maybe in a months time or so.

> Jens are you OK with the patch?

Yes

-- 
Jens Axboe

Re: [PATCH] Don't retry SG_IO (REQ_BLOCK_PC) commands.

[Andries Brouwer]

On Fri, Nov 21, 2003 at 08:12:29AM -0600, James Bottomley wrote:

> To be more specific:
> 
> I think we need to divide the error types up into
> 
> 1. retryable
> 2. fatal
> 
> and to have at least two, possibly three sources
> 
> 1. the transport
> 2. the device
> (3. the driver)
> 
> so a medium error (say a read failure) is a fatal device error;
> something like a no connect would be a fatal transport error, etc.

Yes, the driver is a legitimate source of errors.

If the driver knows that a certain command is not supported
and returns an error condition, then retrying is meaningless.
The error is not necessarily fatal in the sense that the device
does not work, it works just fine, but this particular command
will never get any other reply.

It also happens that the driver needs to allocate memory for
some command. If the allocation fails, that may be a retryable
condition, but immediately retrying is counterproductive.

Andries