[SCSI] sd/scsi_lib simplify sd_rw_intr and scsi_io_completion

This patch simplifies "good_bytes" computation in sd_rw_intr().
sd: "good_bytes" computation is always done in terms of the resolution
of the device's medium, since after that it is the number of good bytes
we pass around and other layers/contexts (as opposed ot sd) can translate
that to their own resolution (block layer:512).  It also makes
scsi_io_completion() processing more straightforward, eliminating the
3rd argument to the function.

It also fixes a couple of bugs like not checking return value,
using "break" instead of "return;", etc.

I've been running with this patch for some time now on a
test (do-it-all) system.

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>

drivers/scsi/scsi_lib.c

@@ -855,8 +855,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
  *		b) We can just use scsi_requeue_command() here.  This would
  *		   be used if we just wanted to retry, for example.
  */
-void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes,
+void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
-			unsigned int block_bytes)
 {
 	int result = cmd->result;
 	int this_count = cmd->bufflen;
@@ -921,87 +920,72 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes,
 	 * Next deal with any sectors which we were able to correctly
 	 * handle.
 	 */
-	if (good_bytes >= 0) {
+	if (good_bytes > 0) {
-		SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, %d bytes done.\n",
+		SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, "
+					      "%d bytes done.\n",
 					      req->nr_sectors, good_bytes));
 		SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n", cmd->use_sg));
 
 		if (clear_errors)
 			req->errors = 0;
-		/*
-		 * If multiple sectors are requested in one buffer, then
-		 * they will have been finished off by the first command.
-		 * If not, then we have a multi-buffer command.
-		 *
-		 * If block_bytes != 0, it means we had a medium error
-		 * of some sort, and that we want to mark some number of
-		 * sectors as not uptodate.  Thus we want to inhibit
-		 * requeueing right here - we will requeue down below
-		 * when we handle the bad sectors.
-		 */
 
-		/*
+		/* A number of bytes were successfully read.  If there
-		 * If the command completed without error, then either
+		 * is leftovers and there is some kind of error
-		 * finish off the rest of the command, or start a new one.
+		 * (result != 0), retry the rest.
 		 */
-		if (scsi_end_request(cmd, 1, good_bytes, result == 0) == NULL)
+		if (scsi_end_request(cmd, 1, good_bytes, !!result) == NULL)
 			return;
 	}
-	/*
+
-	 * Now, if we were good little boys and girls, Santa left us a request
+	/* good_bytes = 0, or (inclusive) there were leftovers and
-	 * sense buffer.  We can extract information from this, so we
+	 * result = 0, so scsi_end_request couldn't retry.
-	 * can choose a block to remap, etc.
 	 */
 	if (sense_valid && !sense_deferred) {
 		switch (sshdr.sense_key) {
 		case UNIT_ATTENTION:
 			if (cmd->device->removable) {
-				/* detected disc change.  set a bit 
+				/* Detected disc change.  Set a bit
 				 * and quietly refuse further access.
 				 */
 				cmd->device->changed = 1;
-				scsi_end_request(cmd, 0,
+				scsi_end_request(cmd, 0, this_count, 1);
-						this_count, 1);
 				return;
 			} else {
-				/*
+				/* Must have been a power glitch, or a
-				* Must have been a power glitch, or a
+				 * bus reset.  Could not have been a
-				* bus reset.  Could not have been a
+				 * media change, so we just retry the
-				* media change, so we just retry the
+				 * request and see what happens.
-				* request and see what happens.  
+				 */
-				*/
 				scsi_requeue_command(q, cmd);
 				return;
 			}
 			break;
 		case ILLEGAL_REQUEST:
-			/*
+			/* If we had an ILLEGAL REQUEST returned, then
-		 	* If we had an ILLEGAL REQUEST returned, then we may
+			 * we may have performed an unsupported
-		 	* have performed an unsupported command.  The only
+			 * command.  The only thing this should be
-		 	* thing this should be would be a ten byte read where
+			 * would be a ten byte read where only a six
-			* only a six byte read was supported.  Also, on a
+			 * byte read was supported.  Also, on a system
-			* system where READ CAPACITY failed, we may have read
+			 * where READ CAPACITY failed, we may have
-			* past the end of the disk.
+			 * read past the end of the disk.
-		 	*/
+			 */
 			if ((cmd->device->use_10_for_rw &&
 			    sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
 			    (cmd->cmnd[0] == READ_10 ||
 			     cmd->cmnd[0] == WRITE_10)) {
 				cmd->device->use_10_for_rw = 0;
-				/*
+				/* This will cause a retry with a
-				 * This will cause a retry with a 6-byte
+				 * 6-byte command.
-				 * command.
 				 */
 				scsi_requeue_command(q, cmd);
-				result = 0;
+				return;
 			} else {
 				scsi_end_request(cmd, 0, this_count, 1);
 				return;
 			}
 			break;
 		case NOT_READY:
-			/*
+			/* If the device is in the process of becoming
-			 * If the device is in the process of becoming
 			 * ready, or has a temporary blockage, retry.
 			 */
 			if (sshdr.asc == 0x04) {
@@ -1021,7 +1005,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes,
 			}
 			if (!(req->flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
-					   "Device not ready: ");
+					    "Device not ready: ");
 				scsi_print_sense_hdr("", &sshdr);
 			}
 			scsi_end_request(cmd, 0, this_count, 1);
@@ -1029,21 +1013,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes,
 		case VOLUME_OVERFLOW:
 			if (!(req->flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
-					   "Volume overflow, CDB: ");
+					    "Volume overflow, CDB: ");
 				__scsi_print_command(cmd->data_cmnd);
 				scsi_print_sense("", cmd);
 			}
-			scsi_end_request(cmd, 0, block_bytes, 1);
+			/* See SSC3rXX or current. */
+			scsi_end_request(cmd, 0, this_count, 1);
 			return;
 		default:
 			break;
 		}
-	}			/* driver byte != 0 */
+	}
 	if (host_byte(result) == DID_RESET) {
-		/*
+		/* Third party bus reset or reset for error recovery
-		 * Third party bus reset or reset for error
+		 * reasons.  Just retry the request and see what
-		 * recovery reasons.  Just retry the request
+		 * happens.
-		 * and see what happens.  
 		 */
 		scsi_requeue_command(q, cmd);
 		return;
@@ -1051,21 +1035,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes,
 	if (result) {
 		if (!(req->flags & REQ_QUIET)) {
 			scmd_printk(KERN_INFO, cmd,
-				   "SCSI error: return code = 0x%x\n", result);
+				    "SCSI error: return code = 0x%08x\n",
-
+				    result);
 			if (driver_byte(result) & DRIVER_SENSE)
 				scsi_print_sense("", cmd);
 		}
-		/*
-		 * Mark a single buffer as not uptodate.  Queue the remainder.
-		 * We sometimes get this cruft in the event that a medium error
-		 * isn't properly reported.
-		 */
-		block_bytes = req->hard_cur_sectors << 9;
-		if (!block_bytes)
-			block_bytes = req->data_len;
-		scsi_end_request(cmd, 0, block_bytes, 1);
 	}
+	scsi_end_request(cmd, 0, this_count, !result);
 }
 EXPORT_SYMBOL(scsi_io_completion);
 
@@ -1169,7 +1145,7 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
 	 * successfully. Since this is a REQ_BLOCK_PC command the
 	 * caller should check the request's errors value
 	 */
-	scsi_io_completion(cmd, cmd->bufflen, 0);
+	scsi_io_completion(cmd, cmd->bufflen);
 }
 
 static void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd)

drivers/scsi/sd.c

@@ -891,11 +891,10 @@ static struct block_device_operations sd_fops = {
 static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+ 	unsigned int xfer_size = SCpnt->request_bufflen;
-	int good_bytes = (result == 0 ? this_count : 0);
+ 	unsigned int good_bytes = result ? 0 : xfer_size;
-	sector_t block_sectors = 1;
+ 	u64 start_lba = SCpnt->request->sector;
-	u64 first_err_block;
+ 	u64 bad_lba;
-	sector_t error_sector;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;
@@ -906,7 +905,6 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 		if (sense_valid)
 			sense_deferred = scsi_sense_is_deferred(&sshdr);
 	}
-
 #ifdef CONFIG_SCSI_LOGGING
 	SCSI_LOG_HLCOMPLETE(1, printk("sd_rw_intr: %s: res=0x%x\n", 
 				SCpnt->request->rq_disk->disk_name, result));
@@ -916,89 +914,72 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 				sshdr.sense_key, sshdr.asc, sshdr.ascq));
 	}
 #endif
-	/*
+	if (driver_byte(result) != DRIVER_SENSE &&
-	   Handle MEDIUM ERRORs that indicate partial success.  Since this is a
+	    (!sense_valid || sense_deferred))
-	   relatively rare error condition, no care is taken to avoid
+		goto out;
-	   unnecessary additional work such as memcpy's that could be avoided.
-	 */
-	if (driver_byte(result) != 0 &&
-		 sense_valid && !sense_deferred) {
-		switch (sshdr.sense_key) {
-		case MEDIUM_ERROR:
-			if (!blk_fs_request(SCpnt->request))
-				break;
-			info_valid = scsi_get_sense_info_fld(
-				SCpnt->sense_buffer, SCSI_SENSE_BUFFERSIZE,
-				&first_err_block);
-			/*
-			 * May want to warn and skip if following cast results
-			 * in actual truncation (if sector_t < 64 bits)
-			 */
-			error_sector = (sector_t)first_err_block;
-			if (SCpnt->request->bio != NULL)
-				block_sectors = bio_sectors(SCpnt->request->bio);
-			switch (SCpnt->device->sector_size) {
-			case 1024:
-				error_sector <<= 1;
-				if (block_sectors < 2)
-					block_sectors = 2;
-				break;
-			case 2048:
-				error_sector <<= 2;
-				if (block_sectors < 4)
-					block_sectors = 4;
-				break;
-			case 4096:
-				error_sector <<=3;
-				if (block_sectors < 8)
-					block_sectors = 8;
-				break;
-			case 256:
-				error_sector >>= 1;
-				break;
-			default:
-				break;
-			}
 
-			error_sector &= ~(block_sectors - 1);
+	switch (sshdr.sense_key) {
-			good_bytes = (error_sector - SCpnt->request->sector) << 9;
+	case HARDWARE_ERROR:
-			if (good_bytes < 0 || good_bytes >= this_count)
+	case MEDIUM_ERROR:
-				good_bytes = 0;
+		if (!blk_fs_request(SCpnt->request))
+			goto out;
+		info_valid = scsi_get_sense_info_fld(SCpnt->sense_buffer,
+						     SCSI_SENSE_BUFFERSIZE,
+						     &bad_lba);
+		if (!info_valid)
+			goto out;
+		if (xfer_size <= SCpnt->device->sector_size)
+			goto out;
+		switch (SCpnt->device->sector_size) {
+		case 256:
+			start_lba <<= 1;
 			break;
-
+		case 512:
-		case RECOVERED_ERROR: /* an error occurred, but it recovered */
-		case NO_SENSE: /* LLDD got sense data */
-			/*
-			 * Inform the user, but make sure that it's not treated
-			 * as a hard error.
-			 */
-			scsi_print_sense("sd", SCpnt);
-			SCpnt->result = 0;
-			memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
-			good_bytes = this_count;
 			break;
-
+		case 1024:
-		case ILLEGAL_REQUEST:
+			start_lba >>= 1;
-			if (SCpnt->device->use_10_for_rw &&
+			break;
-			    (SCpnt->cmnd[0] == READ_10 ||
+		case 2048:
-			     SCpnt->cmnd[0] == WRITE_10))
+			start_lba >>= 2;
-				SCpnt->device->use_10_for_rw = 0;
+			break;
-			if (SCpnt->device->use_10_for_ms &&
+		case 4096:
-			    (SCpnt->cmnd[0] == MODE_SENSE_10 ||
+			start_lba >>= 3;
-			     SCpnt->cmnd[0] == MODE_SELECT_10))
-				SCpnt->device->use_10_for_ms = 0;
 			break;
-
 		default:
+			/* Print something here with limiting frequency. */
+			goto out;
 			break;
 		}
+		/* This computation should always be done in terms of
+		 * the resolution of the device's medium.
+		 */
+		good_bytes = (bad_lba - start_lba)*SCpnt->device->sector_size;
+		break;
+	case RECOVERED_ERROR:
+	case NO_SENSE:
+		/* Inform the user, but make sure that it's not treated
+		 * as a hard error.
+		 */
+		scsi_print_sense("sd", SCpnt);
+		SCpnt->result = 0;
+		memset(SCpnt->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		good_bytes = xfer_size;
+		break;
+	case ILLEGAL_REQUEST:
+		if (SCpnt->device->use_10_for_rw &&
+		    (SCpnt->cmnd[0] == READ_10 ||
+		     SCpnt->cmnd[0] == WRITE_10))
+			SCpnt->device->use_10_for_rw = 0;
+		if (SCpnt->device->use_10_for_ms &&
+		    (SCpnt->cmnd[0] == MODE_SENSE_10 ||
+		     SCpnt->cmnd[0] == MODE_SELECT_10))
+			SCpnt->device->use_10_for_ms = 0;
+		break;
+	default:
+		break;
 	}
-	/*
+ out:
-	 * This calls the generic completion function, now that we know
+	scsi_io_completion(SCpnt, good_bytes);
-	 * how many actual sectors finished, and how many sectors we need
-	 * to say have failed.
-	 */
-	scsi_io_completion(SCpnt, good_bytes, block_sectors << 9);
 }
 
 static int media_not_present(struct scsi_disk *sdkp,

drivers/scsi/sr.c

@@ -292,7 +292,7 @@ static void rw_intr(struct scsi_cmnd * SCpnt)
 	 * how many actual sectors finished, and how many sectors we need
 	 * to say have failed.
 	 */
-	scsi_io_completion(SCpnt, good_bytes, block_sectors << 9);
+	scsi_io_completion(SCpnt, good_bytes);
 }
 
 static int sr_init_command(struct scsi_cmnd * SCpnt)

include/scsi/scsi_cmnd.h

@@ -143,7 +143,7 @@ struct scsi_cmnd {
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
 extern void scsi_put_command(struct scsi_cmnd *);
-extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
+extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 extern void scsi_req_abort_cmd(struct scsi_cmnd *cmd);