[PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Export two SCSI EH command handling functions.  To be used by libata EH.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

James, this patch is a part of libata-EH related updates.  After the
update, libata EH uses functions exported from this patch to retry or
complete failed commands.

 drivers/scsi/scsi_error.c |    7 ++++---
 include/scsi/scsi_eh.h    |    3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

296dd6345057f576a875e6a140f0fd11e2b1d54f
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2333d2..6bac3d2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -584,8 +584,7 @@ static int scsi_request_sense(struct scs
  *    keep a list of pending commands for final completion, and once we
  *    are ready to leave error handling we handle completion for real.
  **/
-static void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
-			       struct list_head *done_q)
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
 	scmd->device->host->host_failed--;
 	scmd->eh_eflags = 0;
@@ -597,6 +596,7 @@ static void scsi_eh_finish_cmd(struct sc
 	scsi_setup_cmd_retry(scmd);
 	list_move_tail(&scmd->eh_entry, done_q);
 }
+EXPORT_SYMBOL(scsi_eh_finish_cmd);
 
 /**
  * scsi_eh_get_sense - Get device sense data.
@@ -1425,7 +1425,7 @@ static void scsi_eh_ready_devs(struct Sc
  * @done_q:	list_head of processed commands.
  *
  **/
-static void scsi_eh_flush_done_q(struct list_head *done_q)
+void scsi_eh_flush_done_q(struct list_head *done_q)
 {
 	struct scsi_cmnd *scmd, *next;
 
@@ -1454,6 +1454,7 @@ static void scsi_eh_flush_done_q(struct 
 		}
 	}
 }
+EXPORT_SYMBOL(scsi_eh_flush_done_q);
 
 /**
  * scsi_unjam_host - Attempt to fix a host which has a cmd that failed.
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index fabd879..d160880 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,9 @@ static inline int scsi_sense_valid(struc
 }
 
 
+extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
+			       struct list_head *done_q);
+extern void scsi_eh_flush_done_q(struct list_head *done_q);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
-- 
1.0.8

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Tejun Heo wrote:
> Export two SCSI EH command handling functions.  To be used by libata EH.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
> James, this patch is a part of libata-EH related updates.  After the
> update, libata EH uses functions exported from this patch to retry or
> complete failed commands.
> 

Sorry, James & SCSI guys.

Please ignore this message.  This was test run gone bad.

-- 
tejun

[PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Export two SCSI EH command handling functions.  To be used by libata EH.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

James, this patch is a part of libata-EH related updates.  After the
update, libata EH uses functions exported from this patch to retry or
complete failed commands.

 drivers/scsi/scsi_error.c |    7 ++++---
 include/scsi/scsi_eh.h    |    3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

296dd6345057f576a875e6a140f0fd11e2b1d54f
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a2333d2..6bac3d2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -584,8 +584,7 @@ static int scsi_request_sense(struct scs
  *    keep a list of pending commands for final completion, and once we
  *    are ready to leave error handling we handle completion for real.
  **/
-static void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
-			       struct list_head *done_q)
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
 	scmd->device->host->host_failed--;
 	scmd->eh_eflags = 0;
@@ -597,6 +596,7 @@ static void scsi_eh_finish_cmd(struct sc
 	scsi_setup_cmd_retry(scmd);
 	list_move_tail(&scmd->eh_entry, done_q);
 }
+EXPORT_SYMBOL(scsi_eh_finish_cmd);
 
 /**
  * scsi_eh_get_sense - Get device sense data.
@@ -1425,7 +1425,7 @@ static void scsi_eh_ready_devs(struct Sc
  * @done_q:	list_head of processed commands.
  *
  **/
-static void scsi_eh_flush_done_q(struct list_head *done_q)
+void scsi_eh_flush_done_q(struct list_head *done_q)
 {
 	struct scsi_cmnd *scmd, *next;
 
@@ -1454,6 +1454,7 @@ static void scsi_eh_flush_done_q(struct 
 		}
 	}
 }
+EXPORT_SYMBOL(scsi_eh_flush_done_q);
 
 /**
  * scsi_unjam_host - Attempt to fix a host which has a cmd that failed.
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index fabd879..d160880 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,9 @@ static inline int scsi_sense_valid(struc
 }
 
 
+extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
+			       struct list_head *done_q);
+extern void scsi_eh_flush_done_q(struct list_head *done_q);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
-- 
1.0.8

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Jeff Garzik]

On Mon, Jan 23, 2006 at 01:09:36PM +0900, Tejun Heo wrote:
> Export two SCSI EH command handling functions.  To be used by libata EH.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
> James, this patch is a part of libata-EH related updates.  After the
> update, libata EH uses functions exported from this patch to retry or
> complete failed commands.

ACK (obviously).  We'll want to un-export scsi_finish_command() once
this is all done.

	Jeff

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Arjan van de Ven]

On Mon, 2006-01-23 at 13:09 +0900, Tejun Heo wrote:
> Export two SCSI EH command handling functions.  To be used by libata EH.


since these are pretty much internal, can we make these _GPL exports?

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Arjan van de Ven wrote:
> On Mon, 2006-01-23 at 13:09 +0900, Tejun Heo wrote:
> 
>>Export two SCSI EH command handling functions.  To be used by libata EH.
> 
> since these are pretty much internal, can we make these _GPL exports?
> 

It's SCSI developers' decision.  I chose EXPORT_SYMBOL because all other
exported symbols in scsi_error.c were using it.  James?

-- 
tejun

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Christoph Hellwig]

On Mon, Jan 23, 2006 at 01:06:03PM +0900, Tejun Heo wrote:
> Export two SCSI EH command handling functions.  To be used by libata EH.

What do you need it for?  Please send the whole patch series (and generally
anything in libata that interacts with the scsi midlayer) to linux-scsi,
please.

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Christoph Hellwig]

On Mon, Jan 23, 2006 at 05:20:42PM +0900, Tejun Heo wrote:
> Arjan van de Ven wrote:
> > On Mon, 2006-01-23 at 13:09 +0900, Tejun Heo wrote:
> > 
> >>Export two SCSI EH command handling functions.  To be used by libata EH.
> > 
> > since these are pretty much internal, can we make these _GPL exports?
> > 
> 
> It's SCSI developers' decision.  I chose EXPORT_SYMBOL because all other
> exported symbols in scsi_error.c were using it.  James?

These are internal functions.  If we're going to export them at all then
as _GPL (which is grossly misnamed and should be _INTERNAL)

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

Christoph Hellwig wrote:
> On Mon, Jan 23, 2006 at 05:20:42PM +0900, Tejun Heo wrote:
> 
>>Arjan van de Ven wrote:
>>
>>>On Mon, 2006-01-23 at 13:09 +0900, Tejun Heo wrote:
>>>
>>>
>>>>Export two SCSI EH command handling functions.  To be used by libata EH.
>>>
>>>since these are pretty much internal, can we make these _GPL exports?
>>>
>>
>>It's SCSI developers' decision.  I chose EXPORT_SYMBOL because all other
>>exported symbols in scsi_error.c were using it.  James?
> 
> 
> These are internal functions.  If we're going to export them at all then
> as _GPL (which is grossly misnamed and should be _INTERNAL)

And from the other thread created by my mistake.

 > What do you need it for?  Please send the whole patch series (and
 > generally anything in libata that interacts with the scsi midlayer) to
 > linux-scsi, please.

It's rather large patchset consisting of 13 libata-eh related patches. 
Only two of the patches are relevant to SCSI.  I'm not sure whether 
cross-posting the whole thing is appropriate.

The first one exports two scsi_eh functions (this thread) and the second 
one uses these two to implement ata_eh_retry/complete helpers.

http://marc.theaimsgroup.com/?l=linux-ide&m=113798939719926&w=2
http://marc.theaimsgroup.com/?l=linux-ide&m=113798939627887&w=2

These are used because libata implements shost->eh_strategy_handler for 
error handling.  libata's ->eh_strategy_handler should do everything 
scsi_unjam_host() does and one chunk is to retry or complete failed SCSI 
commands during or after EH completes.  libata used to do this by 
calling scsi_finish_command() directly, which is an internal interface 
too.  Directly calling scsi_finish_command() also used to have deadlock 
problem when libata shared SCSI's host lock.

As what has to be done is identical, above two patches make libata share 
that part of code with scsi_unjam_host().  And, as Jeff commented, once 
these are settled scsi_fini_command() can be unexported.

-- 
tejun

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Jeff Garzik]

On Mon, Jan 23, 2006 at 09:36:12AM +0000, Christoph Hellwig wrote:
> On Mon, Jan 23, 2006 at 01:06:03PM +0900, Tejun Heo wrote:
> > Export two SCSI EH command handling functions.  To be used by libata EH.
> 
> What do you need it for?  Please send the whole patch series (and generally
> anything in libata that interacts with the scsi midlayer) to linux-scsi,
> please.

It will replace the nasty and insufficient scsi_finish_command() export,
which doesn't retry like Tejun's current patches do.  Exporting these
two functions, and un-exporting scsi_finish_command(), is necessary for
users of ->eh_strategy_handler().

	Jeff

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Tejun Heo]

[PATCH] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

Export two SCSI EH command handling functions.  To be used by libata EH.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

Respin of the patch with EXPORT_SYMBOL_GPL.

Index: work1/drivers/scsi/scsi_error.c
===================================================================
--- work1.orig/drivers/scsi/scsi_error.c	2006-01-23 23:48:55.000000000 +0900
+++ work1/drivers/scsi/scsi_error.c	2006-01-23 23:49:32.000000000 +0900
@@ -584,8 +584,7 @@ static int scsi_request_sense(struct scs
  *    keep a list of pending commands for final completion, and once we
  *    are ready to leave error handling we handle completion for real.
  **/
-static void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
-			       struct list_head *done_q)
+void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
 	scmd->device->host->host_failed--;
 	scmd->eh_eflags = 0;
@@ -597,6 +596,7 @@ static void scsi_eh_finish_cmd(struct sc
 	scsi_setup_cmd_retry(scmd);
 	list_move_tail(&scmd->eh_entry, done_q);
 }
+EXPORT_SYMBOL_GPL(scsi_eh_finish_cmd);
 
 /**
  * scsi_eh_get_sense - Get device sense data.
@@ -1425,7 +1425,7 @@ static void scsi_eh_ready_devs(struct Sc
  * @done_q:	list_head of processed commands.
  *
  **/
-static void scsi_eh_flush_done_q(struct list_head *done_q)
+void scsi_eh_flush_done_q(struct list_head *done_q)
 {
 	struct scsi_cmnd *scmd, *next;
 
@@ -1454,6 +1454,7 @@ static void scsi_eh_flush_done_q(struct 
 		}
 	}
 }
+EXPORT_SYMBOL_GPL(scsi_eh_flush_done_q);
 
 /**
  * scsi_unjam_host - Attempt to fix a host which has a cmd that failed.
Index: work1/include/scsi/scsi_eh.h
===================================================================
--- work1.orig/include/scsi/scsi_eh.h	2006-01-23 23:48:55.000000000 +0900
+++ work1/include/scsi/scsi_eh.h	2006-01-23 23:49:02.000000000 +0900
@@ -35,6 +35,9 @@ static inline int scsi_sense_valid(struc
 }
 
 
+extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
+			       struct list_head *done_q);
+extern void scsi_eh_flush_done_q(struct list_head *done_q);
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Luben Tuikov]

--- Tejun Heo <htejun@gmail.com> wrote:
> Christoph Hellwig wrote:
> > On Mon, Jan 23, 2006 at 05:20:42PM +0900, Tejun Heo wrote:
> > 
> >>Arjan van de Ven wrote:
> >>
> >>>On Mon, 2006-01-23 at 13:09 +0900, Tejun Heo wrote:
> >>>
> >>>
> >>>>Export two SCSI EH command handling functions.  To be used by libata EH.
> >>>
> >>>since these are pretty much internal, can we make these _GPL exports?
> >>>
> >>
> >>It's SCSI developers' decision.  I chose EXPORT_SYMBOL because all other
> >>exported symbols in scsi_error.c were using it.  James?
> > 
> > 
> > These are internal functions.  If we're going to export them at all then
> > as _GPL (which is grossly misnamed and should be _INTERNAL)
> 
> And from the other thread created by my mistake.
> 
>  > What do you need it for?  Please send the whole patch series (and
>  > generally anything in libata that interacts with the scsi midlayer) to
>  > linux-scsi, please.
> 
> It's rather large patchset consisting of 13 libata-eh related patches. 
> Only two of the patches are relevant to SCSI.  I'm not sure whether 
> cross-posting the whole thing is appropriate.
> 
> The first one exports two scsi_eh functions (this thread) and the second 
> one uses these two to implement ata_eh_retry/complete helpers.
> 
> http://marc.theaimsgroup.com/?l=linux-ide&m=113798939719926&w=2
> http://marc.theaimsgroup.com/?l=linux-ide&m=113798939627887&w=2
> 
> These are used because libata implements shost->eh_strategy_handler for 
> error handling.  libata's ->eh_strategy_handler should do everything 
> scsi_unjam_host() does and one chunk is to retry or complete failed SCSI 
> commands during or after EH completes.  libata used to do this by 
> calling scsi_finish_command() directly, which is an internal interface 
> too.  Directly calling scsi_finish_command() also used to have deadlock 
> problem when libata shared SCSI's host lock.
> 
> As what has to be done is identical, above two patches make libata share 
> that part of code with scsi_unjam_host().  And, as Jeff commented, once 
> these are settled scsi_fini_command() can be unexported.

Let me understand:

So when scsi_finish_command() is unexported, then _all_ EH strategies
would have to define local done_q, and splice eh_cmd_q into local work_q,
then from local work_q into local done_q and then that is passed
to SCSI Core, via scsi_eh_flush_done_q().

Is it possible that this is done asynchrounously?  Since EH recovery is
per host, but you may have only a single device which is misbehaving?

I.e. is it possible, in a such and such EH strategy, not necessarily
SCSI Cores's or libata's, to _not_ have a local done_q?  So that,
we have only local work_q (after splicing eh_cmd_q) and after each
command is looked at, we "return" that command back to SCSI Core, so
that SCSI Core can "finish" it. (In effect "per-command", as opposed to
"per-queue-of-commands".)

Alternatively, simulating this using one-command-per-local-done_q and
calling scsi_eh_flush_done_q() each time a single command is added
to local done_q would seem rather inefficient.

    Luben

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Arjan van de Ven]

On Tue, 2006-01-24 at 09:11 -0800, Luben Tuikov wrote:

> So when scsi_finish_command() is unexported, then _all_ EH strategies
> would have to define local done_q, and splice eh_cmd_q into local work_q,
> then from local work_q into local done_q and then that is passed
> to SCSI Core, via scsi_eh_flush_done_q().


custom error handling was supposed to have gone away already, but then
libata came around for now... (until that moves to be a block driver),
so.. I don't see the problem really.

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Jeff Garzik]

On Tue, Jan 24, 2006 at 09:11:17AM -0800, Luben Tuikov wrote:
> So when scsi_finish_command() is unexported, then _all_ EH strategies
> would have to define local done_q, and splice eh_cmd_q into local work_q,
> then from local work_q into local done_q and then that is passed
> to SCSI Core, via scsi_eh_flush_done_q().
> 
> Is it possible that this is done asynchrounously?  Since EH recovery is
> per host, but you may have only a single device which is misbehaving?

No, because the host is stopped during EH, and only restarted after your
EH code completes its run.


> I.e. is it possible, in a such and such EH strategy, not necessarily
> SCSI Cores's or libata's, to _not_ have a local done_q?  So that,
> we have only local work_q (after splicing eh_cmd_q) and after each
> command is looked at, we "return" that command back to SCSI Core, so
> that SCSI Core can "finish" it. (In effect "per-command", as opposed to
> "per-queue-of-commands".)

done_q is simply a method for returning one or more commands back to the
SCSI Core, so that the SCSI Core can finish it.

Per-command is irrelevant during EH, because queue processing is not
restarted until your EH handler ends completely.

One alternative strategy for EH is to override the ->eh_timed_out()
function.  Its ugly, but you may effectively eliminate EH handling via
->eh_strategy_handler(), by moving the error handling completely within
(a) the interrupt handler or (b) ->eh_timed_out().

	Jeff

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Luben Tuikov]

--- Arjan van de Ven <arjan@infradead.org> wrote:
> custom error handling was supposed to have gone away already, but then
> libata came around for now... (until that moves to be a block driver),
> so.. I don't see the problem really.

Shouldn't one want more "custom error handling"?  That is, per protocol.
So in effect what you call "custom error handling" actually means
per protocol error handling, be it FC, SATA, iSCSI, USB, SAS, etc.

So SCSI Core offloads recovery to the protocol stack, which depending
on the transport/interconnect may offload some (but not all) functionality
to the port driver.  (Case in point: tunneling protocols.)

Then you can have SCSI Core only decide whether to retry to command.
(If so asked by the submitter from uppper layer.)

     Luben

Re: [PATCH 07/13] SCSI: export scsi_eh_finish_cmd() and scsi_eh_flush_done_q()

[Luben Tuikov]

--- Jeff Garzik <jgarzik@pobox.com> wrote:
> On Tue, Jan 24, 2006 at 09:11:17AM -0800, Luben Tuikov wrote:
> > So when scsi_finish_command() is unexported, then _all_ EH strategies
> > would have to define local done_q, and splice eh_cmd_q into local work_q,
> > then from local work_q into local done_q and then that is passed
> > to SCSI Core, via scsi_eh_flush_done_q().
> > 
> > Is it possible that this is done asynchrounously?  Since EH recovery is
> > per host, but you may have only a single device which is misbehaving?
> 
> No, because the host is stopped during EH, and only restarted after your
> EH code completes its run.

Look at my sentence above, I just said "Since EH recovery is per host". You're
repeating  what I had just said.

Maybe I was too subtle saying "Since EH recovery is per host, but you may
have only a single device which is misbehaving":  the whole point is not to have
to stop the "host" in order to do error recovery.  Finer grained recovery would
be better, obviously for many reasons, both technologically and logistically.
 
> > I.e. is it possible, in a such and such EH strategy, not necessarily
> > SCSI Cores's or libata's, to _not_ have a local done_q?  So that,
> > we have only local work_q (after splicing eh_cmd_q) and after each
> > command is looked at, we "return" that command back to SCSI Core, so
> > that SCSI Core can "finish" it. (In effect "per-command", as opposed to
> > "per-queue-of-commands".)
> 
> done_q is simply a method for returning one or more commands back to the
> SCSI Core, so that the SCSI Core can finish it.

Thank you for pointing the obvious.

> Per-command is irrelevant during EH, because queue processing is not
> restarted until your EH handler ends completely.

Which would not be the case if finer grained recovery is to be attained.

> One alternative strategy for EH is to override the ->eh_timed_out()
> function.  Its ugly, but you may effectively eliminate EH handling via
> ->eh_strategy_handler(), by moving the error handling completely within
> (a) the interrupt handler or (b) ->eh_timed_out().

Neither.  There's per protocol recovery EHs which override both
eh_timed_out() _and_ eh_strategy_handler(), in order to implement
protocol recovery.

     Luben