[PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Patrick Mansfield]

Revised patch (against scsi-misc-2.5) based on comments to use the
sdev->sdev_lock instead of the sdev->list_lock.

diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/dpt_i2o.c rmcmd_lock-2.5-cur/drivers/scsi/dpt_i2o.c
--- 2.5-cur/drivers/scsi/dpt_i2o.c	Tue May  6 16:22:34 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/dpt_i2o.c	Tue May  6 17:23:14 2003
@@ -2510,7 +2510,7 @@ static void adpt_fail_posted_scbs(adpt_h
 
 	list_for_each_entry(d, &pHba->host->my_devices, siblings) {
 		unsigned long flags;
-		spin_lock_irqsave(&d->list_lock, flags);
+		spin_lock_irqsave(&d->sdev_lock, flags);
 		list_for_each_entry(cmd, &d->cmd_list, list) {
 			if(cmd->serial_number == 0){
 				continue;
@@ -2518,7 +2518,7 @@ static void adpt_fail_posted_scbs(adpt_h
 			cmd->result = (DID_OK << 16) | (QUEUE_FULL <<1);
 			cmd->scsi_done(cmd);
 		}
-		spin_unlock_irqrestore(&d->list_lock, flags);
+		spin_unlock_irqrestore(&d->sdev_lock, flags);
 	}
 }
 
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/hosts.c rmcmd_lock-2.5-cur/drivers/scsi/hosts.c
--- 2.5-cur/drivers/scsi/hosts.c	Tue May  6 16:22:34 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/hosts.c	Tue May  6 17:23:14 2003
@@ -204,7 +204,7 @@ static int scsi_check_device_busy(struct
 	 * device.  If any of them are busy, then set the state
 	 * back to inactive and bail.
 	 */
-	spin_lock_irqsave(&sdev->list_lock, flags);
+	spin_lock_irqsave(&sdev->sdev_lock, flags);
 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
 		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE)
 			goto active;
@@ -217,7 +217,7 @@ static int scsi_check_device_busy(struct
 		if (scmd->request)
 			scmd->request->rq_status = RQ_SCSI_DISCONNECTING;
 	}
-	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
 
 	return 0;
 
@@ -234,7 +234,7 @@ active:
 		}
 	}
 
-	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
 	printk(KERN_ERR "Device busy???\n");
 	return 1;
 }
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi.c rmcmd_lock-2.5-cur/drivers/scsi/scsi.c
--- 2.5-cur/drivers/scsi/scsi.c	Tue Apr 29 12:24:04 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.c	Tue May  6 17:23:14 2003
@@ -219,8 +219,8 @@ static struct scsi_host_cmd_pool scsi_cm
 
 static DECLARE_MUTEX(host_cmd_pool_mutex);
 
-static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
-					    int gfp_mask)
+static struct scsi_cmnd *scsi_alloc_cmd(struct Scsi_Host *shost,
+					       int gfp_mask)
 {
 	struct scsi_cmnd *cmd;
 
@@ -242,71 +242,114 @@ static struct scsi_cmnd *__scsi_get_comm
 	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)
+static void scsi_init_cmd(struct scsi_device *dev, struct scsi_cmnd *cmd)
 {
-	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
-
 	if (likely(cmd != NULL)) {
-		unsigned long flags;
-
 		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);
-		spin_lock_irqsave(&dev->list_lock, flags);
 		list_add_tail(&cmd->list, &dev->cmd_list);
-		spin_unlock_irqrestore(&dev->list_lock, flags);
 	}
+}
+
+/*
+ * Function:	__scsi_get_command()
+ *
+ * Purpose:	Allocate and setup a scsi command block. This function is
+ * 		extern but not exported outside of scsi.
+ *
+ * Arguments:	dev	- parent scsi device
+ *		gfp_mask- allocator flags
+ *
+ * Locks:	Called with sdev_lock held.
+ *
+ * Returns:	The allocated scsi command structure.
+ */
+struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
+{
+	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
 
+	scsi_init_cmd(dev, cmd);
 	return cmd;
 }				
 
 /*
- * Function:	scsi_put_command()
+ * Function:	scsi_get_command()
+ *
+ * Purpose:	Allocate and setup a scsi command block. This function is
+ * 		exported.
+ *
+ * Arguments:	dev	- parent scsi device
+ *		gfp_mask- allocator flags
+ *
+ * Returns:	Nothing.
+ */
+struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
+{
+	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
+	unsigned long flags;
+	
+	spin_lock_irqsave(&dev->sdev_lock, flags);
+	scsi_init_cmd(dev, cmd);
+	spin_unlock_irqrestore(&dev->sdev_lock, flags);
+	return cmd;
+}
+
+/*
+ * Function:	__scsi_put_command()
  *
- * Purpose:	Free a scsi command block
+ * Purpose:	Free a scsi command block. This function is extern but not
+ * 		exported outside of scsi.
  *
  * Arguments:	cmd	- command block to free
  *
  * Returns:	Nothing.
  *
+ * Locks:	Called with sdev_lock held.
+ *
  * Notes:	The command must not belong to any lists.
  */
-void scsi_put_command(struct scsi_cmnd *cmd)
+void __scsi_put_command(struct scsi_cmnd *cmd)
 {
 	struct Scsi_Host *shost = cmd->device->host;
-	unsigned long flags;
 	
 	/* serious error if the command hasn't come from a device list */
-	spin_lock_irqsave(&cmd->device->list_lock, flags);
 	BUG_ON(list_empty(&cmd->list));
 	list_del_init(&cmd->list);
-	spin_unlock(&cmd->device->list_lock);
-	/* changing locks here, don't need to restore the irq state */
 	spin_lock(&shost->free_list_lock);
 	if (unlikely(list_empty(&shost->free_list))) {
 		list_add(&cmd->list, &shost->free_list);
 		cmd = NULL;
 	}
-	spin_unlock_irqrestore(&shost->free_list_lock, flags);
+	spin_unlock(&shost->free_list_lock);
 
 	if (likely(cmd != NULL))
 		kmem_cache_free(shost->cmd_pool->slab, cmd);
 }
 
 /*
+ * Function:	scsi_put_command()
+ *
+ * Purpose:	Free a scsi command block. This function is exported.
+ *
+ * Arguments:	cmd	- command block to free
+ *
+ * Returns:	Nothing.
+ */
+void scsi_put_command(struct scsi_cmnd *cmd)
+{
+	struct scsi_device *sdev = cmd->device;
+	unsigned long flags;
+	
+	spin_lock_irqsave(&sdev->sdev_lock, flags);
+	__scsi_put_command(cmd);
+	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
+}
+
+/*
  * Function:	scsi_setup_command_freelist()
  *
  * Purpose:	Setup the command freelist for a scsi host.
@@ -1240,7 +1283,7 @@ void scsi_device_put(struct scsi_device 
  * scsi_set_device_offline - set scsi_device offline
  * @sdev:	pointer to struct scsi_device to offline. 
  *
- * Locks:	host_lock held on entry.
+ * Locks:	No scsi locks should be held on entry.
  **/
 void scsi_set_device_offline(struct scsi_device *sdev)
 {
@@ -1251,7 +1294,7 @@ void scsi_set_device_offline(struct scsi
 
 	sdev->online = 0;
 
-	spin_lock_irqsave(&sdev->list_lock, flags);
+	spin_lock_irqsave(&sdev->sdev_lock, flags);
 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
 		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE) {
 			/*
@@ -1264,7 +1307,7 @@ void scsi_set_device_offline(struct scsi
 			list_add_tail(&scmd->eh_entry, &active_list);
 		}
 	}
-	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
 
 	if (!list_empty(&active_list)) {
 		list_for_each_safe(lh, lh_sf, &active_list) {
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi.h rmcmd_lock-2.5-cur/drivers/scsi/scsi.h
--- 2.5-cur/drivers/scsi/scsi.h	Tue Apr 29 11:14:15 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.h	Tue May  6 17:23:14 2003
@@ -327,7 +327,6 @@ struct scsi_device {
 	request_queue_t *request_queue;
 	volatile unsigned short device_busy;	/* commands actually active on low-level */
 	spinlock_t sdev_lock;           /* also the request queue_lock */
-	spinlock_t list_lock;
 	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
 	struct list_head starved_entry;
         Scsi_Cmnd *current_cmnd;	/* currently active command */
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_lib.c rmcmd_lock-2.5-cur/drivers/scsi/scsi_lib.c
--- 2.5-cur/drivers/scsi/scsi_lib.c	Tue Apr 29 11:14:15 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_lib.c	Tue May  6 17:23:14 2003
@@ -528,13 +528,13 @@ static struct scsi_cmnd *scsi_end_reques
 	if (blk_rq_tagged(req))
 		blk_queue_end_tag(q, req);
 	end_that_request_last(req);
+	__scsi_put_command(cmd);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
 	/*
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
-	scsi_put_command(cmd);
 	scsi_queue_next_request(q, NULL);
 	return NULL;
 }
@@ -950,7 +950,7 @@ static int scsi_init_io(struct scsi_cmnd
 			req->current_nr_sectors);
 
 	/* release the command and kill it */
-	scsi_put_command(cmd);
+	__scsi_put_command(cmd);
 	return BLKPREP_KILL;
 }
 
@@ -974,7 +974,7 @@ static int scsi_prep_fn(struct request_q
 		struct scsi_request *sreq = req->special;
 
 		if (sreq->sr_magic == SCSI_REQ_MAGIC) {
-			cmd = scsi_get_command(sreq->sr_device, GFP_ATOMIC);
+			cmd = __scsi_get_command(sreq->sr_device, GFP_ATOMIC);
 			if (unlikely(!cmd))
 				goto defer;
 			scsi_init_cmd_from_req(cmd, sreq);
@@ -985,7 +985,7 @@ static int scsi_prep_fn(struct request_q
 		 * Now try and find a command block that we can use.
 		 */
 		if (!req->special) {
-			cmd = scsi_get_command(sdev, GFP_ATOMIC);
+			cmd = __scsi_get_command(sdev, GFP_ATOMIC);
 			if (unlikely(!cmd))
 				goto defer;
 		} else
@@ -1006,10 +1006,10 @@ static int scsi_prep_fn(struct request_q
 	
 	/*
 	 * FIXME: drop the lock here because the functions below
-	 * expect to be called without the queue lock held.  Also,
-	 * previously, we dequeued the request before dropping the
-	 * lock.  We hope REQ_STARTED prevents anything untoward from
-	 * happening now.
+	 * expect to be called without the queue lock held (except
+	 * __scsi_put_command). Also, previously, we dequeued the request
+	 * before dropping the lock. We hope REQ_STARTED prevents
+	 * anything untoward from happening now.
 	 */
 	if (req->flags & (REQ_CMD | REQ_BLOCK_PC)) {
 		int ret;
@@ -1042,7 +1042,7 @@ static int scsi_prep_fn(struct request_q
 		 */
 		if (unlikely(!sdt->init_command(cmd))) {
 			scsi_release_buffers(cmd);
-			scsi_put_command(cmd);
+			__scsi_put_command(cmd);
 			return BLKPREP_KILL;
 		}
 	}
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_priv.h rmcmd_lock-2.5-cur/drivers/scsi/scsi_priv.h
--- 2.5-cur/drivers/scsi/scsi_priv.h	Tue Apr 29 12:24:04 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_priv.h	Tue May  6 17:23:14 2003
@@ -61,6 +61,8 @@ extern void scsi_host_init(void);
 
 /* scsi.c */
 extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
+extern struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int flags);
+extern void __scsi_put_command(struct scsi_cmnd *cmd);
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
 extern void scsi_done(struct scsi_cmnd *cmd);
diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_scan.c rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c
--- 2.5-cur/drivers/scsi/scsi_scan.c	Tue May  6 16:22:34 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c	Tue May  6 17:23:14 2003
@@ -408,7 +408,6 @@ static struct scsi_device *scsi_alloc_sd
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->cmd_list);
 	INIT_LIST_HEAD(&sdev->starved_entry);
-	spin_lock_init(&sdev->list_lock);
 
 	/*
 	 * Some low level driver could use device->type

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Luben Tuikov]

In this patch you're not really just changing the list_lock
to dev_lock, but you're messing with the allocation code.

The allocation code was intended to be called from
anywhere, call from any context, scsi_get/put_command() to
get/put a command.  It really is THAT simple.  Please leave it
alone.

A few pointers:
- leave the allocation code alone, and subsequently lay off
of the spare-cmd-per-device idea,
- when submitting patches, please write a complete (even if
redundant) summary of _everything_ the patch does, in clear
and precise manner, just like the rest of us.  This will make
the maintainers more likely to actually look at your patch,
rather than them look through you code and try to unravel
the patch.

A few more comments of general nature are inlined in your patch:

Patrick Mansfield wrote:
> Revised patch (against scsi-misc-2.5) based on comments to use the
> sdev->sdev_lock instead of the sdev->list_lock.
> 
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/dpt_i2o.c rmcmd_lock-2.5-cur/drivers/scsi/dpt_i2o.c
> --- 2.5-cur/drivers/scsi/dpt_i2o.c	Tue May  6 16:22:34 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/dpt_i2o.c	Tue May  6 17:23:14 2003
> @@ -2510,7 +2510,7 @@ static void adpt_fail_posted_scbs(adpt_h
>  
>  	list_for_each_entry(d, &pHba->host->my_devices, siblings) {
>  		unsigned long flags;
> -		spin_lock_irqsave(&d->list_lock, flags);
> +		spin_lock_irqsave(&d->sdev_lock, flags);
>  		list_for_each_entry(cmd, &d->cmd_list, list) {
>  			if(cmd->serial_number == 0){
>  				continue;
> @@ -2518,7 +2518,7 @@ static void adpt_fail_posted_scbs(adpt_h
>  			cmd->result = (DID_OK << 16) | (QUEUE_FULL <<1);
>  			cmd->scsi_done(cmd);
>  		}
> -		spin_unlock_irqrestore(&d->list_lock, flags);
> +		spin_unlock_irqrestore(&d->sdev_lock, flags);
>  	}
>  }
>  
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/hosts.c rmcmd_lock-2.5-cur/drivers/scsi/hosts.c
> --- 2.5-cur/drivers/scsi/hosts.c	Tue May  6 16:22:34 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/hosts.c	Tue May  6 17:23:14 2003
> @@ -204,7 +204,7 @@ static int scsi_check_device_busy(struct
>  	 * device.  If any of them are busy, then set the state
>  	 * back to inactive and bail.
>  	 */
> -	spin_lock_irqsave(&sdev->list_lock, flags);
> +	spin_lock_irqsave(&sdev->sdev_lock, flags);
>  	list_for_each_entry(scmd, &sdev->cmd_list, list) {
>  		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE)
>  			goto active;
> @@ -217,7 +217,7 @@ static int scsi_check_device_busy(struct
>  		if (scmd->request)
>  			scmd->request->rq_status = RQ_SCSI_DISCONNECTING;
>  	}
> -	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
>  
>  	return 0;
>  
> @@ -234,7 +234,7 @@ active:
>  		}
>  	}
>  
> -	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
>  	printk(KERN_ERR "Device busy???\n");
>  	return 1;
>  }
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi.c rmcmd_lock-2.5-cur/drivers/scsi/scsi.c
> --- 2.5-cur/drivers/scsi/scsi.c	Tue Apr 29 12:24:04 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.c	Tue May  6 17:23:14 2003
> @@ -219,8 +219,8 @@ static struct scsi_host_cmd_pool scsi_cm
>  
>  static DECLARE_MUTEX(host_cmd_pool_mutex);
>  
> -static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
> -					    int gfp_mask)
> +static struct scsi_cmnd *scsi_alloc_cmd(struct Scsi_Host *shost,
> +					       int gfp_mask)

Please lay off of this already.

The choice of PUT and GET is intentional.  Just leave
this code alone.

>  {
>  	struct scsi_cmnd *cmd;
>  
> @@ -242,71 +242,114 @@ static struct scsi_cmnd *__scsi_get_comm
>  	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)
> +static void scsi_init_cmd(struct scsi_device *dev, struct scsi_cmnd *cmd)
>  {
> -	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
> -
>  	if (likely(cmd != NULL)) {
> -		unsigned long flags;
> -
>  		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);
> -		spin_lock_irqsave(&dev->list_lock, flags);
>  		list_add_tail(&cmd->list, &dev->cmd_list);
> -		spin_unlock_irqrestore(&dev->list_lock, flags);
>  	}
> +}
> +
> +/*
> + * Function:	__scsi_get_command()
> + *
> + * Purpose:	Allocate and setup a scsi command block. This function is
> + * 		extern but not exported outside of scsi.
> + *
> + * Arguments:	dev	- parent scsi device
> + *		gfp_mask- allocator flags
> + *
> + * Locks:	Called with sdev_lock held.

I don't like this in principle.

Spinlocks, especially IRQ ones, are supposed to take the absolute
possible minimum amount of code-area.  And here you're grabbing the
lock elsewhere (in your new version of scsi_get_cmnd()) and then
call this fn.

While the current AND BETTER code preserved the above rule.  It is
nice, succinct and straight to the point.  Let's leave it like this,
please.

There is *no reason* to span boundaries for IRQ locks for such
a simple and straightforward thing as the cmnd allocator.
Unless of course...

> + *
> + * Returns:	The allocated scsi command structure.
> + */
> +struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
> +{
> +	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
>  
> +	scsi_init_cmd(dev, cmd);
>  	return cmd;
>  }				

What is the advantage here?  What if scsi_alloc_cmd() fails?
Then scsi_init_cmd() oopses because you removed the checks...

Please lay off of this code already.

Plus, you're making the nesting level TOO deep for something
as trivial as an allocator.
  
>  /*
> - * Function:	scsi_put_command()
> + * Function:	scsi_get_command()
> + *
> + * Purpose:	Allocate and setup a scsi command block. This function is
> + * 		exported.
> + *
> + * Arguments:	dev	- parent scsi device
> + *		gfp_mask- allocator flags
> + *
> + * Returns:	Nothing.
> + */
> +struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
> +{
> +	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
> +	unsigned long flags;
> +	
> +	spin_lock_irqsave(&dev->sdev_lock, flags);
> +	scsi_init_cmd(dev, cmd);
> +	spin_unlock_irqrestore(&dev->sdev_lock, flags);
> +	return cmd;
> +}
> +
> +/*
> + * Function:	__scsi_put_command()
>   *
> - * Purpose:	Free a scsi command block
> + * Purpose:	Free a scsi command block. This function is extern but not
> + * 		exported outside of scsi.
>   *
>   * Arguments:	cmd	- command block to free
>   *
>   * Returns:	Nothing.
>   *
> + * Locks:	Called with sdev_lock held.
> + *
>   * Notes:	The command must not belong to any lists.
>   */
> -void scsi_put_command(struct scsi_cmnd *cmd)
> +void __scsi_put_command(struct scsi_cmnd *cmd)
>  {
>  	struct Scsi_Host *shost = cmd->device->host;
> -	unsigned long flags;
>  	
>  	/* serious error if the command hasn't come from a device list */
> -	spin_lock_irqsave(&cmd->device->list_lock, flags);
>  	BUG_ON(list_empty(&cmd->list));
>  	list_del_init(&cmd->list);
> -	spin_unlock(&cmd->device->list_lock);
> -	/* changing locks here, don't need to restore the irq state */
>  	spin_lock(&shost->free_list_lock);
>  	if (unlikely(list_empty(&shost->free_list))) {
>  		list_add(&cmd->list, &shost->free_list);
>  		cmd = NULL;
>  	}
> -	spin_unlock_irqrestore(&shost->free_list_lock, flags);
> +	spin_unlock(&shost->free_list_lock);
>  
>  	if (likely(cmd != NULL))
>  		kmem_cache_free(shost->cmd_pool->slab, cmd);
>  }

You see, here you're assuming that the sdev_lock is held
with IRQ... just like you're assuming in your code
that the request queue lock is the same as the sdev lock...

Assumptions like this make for a messy code.  Think modularization.

While I see why you _might_ want a separate allocation
for holding and NOT-holding the spinlock,
with the given structure of the scsi_prep_fn, you DON'T
need it.

>  /*
> + * Function:	scsi_put_command()
> + *
> + * Purpose:	Free a scsi command block. This function is exported.
> + *
> + * Arguments:	cmd	- command block to free
> + *
> + * Returns:	Nothing.
> + */
> +void scsi_put_command(struct scsi_cmnd *cmd)
> +{
> +	struct scsi_device *sdev = cmd->device;
> +	unsigned long flags;
> +	
> +	spin_lock_irqsave(&sdev->sdev_lock, flags);
> +	__scsi_put_command(cmd);
> +	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
> +}
> +

Similar comments as the ones above for the spin_lock
being held accross boundaries, *for no particular reason*.

> +/*
>   * Function:	scsi_setup_command_freelist()
>   *
>   * Purpose:	Setup the command freelist for a scsi host.
> @@ -1240,7 +1283,7 @@ void scsi_device_put(struct scsi_device 
>   * scsi_set_device_offline - set scsi_device offline
>   * @sdev:	pointer to struct scsi_device to offline. 
>   *
> - * Locks:	host_lock held on entry.
> + * Locks:	No scsi locks should be held on entry.
>   **/
>  void scsi_set_device_offline(struct scsi_device *sdev)
>  {
> @@ -1251,7 +1294,7 @@ void scsi_set_device_offline(struct scsi
>  
>  	sdev->online = 0;
>  
> -	spin_lock_irqsave(&sdev->list_lock, flags);
> +	spin_lock_irqsave(&sdev->sdev_lock, flags);
>  	list_for_each_entry(scmd, &sdev->cmd_list, list) {
>  		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE) {
>  			/*
> @@ -1264,7 +1307,7 @@ void scsi_set_device_offline(struct scsi
>  			list_add_tail(&scmd->eh_entry, &active_list);
>  		}
>  	}
> -	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
>  
>  	if (!list_empty(&active_list)) {
>  		list_for_each_safe(lh, lh_sf, &active_list) {
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi.h rmcmd_lock-2.5-cur/drivers/scsi/scsi.h
> --- 2.5-cur/drivers/scsi/scsi.h	Tue Apr 29 11:14:15 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.h	Tue May  6 17:23:14 2003
> @@ -327,7 +327,6 @@ struct scsi_device {
>  	request_queue_t *request_queue;
>  	volatile unsigned short device_busy;	/* commands actually active on low-level */
>  	spinlock_t sdev_lock;           /* also the request queue_lock */

Now comments like this should be gone.  This shows structural flaw
and should be corrected with fixing the code.  Even if it means
two separate locks (we can jump the IRQ as we already do in so many
places in SCSI Core).

In the future *this* comment might NOT be true, but having it here
like this, shifts SCSI Core's structural design into cr*p, i.e.
let's NOT bias developers into thinking that this will be true
forever.

> -	spinlock_t list_lock;
>  	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
>  	struct list_head starved_entry;
>          Scsi_Cmnd *current_cmnd;	/* currently active command */
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_lib.c rmcmd_lock-2.5-cur/drivers/scsi/scsi_lib.c
> --- 2.5-cur/drivers/scsi/scsi_lib.c	Tue Apr 29 11:14:15 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_lib.c	Tue May  6 17:23:14 2003
> @@ -528,13 +528,13 @@ static struct scsi_cmnd *scsi_end_reques
>  	if (blk_rq_tagged(req))
>  		blk_queue_end_tag(q, req);
>  	end_that_request_last(req);
> +	__scsi_put_command(cmd);
>  	spin_unlock_irqrestore(q->queue_lock, flags);
>  
>  	/*
>  	 * This will goose the queue request function at the end, so we don't
>  	 * need to worry about launching another command.
>  	 */
> -	scsi_put_command(cmd);
>  	scsi_queue_next_request(q, NULL);
>  	return NULL;
>  }
> @@ -950,7 +950,7 @@ static int scsi_init_io(struct scsi_cmnd
>  			req->current_nr_sectors);
>  
>  	/* release the command and kill it */
> -	scsi_put_command(cmd);
> +	__scsi_put_command(cmd);
>  	return BLKPREP_KILL;
>  }
>  
> @@ -974,7 +974,7 @@ static int scsi_prep_fn(struct request_q
>  		struct scsi_request *sreq = req->special;
>  
>  		if (sreq->sr_magic == SCSI_REQ_MAGIC) {
> -			cmd = scsi_get_command(sreq->sr_device, GFP_ATOMIC);
> +			cmd = __scsi_get_command(sreq->sr_device, GFP_ATOMIC);
>  			if (unlikely(!cmd))
>  				goto defer;
>  			scsi_init_cmd_from_req(cmd, sreq);
> @@ -985,7 +985,7 @@ static int scsi_prep_fn(struct request_q
>  		 * Now try and find a command block that we can use.
>  		 */
>  		if (!req->special) {
> -			cmd = scsi_get_command(sdev, GFP_ATOMIC);
> +			cmd = __scsi_get_command(sdev, GFP_ATOMIC);
>  			if (unlikely(!cmd))
>  				goto defer;
>  		} else
> @@ -1006,10 +1006,10 @@ static int scsi_prep_fn(struct request_q
>  	
>  	/*
>  	 * FIXME: drop the lock here because the functions below
> -	 * expect to be called without the queue lock held.  Also,
> -	 * previously, we dequeued the request before dropping the
> -	 * lock.  We hope REQ_STARTED prevents anything untoward from
> -	 * happening now.
> +	 * expect to be called without the queue lock held (except
> +	 * __scsi_put_command). Also, previously, we dequeued the request
> +	 * before dropping the lock. We hope REQ_STARTED prevents
> +	 * anything untoward from happening now.
>  	 */
>  	if (req->flags & (REQ_CMD | REQ_BLOCK_PC)) {
>  		int ret;
> @@ -1042,7 +1042,7 @@ static int scsi_prep_fn(struct request_q
>  		 */
>  		if (unlikely(!sdt->init_command(cmd))) {
>  			scsi_release_buffers(cmd);
> -			scsi_put_command(cmd);
> +			__scsi_put_command(cmd);
>  			return BLKPREP_KILL;
>  		}
>  	}
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_priv.h rmcmd_lock-2.5-cur/drivers/scsi/scsi_priv.h
> --- 2.5-cur/drivers/scsi/scsi_priv.h	Tue Apr 29 12:24:04 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_priv.h	Tue May  6 17:23:14 2003
> @@ -61,6 +61,8 @@ extern void scsi_host_init(void);
>  
>  /* scsi.c */
>  extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
> +extern struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int flags);
> +extern void __scsi_put_command(struct scsi_cmnd *cmd);
>  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_done(struct scsi_cmnd *cmd);
> diff -purN -X /home/patman/dontdiff 2.5-cur/drivers/scsi/scsi_scan.c rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c
> --- 2.5-cur/drivers/scsi/scsi_scan.c	Tue May  6 16:22:34 2003
> +++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c	Tue May  6 17:23:14 2003
> @@ -408,7 +408,6 @@ static struct scsi_device *scsi_alloc_sd
>  	INIT_LIST_HEAD(&sdev->same_target_siblings);
>  	INIT_LIST_HEAD(&sdev->cmd_list);
>  	INIT_LIST_HEAD(&sdev->starved_entry);
> -	spin_lock_init(&sdev->list_lock);
>  
>  	/*
>  	 * Some low level driver could use device->type
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Luben

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Patrick Mansfield]

On Thu, May 08, 2003 at 01:18:20PM -0400, Luben Tuikov wrote:
> In this patch you're not really just changing the list_lock
> to dev_lock, but you're messing with the allocation code.

The main intent of the patch is for the main line scsi code path (via
scsi_prep_fn and scsi_end_request) to use only one lock where we use two
locks today. There is no change in the allocation algorithm (unlike the
_separate_ patch to have a spare or more appropriately named cached cmd
per device, but I can repost the latest incarnation if anyone really wants
to see it).

> The allocation code was intended to be called from
> anywhere, call from any context, scsi_get/put_command() to
> get/put a command.  It really is THAT simple.  Please leave it
> alone.

The functions can still can be called from anywhere and with any context
with the patch.

The patch assumes that callers from outside scsi core are not the norm,
this is currently true for all (four) users of get/put from outside scsi
core, and all of those could use something better than scsi_get_command.
The aha can use the scmd supplied by the eh; cpq can do nearly the same
but is still referencing scsi_do_cmd(); gdth could use a different and
better method other than allocating a command used to just find a matching
target.

It is not clear what is best for all code that _should_ be using
scsi_get/put_command - such as on stack allocations or like what we now
have in the aic driver (ahc_linux_dv_target).

[The aic does its own allocation of both a scsi_device and a scsi_cmnd,
but it cannot use the mainline scsi_request_fn and so cannot use the
scsi_allocate_request and friends, as it calls its own queue command when
host->host_self_blocked set.]

> >  
> > -static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
> > -					    int gfp_mask)
> > +static struct scsi_cmnd *scsi_alloc_cmd(struct Scsi_Host *shost,
> > +					       int gfp_mask)
> 
> Please lay off of this already.
> 
> The choice of PUT and GET is intentional.  Just leave
> this code alone.

The use of the names put/get in the kernel code is ambigous - some gets
always allocate, some never allocate.

I don't see why you object to the simple name of a static function - I'm
just trying to avoid another level of naming for scsi_get_command. It is
clear what the function does.

> > +/*
> > + * Function:	__scsi_get_command()
> > + *
> > + * Purpose:	Allocate and setup a scsi command block. This function is
> > + * 		extern but not exported outside of scsi.
> > + *
> > + * Arguments:	dev	- parent scsi device
> > + *		gfp_mask- allocator flags
> > + *
> > + * Locks:	Called with sdev_lock held.
> 
> I don't like this in principle.
> 
> Spinlocks, especially IRQ ones, are supposed to take the absolute
> possible minimum amount of code-area.  And here you're grabbing the
> lock elsewhere (in your new version of scsi_get_cmnd()) and then
> call this fn.

Try posting the above suggestion to lkml and see what response you get, or
perhaps read this epic thread:

http://marc.theaimsgroup.com/?t=104587116200002&r=1&w=2

Spin locks should do what makes most sense - if a lock is rarely used and
almost never contended, it makes little sense to go for lower level
locking granularity. If the cost of holding a lock is cheaper than
releasing and reaquiring it, granularity should not be increased, nor
should a new lock be used.

In scsi_get_command there is currently no contention for dev->list_lock
for SCSI block IO, since we already hold the dev->sdev_lock, so it makes
no sense to have a separate lock. This is our main line code path.

The main user of scsi_put_command for block IO is scsi_end_request, today
it gets and releases sdev_lock, and then gets the list_lock. With the
patch, it gets only the sdev_lock. Again, one less lock for each IO
request.

> While the current AND BETTER code preserved the above rule.  It is
> nice, succinct and straight to the point.  Let's leave it like this,
> please.

> There is *no reason* to span boundaries for IRQ locks for such
> a simple and straightforward thing as the cmnd allocator.
> Unless of course...

Again - the current code when called from scsi_prep_fn holds a lock across
the call, the patch improves calls from scsi_prep_fn by removing the use
of the sdev->list_lock. Not only do we execute less code, but we hold
sdev_lock for a shorter time.

> > +struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
> > +{
> > +	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
> >  
> > +	scsi_init_cmd(dev, cmd);
> >  	return cmd;
> >  }				
> 
> What is the advantage here?  What if scsi_alloc_cmd() fails?
> Then scsi_init_cmd() oopses because you removed the checks...

On failure it returns NULL since scsi_init_cmd checks for cmd ==
NULL. scsi_init_cmd just consolidates common code.

> Please lay off of this code already.
> 
> Plus, you're making the nesting level TOO deep for something
> as trivial as an allocator.

The nesting level is no deeper for calls from scsi_prep_fn than it is
today. Stack usage is minimal.

> > -	spin_unlock_irqrestore(&shost->free_list_lock, flags);
> > +	spin_unlock(&shost->free_list_lock);
> >  
> >  	if (likely(cmd != NULL))
> >  		kmem_cache_free(shost->cmd_pool->slab, cmd);
> >  }
> 
> You see, here you're assuming that the sdev_lock is held
> with IRQ... just like you're assuming in your code
> that the request queue lock is the same as the sdev lock...

It assumes the lock is held with irq, as that is exactly what is happening.

If we want to keep locking minimal, and not have an extraneous unlock/lock
in the scsi_request_fn, we have to code knowing that queue_lock is the
same as sdev_lock. This is our current model, I am not advocating that it
change given the current blk request function interface where we are
called with a lock held.

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Luben Tuikov]

Patrick Mansfield wrote:
> 
> The main intent of the patch is for the main line scsi code path (via
> scsi_prep_fn and scsi_end_request) to use only one lock where we use two
> locks today.

I got this -- I just don't know how doable it is.
I'd much rather see locks just lock access to objects, rather
than code.

> There is no change in the allocation algorithm (unlike the
> _separate_ patch to have a spare or more appropriately named cached cmd
> per device, but I can repost the latest incarnation if anyone really wants
> to see it).

No.  Your patch changed the allocation code.  Removed some
checks, renamed a function or two, etc.

>>>+/*
>>>+ * Function:	__scsi_get_command()
>>>+ *
>>>+ * Purpose:	Allocate and setup a scsi command block. This function is
>>>+ * 		extern but not exported outside of scsi.
>>>+ *
>>>+ * Arguments:	dev	- parent scsi device
>>>+ *		gfp_mask- allocator flags
>>>+ *
>>>+ * Locks:	Called with sdev_lock held.
>>
>>I don't like this in principle.
>>
>>Spinlocks, especially IRQ ones, are supposed to take the absolute
>>possible minimum amount of code-area.  And here you're grabbing the
>>lock elsewhere (in your new version of scsi_get_cmnd()) and then
>>call this fn.
> 
> 
> Try posting the above suggestion to lkml and see what response you get, or
> perhaps read this epic thread:
> 
> http://marc.theaimsgroup.com/?t=104587116200002&r=1&w=2

See the word ``principle'' up there, which I used.

The thing is that principles can be _proven_ formally, and special
cases are just that, special cases, out of the norm.

The thing is, if you've got too many special cases, you get spaghetti
code, unmaintanable and cumbersome.

> Spin locks should do what makes most sense - if a lock is rarely used and

No!

A principle should be applied when using spinlocks!  Else you get
deadlocks or unmaintanable code.  The dinosoaur book explains
this nicely (it was my OS course book in university).

> almost never contended, it makes little sense to go for lower level
> locking granularity.

Think IRQ.

> If the cost of holding a lock is cheaper than
> releasing and reaquiring it, granularity should not be increased, nor
> should a new lock be used.

The longer you hold the lock, the more the contention for it;
the less you hold the lock, the more cost for reaquiring it if
need be -- so there's a trade off.

> In scsi_get_command there is currently no contention for dev->list_lock
> for SCSI block IO, since we already hold the dev->sdev_lock, so it makes
> no sense to have a separate lock. This is our main line code path.

Yes, I understand this, and I would like to say that I have absolutely
no opinion on this infrastructure.  I'm not quite fond of it...

Yes, I understand the point of the request queue lock + sdev lock +
the fact that you want to integrate this with the allocator so to
use one lock, etc, etc, etc, but I draw blank -- we'll just wait
and see what perspires -- I hope something elegant eventually.

I think that one cannot _pretend_ that the request_q lock is NOT
the sdev_lock and code as if they are different.  It just doesn't work.

A decision will have to be made if they are to be different, or not ever.
I think this will consolidate things a lot.

I think that the questions are: what _object_ is lock X protecting?
and: will just one lock do? (for the infrastrucure which one wants
to design, or which infrastructure the problem warrants).

Maybe sdev_lock is too general?  Maybe a separate lock for
the request_q will improve the infrastructure? (see bottom of message)

> The main user of scsi_put_command for block IO is scsi_end_request, today
> it gets and releases sdev_lock, and then gets the list_lock. With the
> patch, it gets only the sdev_lock. Again, one less lock for each IO
> request.

Oh, yes, I get that.  This isn't the problem though.

> It assumes the lock is held with irq, as that is exactly what is happening.

Rhetorical: can you always guarantee it?
 
> If we want to keep locking minimal, and not have an extraneous unlock/lock
> in the scsi_request_fn, we have to code knowing that queue_lock is the
> same as sdev_lock.

This is the pickle...

> This is our current model, I am not advocating that it
> change given the current blk request function interface where we are
> called with a lock held.

How about separate request_q lock and sdev_lock?

This way SCSI Core will not have to be aware of the request_q lock,
unless it calls the block routines.

This also makes the block implementation and its injection fn,
scsi_request_fn, modular, so that if the block impementation
changes, we get minimal hit on SCSI Core's implementation.

This also will make it possible for SCSI Core to obtain the 
sdev_lock separately from the request_q lock, say for other
purposes.

I also think that sdev_lock is too general... it's used in
few places (only 1 functional) and I can see that hch has
slated that usage for improvement.

-- 
Luben

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Patrick Mansfield]

On Thu, May 08, 2003 at 11:10:53PM -0400, Luben Tuikov wrote:

> I'd much rather see locks just lock access to objects, rather
> than code.

I totally agree with that approach - lock the data not code.

> No.  Your patch changed the allocation code.  Removed some
> checks, renamed a function or two, etc.

I mean the underlying code - we use the cache alloc, and if that fails,
we try to use the per host spare.

> The thing is, if you've got too many special cases, you get spaghetti
> code, unmaintanable and cumbersome.

IMO I'm reducing the special case: use one lock to protect all of the
scsi_device data.

> > Spin locks should do what makes most sense - if a lock is rarely used and
> 
> No!
> 
> A principle should be applied when using spinlocks!  Else you get
> deadlocks or unmaintanable code.  The dinosoaur book explains
> this nicely (it was my OS course book in university).
> 
> > almost never contended, it makes little sense to go for lower level
> > locking granularity.
> 
> Think IRQ.

Yes, but if we are already holding the lock and already blocking IRQ's
getting another lock increases the hold time and the time IRQ's are
blocked.

> I think that one cannot _pretend_ that the request_q lock is NOT
> the sdev_lock and code as if they are different.  It just doesn't work.

Yes.

> I think that the questions are: what _object_ is lock X protecting?
> and: will just one lock do? (for the infrastrucure which one wants
> to design, or which infrastructure the problem warrants).
> 
> Maybe sdev_lock is too general?  Maybe a separate lock for
> the request_q will improve the infrastructure? (see bottom of message)

> How about separate request_q lock and sdev_lock?
> 
> This way SCSI Core will not have to be aware of the request_q lock,
> unless it calls the block routines.
> 
> This also makes the block implementation and its injection fn,
> scsi_request_fn, modular, so that if the block impementation
> changes, we get minimal hit on SCSI Core's implementation.
> 
> This also will make it possible for SCSI Core to obtain the 
> sdev_lock separately from the request_q lock, say for other
> purposes.

If we had a request function that took one req, and was called with no
lock held the above would be fine (similiar in that we should not hold
host_lock when we call the LLDD queuecommand in scsi_dispatch_cmd). I
think it is best for now to match the current interface (infrastructure)
and have only one lock so we do not release and acquire another lock in
scsi_request_fn.

But, it would be interesting to further split the lock and see what happens.

> I also think that sdev_lock is too general... it's used in
> few places (only 1 functional) and I can see that hch has
> slated that usage for improvement.

Not sure what you mean in the above - IMO we should use it to protect
scsi_device data (not including sdev->siblings). If this is to general and
we see contention, then it can be modified.

There are scsi_device fields (set in scsi_scan.c) that are invariant after
scanning that don't need to be locked - like single_lun, borken, etc.
online is not one of them.

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock (take 2)

[Luben Tuikov]

Patrick Mansfield wrote:
> 
> IMO I'm reducing the special case: use one lock to protect all of the
> scsi_device data.

I'm not sure how well this will play in the future.
With refcounts is there a need to lock *all* of the scsi_device
struct?  In this regard I think that the sdev lock is too general.

> Yes, but if we are already holding the lock and already blocking IRQ's
> getting another lock increases the hold time and the time IRQ's are
> blocked.

Yes, locally.  But if this approach makes you break the
infrastructure which you're striving to get, another lock
would make sense from design point of view.

Then also remember that spinlocks are supposed to be held for
the minimum amount of time -- so I'd rather see another
spinlock _and_ a better infrastructure design, rather than
one lock and a messy code structure.

> If we had a request function that took one req, and was called with no
> lock held the above would be fine (similiar in that we should not hold
> host_lock when we call the LLDD queuecommand in scsi_dispatch_cmd). I
> think it is best for now to match the current interface (infrastructure)
> and have only one lock so we do not release and acquire another lock in
> scsi_request_fn.
> 
> But, it would be interesting to further split the lock and see what happens.

I just think that it's not that great an idea to share
locks across subsystems.  Just as long as we stick to the rule
of holding _a_ sdev_lock for the smallest amount of time elsewhere
in SCSI Core, then obtaining it in scsi_request_fn while the
request_q lock (separate) is being held is not that big of a deal.

I.e. let the request_q lock lock the request_q, and sdev_lock
(possibly to be broken up) lock the device struct members
(or whatever is left/needed).  But I'd much rather see the use
of atomics (where possible) and bit ops.

> Not sure what you mean in the above - IMO we should use it to protect
> scsi_device data (not including sdev->siblings). If this is to general and
> we see contention, then it can be modified.

I'm thinking of properly setting up struct scsi_target { ... }, this will
eliminate sdev->siblings and will properly reflect SAM-3.

Furthermore, SANs will notify the LLDD when a device (target) appears
on the fabric, and then it's up to SCSI Core to obtain the devices (LUs)
of the target, via REPORT LUNS.

Is this worth it? (at this point)
 
> There are scsi_device fields (set in scsi_scan.c) that are invariant after
> scanning that don't need to be locked - like single_lun, borken, etc.
> online is not one of them.

... the more reason to un-generalize sdev_lock, use atomics (where possible)
and bit ops.

-- 
Luben