Bugs in scsi system, need help to fix

Bugs in scsi system, need help to fix

[Alan Stern]

While tracking down some problems involving usb-storage, I traced them to
a couple of bugs in the scsi system.  Unfortunately, I don't know enough
about how things are supposed to work to fix them; I'm hoping to attract
some help.

The first problem is in the error-handling code in scsi_error.c.  The
scsi_eh_lock_done() function is the callback for a special request
inserted at the beginning of the queue while restarting normal operations.  
(It locks the drive's door.)  But this function does not call
scsi_io_completion(), scsi_end_request(), or scsi_queue_next_request(),
with the result that the device's request queue stops once the door-lock
command has been processed.  The callback needs to do _something_ to keep
the queue going, but I don't know what.

Maybe I'm wrong about that, and the problem doesn't lie in 
scsi_eh_lock_done().  But there is no doubt that at the end of error 
recovery, after the door-lock command finishes up no other commands are 
processed.  Logging just stops after the "Notifying upper driver of 
completion" message.


The second problem is a coding mistake in scsi_check_device_busy(), in
hosts.c.  Here is an excerpt from the source:

---------------

static int scsi_check_device_busy(struct scsi_device *sdev)
{
	struct Scsi_Host *shost = sdev->host;
	struct scsi_cmnd *scmd;
	unsigned long flags;

	/*
	 * Loop over all of the commands associated with the
	 * device.  If any of them are busy, then set the state
	 * back to inactive and bail.
	 */
	spin_lock_irqsave(&sdev->list_lock, flags);
	list_for_each_entry(scmd, &sdev->cmd_list, list) {
		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE)
			goto active;

<snip>

active:
	printk(KERN_ERR "SCSI device not inactive - rq_status=%d, target=%d, "
			"pid=%ld, state=%d, owner=%d.\n",
			scmd->request->rq_status, scmd->device->id,
			scmd->pid, scmd->state, scmd->owner);

(A)	list_for_each_entry(sdev, &shost->my_devices, siblings) {
		list_for_each_entry(scmd, &sdev->cmd_list, list) {
(B)			if (scmd->request->rq_status == RQ_SCSI_DISCONNECTING)
				scmd->request->rq_status = RQ_INACTIVE;
		}
	}

(C)	spin_unlock_irqrestore(&sdev->list_lock, flags);
	printk(KERN_ERR "Device busy???\n");
	return 1;
}

---------------

The line labelled (A) is definitely wrong.  Maybe it doesn't belong there
at all -- I don't see any reason why a function devoted to checking
whether a particular device is busy needs to look at any other devices on
the same host.  Furthermore, line (A) changes the value of the sdev
variable, which means that line (C) unlocks the wrong spinlock.  Finally,
judging from the style of the code earlier on, line (B) needs to test that
scmd->request is non-NULL before dereferencing it.


The third problem is very minor.  In scsi_lib.c, line 716 contains an 
extra blank space at the end of a logging text string, just following a 
\n.  This messes up whatever log message follows, because the severity 
indicator (KERN_ERR or whatever) then doesn't fall at the start of a line.  
Here's the line in question, with a few extra lines of context.

---------------

	if (good_sectors >= 0) {
		SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, %d sectors done.\n",
					      req->nr_sectors, good_sectors));
		SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n ", cmd->use_sg));

		if (clear_errors)
			req->errors = 0;

---------------

While I feel perfectly capable of fixing that third bug by myself, I 
certainly need help fixing the first two.

Alan Stern

Re: Bugs in scsi system, need help to fix

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> The first problem is in the error-handling code in scsi_error.c.  The
> scsi_eh_lock_done() function is the callback for a special request
> inserted at the beginning of the queue while restarting normal operations.  
> (It locks the drive's door.)  But this function does not call
> scsi_io_completion(), scsi_end_request(), or scsi_queue_next_request(),
> with the result that the device's request queue stops once the door-lock
> command has been processed.  The callback needs to do _something_ to keep
> the queue going, but I don't know what.

The problem you are hitting is related to missing calls to
scsi_queue_next_request. Patrick created a patch in a previous thread:
http://marc.theaimsgroup.com/?l=linux-scsi&m=104855818826887&w=2

though it did not cover the path scsi_eh_lock_done takes. It appears
that we should add a call to scsi_queue_next_request in
scsi_release_request if sr_command is set.

> 
> Maybe I'm wrong about that, and the problem doesn't lie in 
> scsi_eh_lock_done().  But there is no doubt that at the end of error 
> recovery, after the door-lock command finishes up no other commands are 
> processed.  Logging just stops after the "Notifying upper driver of 
> completion" message.
> 
> 
> The second problem is a coding mistake in scsi_check_device_busy(), in
> hosts.c.  Here is an excerpt from the source:
> 
> ---------------
> 
> static int scsi_check_device_busy(struct scsi_device *sdev)
> {
> 	struct Scsi_Host *shost = sdev->host;
> 	struct scsi_cmnd *scmd;
> 	unsigned long flags;
> 
> 	/*
> 	 * Loop over all of the commands associated with the
> 	 * device.  If any of them are busy, then set the state
> 	 * back to inactive and bail.
> 	 */
> 	spin_lock_irqsave(&sdev->list_lock, flags);
> 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
> 		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE)
> 			goto active;
> 
> <snip>
> 
> active:
> 	printk(KERN_ERR "SCSI device not inactive - rq_status=%d, target=%d, "
> 			"pid=%ld, state=%d, owner=%d.\n",
> 			scmd->request->rq_status, scmd->device->id,
> 			scmd->pid, scmd->state, scmd->owner);
> 
> (A)	list_for_each_entry(sdev, &shost->my_devices, siblings) {
> 		list_for_each_entry(scmd, &sdev->cmd_list, list) {
> (B)			if (scmd->request->rq_status == RQ_SCSI_DISCONNECTING)
> 				scmd->request->rq_status = RQ_INACTIVE;
> 		}
> 	}
> 
> (C)	spin_unlock_irqrestore(&sdev->list_lock, flags);
> 	printk(KERN_ERR "Device busy???\n");
> 	return 1;
> }
> 
> ---------------
> 
> The line labelled (A) is definitely wrong.  Maybe it doesn't belong there
> at all -- I don't see any reason why a function devoted to checking
> whether a particular device is busy needs to look at any other devices on
> the same host.  Furthermore, line (A) changes the value of the sdev
> variable, which means that line (C) unlocks the wrong spinlock.  Finally,
> judging from the style of the code earlier on, line (B) needs to test that
> scmd->request is non-NULL before dereferencing it.
> 

The code you point to is incorrect. It looks like the migration of the
2.4 code left a few lines behind. The short term fix if you are hitting
a problem here is to remove (A) add "scmd->request" to (B). The better
fix is to not call this code anymore.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Bugs in scsi system, need help to fix

[Alan Stern]

On Sun, 13 Apr 2003, Mike Anderson wrote:

> Alan Stern [stern@rowland.harvard.edu] wrote:
> > The first problem is in the error-handling code in scsi_error.c.  The
> > scsi_eh_lock_done() function is the callback for a special request
> > inserted at the beginning of the queue while restarting normal operations.  
> > (It locks the drive's door.)  But this function does not call
> > scsi_io_completion(), scsi_end_request(), or scsi_queue_next_request(),
> > with the result that the device's request queue stops once the door-lock
> > command has been processed.  The callback needs to do _something_ to keep
> > the queue going, but I don't know what.
> 
> The problem you are hitting is related to missing calls to
> scsi_queue_next_request. Patrick created a patch in a previous thread:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=104855818826887&w=2
> 
> though it did not cover the path scsi_eh_lock_done takes. It appears
> that we should add a call to scsi_queue_next_request in
> scsi_release_request if sr_command is set.

<snip>

> The code you point to is incorrect. It looks like the migration of the
> 2.4 code left a few lines behind. The short term fix if you are hitting
> a problem here is to remove (A) add "scmd->request" to (B). The better
> fix is to not call this code anymore.
> 
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com

Thanks, Mike.  Is it safe to assume that a combined fix for all these 
problems will be posted in the next few weeks, as part of the 
hot-unplugging development effort?

Alan Stern

Re: Bugs in scsi system, need help to fix

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> Thanks, Mike.  Is it safe to assume that a combined fix for all these 
> problems will be posted in the next few weeks, as part of the 
> hot-unplugging development effort?

The fix for the scsi_eh_lock_done problem I am testing a patch now and
hopefully will have it out for others to test later today.

The fix / removal of scsi_check_device_busy will be part of the scsi
host remove / offline patches.

-andmike
--
Michael Anderson
andmike@us.ibm.com