major libata update]

linux-scsi should have been included in the CC.

Jeff

After much development and review, I merged a massive pile of libata patches from Tejun Heo and Albert Lee. This update contains the following major libata

CHANGES:
* Rewritten error handling. This is a major piece of work, even though it will be rarely seen. The new libata EH provides the foundation for not only improved error handling, but also new features such as device hotplug or command queueing. (Tejun Heo)

* PIO-based I/O is now IRQ-driven by default, rather than polled in a kernel thread. The polling path will continue to exist for controllers that need it, and other special cases. (Albert Lee)

* Core support for command queueing (Jens Axboe, Tejun Heo)

* Support for NCQ-style command queueing (Jens Axboe, Tejun Heo)

* Increase max-sectors dramatically, for LBA48 devices (Tejun Heo?)

* Other minor changes, from myself and others.

IMPACT:
* If all goes well, this update should improve error handling, solve several outstanding, difficult-to-solve bugs, and provide a good foundation for adding some nifty features in the future.

TESTING:
* Although most drivers by count received few operational changes, the common probe path was updated, so all drivers need fresh "yes, it sees all my disks" regression testing.

* ahci and sata_sil24 were touched a lot, and so need additional testing.

* sata_sil and ata_piix also need healthy re-testing of all basic functionality.

FEEDBACK:
* Please CC [email protected] on all emails and bug reports.

MERGE STATUS:
* Barring major problems in testing, will submit during 2.6.18 merge window.

Patch:
http://www.kernel.org/pub/linux/kernel/people/jgarzik/libata/2.6.17-rc4-git2-libata1.patch.bz2
(diff'd against 2.6.17-rc4-git2, but should apply to most recent 2.6.17-rcX[-gitY] kernels)

The 'upstream' branch of

git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git

contains the following updates:

 drivers/scsi/Makefile       |    2 
 drivers/scsi/ahci.c         |  436 ++++---
 drivers/scsi/ata_piix.c     |   16 
 drivers/scsi/libata-bmdma.c |  143 ++
 drivers/scsi/libata-core.c  | 2437 +++++++++++++++++++++++++++++---------------
 drivers/scsi/libata-eh.c    | 1558 ++++++++++++++++++++++++++++
 drivers/scsi/libata-scsi.c  |  423 ++++---
 drivers/scsi/libata.h       |   24 
 drivers/scsi/pdc_adma.c     |   10 
 drivers/scsi/sata_mv.c      |   30 
 drivers/scsi/sata_nv.c      |    6 
 drivers/scsi/sata_promise.c |   18 
 drivers/scsi/sata_qstor.c   |   13 
 drivers/scsi/sata_sil.c     |   65 -
 drivers/scsi/sata_sil24.c   |  615 ++++++-----
 drivers/scsi/sata_sis.c     |    2 
 drivers/scsi/sata_svw.c     |    4 
 drivers/scsi/sata_sx4.c     |   19 
 drivers/scsi/sata_uli.c     |    2 
 drivers/scsi/sata_via.c     |    2 
 drivers/scsi/sata_vsc.c     |   15 
 drivers/scsi/scsi.c         |   18 
 drivers/scsi/scsi_error.c   |    3 
 drivers/scsi/scsi_lib.c     |    2 
 drivers/scsi/scsi_priv.h    |    1 
 include/linux/ata.h         |   34 
 include/linux/libata.h      |  379 ++++--
 include/scsi/scsi_cmnd.h    |    1 
 include/scsi/scsi_eh.h      |    1 
 include/scsi/scsi_host.h    |    1 
 30 files changed, 4634 insertions(+), 1646 deletions(-)

Albert Lee:
libata: interrupt driven pio for libata-core
libata: interrupt driven pio for LLD
libata irq-pio: add comments and cleanup
libata irq-pio: rename atapi_packet_task() and comments
libata irq-pio: simplify if condition in ata_dataout_task()
libata irq-pio: cleanup ata_qc_issue_prot()
libata: move atapi_send_cdb() and ata_dataout_task()
[libata irq-pio] reorganize ata_pio_sector() and __atapi_pio_bytes() [libata irq-pio] reorganize "buf + offset" in ata_pio_sector() [libata irq-pio] use PageHighMem() to optimize the kmap_atomic() usage libata irq-pio: misc fixes
libata irq-pio: merge the ata_dataout_task workqueue with ata_pio_task workqueue libata irq-pio: eliminate unnecessary queuing in ata_pio_first_block() libata irq-pio: add read/write multiple support
libata-dev: determine err_mask when error is found
libata-dev: filter out noisy ATAPI error messages
libata-dev: Fix array index value in ata_rwcmd_protocol()
libata-dev: Use new ata_queue_pio_task() for PIO polling task
libata-dev: Use new AC_ERR_* flags
libata-dev: Minor comment fix
libata-dev: recognize WRITE_MULTI_FUA_EXT for r/w multiple
libata-dev: Remove trailing whitespaces
libata-dev: Fix merge problem with upstream
libata-dev: Remove atapi_packet_task()
libata-dev: Move out the HSM code from ata_host_intr()
libata-dev: Minor fix for ata_hsm_move() to work with ata_host_intr()
libata-dev: Let ata_hsm_move() work with both irq-pio and polling pio
libata-dev: Convert ata_pio_task() to use the new ata_hsm_move()
libata-dev: Cleanup unused enums/functions
libata-dev: ata_check_atapi_dma() fix for ATA_FLAG_PIO_POLLING LLDDs
libata-dev: Make the the in_wq check as an inline function
libata-dev: irq-pio minor fixes (respin)
libata-dev: fix the device err check sequence (respin)
libata-dev: wait idle after reading the last data block
libata-dev: print out information for ATAPI devices with CDB interrupts
libata-dev: handle DRQ=1 ERR=1 (revised)
libata-dev: irq-pio minor fix
libata-dev: irq-pio minor fix 2
libata: convert ATAPI_ENABLE_DMADIR to module parameter

Bastiaan Jacques:
ahci: add support for VIA VT8251

Jeff Garzik:
[libata irq-pio] build fix
[libata pdc_adma] update for removal of ATA_FLAG_NOINTR [libata pdc_adma] fix for new irq-driven PIO code [libata sata_mv] IRQ PIO build fix
[libata] irq-pio: fix breakage related to err_mask merge [libata sata_promise] irq_pio: fix merge bug
[libata] build fix after merging some pre-packet_task-removal code [libata irq-pio] s/assert/WARN_ON/
[libata] build fix after cdb_len move
sata_vsc build fix
libata: irq-pio build fixes
[libata] irq-pio: fix build breakage
[libata] irq-pio: Fix merge mistake
[libata] kill bogus cut-n-pasted comments in three drivers [libata] bump versions
libata: Fix EH merge difference between this branch and upstream.
libata: Add helper ata_shost_to_port()

Luben Tuikov:
SCSI: Introduce scsi_req_abort_cmd (REPOST)

Tejun Heo:
libata: increase LBA48 max sectors to 65535
libata: fix ata_set_mode() return value
libata: make ata_bus_probe() return negative errno on failure
libata: separate out ata_spd_string()
libata: convert do_probe_reset() to ata_do_reset()
libata: implement ata_dev_enabled and disabled()
libata: make ata_set_mode() handle no-device case properly
libata: reorganize ata_set_mode()
libata: don't disable devices from ata_set_mode()
libata: preserve SATA SPD setting over hard resets
libata: implement ata_dev_absent()
libata: implement ap->sata_spd_limit and helpers
libata: use SATA speed down in ata_drive_probe_reset()
libata: add 5s sleep between resets
libata: implement ata_down_xfermask_limit()
libata: improve ata_bus_probe()
libata: consider disabled devices in ata_dev_xfermask()
libata: report device number when PIO fails
libata: ata_dev_revalidate() printk update
libata: ATA_FLAG_IN_EH is not used, kill it
libata: clean up constants
libata: rename ATA_FLAG_PORT_DISABLED to ATA_FLAG_DISABLED
libata: clear only affected flags during ata_dev_configure()
libata: clear ATA_DFLAG_PIO before setting it
libata: add ATA_QCFLAG_IO
libata: pass qc around intead of ap during PIO
libata: always generate sense if qc->err_mask is non-zero
libata: don't read TF directly from sense generation functions
libata: add @cdb to ata_exec_internal()
libata: dec scmd->retries for qcs with zero err_mask
libata: separate out libata-eh.c
libata: make some libata-core routines extern
libata: print SControl in SATA link status info message
ahci: do not fail softreset if PHY reports no device
libata: set default cbl in probeinit
libata: kill @verbose from ata_reset_fn_t
libata: make reset methods complain when they fail
sata_sil24: fix timeout calculation in sil24_softreset
sata_sil24: better error message from softreset
libata: implement ata_wait_register()
ahci: use ata_wait_register()
sata_sil24: use ata_wait_register()
libata: disable failed devices only once in ata_bus_probe()
libata: cosmetic update to ata_bus_probe()
libata: export ata_set_sata_spd()
sata_sil24: typo fix
sata_sil24: rename PORT_IRQ_SDB_FIS to PORT_IRQ_SDB_NOTIFY
sata_sil24: add more constants
sata_sil24: consolidate host flags into SIL24_COMMON_FLAGS
sata_sil24: implement loss of completion interrupt on PCI-X errta fix
sata_sil24: implement sil24_init_port()
sata_sil24: put port into known state before softresetting
sata_sil24: kill 10ms sleep in softreset
sata_sil24: reimplement hardreset
sata_sil24: don't do hardreset during driver initialization
sata_sil24: fix on-memory structure byteorder
sata_sil24: enable 64bit
SCSI: implement shost->host_eh_scheduled
libata: silly fix in ata_scsi_start_stop_xlat()
libata: rename ata_down_sata_spd_limit() and friends
ahci: hardreset classification fix
libata: unexport ata_scsi_error()
libata: kill duplicate prototypes
libata: fix ->phy_reset class code handling in ata_bus_probe()
libata: clear ap->active_tag atomically w.r.t. command completion
libata: hold host_set lock while finishing internal qc
libata: use preallocated buffers
libata: move ->set_mode() handling into ata_set_mode()
libata: remove postreset handling from ata_do_reset()
libata: implement qc->result_tf
sata_sil24: update TF image only when necessary
libata: init ap->cbl to ATA_CBL_SATA early
libata: implement new SCR handling and port on/offline functions
libata: use new SCR and on/offline functions
libata: kill old SCR functions and sata_dev_present()
libata: add dev->ap
libata: use dev->ap
libata: implement ATA printk helpers
libata: use ATA printk helpers
libata-eh-fw: add flags and operations for new EH
libata-eh-fw: clear SError in ata_std_postreset()
libata-eh-fw: use special reserved tag and qc for internal commands
libata-eh-fw: update ata_qc_from_tag() to enforce normal/EH qc ownership
libata-eh-fw: implement new EH scheduling via error completion
libata-eh-fw: implement ata_port_schedule_eh() and ata_port_abort()
libata-eh-fw: implement freeze/thaw
libata-eh-fw: implement new EH scheduling from PIO
libata-eh-fw: update ata_scsi_error() for new EH
libata-eh-fw: update ata_exec_internal() for new EH
libata-eh-fw: update SCSI command completion path for new EH
libata-eh: add ATA and libata flags for new EH
libata-eh: implement dev->ering
libata-eh: implement ata_eh_info and ata_eh_context
libata-eh: implement new EH
libata-eh: implement BMDMA EH
ata_piix: convert to new EH
sata_sil: convert to new EH
ahci: convert to new EH
ahci: add PIOS interim interrupt handling
sata_sil24: convert to new EH
libata: fix irq-pio merge
libata-ncq: add NCQ related ATA/libata constants and macros
libata-ncq: pass ata_scsi_translate() return value to SCSI midlayer
libata-ncq: rename ap->qactive to ap->qc_allocated
libata-ncq: implement ap->qc_active, ap->sactive and complete helper
libata-ncq: implement NCQ command translation and exclusion
libata-ncq: update EH to handle NCQ
libata-ncq: implement NCQ device configuration
ahci: clean up AHCI constants in preparation for NCQ
ahci: add HOST_CAP_NCQ constant
ahci: kill pp->cmd_tbl_sg
ahci: implement NCQ suppport
sata_sil24: implement NCQ support

-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yes, it should, thanks.

It's a pretty huge change, so I haven't gone over it all. However, two things leap to mind immediately about this patch set

1) your host_eh_scheduled logic looks wrong. It seems to me that you can miss the wakeup if the host is busy? I also don't see a need to move the prototype out of scsi_priv.h ... it should only be used by transport classes, anyway.

2) This scsi_req_abort_cmd() is fundamentally the wrong logic. Everything else is communicated back as a result code from the command in done(). This should be no different ... A status return of DID_FAILED which scsi_decide_disposition() always translates to FAILED would seem to do exactly what you want without all the overhead.

I'll find time to go over the rest of this later ...

James

(CC's restored)

I can't see a case _in libata operation_ where a set of circumstances arises that causes missed wakeups, can you elaborate?

We're talking about all ->eh_strategy_handler() users, which is a valid EH API for an LLDD to choose. Granted libata is really the only one right now.

Long term, ->eh_strategy_handler and transport classes are block layer not SCSI level anyway, so scsi_priv.h is clearly inappropriate.

Inigo sez[1]: I do not think "fundamentally wrong" means what you think it means.

You miss the fact that the timer may have already fired, in which completing a command gets you...... not a damned thing. scsi_done() will simply return, if the timeout has fired. This has always been an annoying problem to work around.

scsi_req_abort_cmd() may perhaps be misnamed, but it GUARANTEES a set of operations which libata EH wants. Certainly, if we can guarantee this set of conditions another way, we are open to any alternate path.

Jeff

[1] from one of my favorite movies. search http://us.imdb.com/title/tt0093779/quotes for "INCONCEIVABLE"

This is scsi_eh_wakeup():

void scsi_eh_wakeup(struct Scsi_Host *shost)

{
	if (shost->host_busy == shost->host_failed) {
		wake_up_process(shost->ehandler);

so if you try a wakeup with no failed commands and the host still busy, nothing happens.

We're busy revoking the LLDD driver, so in future it will be transport classes only.

That can be sorted out if someone actually gets around to moving error handling to the block level. In the meantime, it's SCSI that we're discussing.

No ... in that case the eh is already active, and your API does this:

void scsi_req_abort_cmd(struct scsi_cmnd *cmd)

{
	if (!scsi_delete_timer(cmd))
		return;
		^^^^^^^
	scsi_times_out(cmd);
}

Which likewise does nothing if the timer has already fired, so they both have the same effect.

James

Sigh. They clearly do not have the same effect, because the above code guarantees that a timeout is forced, regardless of whether the timer has fired or not. That in turn guarantees that the timeout callback (->eh_timed_out) is called, and the cmd is in a very specific state.

Completion-or-timeout has none of these attributes.

Any alternative is forced to deal with two very different command, and EH, states... to achieve the same eventual result. Thus, the code presented is the one of least complexity, AFAICS.

Jeff

the API claims to be forcibly aborting a command, which is *not* a timeout ... trying to pretend to the midlayer that it is is the wrong processing model. You may choose to call this API because of a class internal timeout, but you don't need the callback notification that it is a timeout in this case, you already know it is.

James

I can certainly agree the name may not be the best choice. Naming based on implementation, it could be scsi_force_timeout_cmd() or somesuch.

Jeff

But it is not that either!

As far as the LLDD is concerned, it is calling scsi_req_abort_cmd() to request SCSI Core to abort it. At the point of LLDD calling this function the command is not marked anything. This is important!

Why?

Because LLDD/interconnects
1. Do not want to race with SCSI Core changing the state of the command.
2. You want to give LLDD a chance as the command may finish in due time since the interconnect requested it be aborted. I.e. there could be a race in the transport too, which doesn't depend on the transport but on the end device, SDS, and the transport.

For these reasons the best bet is to emulate a time out so that SCSI Core calls back into the transport to tell the transpor that, it, SCSI Core, is about to start ER on the command.

Then LLDD/interconnects have another chance, plus it gives notification to LLDD/IC that SCSI Core has changed its disposition of that command.

Luben

It doesn't matter.

No, it is not the "wrong processng model".

Then if you already know that it is, you MAY decide to ignore the event. It wouldn't matter to you now would it. Take a look at the GIT description:

Introduce scsi_req_abort_cmd(struct scsi_cmnd *).
This function requests that SCSI Core start recovery for the command by deleting the timer and adding the command to the eh queue. It can be called by either LLDDs or SCSI Core. LLDDs who implement their own error recovery MAY ignore the timeout event if they generated scsi_req_abort_cmd.

This function solves a myriad of issues, one of which is the atomic marking of a command aborted by the transports when they've defined BOTH the timeout callback and the error recovery callback to have complete and consistent error recovery.

Also it doesn't matter that it is NOT a timeout as far as SCSI Core is concerned. Why? Because proper status would be returned at Error Recover time. I.e. by the TMFs being called as part of ER.

Luben

It's handled the same way shost->host_failed is handled. scsi_device_unbusy() wakes it up when the condition is met.

void scsi_device_unbusy(struct scsi_device *sdev)

{
	struct Scsi_Host *shost = sdev->host;
	unsigned long flags;

	spin_lock_irqsave(shost->host_lock, flags);
	shost->host_busy--;
	if (unlikely(scsi_host_in_recovery(shost) &&
		     (shost->host_failed || shost->host_eh_scheduled)))
		scsi_eh_wakeup(shost);
	spin_unlock(shost->host_lock);
	spin_lock(sdev->request_queue->queue_lock);
	sdev->device_busy--;
	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
}

[--snip--]

Currently, there is no reliable way to trigger DID_FAILED unconditionally. I thought about adding some host code or whatever to force it but it felt too hackish and went with the scsi_eh_schedule_scmd(). Then, Luben suggested scsi_req_abort_cmd(), so that's what I've ended up with.

tejun

Tejun Heo wrote:
[--snip--]

Oops, wasn't thinking straight. Time to go bad. The obstacle I couldn't avoid was the forced scmd timeout in scsi_softirq_done().

tejun

That's the piece I hadn't spotted, yet ... thanks. That will do it.

could you move ata_schedule_scsi_eh to scsi_error (with the appropriate API rename). That way the SCSI state model changes aren't in libata (and no-one else who wants to use this has to open code the ata_schedule_scsi_eh API).

James

ACK

I certainly can, and it was done that way first time around. Please note the following discussion.

http://thread.gmane.org/gmane.linux.scsi/23853/focus=9760

Luben objected the interface made public because SCSI host is not supposed to know about exception conditions which are not associated with ITL nor ITLQ nexus. Thus, I made it a temporary measure only for libata, which is planned to move out.

So, SCSI contains only the necessary bits required to implement the feature and libata open-codes the rest. As it's not an exported interface, no other SCSI driver is supposed to use it and the SCSI modifications can be easily removed after libata moves out.

As long as libata can do EH not associated with scmd or device, I'm okay either way and think it's your call. So, considering the above discussion, do you want it to be a generic SCSI interface?

Thanks.

tejun

His objection is still valid. However, as the balance of evils, I think, if you have to do this, it's better to contain it in a way where it's obvious what's being done. Plus you don't want someone to modify the host state model and suddenly find libata doesn't work anymore because they failed to spot that it needed to change as well ...

Yes, but in scsi_priv.h, please ... I can actually think of another use for it in terms of getting the SG reset handler to work properly.

James

I see.

Okay, will do in scsi_priv.h

Jeff, it seems that we need to reset #upstream and rebuild it. Do you have any other idea than resetting libata-tj#for-jeff and libata-dev#upstream?

tejun

Just apply an update patch that achieves the desired result.

Jeff

Things used by the transport classes traditionally weren't in scsi_priv.h, that was for scsi_mod internals only. Should we changed that? I'd rather have another header for semi-volatile APIs transport classes use.

Hence my objection... Transport classes also aren't SCSI-specific.

Jeff

well ... OK we don't necessarily have a header for SCSI APIs exported exclusively to transport classes. how about drivers/scsi/scsi_transport_api.h?

James

Are we agreed on this? A new header file drivers/scsi/scsi_transport_api.h?

tejun

libata implemented a feature to schedule EH without an associated EH by manipulating shost->host_eh_scheduled in ata_scsi_schedule_eh() directly. Move this function to scsi_error.c and rename it to scsi_schedule_eh(). It is now an exported API for SCSI transports and exported via new header file drivers/scsi/scsi_transport_api.h

This patch also de-export scsi_eh_wakeup() which was exported specifically for ata_scsi_schedule_eh().

---

Okay, I went with drivers/scsi/scsi_transport_api.h. I intentionally didn't add any banner at the top of the file. Feel free to fill with appropriate description/blurb.

 drivers/scsi/libata-eh.c          |    3 ++-
 drivers/scsi/libata-scsi.c        |   24 ------------------------
 drivers/scsi/scsi_error.c         |   23 ++++++++++++++++++++++-
 drivers/scsi/scsi_priv.h          |    1 +
 drivers/scsi/scsi_transport_api.h |    6 ++++++
 include/scsi/scsi_eh.h            |    1 -
 6 files changed, 31 insertions(+), 27 deletions(-)
 create mode 100644 drivers/scsi/scsi_transport_api.h

6daec98e81a98db0dafc4846a97432746bf94f9c

index 750e734..71b45ad 100644
@@ -39,6 +39,7 @@ #include <scsi/scsi_host.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_cmnd.h>
+#include "scsi_transport_api.h"
 
 #include <linux/libata.h>
 

@@ -432,7 +433,7 @@ void ata_port_schedule_eh(struct ata_por
 	WARN_ON(!ap->ops->error_handler);
 
 	ap->flags |= ATA_FLAG_EH_PENDING;
-	ata_schedule_scsi_eh(ap->host);
+	scsi_schedule_eh(ap->host);
 
 	DPRINTK("port EH scheduled\n");
 }

index f036ae4..2007b4b 100644
@@ -2745,27 +2745,3 @@ void ata_scsi_scan_host(struct ata_port 
 			scsi_scan_target(&ap->host->shost_gendev, 0, i, 0, 0);
 	}
 }
-
-/**
- *	ata_schedule_scsi_eh - schedule EH for SCSI host
- *	@shost:	SCSI host to invoke error handling on.
- *
- *	Schedule SCSI EH without scmd.  This is a hack.
- *
- *	LOCKING:
- *	spin_lock_irqsave(host_set lock)
- **/
-void ata_schedule_scsi_eh(struct Scsi_Host *shost)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(shost->host_lock, flags);
-
-	if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
-	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
-		shost->host_eh_scheduled++;
-		scsi_eh_wakeup(shost);
-	}
-
-	spin_unlock_irqrestore(shost->host_lock, flags);
-}

index 9ca71cb..346ab72 100644
@@ -56,7 +56,28 @@ void scsi_eh_wakeup(struct Scsi_Host *sh
 				printk("Waking error handler thread\n"));
 	}
 }
-EXPORT_SYMBOL_GPL(scsi_eh_wakeup);
+
+/**
+ * scsi_schedule_eh - schedule EH for SCSI host
+ * @shost:	SCSI host to invoke error handling on.
+ *
+ * Schedule SCSI EH without scmd.
+ **/
+void scsi_schedule_eh(struct Scsi_Host *shost)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+
+	if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
+	    scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
+		shost->host_eh_scheduled++;
+		scsi_eh_wakeup(shost);
+	}
+
+	spin_unlock_irqrestore(shost->host_lock, flags);
+}
+EXPORT_SYMBOL_GPL(scsi_schedule_eh);
 
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.

index 0b39081..27c4827 100644
@@ -63,6 +63,7 @@ extern int scsi_delete_timer(struct scsi
 extern void scsi_times_out(struct scsi_cmnd *cmd);
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
+extern void scsi_eh_wakeup(struct Scsi_Host *shost);
 extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
 
 /* scsi_lib.c */

new file mode 100644
index 0000000..934f0e6
@@ -0,0 +1,6 @@
+#ifndef _SCSI_TRANSPORT_API_H
+#define _SCSI_TRANSPORT_API_H
+
+void scsi_schedule_eh(struct Scsi_Host *shost);
+
+#endif /* _SCSI_TRANSPORT_API_H */

index 212c983..d160880 100644
@@ -35,7 +35,6 @@ static inline int scsi_sense_valid(struc
 }
 
 
-extern void scsi_eh_wakeup(struct Scsi_Host *shost);
 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);

1.3.2

Clearly. And where in the code do you see that this condition will strike?

If we are talking about impossible runtime conditions, then the objection is academic.

Its an API-which-only-libata-uses that we're discussing. And because its moving to the block layer, its also a temporary-API-which-only-libata-uses.

Jeff

OK ... this may be the root of the problem. I really would like libata to migrate to being block only ... especially as PATA looks to be trying to follow you into the SCSI subsystem. However, this has been the statement for the past two years (at least), and really, few enhancements have been made to block that you need to make good on this. I think one of the things we'll try to find time to do at the storage summit is to take a hard look at block to see exactly what has to be added to make libata solely dependent upon it.

However, the bottom line is that if you want to modify the *SCSI* API then you follow the same process as everyone else (i.e. demonstrate justification and utility and worry about long lived maintainability of the API).

James

100% agreed...

The general list, off the top of my head:

* objects: storage message, storage device, storage host, and the requisite interconnections
* queuecommand-style API
* EH thread(s)
* timers, for command timeouts
* SCSI-style MLqueue and state stuff, i.e. ability to return "device busy", "host busy", "retry this command", ...

And once libata is happy at the block layer, move SCSI to using this stuff too :)

FWIW, as ATAPI continues to align ever more closely with SCSI MMC, I strongly feel that libata should continue to use the "SCSI MMC device class driver" (i.e. sr), and other applicable SCSI device class drivers (st, ...).

Like modern SAS controllers, which support plugging SATA drives, libata must mix SCSI and non-SCSI. Thus, most of the above infrastructure basically _must_ live outside the SCSI layer, if we are going to properly support modern controllers in a modular fashion.

Well...
* ->eh_strategy_handler() API was unusable before libata (evidence: all the bugs we've fixed)
* there were zero users before libata (evidence: grep past 2.4.0)
* there is only one user today, libata (evidence: grep)

So you'll pardon my skepticism when newly elevated standards for this API suddenly appear. libata is just trying to Do What Needs To Be Done, And Nothing More.

Jeff

Ditto! I'd be more than willing to implement some of these features (and already started to, the per command timeout for instance), but I was starting to write off libata moving to block as a silly pipe dream in all honesty... But if momentum is picking up behind this move, then I'll all for it.

Storage message -> request. The rq-cmd-type branch of the block repo has most/some of that done. For an explicit storage device + host, I have no plans to expland on what we have.

That's a style issue, rather than a required item. You can roll that on top of the current api by just doing a:

int queuecommand_helper(request_queue_t *q, struct request *rq)

{
        /* issue request */
        ...
        return OK/DEFER/REJECT/WHATEVER
}

blk_queuecommand_helper(request_queue_t *q, queue_command_fn *fn)

{
        struct request *rq;
        int ret;

        do {
                rq = elv_next_request(q);
                if (!rq)
                        break;

                ret = fn(q, rq);
                if (ret == OK)
                        continue;

                /* handle replugging/killing/whatever */
        } while (1);
}

if you really wanted.

Agree, we can abstract out the grunt handling of timeouts and the context required in the block layer.

Comes with the blk_queuecommand_helper() type setup from above.

Definitely.

Jens Axboe

Just gotta be patient. Rome wasn't built in a day, and all that :)

Like I mentioned in another message, the ideal world is that libata uses an ATA disk driver and a SCSI MMC driver -- just like a modern SAS controller (which likely supports SATA too) will use both an ATA disk driver and a SCSI disk driver.

Given this "ideal world", its IMO best that the "storage driver" infrastructure lives in the block layer not SCSI layer.

Agreed that storage message == request.

storage device and storage host are key objects included in the infrastructure libata uses SCSI for. They fall naturally out of the infrastructure that provides "device busy", "host busy", EH and EH synchronization across multiple devices, etc. Though these, SCSI also provides infrastructure through which an LLDD may export a bus topology to the user.

That's not an optional piece. Given the needed timeout / device / host infrastructure, you inevitably wind up with the following code pattern:

infrastructure code
send fully prepared request to hardware
infrastructure code

At this point I should note that all of what I've been describing is an _optional addition_ to the block layer. Its all helpers and a few new, optional structs. This SHOULD NOT involve changing the core block layer at all. Well, maybe struct request would like the addition of a timer.
But that's it, and such a mod is easy to do.

Jeff

This is one of the questions. Currently block has no concept of "host". All it knows about are queues (which may be per host or per device depending on the implementation). Do we need to introduce the concept of something like queue grouping (a sort of lightweight infrastructure that could be used by the underlying transport to implement a host concept without introducing hosts at the block layer)?

James

Boy, this sounds interesting. Could also be a more sane way to implement can_queue depths for the host. Another thing comes to mind - queue depths per target, which has always been missing from Linux. Although, any grouping immediately brings to mind scheduling policies within the group.

-- james s

Yes, and not only that... you must describe the queue pipeline too. i.e. N logical devices can be bottlenecked behind a bridge (expander, port multiplier, tunnel) of queue depth Q, which may in turn be behind another bottleneck. :)

But overall, libata and SAS controllers are forced to deal with the reality of the situation: they all wind up either using, or recreating from scratch, objects for host/device/bus/etc. in order to sanely allow all the infrastructure to interoperate.

You'll all note that struct Scsi_Host and struct scsi_cmnd have very little to do with SCSI. Its almost all infrastructure and driver management. That's the _useful_ stuff that libata uses SCSI for.

Thus, moving libata to the block layer entails either s/Scsi_Host/Storage_Host/g or a highly similar infrastructure, to achieve the same gains.

It is _trivial_ to write a new SCSI driver [even if your hardware is not SCSI], and there are good reasons for that. Please all, examine those reasons...

Jeff

Well ... no, I'm not convinced of this. Block is currently a nice, fast abstraction. It's designed to manage storage infrastructure and provide helpers to implementation. The question is how much more common infrastructure do we need to slim down all of our storage stacks. I.e. block provides the building blocks to allow the storage implementation to do what it wants, but it doesn't necessarily provide the full implementation.

but that doesn't go for all storage ... look at the way usb and firewire implement host in SCSI at the moment.

Some is driver management, others are SCSI specific. We'll never get away from the need for Scsi_Host and scsi_cmnd, but we can make sure they contain only truly SCSI specific pieces. scsi_cmnd is the closest since it pretty much has a one to one mapping with a block request.

I'm not sure. Block is currently nicely lightweight. A large number of implementations have no use for a host concept ... I don't think we should be forcing it on them.

I really think the initial question is what more could be moved up to block as common infrastructure ... I don't think we need to take concepts up wholesale as long as we get the infrastructure right.

James

My central thesis is that
* SCSI provides a generic _storage driver_ infrastructure, encapsulating many common idioms generic to SCSI and non-SCSI hardware alike.
* Any such storage driver infrastructure, outside of SCSI, should impose no burdens on existing block drivers.

Call such storage driver infrastructure "libstorage" if you will.

Let's just stop using the word host, its too confusing for all involved :) I'm well aware of this, look at how libata uses Scsi_Host too...

Agreed, though IMO I claim that "a lot" is driver management.

Agreed.

Like I said above, think "libstorage". I think block as-is, too.

Jeff

Er, make that "I like block as-is, too"

Jeff

That's good.

I don't understand why you'd ever _want_ a concept of "host". The whole concept is broken and unnecessary. At no point should you even need to map from request to host, but if you do, you don't need to introduce any generic "host" notion, you can do it easily per-queue.

We already have it. Each queue has its lock pointer. If you want to have a "host" notion, you do it by just setting the queue lock to point to the host lock (since if they are dependent, you'd _better_ share the lock between the queues anyway), and then you do

struct myhost_struct *queue_host(struct request_queue *queue)

  {
	return container_of(queue->queue_lock, myhost_struct, host_lock);

}

and there are no changes necessary to the queue layer.

You can do it other ways too: the "struct kobject" in the queue obviously contains a pointer to the parent of the queue, and that might well be your "host" object. Again, exact same deal, except now you'd use

	container_of(queue->kobj.parent, myhost_struct, host_kobject);

instead. Entirely up to you.

The whole fixation with "host" in the SCSI layer is a bug, I think. What does it matter, really? And when do you actually have a "request_queue" entry without already knowing which controller it is connected to (ie why do you even need that mapping)?

Linus

Maybe "host" is the wrong word, but there is some usefulness to look at dependencies of queues.

That gets you the "parent" structure (or "host") for the queues, but it doesn't help you with managing them. One issue could be a shared tag queue - we already solved that by sharing the tag map between the queues, but that still allows on queue to deplete more entries that it should be allowed to. And so on.

Some devices need serialization between them, and the only way to achieve that currently is by sharing a queue. But that fails eg at the io scheduler level - and other places also assume a 1:1 mapping between queues and devices. And this you cannot do generically right now, and it's one thing I would like to handle.

Jens Axboe

Right. But all of this is very much dependent on the actual hardware. It makes no sense to have a "generic host" kind of structure like SCSI has, except for historical reasons.

It makes sense to make _individual_ design issues be able to share some resource(s) between queues, and that's exactly what we do. We have the notion of having a shared lock (which makes sense because while the queues might be otherwise independent, they may go through a shared hw interface), and as you point out, we can also share the tags for exactly the same reason.

But in _no_ case is is valid to think that a shared resource would somehow mean anything more being shared. There simply is no "host" level sharing. It's very much -not- an all-or-nothing thing. It's a "some hardware may share some stuff, and it very much depends on the hw which parts it might end up sharing between the queues.

So what I was objecting to was introducing the overlying "host" notion that SCSI has. I don't think it makes any sense from any generic standpoint, and I wanted to point out that _if_ you have a "all or nothing" shared model like SCSI has, you can use any of the fields that we _can_ share (eg the lock field) as a way to tie that one field together with all the other fields.

But it would be wrong to see that as anything but a special case.

Hmm? No. We share the queue for some things, but the most _common_ example of a device that needs serialization between queues is IDE, and we don't share queues there. We have independent queues, they just end up sharing certain infrastructure (tags and locking).

But they _are_ independent, and you can have different elevators, different merging rules, and even different request functions for the different queues - even if they also have some things they share.

And yes, you can see an ATA host as a host in the SCSI sense, but I wanted to point out that you don't _need_ to see it that way, and indeed, the requests queues don't. The fact that a driver can then _use_ the request queues as if there was a "host" is fine. But we should not _enforce_ such a totally broken and limited model on the queues.

Linus

Strongly agreed.

Note that libata uses independent queues, but serializes access to them.
This is the sort of "group of queues" infrastructure I find helpful.

Yep.

Actually libata presents one Scsi_Host per SATA port on each controller, FWIW :) So the Scsi_Host notion WRT queues _is_ limiting, which is the reason why I refer to "group of queues".

Jeff

"host" is ultimately the wrong word, sure.

It's more about (a) grouping queues into a topology that the user expects, as exported by sysfs, and (b) grouping queues for the purposes of useful and/or necessary hardware operations like "stop <these> queues, so that we can bitbang the hardware".

That grouping of queues, along with the lib-ification of highly common request management code[1], is part of the non-SCSI utility that libata derives from drivers/scsi.

"group-wide operations" are highly common, and generic code inevitably results from that. But IMO that's helper code, living on top of the perfectly-fine existing code.

True, the mapping is obtained from request_queue not request.

The entry point into a block driver is via q->request_fn(), which only has a request_queue for an argument. So in practice, one usually obtains the private controller-info and bus-info data via the request_queue's ->queuedata.

Deep into sub-APIs, I've seen that sometimes you'll see only the request passed as an argument, because it's easier to walk request->queue->controller_info than to pass additional arguments to every function.

Jeff

[1] "resource management": refers to drivers/scsi's handling of 'device busy', 'group-of-queues busy' style transient errors, well integrated with block layer's command queueing and well synchronized with the EH thread.

I'll bite.

through, the host is all I want. I'm happy to completely forget about devices as they are loosely defined at the moment. I view a host (port) and a network interface as (almost) the same thing. Carrying on with the analogy, I view a device as an IP address.

Currently linux has a storage device for each unique instance of this tuple:
<host_port, target_port, lu_name>
So if a dual ported disk is fully connected to three HBAs (hosts) on the same machine then the OS should show six different devices for the same disk. Just knowing the wwn of a device (i.e. lu_name) is not sufficient for error processing, hot plug processing, etc.

There is also a natural hierarchy of errors (seen from the OS). Fatal errors associated with the host (e.g. it is hot unplugged) make starting or ongoing error recovery at a lower level (i.e. target or lu) futile.

The linux block layer still shares the quaint Unix idea of foreseeing errors so it can disallow requests. This is in the hope that error processing associated with responses (or lack of them) will be simplified.

Doug Gilbert

Hell no it isn't.

Trying to make network interfaces and block device queues sound like they are the same thing is bogus. They are NOT the same thing.

No it doesn't.

That's some SCSI internal brain-damage.

Linux has a set of queues, and a way to map from device nodes to queues.

That's all. There's no "host port". There's no "target port". There's a queue. You can certainly combine one queue into multiple queues, or you can take multiple queues and combine them into one. But don't at _any_ point think that it's anything but just a queue.

The queue has a lot of data associated with it (rules for how requests can be combined, what the DMA alignment is, etc etc). But it has not, and should not have, any idiotic association like a "host" or "target". It's a queue, dammit, nothing more.

The thing that associates it with an actual device is a combination of queue mappers and the queue lookup logic.

It should have six queues, any of which can access the same physical disk. And you can decide how to route the requests between those queues with the mapping layer or queue lookup. At no point has this got anything to do with a "host".

The moment you confuse the issue with a "host", you lose sight of the fact that you often don't have a host at all. A queue might be a totally virtual thing, that just feeds to other queues (mirroring). It _has_ no host. Thinking it has a host is setting yourself up for just doing totally idiotic and wrong things.

And even if you can point to a physical chip and say "that is the host", that's a totally irrelevant thing. That physical chip migth be an ethernet chip, but why the hell would you care, if the _real_ issue is that you created a queue that gets transported over TCP on port 666. Another queue that gets transported on UDP with some random other protocol on port 1234 over the same ethernet chip doesn't make those "connected" in any way. The "host chip" simply has nothing to do with the block layer.

Similarly, you might have a SCSI chip that implements several "hosts" and it's all driven by DMA engines and a nice in-memory mailbox setup, but then it shares some of the control registers across all of them, so you might have to have a single driver lock. They are really totally independent, but they share one point of control - they don't really in any way share any "hostness", even if there's one physical connection at one point and there migth be soem shared locking to access the setup registers.

So the whole "host" notion is broken. It's a totally made-up abstraction, that SCSI people seem to have a damn hard time to just _let_go_ of.

There are _real_ points of connection. But "host" doesn't define them.

Linus

You are correct. The concept of "host" should NOT be exported to higher than PCI/SCSI* level.

* See below.

Not quite, the concept of a "host" is needed as it is the "host" which provides "SCSI ports" to the "SCSI domain".

Luben

LSI have chosen to put a SAT layer in the HBA firmware of their MPT fusion SAS cards. This makes both directly connected SATA disks (not strictly speaking SAS) and expander connected SATA disks (using the STP protocol) look like SCSI devices to the OS. Interestingly LSI have chosen not to implement the ATA PASSTHROUGH scsi commands at this time. That makes the job of smartmontools difficult.

More generally if you start putting SATA disks in a multi-initiator fabric (FC or SAS) then there are issues that need to be addressed. One issue is when two initiators try to access a SATA disk at around the same time. SAS has optional "affiliations" for protecting individual SATA commands but I'm unaware of anything like SCSI's persistent reservations. In the case of SAS affiliations, as far as I can see, some additional help is needed via SAS's management protocol (SMP). Command queueing (at the disk) is another issue.

A SAT layer in the OS (e.g. like libata) may not be able address these issues, but a SATL in (or aware of) the transport might.

So the point I'm making is the appropriate command set(s) for a hard disk not only depends on what the device itself talks, but also the way in which it is connected.

libata demonstrates that it is viable to use both the SCSI commands (for data) and SATA commands (for maintenance) on the same SATA device.

Doug Gilbert

:-)

Right

James/others already touched on that, and I agree it's a useful abstraction. It's something that we can use for other drivers right now, such as cciss.

I think we have a different opinion on what 'optional' is then - because things can certainly work just fine the way they current do. And it's faster, too.

But yes, you can make the code nicer for _some_ things with a ->queueone() type setup.

The timer is a given, we can't escape that. And the ->queueone() is basically hashed out above, no infrastructure changes needed. queuecommand_helper would be driver supplied, blk_queuecommand_helper() would be a block layer helper. With better names of course, I truly do suck at naming functions :-)

Jens Axboe

That ->queueone() maps the closest to what most hardware appears to want: "attempt to push request onto an async hardware queue".

It also enables additional entry points for returning 'device busy' or (in SCSI lingo) 'host busy'. The ability to signal and handle random transient conditions like that when hardware resources disappear is easy to overlook, but its really powerful.

Likewise :)

Jeff

'queue a request' is certainly how most hardware works. The fact is that ->queueone() doesn't enable _anything_ that you cannot already do. In fact it takes information _away_ from you, since you can no longer peek at the queue and see if there's one more there for you to issue. The only selling point for ->queueone() is that it makes it more logical to structure the code layout for handling the issue.

Which is why I never added this helper. From the block layer perspective, the driver can only really tell you three things:

- request issued, give me another one
- request not issued, transient error (meaning, call me with this request again in the future).
- request not issued, permanent error. kill this request.

A ->queueone() setup deals with returning the right issue error code, a ->queuemany() setup deals with calling the right function for the condition.

Jens Axboe

I think I just explained this. Let me try again.

They don't both have the same effect. What you're suggesting. calling done() with status so that, EH can turn around and recover the command, as in this thread and in this thread: http://marc.theaimsgroup.com/?l=linux-scsi&m=113833937421677&w=2,
_terminates_ the command nexus with the driver!

Secondly, this function, solves races between the transport telling you "I want to abort this command" and the command timing out. When the driver get transport notification that this command should be aborted, it doesn't need to care where and in what state with SCSI Core the command is. As long as it has nexus it can call this function and then the command would be recovered with ABORT TASK or another TMF as it pertinent to the transport.

Calling scsi_req_abort_cmd() _guaranees_ that this command would be recovered either way. This gives LLDD and interconnects guarantee that ABORT TASK would be called and then they can do recovert for the command/device.

Luben

The physical stack (HW) looks like:
Block ->
SCSI ->

    Transport ->
      Interconnect ->
        Device.

Not sure how you're going to achieve a SW abstraction whereby "->eh_strategy_handler and transport classes are block layer", when, the HW abstraction is different.

Luben

You objected to this already, when I first posted the patch, but I didn't have time to explain:
http://marc.theaimsgroup.com/?l=linux-scsi&m=113833937421677&w=2

Let me now explain, after your twice objecting in the same way to the patch:

You CANNOT call the done() function on a command (and return DID_FAILED) because this terminates the nexus established when queuecommand() got called, and then later try to "recover" the command as you're suggesting here and in the thread I included the link to above.

That is, when queuecommand() got called, the nexus is now with the driver, and/or the FW and/or the transport and/or the device, _depending_ on how far the driver/fw/transport/device got to executing the command.

The only way to _terminate_ the command nexus with the driver is by calling done(), which means,

"I (the driver) am completely done with this command, I'm not referencing it,
nor will you hear about it any more, and I'm giving it back to you, SCSI Core."

This is _basic_ SCSI knowledge.

For this reason, "scsi_req_abort_cmd()" needs to exist. It causes recovery of the command to begin, in a universal way, independent of if the timeout and/or the eh had been differently defined. As well as, scsi_req_abort_cmd(), can be called by either SCSI Core or a driver, from any context, without worrying where the command is.

Luben

The only current proposed user of this is libata-eh. If you look, you'll see the caller is from ata_qc_complete() which is only called *after* the nexus is terminated.

James

"*after* the nexus is terminated"?

Are you saying that libata calls "done()" and then calls "scsi_req_abort_cmd()" on the same command in succession?

I haven't looked in the code (which GIT tree is it?) but I don't believe that Tejun would do such a thing.

Luben

no, I'm saying ata_qc_complete() is an indication from the driver that the nexus is terminated.

It's in the libata-dev tree on www.kernel.org/git

James

But this is completely out of SCSI Core's domain.

The nexus between the driver and SCSI Core is terminated only when done() is called. done() is the antagonist of queuecommand().

Until done() is called, if the LLDD/IC/SCSI Core want to request that a command be aborted for whatever reason, it calls scsi_req_abort_cmd().

That is, until the nexus created by calling queuecommand(), is returned to SCSI Core (by the driver calling done()), if either the driver or SCSI Core want to request the command be aborted, they call scsi_req_abort_cmd().

What scsi_req_abort_cmd() accomplishes is that both parties of the nexus are "on the same page" wrt the state of the command, independently of which party called scsi_req_abort_cmd().

Luben

Erm, no, actually, I think if you look at SAM-3 (section 4.12) you'll find that a nexus can only be maintained with the consent of both the initiator and the target. The (transport dependent) signal of completion is an indicator of nexus termination (except in the linked command case).

James

Erm, no, actually, I'm not talking about a SAM nexus. I'm talking about the nexus established between SCSI Core and LLDD when SCSI Core called queuecommand(). This nexus exists until the LLDD calls done().

Now in between that time, if either one wants to abort the command then they call scsi_req_abort_cmd().

Most likely that would be the LLDD/IC.

Luben

So you want the mid layer to have a different view of the nexus from SAM? I really think we should be following SAM unless there's a very good reason not to.

So, in this case, what's the reason the mid-layer should preserve a nexus SAM says (and the LLD knows) has been terminated?

James

Apparently you have no detailed knowlege of the implementation of SCSI storage stacks, for example: driver->fw->interconnect->device.

You also fail to understand who the "I" and "T" could be in an I_T_x_x nexus at each stage of an implementation.

You also fail to understand that a SAM nexus is not necessarily established when queuecommand() is called.

I hope you're pretending that you don't understand, on purpose or as a joke. The alternative would simply be too scary.

Good luck!
Luben

Well, I'm sure I read about it in a comic book somewhere ...

I didn't say it was. The point, which you're ignoring, is that when the driver knows the SAM nexus is terminated, it should call ->done() ASAP to inform the mid-layer of this condition, so the mid-layer model of the device interactions follows the SAM interactions as closely as possible. The API you want to introduce and the implementation of it in libata lie to the mid-layer and pretend the nexus still exists when the driver knows it doesn't ... what I'm saying is that I see no reason to do that.

James

And so your correspondence agrees...

No, I'm not saying that. Not at all. You simply don't understand.

There is no way I can explain it to you unless I educate you. And I'm not willing to do that.

Stop trying to convince the world that SCSI Core has any resemblance to SAM. It doesn't. It is a far cry from it.

There is no "mid-layer model" and there is no SAM resemblance.

Luben

James,

The following patch seems to be missed in scsi-misc-2.6.

http://marc.theaimsgroup.com/?l=linux-scsi&m=114184745819730&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=114184745830216&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=114184745819007&w=2

Please advise...

-- james s

List:       linux-scsi
Subject:    [PATCH Repost 0/2] Block I/O while SG reset operation in progress
From:       James Smart
Date:       2006-03-08 19:49:47

Following the discussion with Mike Anderson on the original patch:
http://marc.theaimsgroup.com/?l=linux-scsi&m=114081263725777&w=2
and
http://marc.theaimsgroup.com/?l=linux-scsi&m=114080001928462&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=114080001916527&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=114080002012180&w=2

I've implemented the missing item of waking up those waiting for recovery to complete as well as restarting queues that may have been stopped.

In conversations with Mike, it's agreed that this patch is sufficient to block i/o while performing resets via SG - the original immediate goal. However, we've noted several other issues in this interface that will need to be addressed eventually.

-- james s