Skip to contentSkip to conversation

Fix eh_abort race condition

PATCH In review Brian King 2 participants 3 messages Feb 25, 2004 SCSI

lore
Share & tools
lore

Brian King

author
#0 lore raw

PATCH 1/1 Fix eh_abort race condition

The following patch fixes the race condition discussed here:

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

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center

The following patch fixes a race condition in abort processing. With this patch, the mid-layer can now guarantee to LLDs that it will only call eh_abort for ops which returned 0 in queuecommand and have not yet had their ->done function called.

---

drivers/scsi/scsi_error.c renamed from drivers/scsi/scsi_error.c~eh_abort +10 -9
diff -puN drivers/scsi/scsi_error.c~eh_abort drivers/scsi/scsi_error.c
@@ -471,8 +471,9 @@ static int scsi_send_eh_cmnd(struct scsi
 		 * we should treat them differently anyways.
 		 */
 		spin_lock_irqsave(scmd->device->host->host_lock, flags);
-		if (scmd->device->host->hostt->eh_abort_handler)
-			scmd->device->host->hostt->eh_abort_handler(scmd);
+		if (scmd->serial_number != 0)
+			if (scmd->device->host->hostt->eh_abort_handler)
+				scmd->device->host->hostt->eh_abort_handler(scmd);
 		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 			
 		scmd->request->rq_status = RQ_SCSI_DONE;

@@ -687,17 +688,17 @@ static int scsi_try_to_abort_cmd(struct 
 	if (!scmd->device->host->hostt->eh_abort_handler)
 		return rtn;
 
+	spin_lock_irqsave(scmd->device->host->host_lock, flags);
 	/*
 	 * scsi_done was called just after the command timed out and before
 	 * we had a chance to process it. (db)
 	 */
-	if (scmd->serial_number == 0)
-		return SUCCESS;
-
-	scmd->owner = SCSI_OWNER_LOWLEVEL;
-
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
-	rtn = scmd->device->host->hostt->eh_abort_handler(scmd);
+	if (scmd->serial_number == 0) {
+		rtn = SUCCESS;
+	} else {
+		scmd->owner = SCSI_OWNER_LOWLEVEL;
+		rtn = scmd->device->host->hostt->eh_abort_handler(scmd);
+	}
 	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	return rtn;

_

#1 lore raw
Quoted 5 lines · Brian King [[email protected]] wrote:

The following patch fixes a race condition in abort processing. With this patch, the mid-layer can now guarantee to LLDs that it will only call eh_abort for ops which returned 0 in queuecommand and have not yet had their ->done function called.

How does this address the race of scsi_times_out has fired and the LLD calls scsi_done. scsi_done will return prior to setting the serial number to 0 if the timer has fired. The LLDD will believe scsi_done added the command to the scsi_done_q, but it did not.

-andmike
--
Michael Anderson
[email protected]

author
#2 lore raw
Quoted 8 lines · Mike Anderson
Quoted 3 lines · Brian King [[email protected]] wrote:

The following patch fixes a race condition in abort processing. With this
patch, the mid-layer can now guarantee to LLDs that it will only call
eh_abort for ops which returned 0 in queuecommand and have not yet had their ->done function called.

Quoted 4 lines

How does this address the race of scsi_times_out has fired and the LLD calls scsi_done. scsi_done will return prior to setting the serial number to 0 if the timer has fired. The LLDD will believe scsi_done added the command to the scsi_done_q, but it did not.

It doesn't;) I missed that... Is the fix as simple as changing scsi_times_out to zero the serial_number before calling scsi_delete_timer, or am I missing something?

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center