Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 29 May 2012 23:15:23 +0000 (UTC)
From:      Jim Harris <jimharris@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r236264 - stable/8/sys/dev/isci
Message-ID:  <201205292315.q4TNFNVn008988@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jimharris
Date: Tue May 29 23:15:23 2012
New Revision: 236264
URL: http://svn.freebsd.org/changeset/base/236264

Log:
  MFC r234106, r235751:
  
  Queue CCBs internally instead of using CAM_REQUEUE_REQ status.  This fixes
  problem where userspace apps such as smartctl fail due to CAM_REQUEUE_REQ
  status getting returned when tagged commands are outstanding when smartctl
  sends its I/O using the pass(4) interface.
  
  Sponsored by: Intel

Modified:
  stable/8/sys/dev/isci/isci.h
  stable/8/sys/dev/isci/isci_controller.c
  stable/8/sys/dev/isci/isci_interrupt.c
  stable/8/sys/dev/isci/isci_io_request.c
  stable/8/sys/dev/isci/isci_remote_device.c
Directory Properties:
  stable/8/sys/   (props changed)

Modified: stable/8/sys/dev/isci/isci.h
==============================================================================
--- stable/8/sys/dev/isci/isci.h	Tue May 29 23:10:10 2012	(r236263)
+++ stable/8/sys/dev/isci/isci.h	Tue May 29 23:15:23 2012	(r236264)
@@ -30,6 +30,9 @@
  * $FreeBSD$
  */
 
+#ifndef _ISCI_H
+#define _ISCI_H
+
 #include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/kernel.h>
@@ -86,6 +89,31 @@ struct ISCI_REMOTE_DEVICE {
 	BOOL				is_resetting;
 	uint32_t			frozen_lun_mask;
 	SCI_FAST_LIST_ELEMENT_T		pending_device_reset_element;
+
+	/*
+	 * This queue maintains CCBs that have been returned with
+	 *  SCI_IO_FAILURE_INVALID_STATE from the SCI layer.  These CCBs
+	 *  need to be retried, but we cannot return CAM_REQUEUE_REQ because
+	 *  this status gets passed all the way back up to users of the pass(4)
+	 *  interface and breaks things like smartctl.  So instead, we queue
+	 *  these CCBs internally.
+	 */
+	TAILQ_HEAD(,ccb_hdr)		queued_ccbs;
+
+	/*
+	 * Marker denoting this remote device needs its first queued ccb to
+	 *  be retried.
+	 */
+	BOOL				release_queued_ccb;
+
+	/*
+	 * Points to a CCB in the queue that is currently being processed by
+	 *  SCIL.  This allows us to keep in flight CCBs in the queue so as to
+	 *  maintain ordering (i.e. in case we retry an I/O and then find out
+	 *  it needs to be retried again - it just keeps its same place in the
+	 *  queue.
+	 */
+	union ccb *			queued_ccb_in_progress;
 };
 
 struct ISCI_DOMAIN {
@@ -125,6 +153,7 @@ struct ISCI_CONTROLLER
 	BOOL			has_been_scanned;
 	uint32_t		initial_discovery_mask;
 	BOOL			is_frozen;
+	BOOL			release_queued_ccbs;
 	uint8_t			*remote_device_memory;
 	struct ISCI_MEMORY	cached_controller_memory;
 	struct ISCI_MEMORY	uncached_controller_memory;
@@ -290,6 +319,8 @@ int isci_controller_attach_to_cam(struct
 
 void isci_controller_start(void *controller);
 
+void isci_controller_release_queued_ccbs(struct ISCI_CONTROLLER *controller);
+
 void isci_domain_construct(struct ISCI_DOMAIN *domain, uint32_t domain_index,
     struct ISCI_CONTROLLER *controller);
 
@@ -300,3 +331,5 @@ void isci_log_message(uint32_t	verbosity
     char *log_message, ...);
 
 extern uint32_t g_isci_debug_level;
+
+#endif /* #ifndef _ISCI_H */

Modified: stable/8/sys/dev/isci/isci_controller.c
==============================================================================
--- stable/8/sys/dev/isci/isci_controller.c	Tue May 29 23:10:10 2012	(r236263)
+++ stable/8/sys/dev/isci/isci_controller.c	Tue May 29 23:15:23 2012	(r236264)
@@ -201,6 +201,7 @@ void isci_controller_construct(struct IS
 
 	controller->is_started = FALSE;
 	controller->is_frozen = FALSE;
+	controller->release_queued_ccbs = FALSE;
 	controller->sim = NULL;
 	controller->initial_discovery_mask = 0;
 
@@ -430,6 +431,9 @@ int isci_controller_allocate_memory(stru
 		remote_device->frozen_lun_mask = 0;
 		sci_fast_list_element_init(remote_device,
 		    &remote_device->pending_device_reset_element);
+		TAILQ_INIT(&remote_device->queued_ccbs);
+		remote_device->release_queued_ccb = FALSE;
+		remote_device->queued_ccb_in_progress = NULL;
 
 		/*
 		 * For the first SCI_MAX_DOMAINS device objects, do not put
@@ -693,3 +697,47 @@ void isci_action(struct cam_sim *sim, un
 	}
 }
 
+/*
+ * Unfortunately, SCIL doesn't cleanly handle retry conditions.
+ *  CAM_REQUEUE_REQ works only when no one is using the pass(4) interface.  So
+ *  when SCIL denotes an I/O needs to be retried (typically because of mixing
+ *  tagged/non-tagged ATA commands, or running out of NCQ slots), we queue
+ *  these I/O internally.  Once SCIL completes an I/O to this device, or we get
+ *  a ready notification, we will retry the first I/O on the queue.
+ *  Unfortunately, SCIL also doesn't cleanly handle starting the new I/O within
+ *  the context of the completion handler, so we need to retry these I/O after
+ *  the completion handler is done executing.
+ */
+void
+isci_controller_release_queued_ccbs(struct ISCI_CONTROLLER *controller)
+{
+	struct ISCI_REMOTE_DEVICE *dev;
+	struct ccb_hdr *ccb_h;
+	int dev_idx;
+
+	KASSERT(mtx_owned(&controller->lock), ("controller lock not owned"));
+
+	controller->release_queued_ccbs = FALSE;
+	for (dev_idx = 0;
+	     dev_idx < SCI_MAX_REMOTE_DEVICES;
+	     dev_idx++) {
+
+		dev = controller->remote_device[dev_idx];
+		if (dev != NULL &&
+		    dev->release_queued_ccb == TRUE &&
+		    dev->queued_ccb_in_progress == NULL) {
+			dev->release_queued_ccb = FALSE;
+			ccb_h = TAILQ_FIRST(&dev->queued_ccbs);
+
+			if (ccb_h == NULL)
+				continue;
+
+			isci_log_message(1, "ISCI", "release %p %x\n", ccb_h,
+			    ((union ccb *)ccb_h)->csio.cdb_io.cdb_bytes[0]);
+
+			dev->queued_ccb_in_progress = (union ccb *)ccb_h;
+			isci_io_request_execute_scsi_io(
+			    (union ccb *)ccb_h, controller);
+		}
+	}
+}

Modified: stable/8/sys/dev/isci/isci_interrupt.c
==============================================================================
--- stable/8/sys/dev/isci/isci_interrupt.c	Tue May 29 23:10:10 2012	(r236263)
+++ stable/8/sys/dev/isci/isci_interrupt.c	Tue May 29 23:15:23 2012	(r236264)
@@ -177,6 +177,9 @@ isci_interrupt_legacy_handler(void *arg)
 			if (interrupt_handler(scic_controller_handle)) {
 				mtx_lock(&controller->lock);
 				completion_handler(scic_controller_handle);
+				if (controller->release_queued_ccbs == TRUE)
+					isci_controller_release_queued_ccbs(
+					    controller);
 				mtx_unlock(&controller->lock);
 			}
 		}
@@ -204,6 +207,13 @@ isci_interrupt_msix_handler(void *arg)
 	if (interrupt_handler(scic_controller_handle)) {
 		mtx_lock(&controller->lock);
 		completion_handler(scic_controller_handle);
+		/*
+		 * isci_controller_release_queued_ccb() is a relatively
+		 *  expensive routine, so we don't call it until the controller
+		 *  level flag is set to TRUE.
+		 */
+		if (controller->release_queued_ccbs == TRUE)
+			isci_controller_release_queued_ccbs(controller);
 		mtx_unlock(&controller->lock);
 	}
 }

Modified: stable/8/sys/dev/isci/isci_io_request.c
==============================================================================
--- stable/8/sys/dev/isci/isci_io_request.c	Tue May 29 23:10:10 2012	(r236263)
+++ stable/8/sys/dev/isci/isci_io_request.c	Tue May 29 23:15:23 2012	(r236264)
@@ -85,7 +85,9 @@ isci_io_request_complete(SCI_CONTROLLER_
 	struct ISCI_CONTROLLER *isci_controller;
 	struct ISCI_REMOTE_DEVICE *isci_remote_device;
 	union ccb *ccb;
+	BOOL complete_ccb;
 
+	complete_ccb = TRUE;
 	isci_controller = (struct ISCI_CONTROLLER *) sci_object_get_association(scif_controller);
 	isci_remote_device =
 		(struct ISCI_REMOTE_DEVICE *) sci_object_get_association(remote_device);
@@ -163,9 +165,7 @@ isci_io_request_complete(SCI_CONTROLLER_
 
 	case SCI_IO_FAILURE_INVALID_STATE:
 	case SCI_IO_FAILURE_INSUFFICIENT_RESOURCES:
-		ccb->ccb_h.status |= CAM_REQUEUE_REQ;
-		isci_remote_device_freeze_lun_queue(isci_remote_device,
-		    ccb->ccb_h.target_lun);
+		complete_ccb = FALSE;
 		break;
 
 	case SCI_IO_FAILURE_INVALID_REMOTE_DEVICE:
@@ -189,7 +189,7 @@ isci_io_request_complete(SCI_CONTROLLER_
 			    scif_remote_device_get_max_queue_depth(remote_device);
 			xpt_action((union ccb *)&ccb_relsim);
 			xpt_free_path(path);
-			ccb->ccb_h.status |= CAM_REQUEUE_REQ;
+			complete_ccb = FALSE;
 		}
 		break;
 
@@ -209,17 +209,6 @@ isci_io_request_complete(SCI_CONTROLLER_
 		break;
 	}
 
-	if (ccb->ccb_h.status != CAM_REQ_CMP) {
-		/* ccb will be completed with some type of non-success
-		 *  status.  So temporarily freeze the queue until the
-		 *  upper layers can act on the status.  The CAM_DEV_QFRZN
-		 *  flag will then release the queue after the status is
-		 *  acted upon.
-		 */
-		ccb->ccb_h.status |= CAM_DEV_QFRZN;
-		xpt_freeze_devq(ccb->ccb_h.path, 1);
-	}
-
 	callout_stop(&isci_request->parent.timer);
 	bus_dmamap_sync(isci_request->parent.dma_tag,
 	    isci_request->parent.dma_map,
@@ -228,20 +217,82 @@ isci_io_request_complete(SCI_CONTROLLER_
 	bus_dmamap_unload(isci_request->parent.dma_tag,
 	    isci_request->parent.dma_map);
 
-	if (isci_remote_device->frozen_lun_mask != 0 &&
-	    !(ccb->ccb_h.status & CAM_REQUEUE_REQ))
-		isci_remote_device_release_device_queue(isci_remote_device);
-
-	xpt_done(ccb);
 	isci_request->ccb = NULL;
 
-	if (isci_controller->is_frozen == TRUE) {
-		isci_controller->is_frozen = FALSE;
-		xpt_release_simq(isci_controller->sim, TRUE);
-	}
-
 	sci_pool_put(isci_controller->request_pool,
 	    (struct ISCI_REQUEST *)isci_request);
+
+	if (complete_ccb) {
+		if ((ccb->ccb_h.status & CAM_STATUS_MASK) != CAM_REQ_CMP) {
+			/* ccb will be completed with some type of non-success
+			 *  status.  So temporarily freeze the queue until the
+			 *  upper layers can act on the status.  The
+			 *  CAM_DEV_QFRZN flag will then release the queue
+			 *  after the status is acted upon.
+			 */
+			ccb->ccb_h.status |= CAM_DEV_QFRZN;
+			xpt_freeze_devq(ccb->ccb_h.path, 1);
+		}
+
+		if (ccb->ccb_h.status & CAM_SIM_QUEUED) {
+
+			KASSERT(ccb == isci_remote_device->queued_ccb_in_progress,
+			    ("multiple internally queued ccbs in flight"));
+
+			TAILQ_REMOVE(&isci_remote_device->queued_ccbs,
+			    &ccb->ccb_h, sim_links.tqe);
+			ccb->ccb_h.status &= ~CAM_SIM_QUEUED;
+
+			/*
+			 * This CCB that was in the queue was completed, so
+			 *  set the in_progress pointer to NULL denoting that
+			 *  we can retry another CCB from the queue.  We only
+			 *  allow one CCB at a time from the queue to be
+			 *  in progress so that we can effectively maintain
+			 *  ordering.
+			 */
+			isci_remote_device->queued_ccb_in_progress = NULL;
+		}
+
+		if (isci_remote_device->frozen_lun_mask != 0) {
+			isci_remote_device_release_device_queue(isci_remote_device);
+		}
+
+		xpt_done(ccb);
+
+		if (isci_controller->is_frozen == TRUE) {
+			isci_controller->is_frozen = FALSE;
+			xpt_release_simq(isci_controller->sim, TRUE);
+		}
+	} else {
+		isci_remote_device_freeze_lun_queue(isci_remote_device,
+		    ccb->ccb_h.target_lun);
+
+		if (ccb->ccb_h.status & CAM_SIM_QUEUED) {
+
+			KASSERT(ccb == isci_remote_device->queued_ccb_in_progress,
+			    ("multiple internally queued ccbs in flight"));
+
+			/*
+			 *  Do nothing, CCB is already on the device's queue.
+			 *   We leave it on the queue, to be retried again
+			 *   next time a CCB on this device completes, or we
+			 *   get a ready notification for this device.
+			 */
+			isci_log_message(1, "ISCI", "already queued %p %x\n",
+			    ccb, ccb->csio.cdb_io.cdb_bytes[0]);
+
+			isci_remote_device->queued_ccb_in_progress = NULL;
+
+		} else {
+			isci_log_message(1, "ISCI", "queue %p %x\n", ccb,
+			    ccb->csio.cdb_io.cdb_bytes[0]);
+			ccb->ccb_h.status |= CAM_SIM_QUEUED;
+
+			TAILQ_INSERT_TAIL(&isci_remote_device->queued_ccbs,
+			    &ccb->ccb_h, sim_links.tqe);
+		}
+	}
 }
 
 /**

Modified: stable/8/sys/dev/isci/isci_remote_device.c
==============================================================================
--- stable/8/sys/dev/isci/isci_remote_device.c	Tue May 29 23:10:10 2012	(r236263)
+++ stable/8/sys/dev/isci/isci_remote_device.c	Tue May 29 23:15:23 2012	(r236264)
@@ -289,9 +289,26 @@ isci_remote_device_release_lun_queue(str
 
 void
 isci_remote_device_release_device_queue(
-    struct ISCI_REMOTE_DEVICE *remote_device)
+    struct ISCI_REMOTE_DEVICE *device)
 {
-	lun_id_t lun;
-	for (lun = 0; lun < ISCI_MAX_LUN; lun++)
-		isci_remote_device_release_lun_queue(remote_device, lun);
+	if (TAILQ_EMPTY(&device->queued_ccbs)) {
+		lun_id_t lun;
+
+		for (lun = 0; lun < ISCI_MAX_LUN; lun++)
+			isci_remote_device_release_lun_queue(device, lun);
+	} else {
+		/*
+		 * We cannot unfreeze the devq, because there are still
+		 *  CCBs in our internal queue that need to be processed
+		 *  first.  Mark this device, and the controller, so that
+		 *  the first CCB in this device's internal queue will be
+		 *  resubmitted after the current completion context
+		 *  unwinds.
+		 */
+		device->release_queued_ccb = TRUE;
+		device->domain->controller->release_queued_ccbs = TRUE;
+
+		isci_log_message(1, "ISCI", "schedule %p for release\n",
+		    device);
+	}
 }



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205292315.q4TNFNVn008988>