Skip to contentSkip to conversation

Fix error handler offline behaviour

PATCH New James Bottomley 1 participant 1 message Mar 16, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix error handler offline behaviour

No-one seems to have noticed, but there's a bug in our offline handling which can cause the eh to loop forever when it tries to offline a device (basically offline I/O rejections are done in prep, which doesn't work for already prepped commands).

The attached fixes this, and also sweeps offline up into a fairly fully fledged scsi state model.

With this applied I can now offline my root device under load without causing a SCSI hang (ext2 BUG()s rather unhappily but at least that's not a SCSI problem).

James

===== drivers/scsi/dpt_i2o.c 1.35 vs edited =====

drivers/scsi/dpt_i2o.c +4 -4
@@ -592,7 +592,7 @@
 				unit = d->pI2o_dev->lct_data.tid;
 				len += sprintf(buffer+len, "\tTID=%d, (Channel=%d, Target=%d, Lun=%d)  (%s)\n\n",
 					       unit, (int)d->scsi_channel, (int)d->scsi_id, (int)d->scsi_lun,
-					       d->pScsi_dev->online? "online":"offline"); 
+					       scsi_device_online(d->pScsi_dev)? "online":"offline"); 
 				pos = begin + len;
 
 				/* CHECKPOINT */
@@ -2436,11 +2436,11 @@
 			// We found an old device - check it
 			while(pDev) {
 				if(pDev->scsi_lun == scsi_lun) {
-					if(pDev->pScsi_dev->online == FALSE) {
+					if(!scsi_device_online(pDev->pScsi_dev)) {
 						printk(KERN_WARNING"%s: Setting device (%d,%d,%d) back online\n",
 								pHba->name,bus_no,scsi_id,scsi_lun);
 						if (pDev->pScsi_dev) {
-							pDev->pScsi_dev->online = TRUE;
+							scsi_device_set_state(pDev->pScsi_dev, SDEV_RUNNING);
 						}
 					}
 					d = pDev->pI2o_dev;
@@ -2471,7 +2471,7 @@
 			pDev->state = DPTI_DEV_OFFLINE;
 			printk(KERN_WARNING"%s: Device (%d,%d,%d) offline\n",pHba->name,pDev->scsi_channel,pDev->scsi_id,pDev->scsi_lun);
 			if (pDev->pScsi_dev) {
-				pDev->pScsi_dev->online = FALSE;
+				scsi_device_set_state(pDev->pScsi_dev, SDEV_OFFLINE);
 			}
 		}
 	}

===== drivers/scsi/scsi_error.c 1.71 vs edited =====

drivers/scsi/scsi_error.c +18 -11
@@ -186,12 +186,16 @@
  **/
 int scsi_block_when_processing_errors(struct scsi_device *sdev)
 {
+	int online;
+
 	wait_event(sdev->host->host_wait, (!test_bit(SHOST_RECOVERY, &sdev->host->shost_state)));
 
+	online = scsi_device_online(sdev);
+
 	SCSI_LOG_ERROR_RECOVERY(5, printk("%s: rtn: %d\n", __FUNCTION__,
-					  sdev->online));
+					  online));
 
-	return sdev->online;
+	return online;
 }
 
 #ifdef CONFIG_SCSI_LOGGING
@@ -792,7 +796,8 @@
 		rtn = scsi_try_to_abort_cmd(scmd);
 		if (rtn == SUCCESS) {
 			scsi_eh_eflags_clr(scmd,  SCSI_EH_CANCEL_CMD);
-			if (!scmd->device->online || !scsi_eh_tur(scmd)) {
+			if (!scsi_device_online(scmd->device) ||
+			    !scsi_eh_tur(scmd)) {
 				scsi_eh_finish_cmd(scmd, done_q);
 			}
 				
@@ -920,7 +925,8 @@
 						  " 0x%p\n", current->comm, sdev));
 
 		if (!scsi_eh_try_stu(stu_scmd)) {
-			if (!sdev->online || !scsi_eh_tur(stu_scmd)) {
+			if (!scsi_device_online(sdev) ||
+			    !scsi_eh_tur(stu_scmd)) {
 				list_for_each_safe(lh, lh_sf, work_q) {
 					scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
 					if (scmd->device == sdev)
@@ -974,7 +980,8 @@
 						  sdev));
 		rtn = scsi_try_bus_device_reset(bdr_scmd);
 		if (rtn == SUCCESS) {
-			if (!sdev->online || !scsi_eh_tur(bdr_scmd)) {
+			if (!scsi_device_online(sdev) ||
+			    !scsi_eh_tur(bdr_scmd)) {
 				list_for_each_safe(lh, lh_sf,
 						   work_q) {
 					scmd = list_entry(lh, struct
@@ -1105,7 +1112,7 @@
 				scmd = list_entry(lh, struct scsi_cmnd,
 						  eh_entry);
 				if (channel == scmd->device->channel)
-					if (!scmd->device->online ||
+					if (!scsi_device_online(scmd->device) ||
 					    !scsi_eh_tur(scmd))
 						scsi_eh_finish_cmd(scmd,
 								   done_q);
@@ -1143,7 +1150,7 @@
 		if (rtn == SUCCESS) {
 			list_for_each_safe(lh, lh_sf, work_q) {
 				scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
-				if (!scmd->device->online ||
+				if (!scsi_device_online(scmd->device) ||
 				    (!scsi_eh_try_stu(scmd) && !scsi_eh_tur(scmd)) ||
 				    !scsi_eh_tur(scmd))
 					scsi_eh_finish_cmd(scmd, done_q);
@@ -1178,7 +1185,7 @@
 				scmd->device->channel,
 				scmd->device->id,
 				scmd->device->lun);
-		scmd->device->online = FALSE;
+		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
 		if (scsi_eh_eflags_chk(scmd, SCSI_EH_CANCEL_CMD)) {
 			/*
 			 * FIXME: Handle lost cmds.
@@ -1248,7 +1255,7 @@
 	 * if the device is offline, then we clearly just pass the result back
 	 * up to the top level.
 	 */
-	if (!scmd->device->online) {
+	if (!scsi_device_online(scmd->device)) {
 		SCSI_LOG_ERROR_RECOVERY(5, printk("%s: device offline - report"
 						  " as SUCCESS\n",
 						  __FUNCTION__));
@@ -1480,7 +1487,7 @@
 	 * is no point trying to lock the door of an off-line device.
 	 */
 	shost_for_each_device(sdev, shost) {
-		if (sdev->online && sdev->locked)
+		if (scsi_device_online(sdev) && sdev->locked)
 			scsi_eh_lock_door(sdev);
 	}
 
@@ -1535,7 +1542,7 @@
 	list_for_each_safe(lh, lh_sf, done_q) {
 		scmd = list_entry(lh, struct scsi_cmnd, eh_entry);
 		list_del_init(lh);
-		if (scmd->device->online &&
+		if (scsi_device_online(scmd->device) &&
 		    	(++scmd->retries < scmd->allowed)) {
 			SCSI_LOG_ERROR_RECOVERY(3, printk("%s: flush"
 							  " retry cmd: %p\n",

===== drivers/scsi/scsi_lib.c 1.123 vs edited =====

drivers/scsi/scsi_lib.c +86 -30

170 lines not on this page · raw message

===== drivers/scsi/scsi_scan.c 1.117 vs edited =====

drivers/scsi/scsi_scan.c +1 -2
@@ -205,7 +205,6 @@
 	sdev->id = id;
 	sdev->lun = lun;
 	sdev->channel = channel;
-	sdev->online = TRUE;
 	sdev->sdev_state = SDEV_CREATED;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
@@ -552,7 +551,7 @@
 	if (((inq_result[0] >> 5) & 7) == 1) {
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: peripheral"
 				" qualifier of 1, device offlined\n"));
-		sdev->online = FALSE;
+		scsi_device_set_state(sdev, SDEV_OFFLINE);
 	}
 
 	sdev->removable = (0x80 & inq_result[1]) >> 7;

===== drivers/scsi/scsi_sysfs.c 1.43 vs edited =====

drivers/scsi/scsi_sysfs.c +70 -4

118 lines not on this page · raw message

===== drivers/scsi/sd.c 1.142 vs edited =====

drivers/scsi/sd.c +5 -5
@@ -245,7 +245,7 @@
 	SCSI_LOG_HLQUEUE(1, printk("sd_init_command: disk=%s, block=%llu, "
 			    "count=%d\n", disk->disk_name, (unsigned long long)block, this_count));
 
-	if (!sdp || !sdp->online ||
+	if (!sdp || !scsi_device_online(sdp) ||
  	    block + SCpnt->request->nr_sectors > get_capacity(disk)) {
 		SCSI_LOG_HLQUEUE(2, printk("Finishing %ld sectors\n", 
 				 SCpnt->request->nr_sectors));
@@ -452,7 +452,7 @@
 	 * open actually succeeded.
 	 */
 	retval = -ENXIO;
-	if (!sdev->online)
+	if (!scsi_device_online(sdev))
 		goto error_out;
 
 	if (!sdkp->openers++ && sdev->removable) {
@@ -619,7 +619,7 @@
 	 * can deal with it then.  It is only because of unrecoverable errors
 	 * that we would ever take a device offline in the first place.
 	 */
-	if (!sdp->online)
+	if (!scsi_device_online(sdp))
 		goto not_present;
 
 	/*
@@ -1254,7 +1254,7 @@
 	 * If the device is offline, don't try and read capacity or any
 	 * of the other niceties.
 	 */
-	if (!sdp->online)
+	if (!scsi_device_online(sdp))
 		goto out;
 
 	sreq = scsi_allocate_request(sdp, GFP_KERNEL);
@@ -1474,7 +1474,7 @@
 	if (!sdkp)
                return;         /* this can happen */
 
-	if (!sdp->online || !sdkp->WCE)
+	if (!scsi_device_online(sdp) || !sdkp->WCE)
 		return;
 
 	printk(KERN_NOTICE "Synchronizing SCSI cache for disk %s: ",

===== drivers/scsi/sg.c 1.84 vs edited =====

drivers/scsi/sg.c +1 -1
@@ -2920,7 +2920,7 @@
 			      1,
 			      (int) scsidp->queue_depth,
 			      (int) scsidp->device_busy,
-			      (int) scsidp->online);
+			      (int) scsi_device_online(scsidp));
 	else
 		seq_printf(s, "-1\t-1\t-1\t-1\t-1\t-1\t-1\t-1\t-1\n");
 	return 0;

===== drivers/scsi/sr.c 1.100 vs edited =====

drivers/scsi/sr.c +1 -1
@@ -267,7 +267,7 @@
 	SCSI_LOG_HLQUEUE(1, printk("Doing sr request, dev = %s, block = %d\n",
 				cd->disk->disk_name, block));
 
-	if (!cd->device || !cd->device->online) {
+	if (!cd->device || !scsi_device_online(cd->device)) {
 		SCSI_LOG_HLQUEUE(2, printk("Finishing %ld sectors\n",
 					SCpnt->request->nr_sectors));
 		SCSI_LOG_HLQUEUE(2, printk("Retry with 0x%p\n", SCpnt));

===== include/scsi/scsi_device.h 1.14 vs edited =====

include/scsi/scsi_device.h +10 -3
@@ -12,7 +12,9 @@
 
 
 /*
- * sdev state
+ * sdev state: If you alter this, you also need to alter scsi_sysfs.c
+ * (for the ascii descriptions) and the state model enforcer:
+ * scsi_lib:scsi_device_set_state().
  */
 enum scsi_device_state {
 	SDEV_CREATED = 1,	/* device created but not added to sysfs
@@ -26,6 +28,8 @@
 	SDEV_QUIESCE,		/* Device quiescent.  No block commands
 				 * will be accepted, only specials (which
 				 * originate in the mid-layer) */
+	SDEV_OFFLINE,		/* Device offlined (by error handling or
+				 * user request */
 };
 
 struct scsi_device {
@@ -67,8 +71,6 @@
 	unsigned char current_tag;	/* current tag */
 	struct scsi_target      *sdev_target;   /* used only for single_lun */
 
-	unsigned online:1;
-
 	unsigned writeable:1;
 	unsigned removable:1;
 	unsigned changed:1;	/* Data invalid due to media change */
@@ -177,4 +179,9 @@
 				 enum scsi_device_state state);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
+extern const char *scsi_device_state_name(enum scsi_device_state);
+static int inline scsi_device_online(struct scsi_device *sdev)
+{
+	return sdev->sdev_state != SDEV_OFFLINE;
+}
 #endif /* _SCSI_SCSI_DEVICE_H */