Skip to contentSkip to conversation

fixes and cleanups for the new command allocation code

PATCH In review Christoph Hellwig 4 participants 15 messages Feb 4, 2003 – Feb 5, 2003

Subsystems (7)
SCSI SCSI SG driver Aacraid SCSI RAID driver Advansys SCSI driver +3 more subsystems
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 fixes and cleanups for the new command allocation code

I had some discussion with JAmes on the new command block allocation code and we agreed that each struct Scsi_Host should have a pointer to it's command pool because

(a) this way we don't need a GFP_DMA pool for the large number of systems that don't have old-style ISA HBAs
(b) easily get the right pool for allocation/freeing (the current code gets this wrong in at least two places)
(c) easily allow adding more pools if there's a reason to do so.

To implement this there are two new functions

	extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
	extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);

called in the HBA setup/teardown code, they also nicely abstract away all knowledge of the command handling from hosts.c.

In addition to that I've removed all dead code from the old command block allocator (and there is _lots_ of that!), moved the new command block allocator to scsi.c where the rest of the scsi command block handling lives, properly commented it and adopted it to Linux coding style conventions. struct scsi_core_data is gone, the old scsi_get_command and scsi_setup_command are now private to scsi.c, in addition to the above two functions only the new scsi_get_command ( previously scsi_getset command, but that name is misleading) and scsi_put_command are public.

 53c700.c      |    6 
 advansys.c    |   19 -
 cpqfcTSinit.c |    2 
 gdth.c        |    4 
 gdth_proc.c   |    4 
 hosts.c       |   38 +--
 hosts.h       |    4 
 scsi.c        |  594 +++++++++++++++-------------------------------------------
 scsi.h        |   62 ------
 scsi_lib.c    |  147 --------------
 scsi_scan.c   |    3 
 scsi_syms.c   |    8 
 scsi_sysfs.c  |    2 
 13 files changed, 196 insertions(+), 697 deletions(-)
drivers/scsi/53c700.c +3 -3
@@ -1718,10 +1718,10 @@
 	hostdata = (struct NCR_700_Host_Parameters *)host->hostdata[0];
 	len += sprintf(&buf[len], "Total commands outstanding: %d\n", hostdata->command_slot_count);
 	len += sprintf(&buf[len],"\
-Target	Depth  Active  Next Tag\n\
-======	=====  ======  ========\n");
+Target	Active  Next Tag\n\
+======	======  ========\n");
 	list_for_each_entry(SDp, &host->my_devices, siblings) {
-		len += sprintf(&buf[len]," %2d:%2d   %4d    %4d      %4d\n", SDp->id, SDp->lun, SDp->current_queue_depth, NCR_700_get_depth(SDp), SDp->current_tag);
+		len += sprintf(&buf[len]," %2d:%2d   %4d      %4d\n", SDp->id, SDp->lun, NCR_700_get_depth(SDp), SDp->current_tag);
 	}
 	if((len -= offset) <= 0)
 		return 0;

===== drivers/scsi/advansys.c 1.25 vs edited =====

drivers/scsi/advansys.c +0 -19
@@ -8417,25 +8417,6 @@
         chip_scsi_id = boardp->dvc_var.adv_dvc_var.chip_scsi_id;
     }
 
-    if (boardp->flags & ASC_SELECT_QUEUE_DEPTHS) {
-        len = asc_prt_line(cp, leftlen, " queue_depth:");
-        ASC_PRT_NEXT();
-        for (i = 0; i <= ADV_MAX_TID; i++) {
-            if ((chip_scsi_id == i) ||
-                ((boardp->init_tidmask & ADV_TID_TO_TIDMASK(i)) == 0)) {
-                continue;
-            }
-            if (boardp->device[i] == NULL) {
-                continue;
-            }
-            len = asc_prt_line(cp, leftlen, " %X:%d",
-                i, boardp->device[i]->current_queue_depth);
-            ASC_PRT_NEXT();
-        }
-        len = asc_prt_line(cp, leftlen, "\n");
-        ASC_PRT_NEXT();
-    }
-
     return totlen;
 }

===== drivers/scsi/cpqfcTSinit.c 1.29 vs edited =====

drivers/scsi/cpqfcTSinit.c +1 -1
@@ -1604,7 +1604,7 @@
 
   scsi_cdb[0] = RELEASE;
 
-  SCpnt = scsi_getset_command(ScsiDev, GFP_KERNEL);
+  SCpnt = scsi_get_command(ScsiDev, GFP_KERNEL);
   {
     CPQFC_DECLARE_COMPLETION(wait);

===== drivers/scsi/gdth.c 1.19 vs edited =====

drivers/scsi/gdth.c +2 -2
@@ -4637,7 +4637,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_tab[hanum]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     scp->cmd_len = 12;
     scp->use_sg = 0;
 #else

@@ -4711,7 +4711,7 @@
         memset(cmnd, 0xff, MAX_COMMAND_SIZE);
 #if LINUX_VERSION_CODE >= 0x020322
         sdev = scsi_get_host_dev(gdth_ctr_tab[hanum]);
-        scp  = scsi_getset_command(sdev, GFP_KERNEL);
+        scp  = scsi_get_command(sdev, GFP_KERNEL);
         scp->cmd_len = 12;
         scp->use_sg = 0;
 #else

===== drivers/scsi/gdth_proc.c 1.10 vs edited =====

drivers/scsi/gdth_proc.c +2 -2
@@ -48,7 +48,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_vtab[vh]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
     scp->cmd_len = 12;

@@ -712,7 +712,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_vtab[vh]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
     scp->cmd_len = 12;

===== drivers/scsi/hosts.c 1.47 vs edited =====

drivers/scsi/hosts.c +13 -25
@@ -346,17 +346,8 @@
 	}
 
 	shost->hostt->present--;
-
-	/* Cleanup proc */
 	scsi_proc_host_rm(shost);
-
-	while (!list_empty(&shost->free_list)) {
-		struct scsi_cmnd *cmd;
-		cmd = list_entry(shost->free_list.next,struct scsi_cmnd,list);
-		list_del_init(&cmd->list);
-		kmem_cache_free(scsi_core->scsi_cmd_cache, cmd);
-	}
-
+	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }
 

@@ -377,8 +368,7 @@
 struct Scsi_Host * scsi_register(Scsi_Host_Template *shost_tp, int xtr_bytes)
 {
 	struct Scsi_Host *shost, *shost_scr;
-	struct scsi_cmnd *cmd = NULL;
-	int gfp_mask;
+	int gfp_mask, rval;
 	DECLARE_COMPLETION(sem);
 
         /* Check to see if this host has any error handling facilities */

@@ -445,7 +435,7 @@
 	shost->unchecked_isa_dma = shost_tp->unchecked_isa_dma;
 	shost->use_clustering = shost_tp->use_clustering;
 	if (!blk_nohighio)
-	shost->highmem_io = shost_tp->highmem_io;
+		shost->highmem_io = shost_tp->highmem_io;
 
 	shost->max_sectors = shost_tp->max_sectors;
 	shost->use_blk_tcq = shost_tp->use_blk_tcq;

@@ -467,16 +457,9 @@
 found:
 	spin_unlock(&scsi_host_list_lock);
 
-	spin_lock_init(&shost->free_list_lock);
-	INIT_LIST_HEAD(&shost->free_list);
-
-	/* Get one backup command for this host. */
-	cmd = scsi_get_command(shost, GFP_KERNEL);
-	if (cmd)
-		list_add(&cmd->list, &shost->free_list);		
-	else
-		printk(KERN_NOTICE "The system is running low in memory.\n");
-
+	rval = scsi_setup_command_freelist(shost);
+	if (rval)
+		goto fail;
 	scsi_proc_host_add(shost);
 
 	shost->eh_notify = &sem;

@@ -487,10 +470,15 @@
 	 */
 	wait_for_completion(&sem);
 	shost->eh_notify = NULL;
-
 	shost->hostt->present++;
-
 	return shost;
+
+fail:
+	spin_lock(&scsi_host_list_lock);
+	list_del(&shost->sh_list);
+	spin_unlock(&scsi_host_list_lock);
+	kfree(shost);
+	return NULL;
 }
 
 /**

===== drivers/scsi/hosts.h 1.50 vs edited =====

drivers/scsi/hosts.h +4 -0
@@ -29,6 +29,9 @@
 #include <linux/types.h>
 #include <linux/pci.h>
 
+struct scsi_host_cmd_pool;
+
+
 /* It is senseless to set SG_ALL any higher than this - the performance
  *  does not get any better, and it wastes memory
  */

@@ -375,6 +378,7 @@
     struct list_head      sh_list;
     struct list_head	  my_devices;
 
+    struct scsi_host_cmd_pool *cmd_pool;
     spinlock_t            free_list_lock;
     struct list_head      free_list;   /* backup store of cmd structs */

===== drivers/scsi/scsi.c 1.85 vs edited =====

drivers/scsi/scsi.c +161 -433

705 lines not on this page · raw message

===== drivers/scsi/scsi.h 1.57 vs edited =====

drivers/scsi/scsi.h +5 -57

93 lines not on this page · raw message

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

drivers/scsi/scsi_lib.c +2 -145

168 lines not on this page · raw message

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

drivers/scsi/scsi_scan.c +0 -3
@@ -471,9 +471,6 @@
 
 		sdev->request_queue->queuedata = sdev;
 		scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
-		if (sdev->current_queue_depth == 0) {
-			goto out_bail;
-		}
 		init_waitqueue_head(&sdev->scpnt_wait);
 
 		if (shost->hostt->slave_alloc)

===== drivers/scsi/scsi_syms.c 1.25 vs edited =====

drivers/scsi/scsi_syms.c +3 -5
@@ -81,6 +81,9 @@
 EXPORT_SYMBOL(scsi_slave_detach);
 EXPORT_SYMBOL(scsi_device_get);
 EXPORT_SYMBOL(scsi_device_put);
+EXPORT_SYMBOL(scsi_get_command);
+EXPORT_SYMBOL(scsi_put_command);
+
 
 /*
  * This symbol is for the highlevel drivers (e.g. sg) only.

@@ -112,8 +115,3 @@
  * sysfs support
  */
 EXPORT_SYMBOL(shost_devclass);
-
-EXPORT_SYMBOL(scsi_get_command);
-EXPORT_SYMBOL(scsi_put_command);
-EXPORT_SYMBOL(scsi_setup_command);
-

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

drivers/scsi/scsi_sysfs.c +0 -2
@@ -210,7 +210,6 @@
  * Create the actual show/store functions and data structures.
  */
 sdev_rd_attr (device_blocked, "%d\n");
-sdev_rd_attr (current_queue_depth, "%d\n");
 sdev_rd_attr (new_queue_depth, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");

@@ -222,7 +221,6 @@
 
 static struct device_attribute * const sdev_attrs[] = {
 	&dev_attr_device_blocked,
-	&dev_attr_current_queue_depth,
 	&dev_attr_new_queue_depth,
 	&dev_attr_type,
 	&dev_attr_scsi_level,
#1 lore raw

drivers/scsi/scsi.c line 300

Quoted 12 lines · Christoph Hellwig
drivers/scsi/scsi.c +0 -12
-static int check_all_luns(struct Scsi_Host *shost, struct scsi_device *myself)
-{
-	struct scsi_device *sdev;
-
-	list_for_each_entry(sdev, &myself->same_target_siblings,
-		       	same_target_siblings) {
-		if (atomic_read(&sdev->device_active))
-			return 1;
-	}
-
-	return 0;
-}

We should still be calling the above from somewhere.

Quoted 25 lines
drivers/scsi/scsi.c +0 -25
-struct scsi_cmnd *scsi_allocate_device(struct scsi_device *sdev, int wait)
-{
-	DECLARE_WAITQUEUE(wq, current);
-	struct Scsi_Host *shost = sdev->host;
-	struct scsi_cmnd *scmnd;
-	unsigned long flags;
-
-	spin_lock_irqsave(&device_request_lock, flags);
-	while (1) {
-		if (sdev->device_blocked)
-			goto busy;
-		if (sdev->single_lun && check_all_luns(shost, sdev))
-			goto busy;
-
-		/*
-		 * Now we can check for a free command block for this device.
-		 */
-		for (scmnd = sdev->device_queue; scmnd; scmnd = scmnd->next)
-			if (!scmnd->request)
-				goto found;
-
-busy:
-		if (!wait)
-			goto fail;
-

drivers/scsi/scsi.c line 362

Quoted 10 lines
drivers/scsi/scsi.c +0 -10
-		/*
-		 * We need to wait for a free commandblock.  We need to
-		 * insert ourselves into the list before we release the
-		 * lock.  This way if a block were released the same
-		 * microsecond that we released the lock, the call
-		 * to schedule() wouldn't block (well, it might switch,
-		 * but the current task will still be schedulable.
-		 */
-		add_wait_queue(&sdev->scpnt_wait, &wq);
-		set_current_state(TASK_UNINTERRUPTIBLE);

We are missing the above checks and functionallity in scsi_get_command.

The limit previously imposed by current_queue_depth via the number of scmnd's allocated is not checked - scsi_get_command should check device_busy < new_queue_depth.

I was trying to fix/hit this - surprisingly, I did not see performance problems (i.e. getting tons of QUEUE_FULLs), probably because my request queue limits are 128, and the disks are not old.

Also the above did not properly get the host_lock/queue_lock when wait != 0 (the caller does not lock, so just when we might sleep we also need to get/release a lock).

Quoted 46 lines
drivers/scsi/scsi.c +45 -0
+struct scsi_host_cmd_pool {
+	kmem_cache_t	*slab;
+	unsigned int	users;
+	char		*name;
+	unsigned int	slab_flags;
+	unsigned int	gfp_mask;
+};
 
+static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
+					    int gfp_mask)
+{
+	struct scsi_cmnd *cmd;
+	
+	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
+			gfp_mask | shost->cmd_pool->gfp_mask);
+
+	if (unlikely(!cmd)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&shost->free_list_lock, flags);
+		if (likely(!list_empty(&shost->free_list))) {
+			cmd = list_entry(shost->free_list.next,
+					 struct scsi_cmnd, list);
+			list_del_init(&cmd->list);
+		}
+		spin_unlock_irqrestore(&shost->free_list_lock, flags);
+	}
+
+	return cmd;
+}
+
+/*
+ * Function:	scsi_get_command()
+ *
+ * Purpose:	Allocate and setup a scsi command block
+ *
+ * Arguments:	dev	- parent scsi device
+ *		gfp_mask- allocator flags
+ *
+ * Returns:	The allocated scsi command structure.
+ */
+struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
+{
+	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
+
+	if (likely(cmd)) {

drivers/scsi/scsi.c line 413

Quoted 10 lines
drivers/scsi/scsi.c +10 -0
+		memset(cmd, 0, sizeof(*cmd));
+		cmd->device = dev;
+		cmd->state = SCSI_STATE_UNUSED;
+		cmd->owner = SCSI_OWNER_NOBODY;
+		init_timer(&cmd->eh_timeout);
+		INIT_LIST_HEAD(&cmd->list);
+	}
+
+	return cmd;
+}

So one of the above needs to (conditionally ... based on gfp_mask?) get host/queue_lock, check limits, and conditionally add_wait_queue().

Quoted 30 lines
drivers/scsi/scsi.c +30 -0
+int scsi_setup_command_freelist(struct Scsi_Host *shost)
+{
+	struct scsi_host_cmd_pool *pool;
+	struct scsi_cmnd *cmd;
+
+	spin_lock_init(&shost->free_list_lock);
+	INIT_LIST_HEAD(&shost->free_list);
+
+	/*
+	 * Select a command slab for this host and create it if not
+	 * yet existant.
+	 */
+	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
+	if (!pool->users) {
+		pool->slab = kmem_cache_create(pool->name,
+				sizeof(struct scsi_cmnd), 0,
+				pool->slab_flags, NULL, NULL);
+		if (!pool->slab)
+			return -ENOMEM;
+	}
+
+	pool->users++;
+	shost->cmd_pool = pool;
+
+	/*
+	 * Get one backup command for this host.
+	 */
+	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
+			GFP_KERNEL | shost->cmd_pool->gfp_mask);
+	if (!cmd)

drivers/scsi/scsi.c line 490

Quoted 10 lines
drivers/scsi/scsi.c +10 -0
+		goto fail;
+	list_add(&cmd->list, &shost->free_list);		
+	return 0;
+
+fail:
+	if (!--pool->users)
+		kmem_cache_destroy(pool->slab);
+	return -ENOMEM;
+
+}

What protects pool->users?

-- Patrick Mansfield

author
#2 lore raw
Quoted 9 lines · Patrick Mansfield

We are missing the above checks and functionallity in scsi_get_command.

The limit previously imposed by current_queue_depth via the number of scmnd's allocated is not checked - scsi_get_command should check device_busy < new_queue_depth.

I was trying to fix/hit this - surprisingly, I did not see performance problems (i.e. getting tons of QUEUE_FULLs), probably because my request queue limits are 128, and the disks are not old.

I wonder whether we really need it or whether the queue limits shouldn't be enough. If there's a chance I'd like to avoid having throttewling in too many places.

Quoted 2 lines

So one of the above needs to (conditionally ... based on gfp_mask?) get host/queue_lock, check limits, and conditionally add_wait_queue().

I don't think we need to add the waitqeue. The scsi midlayer always calls scsi_get_command with an GFP_ATOMIC argument, so we can't ever wait, so this would only apply to the gdth drivers that calls it directly. And even this driver only uses it for administrative commands (i.e. not in the I/O) and the driver doesn't even compile in 2.5 :)

Quoted 1 line

What protects pool->users?

I assumed adding/removing hosts was serialized but it isn't. I'll add code to fix this in the next drop.

#3 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Patrick Mansfield

I was trying to fix/hit this - surprisingly, I did not see performance problems (i.e. getting tons of QUEUE_FULLs), probably because my request queue limits are 128, and the disks are not old.

I wonder whether we really need it or whether the queue limits shouldn't be enough. If there's a chance I'd like to avoid having throttewling in too many places.

We really need to limit to what the scsi_device (thinks it) can handle (currently new_queue_depth). Otherwise we could have QUEUE_FULL storms, plus we really don't want that many scsi_cmnd's outstanding (i.e. limited by the amount of memory we can allocate) when we have many scsi_devices on the system. If we lowered the request queue limit that would hurt scsi_devices (and maybe adapters) with a low queue limits.

Quoted 8 lines
Quoted 2 lines

So one of the above needs to (conditionally ... based on gfp_mask?) get host/queue_lock, check limits, and conditionally add_wait_queue().

I don't think we need to add the waitqeue. The scsi midlayer always calls scsi_get_command with an GFP_ATOMIC argument, so we can't ever wait, so this would only apply to the gdth drivers that calls it directly. And even this driver only uses it for administrative commands (i.e. not in the I/O) and the driver doesn't even compile in 2.5 :)

Agree.

So, we should not externalize it (and we should get rid of scsi_do_cmd); drivers should already be using scsi_allocate_request/scsi_{do,wait}_req.

-- Patrick Mansfield

#4 lore raw
Quoted 14 lines · Patrick Mansfield
Quoted 10 lines · Christoph Hellwig
Quoted 3 lines · Patrick Mansfield

I was trying to fix/hit this - surprisingly, I did not see performance problems (i.e. getting tons of QUEUE_FULLs), probably because my request queue limits are 128, and the disks are not old.

I wonder whether we really need it or whether the queue limits shouldn't be enough. If there's a chance I'd like to avoid having throttewling in too many places.

Quoted 6 lines

We really need to limit to what the scsi_device (thinks it) can handle (currently new_queue_depth). Otherwise we could have QUEUE_FULL storms, plus we really don't want that many scsi_cmnd's outstanding (i.e. limited by the amount of memory we can allocate) when we have many scsi_devices on the system. If we lowered the request queue limit that would hurt scsi_devices (and maybe adapters) with a low queue limits.

The sole functionality of scsi_get/put_command() is/was(?) to just give/take back a command structure. This was my intention, to abstract it away, with a well defined functionality.

Queue depths, limits and such checks should be provided elsewhere, i.e. in the enqueuing piece of code of a scsi_cmnd into a LLDD. Again, this should be abstracted away with a well defined/sole functionality.

LLDD may call scsi_get/put_command() to piggy-back memory management to SCSI Core, but this doesn't necessarily mean that they will enqueue those commands.

BTW, is it the trend to have ether-pointers (as I call them) in SCSI Core all over the place or to put SCSI Core data in a structure (i.e. to reflect that it *is* SCSI Core), aka struct scsi_core_data? I see this is now gone.

Signature · 1 line
Luben
author
#5 lore raw
Quoted 7 lines · Luben Tuikov

The sole functionality of scsi_get/put_command() is/was(?) to just give/take back a command structure. This was my intention, to abstract it away, with a well defined functionality.

Queue depths, limits and such checks should be provided elsewhere, i.e. in the enqueuing piece of code of a scsi_cmnd into a LLDD. Again, this should be abstracted away with a well defined/sole functionality.

scsi_get_command/scsi_put_command is only called from the scsi midlayer. There are a few stale references in LLDDs, but it's either stubbed out (cpqfc) or the driver is far away from beeing useable on 2.5 (gdth). Both of them need updating to the scsi_request based APIs anyway.

Quoted 4 lines

BTW, is it the trend to have ether-pointers (as I call them) in SCSI Core all over the place or to put SCSI Core data in a structure (i.e. to reflect that it *is* SCSI Core), aka struct scsi_core_data? I see this is now gone.

The trend is to have as much variables as possible static to one source file and if that is not possible have it global in C language scope but not exported to modules.

#6 lore raw
Quoted 4 lines · Christoph Hellwig

The trend is to have as much variables as possible static to one source file and if that is not possible have it global in C language scope but not exported to modules.

I was (of course) referring to those ``global in C language scope''. I think that it would be cleaner to encapsulate them in a variable of type structure, which will abide by the same rules of course. My opinion is that this would be cleaner.

Signature · 1 line
Luben
#7 lore raw
In reply to Patrick Mansfield
Quoted 6 lines · Patrick Mansfield

We really need to limit to what the scsi_device (thinks it) can handle (currently new_queue_depth). Otherwise we could have QUEUE_FULL storms, plus we really don't want that many scsi_cmnd's outstanding (i.e. limited by the amount of memory we can allocate) when we have many scsi_devices on the system. If we lowered the request queue limit that would hurt scsi_devices (and maybe adapters) with a low queue limits.

I agree with this. It is a guarantee the mid-layer makes to the LLD (and there are some LLDs with static issue queues for which this is a hard requirement). I think (once the dust has settled and we've agreed which field holds the current queue depth) what's needed is a check in the scsi_request_fn() to see if we're over the LLD's current depth for the device and plug the queue and exit if we are. The next returning command will unplug and send.

This way of doing things means that we're free to prep as many commands as we can, but we guarantee only to have the correct number outstanding to the LLD.

James

author
#8 lore raw
Quoted 11 lines · James Bottomley

I agree with this. It is a guarantee the mid-layer makes to the LLD (and there are some LLDs with static issue queues for which this is a hard requirement). I think (once the dust has settled and we've agreed which field holds the current queue depth) what's needed is a check in the scsi_request_fn() to see if we're over the LLD's current depth for the device and plug the queue and exit if we are. The next returning command will unplug and send.

This way of doing things means that we're free to prep as many commands as we can, but we guarantee only to have the correct number outstanding to the LLD.

Okay, here's a new versin of the patch. Changes:

* throttel on number of inflight command blocks
* rename scsi_cmnd->new_queue_depth to scsi_cmnd->queue_depth
* remove scsi_do_cmd
* serialize pool handling

drivers/scsi/53c700.c +3 -3
@@ -1718,10 +1718,10 @@
 	hostdata = (struct NCR_700_Host_Parameters *)host->hostdata[0];
 	len += sprintf(&buf[len], "Total commands outstanding: %d\n", hostdata->command_slot_count);
 	len += sprintf(&buf[len],"\
-Target	Depth  Active  Next Tag\n\
-======	=====  ======  ========\n");
+Target	Active  Next Tag\n\
+======	======  ========\n");
 	list_for_each_entry(SDp, &host->my_devices, siblings) {
-		len += sprintf(&buf[len]," %2d:%2d   %4d    %4d      %4d\n", SDp->id, SDp->lun, SDp->current_queue_depth, NCR_700_get_depth(SDp), SDp->current_tag);
+		len += sprintf(&buf[len]," %2d:%2d   %4d      %4d\n", SDp->id, SDp->lun, NCR_700_get_depth(SDp), SDp->current_tag);
 	}
 	if((len -= offset) <= 0)
 		return 0;

===== drivers/scsi/advansys.c 1.25 vs edited =====

drivers/scsi/advansys.c +0 -19
@@ -8417,25 +8417,6 @@
         chip_scsi_id = boardp->dvc_var.adv_dvc_var.chip_scsi_id;
     }
 
-    if (boardp->flags & ASC_SELECT_QUEUE_DEPTHS) {
-        len = asc_prt_line(cp, leftlen, " queue_depth:");
-        ASC_PRT_NEXT();
-        for (i = 0; i <= ADV_MAX_TID; i++) {
-            if ((chip_scsi_id == i) ||
-                ((boardp->init_tidmask & ADV_TID_TO_TIDMASK(i)) == 0)) {
-                continue;
-            }
-            if (boardp->device[i] == NULL) {
-                continue;
-            }
-            len = asc_prt_line(cp, leftlen, " %X:%d",
-                i, boardp->device[i]->current_queue_depth);
-            ASC_PRT_NEXT();
-        }
-        len = asc_prt_line(cp, leftlen, "\n");
-        ASC_PRT_NEXT();
-    }
-
     return totlen;
 }

===== drivers/scsi/aic7xxx_old.c 1.40 vs edited =====

drivers/scsi/aic7xxx_old.c +1 -1
@@ -4068,7 +4068,7 @@
              * normal.
              */
 	    scsi_adjust_queue_depth(scb->cmd->device, MSG_SIMPLE_TAG,
-			    scb->cmd->device->new_queue_depth);
+			    scb->cmd->device->queue_depth);
             scb->tag_action = MSG_SIMPLE_Q_TAG;
             scb->hscb->control &= ~SCB_TAG_TYPE;
             scb->hscb->control |= MSG_SIMPLE_Q_TAG;

===== drivers/scsi/cpqfcTSinit.c 1.29 vs edited =====

drivers/scsi/cpqfcTSinit.c +1 -1
@@ -1604,7 +1604,7 @@
 
   scsi_cdb[0] = RELEASE;
 
-  SCpnt = scsi_getset_command(ScsiDev, GFP_KERNEL);
+  SCpnt = scsi_get_command(ScsiDev, GFP_KERNEL);
   {
     CPQFC_DECLARE_COMPLETION(wait);

===== drivers/scsi/eata.c 1.25 vs edited =====

drivers/scsi/eata.c +4 -4
@@ -895,7 +895,7 @@
       tag_suffix = "";
       }
 
-   if (TLDEV(dev->type) && linked_comm && dev->new_queue_depth > 2)
+   if (TLDEV(dev->type) && linked_comm && dev->queue_depth > 2)
       link_suffix = ", sorted";
    else if (TLDEV(dev->type))
       link_suffix = ", unsorted";

@@ -904,7 +904,7 @@
 
    printk("%s: scsi%d, channel %d, id %d, lun %d, cmds/lun %d%s%s.\n",
           BN(j), host->host_no, dev->channel, dev->id, dev->lun,
-          dev->new_queue_depth, link_suffix, tag_suffix);
+          dev->queue_depth, link_suffix, tag_suffix);
 
    return FALSE;
 }

@@ -1699,7 +1699,7 @@
    /* Map DMA buffers and SG list */
    map_dma(i, j);
 
-   if (linked_comm && SCpnt->device->new_queue_depth > 2
+   if (linked_comm && SCpnt->device->queue_depth > 2
                                      && TLDEV(SCpnt->device->type)) {
       HD(j)->cp_stat[i] = READY;
       flush_dev(SCpnt->device, SCpnt->request->sector, j, FALSE);

@@ -2207,7 +2207,7 @@
 
    sync_dma(i, j);
 
-   if (linked_comm && SCpnt->device->new_queue_depth > 2
+   if (linked_comm && SCpnt->device->queue_depth > 2
                                      && TLDEV(SCpnt->device->type))
       flush_dev(SCpnt->device, SCpnt->request->sector, j, TRUE);

===== drivers/scsi/gdth.c 1.19 vs edited =====

drivers/scsi/gdth.c +2 -2
@@ -4637,7 +4637,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_tab[hanum]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     scp->cmd_len = 12;
     scp->use_sg = 0;
 #else

@@ -4711,7 +4711,7 @@
         memset(cmnd, 0xff, MAX_COMMAND_SIZE);
 #if LINUX_VERSION_CODE >= 0x020322
         sdev = scsi_get_host_dev(gdth_ctr_tab[hanum]);
-        scp  = scsi_getset_command(sdev, GFP_KERNEL);
+        scp  = scsi_get_command(sdev, GFP_KERNEL);
         scp->cmd_len = 12;
         scp->use_sg = 0;
 #else

===== drivers/scsi/gdth_proc.c 1.10 vs edited =====

drivers/scsi/gdth_proc.c +2 -2
@@ -48,7 +48,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_vtab[vh]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
     scp->cmd_len = 12;

@@ -712,7 +712,7 @@
 
 #if LINUX_VERSION_CODE >= 0x020322
     sdev = scsi_get_host_dev(gdth_ctr_vtab[vh]);
-    scp  = scsi_getset_command(sdev, GFP_KERNEL);
+    scp  = scsi_get_command(sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
     scp->cmd_len = 12;

===== drivers/scsi/hosts.c 1.47 vs edited =====

drivers/scsi/hosts.c +13 -25
@@ -346,17 +342,8 @@
 	}
 
 	shost->hostt->present--;
-
-	/* Cleanup proc */
 	scsi_proc_host_rm(shost);
-
-	while (!list_empty(&shost->free_list)) {
-		struct scsi_cmnd *cmd;
-		cmd = list_entry(shost->free_list.next,struct scsi_cmnd,list);
-		list_del_init(&cmd->list);
-		kmem_cache_free(scsi_core->scsi_cmd_cache, cmd);
-	}
-
+	scsi_destroy_command_freelist(shost);
 	kfree(shost);
 }
 

@@ -377,8 +364,7 @@
 struct Scsi_Host * scsi_register(Scsi_Host_Template *shost_tp, int xtr_bytes)
 {
 	struct Scsi_Host *shost, *shost_scr;
-	struct scsi_cmnd *cmd = NULL;
-	int gfp_mask;
+	int gfp_mask, rval;
 	DECLARE_COMPLETION(sem);
 
         /* Check to see if this host has any error handling facilities */

@@ -445,7 +431,7 @@
 	shost->unchecked_isa_dma = shost_tp->unchecked_isa_dma;
 	shost->use_clustering = shost_tp->use_clustering;
 	if (!blk_nohighio)
-	shost->highmem_io = shost_tp->highmem_io;
+		shost->highmem_io = shost_tp->highmem_io;
 
 	shost->max_sectors = shost_tp->max_sectors;
 	shost->use_blk_tcq = shost_tp->use_blk_tcq;

@@ -467,16 +453,9 @@
 found:
 	spin_unlock(&scsi_host_list_lock);
 
-	spin_lock_init(&shost->free_list_lock);
-	INIT_LIST_HEAD(&shost->free_list);
-
-	/* Get one backup command for this host. */
-	cmd = scsi_get_command(shost, GFP_KERNEL);
-	if (cmd)
-		list_add(&cmd->list, &shost->free_list);		
-	else
-		printk(KERN_NOTICE "The system is running low in memory.\n");
-
+	rval = scsi_setup_command_freelist(shost);
+	if (rval)
+		goto fail;
 	scsi_proc_host_add(shost);
 
 	shost->eh_notify = &sem;

@@ -487,10 +466,15 @@
 	 */
 	wait_for_completion(&sem);
 	shost->eh_notify = NULL;
-
 	shost->hostt->present++;
-
 	return shost;
+
+fail:
+	spin_lock(&scsi_host_list_lock);
+	list_del(&shost->sh_list);
+	spin_unlock(&scsi_host_list_lock);
+	kfree(shost);
+	return NULL;
 }
 
 /**

===== drivers/scsi/hosts.h 1.50 vs edited =====

drivers/scsi/hosts.h +4 -0
@@ -29,6 +29,9 @@
 #include <linux/types.h>
 #include <linux/pci.h>
 
+struct scsi_host_cmd_pool;
+
+
 /* It is senseless to set SG_ALL any higher than this - the performance
  *  does not get any better, and it wastes memory
  */

@@ -375,6 +378,7 @@
     struct list_head      sh_list;
     struct list_head	  my_devices;
 
+    struct scsi_host_cmd_pool *cmd_pool;
     spinlock_t            free_list_lock;
     struct list_head      free_list;   /* backup store of cmd structs */

===== drivers/scsi/qla1280.c 1.26 vs edited =====

drivers/scsi/qla1280.c +1 -1
@@ -1820,7 +1820,7 @@
 		/* device->queue_depth = 20; */
 		printk(KERN_INFO "scsi(%li:%d:%d:%d): Enabled tagged queuing, "
 		       "queue depth %d.\n", p->host_no, device->channel,
-		       device->id, device->lun, device->new_queue_depth);
+		       device->id, device->lun, device->queue_depth);
 	} else {
 		scsi_adjust_queue_depth(device, 0 /* TCQ off */, 3);
 	}

===== drivers/scsi/scsi.c 1.85 vs edited =====

drivers/scsi/scsi.c +172 -557

860 lines not on this page · raw message

===== drivers/scsi/scsi.h 1.57 vs edited =====

drivers/scsi/scsi.h +6 -63

96 lines not on this page · raw message

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

drivers/scsi/scsi_lib.c +12 -154

187 lines not on this page · raw message

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

drivers/scsi/scsi_scan.c +0 -3
@@ -471,9 +471,6 @@
 
 		sdev->request_queue->queuedata = sdev;
 		scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
-		if (sdev->current_queue_depth == 0) {
-			goto out_bail;
-		}
 		init_waitqueue_head(&sdev->scpnt_wait);
 
 		if (shost->hostt->slave_alloc)

===== drivers/scsi/scsi_syms.c 1.25 vs edited =====

drivers/scsi/scsi_syms.c +0 -6
@@ -39,7 +39,6 @@
 EXPORT_SYMBOL(scsicam_bios_param);
 EXPORT_SYMBOL(scsi_partsize);
 EXPORT_SYMBOL(scsi_bios_ptable);
-EXPORT_SYMBOL(scsi_do_cmd);
 EXPORT_SYMBOL(scsi_ioctl);
 EXPORT_SYMBOL(print_command);
 EXPORT_SYMBOL(print_sense);

@@ -112,8 +111,3 @@
  * sysfs support
  */
 EXPORT_SYMBOL(shost_devclass);
-
-EXPORT_SYMBOL(scsi_get_command);
-EXPORT_SYMBOL(scsi_put_command);
-EXPORT_SYMBOL(scsi_setup_command);
-

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

drivers/scsi/scsi_sysfs.c +2 -4
@@ -210,8 +210,7 @@
  * Create the actual show/store functions and data structures.
  */
 sdev_rd_attr (device_blocked, "%d\n");
-sdev_rd_attr (current_queue_depth, "%d\n");
-sdev_rd_attr (new_queue_depth, "%d\n");
+sdev_rd_attr (queue_depth, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
 sdev_rd_attr (access_count, "%d\n");

@@ -222,8 +221,7 @@
 
 static struct device_attribute * const sdev_attrs[] = {
 	&dev_attr_device_blocked,
-	&dev_attr_current_queue_depth,
-	&dev_attr_new_queue_depth,
+	&dev_attr_queue_depth,
 	&dev_attr_type,
 	&dev_attr_scsi_level,
 	&dev_attr_access_count,

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

drivers/scsi/sg.c +2 -2
@@ -842,7 +842,7 @@
 			__put_user((int) sdp->device->type, &sg_idp->scsi_type);
 			__put_user((short) sdp->device->host->cmd_per_lun,
 				   &sg_idp->h_cmd_per_lun);
-			__put_user((short) sdp->device->new_queue_depth,
+			__put_user((short) sdp->device->queue_depth,
 				   &sg_idp->d_queue_depth);
 			__put_user(0, &sg_idp->unused[0]);
 			__put_user(0, &sg_idp->unused[1]);

@@ -2982,7 +2982,7 @@
 				   scsidp->host->host_no, scsidp->channel,
 				   scsidp->id, scsidp->lun, (int) scsidp->type,
 				   (int) scsidp->access_count,
-				   (int) scsidp->new_queue_depth,
+				   (int) scsidp->queue_depth,
 				   (int) scsidp->device_busy,
 				   (int) scsidp->online);
 		else

===== drivers/scsi/u14-34f.c 1.21 vs edited =====

drivers/scsi/u14-34f.c +4 -4
@@ -671,7 +671,7 @@
       tag_suffix = "";
       }
 
-   if (TLDEV(dev->type) && linked_comm && dev->new_queue_depth > 2)
+   if (TLDEV(dev->type) && linked_comm && dev->queue_depth > 2)
       link_suffix = ", sorted";
    else if (TLDEV(dev->type))
       link_suffix = ", unsorted";

@@ -680,7 +680,7 @@
 
    printk("%s: scsi%d, channel %d, id %d, lun %d, cmds/lun %d%s%s.\n",
           BN(j), host->host_no, dev->channel, dev->id, dev->lun,
-          dev->new_queue_depth, link_suffix, tag_suffix);
+          dev->queue_depth, link_suffix, tag_suffix);
 
    return FALSE;
 }

@@ -1264,7 +1264,7 @@
    /* Map DMA buffers and SG list */
    map_dma(i, j);
 
-   if (linked_comm && SCpnt->device->new_queue_depth > 2
+   if (linked_comm && SCpnt->device->queue_depth > 2
                                      && TLDEV(SCpnt->device->type)) {
       HD(j)->cp_stat[i] = READY;
       flush_dev(SCpnt->device, SCpnt->request->sector, j, FALSE);

@@ -1761,7 +1761,7 @@
 
    sync_dma(i, j);
 
-   if (linked_comm && SCpnt->device->new_queue_depth > 2
+   if (linked_comm && SCpnt->device->queue_depth > 2
                                      && TLDEV(SCpnt->device->type))
       flush_dev(SCpnt->device, SCpnt->request->sector, j, TRUE);

===== drivers/scsi/aacraid/linit.c 1.7 vs edited =====

drivers/scsi/aacraid/linit.c +1 -1
@@ -536,7 +536,7 @@
 
 	dprintk((KERN_DEBUG "(scsi%d:%d:%d:%d) Tagged Queue depth %2d, "
 				"%s\n", dev->host->host_no, dev->channel,
-				dev->id, dev->lun, dev->new_queue_depth,
+				dev->id, dev->lun, dev->queue_depth,
 				dev->online ? "OnLine" : "OffLine"));
 	return 0;
 }

===== drivers/scsi/aic7xxx_old/aic7xxx_proc.c 1.7 vs edited =====

drivers/scsi/aic7xxx_old/aic7xxx_proc.c +1 -1
@@ -313,7 +313,7 @@
                     p->user[tindex].options);
     if(sdptr->simple_tags)
     {
-      size += sprintf(BLS, "  Tagged Command Queueing Enabled, Ordered Tags %s, Depth %d/%d\n", sdptr->ordered_tags ? "Enabled" : "Disabled", sdptr->new_queue_depth, aic_dev->max_q_depth);
+      size += sprintf(BLS, "  Tagged Command Queueing Enabled, Ordered Tags %s, Depth %d/%d\n", sdptr->ordered_tags ? "Enabled" : "Disabled", sdptr->queue_depth, aic_dev->max_q_depth);
     }
     if(aic_dev->barrier_total)
       size += sprintf(BLS, "  Total transfers %ld:\n    (%ld/%ld/%ld/%ld reads/writes/REQ_BARRIER/Ordered Tags)\n",
#9 lore raw
Quoted 11 lines · Christoph Hellwig
drivers/scsi/scsi_lib.c +0 -1
@@ -35,7 +35,6 @@
 }; 	
 #undef SP
 
-struct scsi_core_data *scsi_core;
 
 /*
  * Function:    scsi_insert_special_cmd()

drivers/scsi/scsi_lib.c line 819

Quoted 10 lines
drivers/scsi/scsi_lib.c +4 -3
@@ -814,9 +813,10 @@
 		SCpnt = (Scsi_Cmnd *) req->special;
 		SRpnt = (Scsi_Request *) req->special;
 
-		if( SRpnt->sr_magic == SCSI_REQ_MAGIC ) {
-			SCpnt = scsi_getset_command(SRpnt->sr_device,
-						    GFP_ATOMIC);
+		if (SRpnt->sr_magic == SCSI_REQ_MAGIC) {
+			if (SDpnt->device_busy >= SDpnt->queue_depth)
+				return BLKPREP_DEFER;
+			SCpnt = scsi_get_command(SRpnt->sr_device, GFP_ATOMIC);

Actually, I don't necessarily think we want to do this.

I think we should throttle in the request_fn not in the prep_fn. The reason is the way the block queue works: If we have a prepped request ready to roll, it will go straight through the request fn (without sending it back through prep again). Thus, we can have a fully prepared command ready for immediate issue as soon as the device returns one of its slots and therefore we keep the pipeline packed as tightly as possible given the maximum number of outstanding commands constraint. It also means that we have all the memory allocations completed by the time we try to issue the command, rather than trying the memory allocations as part of the prep function for the next issue.

James

#10 lore raw
Quoted 36 lines · James Bottomley
Quoted 11 lines · Christoph Hellwig
drivers/scsi/scsi_lib.c +0 -1
@@ -35,7 +35,6 @@
 }; 	
 #undef SP
 
-struct scsi_core_data *scsi_core;
 
 /*
  * Function:    scsi_insert_special_cmd()

drivers/scsi/scsi_lib.c line 819

Quoted 10 lines
drivers/scsi/scsi_lib.c +4 -3
@@ -814,9 +813,10 @@
 		SCpnt = (Scsi_Cmnd *) req->special;
 		SRpnt = (Scsi_Request *) req->special;
 
-		if( SRpnt->sr_magic == SCSI_REQ_MAGIC ) {
-			SCpnt = scsi_getset_command(SRpnt->sr_device,
-						    GFP_ATOMIC);
+		if (SRpnt->sr_magic == SCSI_REQ_MAGIC) {
+			if (SDpnt->device_busy >= SDpnt->queue_depth)
+				return BLKPREP_DEFER;
+			SCpnt = scsi_get_command(SRpnt->sr_device, GFP_ATOMIC);

Actually, I don't necessarily think we want to do this.

I think we should throttle in the request_fn not in the prep_fn. The reason is the way the block queue works: If we have a prepped request ready to roll, it will go straight through the request fn (without sending it back through prep again). Thus, we can have a fully prepared command ready for immediate issue as soon as the device returns one of its slots and therefore we keep the pipeline packed as tightly as possible given the maximum number of outstanding commands constraint. It also means that we have all the memory allocations completed by the time we try to issue the command, rather than trying the memory allocations as part of the prep function for the next issue.

Quoted 1 line

James

But if we have a lot of prepped commands around we will be allocating memory that is not (currently) needed, we could end up using an equal number of scsi_cmnd's as we have blk requests. I would rather we not allocate or hold onto scsi_cmnd in such cases. (And generally in all cases, but that goes beyond.)

I doubt (especially with new scsi_get_command) that the scsi_prep_fn takes much time compared to the IO completion path and the scsi_request_fn code,

We are arguing time versus space tradeoffs, and we don't know how much space we are using, or how much time scsi_prep_fn takes (with scsi_get_command). Current performance was good even with the crappy scsi_allocate_device() allocation, scsi_get_command should be faster, so I would rather save space.

In any case so, the device_busy check should be in scsi_request_fn whether we have a command or not, so pre-prepped commands (i.e. that don't call scsi_prep_fn) will not be sent if we have reached to sdev->queue_depth.

We are still missing single_lun checking.

I would like to see the patch in a tree before 2.5.60 (probably already too late), and then we can take care of these other issues. I think some systems/disks will barf if they are flooded with QUEUE FULLs.

-- Patrick Mansfield

#11 lore raw
Quoted 5 lines · Patrick Mansfield

But if we have a lot of prepped commands around we will be allocating memory that is not (currently) needed, we could end up using an equal number of scsi_cmnd's as we have blk requests. I would rather we not allocate or hold onto scsi_cmnd in such cases. (And generally in all cases, but that goes beyond.)

But that's not how the block queue works: prep is actually called from elv_next_request(). Therefore, if you plug the queue in the request function when you're over the queue limit, you only get a single fully prepped request waiting in the queue, which, I think, is the desired behaviour. Even if the queue is restarted because of I/O pressure, it will begin with the prepped request and re-block.

Quoted 1 line

We are still missing single_lun checking.

And the slave_{alloc,configure,destroy} needs fixing too.

Quoted 3 lines

I would like to see the patch in a tree before 2.5.60 (probably already too late), and then we can take care of these other issues. I think some systems/disks will barf if they are flooded with QUEUE FULLs.

OK, I concur, we can't do without throttling. I'll put it in and we can try to fix up around it.

James

#12 lore raw
Quoted 8 lines · James Bottomley

On Tue, 2003-02-04 at 19:25, Patrick Mansfield wrote:

But that's not how the block queue works: prep is actually called from elv_next_request(). Therefore, if you plug the queue in the request function when you're over the queue limit, you only get a single fully prepped request waiting in the queue, which, I think, is the desired behaviour. Even if the queue is restarted because of I/O pressure, it will begin with the prepped request and re-block.

OK that sounds great, except we should not plug the queue since we have outstanding IO.

Quoted 1 line

And the slave_{alloc,configure,destroy} needs fixing too.

What is broken?

-- Patrick Mansfield

#13 lore raw
Quoted 2 lines · Patrick Mansfield

OK that sounds great, except we should not plug the queue since we have outstanding IO.

Yes, that's true. The guaranteed returning I/O will restart it.

Quoted 3 lines
Quoted 1 line

And the slave_{alloc,configure,destroy} needs fixing too.

What is broken?

This one, I'm not sure. The patch just drew my attention to the breakage. The way I understand the API:

slave_alloc tells the LLD we're going to issue probes to the device slave_configure tells the LLD we've probed it and we're now going to configure and use it
slave_destroy tells the LLD we're no longer interested.

The patch removes the slave_configure call from scsi_slave_attach() which is called by the upper level drivers. However, it looks like scsi_add_lun always calls it.

The correct thing, I think, is not to call slave_configure until we're sure an ULD is attaching (there's no point getting the LLD to allocate resources to a command queue until we have an attachment).

James

#14 lore raw

The patches to block after prepping the command are attached.

James

===== ./scsi_lib.c 1.65 vs edited =====

./scsi_lib.c renamed from drivers/scsi/scsi_lib.c +3 -4
@@ -814,8 +814,6 @@
 		SRpnt = (Scsi_Request *) req->special;
 		
 		if (SRpnt->sr_magic == SCSI_REQ_MAGIC) {
-			if (SDpnt->device_busy >= SDpnt->queue_depth)
-				return BLKPREP_DEFER;
 			SCpnt = scsi_get_command(SRpnt->sr_device, GFP_ATOMIC);
 			if (!SCpnt)
 				return BLKPREP_DEFER;

@@ -827,8 +825,6 @@
 		 * Now try and find a command block that we can use.
 		 */
 		if (!req->special) {
-			if (SDpnt->device_busy >= SDpnt->queue_depth)
-				return BLKPREP_DEFER;
 			SCpnt = scsi_get_command(SDpnt, GFP_ATOMIC);
 			if (unlikely(!SCpnt))
 				return BLKPREP_DEFER;

@@ -950,6 +946,9 @@
 		 * lower down.
 		 */
 		req = elv_next_request(q);
+
+		if (SDpnt->device_busy >= SDpnt->queue_depth)
+			break;
 
 		if(SHpnt->host_busy == 0 && SHpnt->host_blocked) {
 			/* unblock after host_blocked iterates to zero */