[PATCH] scsi-misc-2.5 remove scsi_device list_lock

[PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Patrick Mansfield]

Patch against scsi-misc-2.5 to use the sdev->sdev_lock (or queue_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 Mar 11 11:23:06 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/dpt_i2o.c	Thu Apr 24 09:14:04 2003
@@ -2512,7 +2512,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;
@@ -2520,7 +2520,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	Mon Apr 21 16:15:43 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/hosts.c	Thu Apr 24 09:14:04 2003
@@ -214,7 +214,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->request_queue->queue_lock, flags);
 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
 		if (scmd->request && scmd->request->rq_status != RQ_INACTIVE)
 			goto active;
@@ -227,7 +227,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->request_queue->queue_lock, flags);
 
 	return 0;
 
@@ -244,7 +244,7 @@ active:
 		}
 	}
 
-	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	spin_unlock_irqrestore(sdev->request_queue->queue_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	Mon Apr 21 16:15:43 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.c	Thu Apr 24 09:14:04 2003
@@ -254,7 +254,7 @@ 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,
+static struct scsi_cmnd *scsi_alloc_cmd(struct Scsi_Host *shost,
 					    int gfp_mask)
 {
 	struct scsi_cmnd *cmd;
@@ -278,59 +278,82 @@ static struct scsi_cmnd *__scsi_get_comm
 }
 
 /*
- * Function:	scsi_get_command()
+ * Function:	__scsi_get_command()
  *
- * Purpose:	Allocate and setup a scsi command block
+ * 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 queue_lock held.
+ *
  * Returns:	The allocated scsi command structure.
  */
-struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
+struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
 {
-	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
+	struct scsi_cmnd *cmd = scsi_alloc_cmd(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);
 	}
 
 	return cmd;
 }				
 
 /*
- * Function:	scsi_put_command()
+ * Function:	scsi_get_command()
+ *
+ * Purpose:	Allocate and setup a scsi command block. This function is
+ * 		exported.
+ *
+ * Arguments:	cmd	- command block to free
+ *
+ * Returns:	Nothing.
+ *
+ * Notes:	Mabye drop the gpf_mask argument, as we are (currently) always
+ * 		called with GFP_KERNEL.
+ */
+struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
+{
+	struct scsi_cmnd *cmd;
+	unsigned long flags;
+	
+	spin_lock_irqsave(dev->request_queue->queue_lock, flags);
+	cmd = __scsi_get_command(dev, gfp_mask);
+	spin_unlock_irqrestore(dev->request_queue->queue_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 queue_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);
+	spin_lock_irqsave(&shost->free_list_lock, flags);
 	if (unlikely(list_empty(&shost->free_list))) {
 		list_add(&cmd->list, &shost->free_list);
 		cmd = NULL;
@@ -342,6 +365,25 @@ void scsi_put_command(struct scsi_cmnd *
 }
 
 /*
+ * 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->request_queue->queue_lock, flags);
+	__scsi_put_command(cmd);
+	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+}
+
+/*
  * Function:	scsi_setup_command_freelist()
  *
  * Purpose:	Setup the command freelist for a scsi host.
@@ -1298,7 +1340,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)
 {
@@ -1307,9 +1349,8 @@ void scsi_set_device_offline(struct scsi
 	struct list_head *lh, *lh_sf;
 	unsigned long flags;
 
+	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	sdev->online = FALSE;
-
-	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) {
 			/*
@@ -1323,7 +1364,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->request_queue->queue_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	Thu Apr 24 08:58:43 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi.h	Thu Apr 24 09:14:04 2003
@@ -413,7 +413,9 @@ extern void scsi_exit_queue(void);
 extern int scsi_dispatch_cmd(Scsi_Cmnd * SCpnt);
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
+extern struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int flags);
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int flags);
+extern void __scsi_put_command(struct scsi_cmnd *cmd);
 extern void scsi_put_command(struct scsi_cmnd *cmd);
 extern void scsi_adjust_queue_depth(Scsi_Device *, int, int);
 extern int scsi_track_queue_full(Scsi_Device *, int);
@@ -546,7 +548,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	Mon Apr 21 16:24:39 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_lib.c	Thu Apr 24 09:14:04 2003
@@ -527,13 +527,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;
 }
@@ -949,7 +949,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;
 }
 
@@ -973,7 +973,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);
@@ -984,7 +984,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
@@ -1005,10 +1005,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;
@@ -1041,7 +1041,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_proc.c rmcmd_lock-2.5-cur/drivers/scsi/scsi_proc.c
--- 2.5-cur/drivers/scsi/scsi_proc.c	Thu Apr 24 08:58:43 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_proc.c	Thu Apr 24 09:14:04 2003
@@ -360,7 +360,8 @@ static void scsi_dump_status(int level)
 		list_for_each_entry(SDpnt, &shpnt->my_devices, siblings) {
 			unsigned long flags;
 
-			spin_lock_irqsave(&SDpnt->list_lock, flags);
+			spin_lock_irqsave(SDpnt->request_queue->queue_lock,
+					  flags);
 			list_for_each_entry(SCpnt, &SDpnt->cmd_list, list) {
 				/*  (0) h:c:t:l (dev sect nsect cnumsec sg) (ret all flg) (to/cmd to ito) cmd snse result %d %x      */
 				printk(KERN_INFO "(%3d) %2d:%1d:%2d:%2d (%6s %4llu %4ld %4ld %4x %1d) (%1d %1d 0x%2x) (%4d %4d %4d) 0x%2.2x 0x%2.2x 0x%8.8x\n",
@@ -391,7 +392,8 @@ static void scsi_dump_status(int level)
 				       SCpnt->sense_buffer[2],
 				       SCpnt->result);
 			}
-			spin_unlock_irqrestore(&SDpnt->list_lock, flags);
+			spin_unlock_irqrestore(SDpnt->request_queue->queue_lock,
+					       flags);
 		}
 	}
 }
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	Thu Apr 24 08:58:43 2003
+++ rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c	Thu Apr 24 09:14:04 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

[PATCH] scsi-misc-2.5 user per-device spare command

[Patrick Mansfield]

Patch against scsi-misc-2.5

Use a per-device spare command rather than a per-host spare.

Use the sdev->sdev_lock (or queue_lock) to protect use of the spare
command rather than the free_list_lock.

diff -purN -X /home/patman/dontdiff rmcmd_lock-2.5-cur/drivers/scsi/hosts.h rmhost_flock-2.5-cur/drivers/scsi/hosts.h
--- rmcmd_lock-2.5-cur/drivers/scsi/hosts.h	Sat Apr 12 16:25:38 2003
+++ rmhost_flock-2.5-cur/drivers/scsi/hosts.h	Thu Apr 24 09:14:47 2003
@@ -378,8 +378,6 @@ struct Scsi_Host
     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 */
     struct list_head      starved_list;
 
     spinlock_t		  default_lock;
diff -purN -X /home/patman/dontdiff rmcmd_lock-2.5-cur/drivers/scsi/scsi.c rmhost_flock-2.5-cur/drivers/scsi/scsi.c
--- rmcmd_lock-2.5-cur/drivers/scsi/scsi.c	Thu Apr 24 09:14:04 2003
+++ rmhost_flock-2.5-cur/drivers/scsi/scsi.c	Thu Apr 24 09:14:47 2003
@@ -254,26 +254,20 @@ static struct scsi_host_cmd_pool scsi_cm
 
 static DECLARE_MUTEX(host_cmd_pool_mutex);
 
-static struct scsi_cmnd *scsi_alloc_cmd(struct Scsi_Host *shost,
+static struct scsi_cmnd *scsi_alloc_cmd(struct scsi_device *sdev,
 					    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);
-	}
-
+	/*
+	 * Use any spare command first.
+	 */
+	cmd = sdev->spare_cmd;
+	if (cmd != NULL)
+		sdev->spare_cmd = NULL;
+	else 
+		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
+				gfp_mask | sdev->host->cmd_pool->gfp_mask);
 	return cmd;
 }
 
@@ -292,7 +286,7 @@ static struct scsi_cmnd *scsi_alloc_cmd(
  */
 struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
 {
-	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev->host, gfp_mask);
+	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev, gfp_mask);
 
 	if (likely(cmd != NULL)) {
 		memset(cmd, 0, sizeof(*cmd));
@@ -347,21 +341,13 @@ struct scsi_cmnd *scsi_get_command(struc
  */
 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 */
 	BUG_ON(list_empty(&cmd->list));
-	list_del_init(&cmd->list);
-	spin_lock_irqsave(&shost->free_list_lock, flags);
-	if (unlikely(list_empty(&shost->free_list))) {
-		list_add(&cmd->list, &shost->free_list);
-		cmd = NULL;
-	}
-	spin_unlock_irqrestore(&shost->free_list_lock, flags);
-
-	if (likely(cmd != NULL))
-		kmem_cache_free(shost->cmd_pool->slab, cmd);
+	list_del(&cmd->list);
+	if (!cmd->device->spare_cmd)
+		cmd->device->spare_cmd = cmd;
+	else
+		kmem_cache_free(cmd->device->host->cmd_pool->slab, cmd);
 }
 
 /*
@@ -384,6 +370,41 @@ void scsi_put_command(struct scsi_cmnd *
 }
 
 /*
+ * Function:	scsi_setup_sdev_spare_command()
+ *
+ * Purpose:	Setup the spare commands for a scsi_device
+ *
+ * Arguments:	sdev - host to allocate spares for
+ *
+ * Returns:	0 if no error, else -ENOMEM.
+ */
+int scsi_setup_sdev_spare_command(struct scsi_device *sdev)
+{
+	sdev->spare_cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
+			       GFP_KERNEL | sdev->host->cmd_pool->gfp_mask);
+	if (sdev->spare_cmd)
+		return 0;
+	else
+		return -ENOMEM;
+}
+
+/*
+ * Function:	scsi_destroy_sdev_spare_command()
+ *
+ * Purpose:	destroy the spare commands for a scsi_device
+ *
+ * Arguments:	sdev - host to free spares from
+ *
+ * Returns:	0 if no error, else -ENOMEM.
+ */
+void scsi_destroy_sdev_spare_command(struct scsi_device *sdev)
+{
+	if (sdev->spare_cmd)
+		kmem_cache_free(sdev->host->cmd_pool->slab, sdev->spare_cmd);
+	sdev->spare_cmd = NULL;
+}
+
+/*
  * Function:	scsi_setup_command_freelist()
  *
  * Purpose:	Setup the command freelist for a scsi host.
@@ -395,10 +416,6 @@ void scsi_put_command(struct scsi_cmnd *
 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
@@ -413,25 +430,11 @@ int scsi_setup_command_freelist(struct S
 		if (!pool->slab)
 			goto fail;
 	}
-
 	pool->users++;
 	shost->cmd_pool = pool;
 	up(&host_cmd_pool_mutex);
-
-	/*
-	 * Get one backup command for this host.
-	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
-			GFP_KERNEL | shost->cmd_pool->gfp_mask);
-	if (!cmd)
-		goto fail2;
-	list_add(&cmd->list, &shost->free_list);		
 	return 0;
 
- fail2:
-	if (!--pool->users)
-		kmem_cache_destroy(pool->slab);
-	return -ENOMEM;
  fail:
 	up(&host_cmd_pool_mutex);
 	return -ENOMEM;
@@ -447,14 +450,6 @@ int scsi_setup_command_freelist(struct S
  */
 void scsi_destroy_command_freelist(struct Scsi_Host *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(shost->cmd_pool->slab, cmd);
-	}
-
 	down(&host_cmd_pool_mutex);
 	if (!--shost->cmd_pool->users)
 		kmem_cache_destroy(shost->cmd_pool->slab);
diff -purN -X /home/patman/dontdiff rmcmd_lock-2.5-cur/drivers/scsi/scsi.h rmhost_flock-2.5-cur/drivers/scsi/scsi.h
--- rmcmd_lock-2.5-cur/drivers/scsi/scsi.h	Thu Apr 24 09:14:04 2003
+++ rmhost_flock-2.5-cur/drivers/scsi/scsi.h	Thu Apr 24 09:14:47 2003
@@ -411,6 +411,8 @@ extern void scsi_exit_queue(void);
  * Prototypes for functions in scsi.c
  */
 extern int scsi_dispatch_cmd(Scsi_Cmnd * SCpnt);
+extern int scsi_setup_sdev_spare_command(struct scsi_device *sdev);
+extern void scsi_destroy_sdev_spare_command(struct scsi_device *sdev);
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
 extern struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int flags);
@@ -550,6 +552,7 @@ struct scsi_device {
 	spinlock_t sdev_lock;           /* also the request queue_lock */
 	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
 	struct list_head starved_entry;
+	struct scsi_cmnd *spare_cmd;
         Scsi_Cmnd *current_cmnd;	/* currently active command */
 	unsigned short queue_depth;	/* How deep of a queue we want */
 	unsigned short last_queue_full_depth; /* These two are used by */
diff -purN -X /home/patman/dontdiff rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c rmhost_flock-2.5-cur/drivers/scsi/scsi_scan.c
--- rmcmd_lock-2.5-cur/drivers/scsi/scsi_scan.c	Thu Apr 24 09:14:04 2003
+++ rmhost_flock-2.5-cur/drivers/scsi/scsi_scan.c	Thu Apr 24 09:14:47 2003
@@ -422,9 +422,13 @@ static struct scsi_device *scsi_alloc_sd
 	sdev->borken = 1;
 
 	spin_lock_init(&sdev->sdev_lock);
+
+	if (scsi_setup_sdev_spare_command(sdev))
+		goto out_free_dev;
+
 	sdev->request_queue = scsi_alloc_queue(sdev);
 	if (!sdev->request_queue)
-		goto out_free_dev;
+		goto out_free_cmd;
 
 	sdev->request_queue->queuedata = sdev;
 	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
@@ -465,6 +469,8 @@ static struct scsi_device *scsi_alloc_sd
 
 out_free_queue:
 	scsi_free_queue(sdev->request_queue);
+out_free_cmd:
+	scsi_destroy_sdev_spare_command(sdev);
 out_free_dev:
 	kfree(sdev);
 out:
@@ -487,6 +493,7 @@ static void scsi_free_sdev(struct scsi_d
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
 
+	scsi_destroy_sdev_spare_command(sdev);
 	if (sdev->request_queue)
 		scsi_free_queue(sdev->request_queue);
 	if (sdev->host->hostt->slave_destroy)

[PATCH] scsi-misc-2.5 fold scsi_alloc_cmd into __scsi_get_command

[Patrick Mansfield]

Patch against scsi-misc-2.5

scsi_alloc_cmd is real simple now, just fold it into __scsi_get_command.

diff -purN -X /home/patman/dontdiff rmhost_flock-2.5-cur/drivers/scsi/scsi.c rmsacmd-2.5-cur/drivers/scsi/scsi.c
--- rmhost_flock-2.5-cur/drivers/scsi/scsi.c	Thu Apr 24 09:14:47 2003
+++ rmsacmd-2.5-cur/drivers/scsi/scsi.c	Thu Apr 24 09:15:07 2003
@@ -254,23 +254,6 @@ static struct scsi_host_cmd_pool scsi_cm
 
 static DECLARE_MUTEX(host_cmd_pool_mutex);
 
-static struct scsi_cmnd *scsi_alloc_cmd(struct scsi_device *sdev,
-					    int gfp_mask)
-{
-	struct scsi_cmnd *cmd;
-
-	/*
-	 * Use any spare command first.
-	 */
-	cmd = sdev->spare_cmd;
-	if (cmd != NULL)
-		sdev->spare_cmd = NULL;
-	else 
-		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
-				gfp_mask | sdev->host->cmd_pool->gfp_mask);
-	return cmd;
-}
-
 /*
  * Function:	__scsi_get_command()
  *
@@ -286,7 +269,17 @@ static struct scsi_cmnd *scsi_alloc_cmd(
  */
 struct scsi_cmnd *__scsi_get_command(struct scsi_device *dev, int gfp_mask)
 {
-	struct scsi_cmnd *cmd = scsi_alloc_cmd(dev, gfp_mask);
+	struct scsi_cmnd *cmd;
+
+	/*
+	 * Use any spare command first.
+	 */
+	cmd = dev->spare_cmd;
+	if (cmd != NULL)
+		dev->spare_cmd = NULL;
+	else 
+		cmd = kmem_cache_alloc(dev->host->cmd_pool->slab,
+				gfp_mask | dev->host->cmd_pool->gfp_mask);
 
 	if (likely(cmd != NULL)) {
 		memset(cmd, 0, sizeof(*cmd));

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Christoph Hellwig]

> -		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){

<snip>

> -	spin_lock_irqsave(&sdev->list_lock, flags);
> +	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
>  	list_for_each_entry(scmd, &sdev->cmd_list, list) {


Oh, please, please use the lock consistantly.  Yes, I know that the
two are the same currently, but it might not be obvious to every reader
and queue_lock might change to something else in the future.

> -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);
> +	spin_lock_irqsave(&shost->free_list_lock, flags);

As this is always called under sdev_lock you don't need to care for
irq disabling here.

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Christoph Hellwig]

On Thu, Apr 24, 2003 at 10:03:17AM -0700, Patrick Mansfield wrote:
> Patch against scsi-misc-2.5
> 
> Use a per-device spare command rather than a per-host spare.

Why?  This means we'll have a much bigger number of spare commands
around.

> +	/*
> +	 * Use any spare command first.
> +	 */
> +	cmd = sdev->spare_cmd;
> +	if (cmd != NULL)
> +		sdev->spare_cmd = NULL;
> +	else 
> +		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
> +				gfp_mask | sdev->host->cmd_pool->gfp_mask);

This logical is flawed.  We don't need a spare command if we always use
it first.  In addition the sdev->spare_cmd access is racy.

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Jens Axboe]

On Fri, Apr 25 2003, Christoph Hellwig wrote:
> > -		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){
> 
> <snip>
> 
> > -	spin_lock_irqsave(&sdev->list_lock, flags);
> > +	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
> >  	list_for_each_entry(scmd, &sdev->cmd_list, list) {
> 
> 
> Oh, please, please use the lock consistantly.  Yes, I know that the
> two are the same currently, but it might not be obvious to every reader
> and queue_lock might change to something else in the future.

Again... Besides, this alias _must_ only be used by the block layer.
Every other driver should use the real one.

-- 
Jens Axboe

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Luben Tuikov]

Christoph Hellwig wrote:
>>-		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){
> 
> 
> <snip>
> 
>>-	spin_lock_irqsave(&sdev->list_lock, flags);
>>+	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
>> 	list_for_each_entry(scmd, &sdev->cmd_list, list) {
> 
> 
> 
> Oh, please, please use the lock consistantly.  Yes, I know that the
> two are the same currently, but it might not be obvious to every reader
> and queue_lock might change to something else in the future.

I concur.

-- 
Luben

P.S. This same thing was discussed before at the intro of starved
devs's code, seems it didn't get through then, maybe it will now...

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Luben Tuikov]

Christoph Hellwig wrote:
> On Thu, Apr 24, 2003 at 10:03:17AM -0700, Patrick Mansfield wrote:
> 
> 
>>+	/*
>>+	 * Use any spare command first.
>>+	 */
>>+	cmd = sdev->spare_cmd;
>>+	if (cmd != NULL)
>>+		sdev->spare_cmd = NULL;
>>+	else 
>>+		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
>>+				gfp_mask | sdev->host->cmd_pool->gfp_mask);
> 
> 
> This logical is flawed.  We don't need a spare command if we always use
> it first.  In addition the sdev->spare_cmd access is racy.

Good call Christoph!

I was going to comment on this last night, but it was too late
and I only marked the messages for this morning.

Patrick, take a look at how the logic now works, and if you're
doing something similar, try to at least emulate it, as if not
improve on it (although Christoph has already done this).

Here are a few pointers:

1. Use the cache *first*, and if it's depleted, *then*
use the spare command struct.

2. Why spare_cmd is a pointer? Why? Why?
Wouldn't it be much more *flexible* to be a list_head,
so that maybe we can hook up more commands in the future?
I.e. keep your options open...

3. As Christoph pointed out, access to your spare is racy.

4. Why are you using list_del()???? You should use list_del_init()!
I can easily envision a hard to catch bug (and quite rare), when
some LLDD gets the spare and does a check on the list_head...

Furthermore, cannot you see the infrastructure to which we've
tried to get closer to?  More specifically a command struct travels
list_heads as it comes out of the cache/spare_list and comes
back to the cache/spare_list.

-- 
Luben

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Patrick Mansfield]

Christoph -

On Fri, Apr 25, 2003 at 11:12:27AM +0100, Christoph Hellwig wrote:
> On Thu, Apr 24, 2003 at 10:03:17AM -0700, Patrick Mansfield wrote:
> > Patch against scsi-misc-2.5
> > 
> > Use a per-device spare command rather than a per-host spare.
> 
> Why?  This means we'll have a much bigger number of spare commands
> around.

So we are guaranteed to be able to have at least one IO in flight for all
devices. With a per-host spare, some device(s) might have to wait
(generally polling based on an interval dependent on whatever blk_plug_device
gives us) for a command to become available, and it is not clear what the
IO behaviour - especially for swap - will be in such cases.

It simplifies the code, and allows removal of a lock from the IO
completion path.

[I'm not sure what happens for swap striping or md. Is swap stripe size
always a multiple of the page size?  So one IO completion to a swap device
(striped or not) is guaranteed to free up at least one page? What about
users putting swap on top of md, RAID-0 or RAID-5? Is this not allowed,
or considered a dumb configuration?]

For systems with one disk we are better off (than with a per host spare) -
we have one spare, we always use it, and we don't allocate or use the per
host free_list_lock.

For larger numbers of disks, if we normally have IO in flight to all
devices, there is not much difference. Generally, we have one extra
command per device that does not have IO in flight.

Ideally we should have some sysfs attribute to specify whether a spare (or
spares) is needed or not, default to no spare, and then have some user
land method to normally add a spare for any scsi swap devices (maybe
swapon?).

But, a sysfs attribute (one inode) would use almost as much memory as the
spare command itself (340 bytes, but should be smaller), and adds a bit of
code to the put case. (we could save almost as much memory as we are
allocating for the spare just by removing for example the scsi "rev" sysfs
attribute, or even more by exposing all of the inquiry as a binary file).

If the sysfs attributes were lighter, something lighter is available, or
general sentiment is to just go ahead and use a sysfs attribute, I can
change the code to use it. (I don't think having more than one spare per
device is useful.)

> > +	/*
> > +	 * Use any spare command first.
> > +	 */
> > +	cmd = sdev->spare_cmd;
> > +	if (cmd != NULL)
> > +		sdev->spare_cmd = NULL;
> > +	else 
> > +		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
> > +				gfp_mask | sdev->host->cmd_pool->gfp_mask);
> 
> This logical is flawed.  We don't need a spare command if we always use
> it first.  In addition the sdev->spare_cmd access is racy.

I had originally coded it to only use the spare on failure of the cache,
but then we would normally never use the spare (similiar to our per-host
spare today).

The sdev->sdev_lock (i.e. queue_lock) protects access to sdev->spare_cmd.

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Christoph Hellwig]

On Fri, Apr 25, 2003 at 09:37:18AM -0700, Patrick Mansfield wrote:
> So we are guaranteed to be able to have at least one IO in flight for all
> devices. With a per-host spare, some device(s) might have to wait
> (generally polling based on an interval dependent on whatever blk_plug_device
> gives us) for a command to become available, and it is not clear what the
> IO behaviour - especially for swap - will be in such cases.

Ok.

> The sdev->sdev_lock (i.e. queue_lock) protects access to sdev->spare_cmd.

Then you'll sleep with the lock help for !GFP_ATOMIC allocations.  That's
bad.

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Patrick Mansfield]

On Fri, Apr 25, 2003 at 10:12:51AM -0400, Luben Tuikov wrote:

> 1. Use the cache *first*, and if it's depleted, *then*
> use the spare command struct.

Then we would allocate and almost never use the spare. This is a bit
different as compared to the per-host spare, when we shoulid use the
spare only as a last resort.

> 2. Why spare_cmd is a pointer? Why? Why?
> Wouldn't it be much more *flexible* to be a list_head,
> so that maybe we can hook up more commands in the future?
> I.e. keep your options open...

When or if we use more than one spare we can change the code to a
list_head. I don't plan to add such code, so I see no reason to use a
list_head.

> 3. As Christoph pointed out, access to your spare is racy.

spare_cmd is protected via sdev_lock.

> 4. Why are you using list_del()???? You should use list_del_init()!
> I can easily envision a hard to catch bug (and quite rare), when
> some LLDD gets the spare and does a check on the list_head...

__scsi_get_command is always setting the list_head, whether it is the
spare or not. If the LLDD or scsi core got a command with a bad list_head,
that is a bug.

> Furthermore, cannot you see the infrastructure to which we've
> tried to get closer to?  More specifically a command struct travels
> list_heads as it comes out of the cache/spare_list and comes
> back to the cache/spare_list.

If we are not using the list_head post scsi_put_command, there is no
reason to use list_del_init (versus list_del).

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Patrick Mansfield]

On Fri, Apr 25, 2003 at 12:47:46PM +0200, Jens Axboe wrote:

> Again... Besides, this alias _must_ only be used by the block layer.
> Every other driver should use the real one.

So we should never use queue_lock in the scsi code? 

I wasn't sure which way to go, previous code used queue_lock in scsi queue
related code (well almost everywhere) rather than host_lock.

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Christoph Hellwig]

On Fri, Apr 25, 2003 at 09:50:55AM -0700, Patrick Mansfield wrote:
> On Fri, Apr 25, 2003 at 10:12:51AM -0400, Luben Tuikov wrote:
> 
> > 1. Use the cache *first*, and if it's depleted, *then*
> > use the spare command struct.
> 
> Then we would allocate and almost never use the spare. This is a bit
> different as compared to the per-host spare, when we shoulid use the
> spare only as a last resort.

Again, it's no more a spare if you use it first, it's a cache then
and will always be gone when you need it.

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[James Bottomley]

On Fri, 2003-04-25 at 12:37, Patrick Mansfield wrote:
> Christoph -
> 
> On Fri, Apr 25, 2003 at 11:12:27AM +0100, Christoph Hellwig wrote:
> > On Thu, Apr 24, 2003 at 10:03:17AM -0700, Patrick Mansfield wrote:
> > > Patch against scsi-misc-2.5
> > > 
> > > Use a per-device spare command rather than a per-host spare.
> > 
> > Why?  This means we'll have a much bigger number of spare commands
> > around.
> 
> So we are guaranteed to be able to have at least one IO in flight for all
> devices. With a per-host spare, some device(s) might have to wait
> (generally polling based on an interval dependent on whatever blk_plug_device
> gives us) for a command to become available, and it is not clear what the
> IO behaviour - especially for swap - will be in such cases.

Yes, but we all agreed that we really only needed one guaranteed
commmand for the system to make forward progress.  It could be argued
that we only need one globally.  One per host seems a reasonable
compromise.

I don't really buy the in flight I/O argument because under normal
circumstances the slab allocation will cope correctly.  The slab
allocation only fails when we need to grow the slab *and* we're out of
readily available pages, so the system tips into paging mode anyway and
its throughput tanks.

Just the act of pre-allocating a single command means that we pin a slab
page and thus have at least a page full of commands to throw about even
in an out of memory situation.

James

Re: [PATCH] scsi-misc-2.5 remove scsi_device list_lock

[Jens Axboe]

On Fri, Apr 25 2003, Patrick Mansfield wrote:
> On Fri, Apr 25, 2003 at 12:47:46PM +0200, Jens Axboe wrote:
> 
> > Again... Besides, this alias _must_ only be used by the block layer.
> > Every other driver should use the real one.
> 
> So we should never use queue_lock in the scsi code? 

Basically, no. The only place where it _might_ be appropriate, is the
entry points from the block layer where it is already held (for
clarity). That's basically scsi_request_fn only.

-- 
Jens Axboe

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Luben Tuikov]

Patrick Mansfield wrote:
> Christoph -
> 
> On Fri, Apr 25, 2003 at 11:12:27AM +0100, Christoph Hellwig wrote:
> 
>>On Thu, Apr 24, 2003 at 10:03:17AM -0700, Patrick Mansfield wrote:
>>
>>>Patch against scsi-misc-2.5
>>>
>>>Use a per-device spare command rather than a per-host spare.
>>
>>Why?  This means we'll have a much bigger number of spare commands
>>around.
> 
> 
> So we are guaranteed to be able to have at least one IO in flight for all
> devices. With a per-host spare, some device(s) might have to wait
> (generally polling based on an interval dependent on whatever blk_plug_device
> gives us) for a command to become available, and it is not clear what the
> IO behaviour - especially for swap - will be in such cases.
> 
> It simplifies the code, and allows removal of a lock from the IO
> completion path.

I don't buy both arguments.  We've discussed this before as James
pointed out.

Futhermore, we should have an *empirical* result that the current
infrastructure (spare cmd) fails, before braking it and introducing
the *inferior* implementation you suggested.

> For systems with one disk we are better off (than with a per host spare) -
> we have one spare, we always use it, and we don't allocate or use the per
> host free_list_lock.

No, this is NOT true.  Some *devices* (not LLDD) will be able to
have more than ONE active/pending command.

So your solution will take the spare command out, when the
first request is sent to the device and from that point on,
it will try the cache... Uuuurrrggghh (shudders)

(Note what James pointed out for allocating from the slab
and pinning of a page (you can see this from /proc/slabinfo).)

But as soon as we touch the cache to get one command struct,
we already have PAGE_SIZE/sizof(struct scsi_cmnd) - 1 command
structs available... AND the spare gone.... Uuuurrrggghh (shudders)
 
> For larger numbers of disks, if we normally have IO in flight to all
> devices, there is not much difference.

This is not necessarily true (IO to *all* devices at all times).
*And* if there's not much difference, then we should stick with
the current infrastructure because it is WAY bettter.

> Generally, we have one extra
> command per device that does not have IO in flight.

Not with your patch we not!

> Ideally we should have some sysfs attribute to specify whether a spare (or
> spares) is needed or not, default to no spare, and then have some user
> land method to normally add a spare for any scsi swap devices (maybe
> swapon?).

This has been discussed long ago (I think Linus got involved) and it
was voted down.

[Kernel] Settings like this are NOT and SHOULD NOT be user land settable!!!

Giving radio knobs for all kinds of kernel settings to lusers,
will render the kernel untunable and will f*ck the whole thing over.

No!

> But, a sysfs attribute (one inode) would use almost as much memory as the
> spare command itself (340 bytes, but should be smaller), and adds a bit of
> code to the put case. (we could save almost as much memory as we are
> allocating for the spare just by removing for example the scsi "rev" sysfs
> attribute, or even more by exposing all of the inquiry as a binary file).
> 
> If the sysfs attributes were lighter, something lighter is available, or
> general sentiment is to just go ahead and use a sysfs attribute, I can
> change the code to use it. (I don't think having more than one spare per
> device is useful.)

Don't bother changing anything.  This patch should not go in -- not
enough necessity for it (not at all, that is).
 
>>>+	/*
>>>+	 * Use any spare command first.
>>>+	 */
>>>+	cmd = sdev->spare_cmd;
>>>+	if (cmd != NULL)
>>>+		sdev->spare_cmd = NULL;
>>>+	else 
>>>+		cmd = kmem_cache_alloc(sdev->host->cmd_pool->slab,
>>>+				gfp_mask | sdev->host->cmd_pool->gfp_mask);
>>
>>This logical is flawed.  We don't need a spare command if we always use
>>it first.  In addition the sdev->spare_cmd access is racy.
> 
> 
> I had originally coded it to only use the spare on failure of the cache,
> but then we would normally never use the spare (similiar to our per-host
> spare today).

Thats the whole point!!! (all caps -- not typed)

-- 
Luben

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Luben Tuikov]

Patrick Mansfield wrote:
> On Fri, Apr 25, 2003 at 10:12:51AM -0400, Luben Tuikov wrote:
> 
> 
>>1. Use the cache *first*, and if it's depleted, *then*
>>use the spare command struct.
> 
> 
> Then we would allocate and almost never use the spare. This is a bit
> different as compared to the per-host spare, when we shoulid use the
> spare only as a last resort.

No, it is NOT different, because I can see your patch deleting
the good code which currently impements just that.

All we want is for IO to keep going.  It does, so leave it alone.
 
> 
>>2. Why spare_cmd is a pointer? Why? Why?
>>Wouldn't it be much more *flexible* to be a list_head,
>>so that maybe we can hook up more commands in the future?
>>I.e. keep your options open...
> 
> 
> When or if we use more than one spare we can change the code to a
> list_head. I don't plan to add such code, so I see no reason to use a
> list_head.

Yeah, and then you're going to change a WHOLE bunch of code
everywhere.

It's good not to overengineer, but it's even BETTER to know
when to do so and when to NOT do so.

Ideally, commands come out of cache/free_list, travel through
lists as they go about SCSI Core, and go back to the cache/free_list.

So the following axiom: scsi command always belongs to a list.

...

-- 
Luben

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Patrick Mansfield]

On Fri, Apr 25, 2003 at 01:45:58PM -0400, Luben Tuikov wrote:
> Patrick Mansfield wrote:

> No, it is NOT different, because I can see your patch deleting
> the good code which currently impements just that.
> 
> All we want is for IO to keep going.  It does, so leave it alone.

It is not clear that the IO will always make forward progress.

> >>2. Why spare_cmd is a pointer? Why? Why?
> >>Wouldn't it be much more *flexible* to be a list_head,
> >>so that maybe we can hook up more commands in the future?
> >>I.e. keep your options open...
> > 
> > 
> > When or if we use more than one spare we can change the code to a
> > list_head. I don't plan to add such code, so I see no reason to use a
> > list_head.
> 
> Yeah, and then you're going to change a WHOLE bunch of code
> everywhere.

No, maybe a few lines, maybe less code than is needed to actually
support dyanmically changing the number of spared/cached commands.

> It's good not to overengineer, but it's even BETTER to know
> when to do so and when to NOT do so.

Well we should never overengineer, but the term is subjective.

We should not have code for functionallity that we do not and do not plan
to use.

> Ideally, commands come out of cache/free_list, travel through
> lists as they go about SCSI Core, and go back to the cache/free_list.
> 
> So the following axiom: scsi command always belongs to a list.

If they are freed (into cache), they are not on any list.

-- Patrick Mansfield

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Luben Tuikov]

Patrick Mansfield wrote:
> On Fri, Apr 25, 2003 at 01:45:58PM -0400, Luben Tuikov wrote:
> 
>>Patrick Mansfield wrote:
> 
> 
>>No, it is NOT different, because I can see your patch deleting
>>the good code which currently impements just that.
>>
>>All we want is for IO to keep going.  It does, so leave it alone.
> 
> 
> It is not clear that the IO will always make forward progress.

It is -- we have one spare per host, which will be used
when the cache fails.

10 When a commands completes, we will keep going.
20 If no command completes then there's something wrong with
the LLDD or the device, in which case the EH will grab it back.
GOTO 10
 
>>>>2. Why spare_cmd is a pointer? Why? Why?
>>>>Wouldn't it be much more *flexible* to be a list_head,
>>>>so that maybe we can hook up more commands in the future?
>>>>I.e. keep your options open...
>>>
>>>
>>>When or if we use more than one spare we can change the code to a
>>>list_head. I don't plan to add such code, so I see no reason to use a
>>>list_head.
>>
>>Yeah, and then you're going to change a WHOLE bunch of code
>>everywhere.
> 
> 
> No, maybe a few lines, maybe less code than is needed to actually
> support dyanmically changing the number of spared/cached commands.

The whole point is where to draw the line -- I say that
free_list is a good thing and free_ptr is a bad thing.
 
>>It's good not to overengineer, but it's even BETTER to know
>>when to do so and when to NOT do so.
> 
> Well we should never overengineer, but the term is subjective.

No, not never. The point is knowing when to do so and when not to do so.
 
>>Ideally, commands come out of cache/free_list, travel through
>>lists as they go about SCSI Core, and go back to the cache/free_list.
>>
>>So the following axiom: scsi command always belongs to a list.
> 
> 
> If they are freed (into cache), they are not on any list.

This is absolutely irrelevant.

BTW, did you really think that *that* was my point???

Just use your imagination... (next time)

(Patterns, consistency, etc.)

-- 
Luben

Re: [PATCH] scsi-misc-2.5 user per-device spare command

[Patrick Mansfield]

On Fri, Apr 25, 2003 at 12:57:42PM -0400, James Bottomley wrote:
> On Fri, 2003-04-25 at 12:37, Patrick Mansfield wrote:

> > So we are guaranteed to be able to have at least one IO in flight for all
> > devices. With a per-host spare, some device(s) might have to wait
> > (generally polling based on an interval dependent on whatever blk_plug_device
> > gives us) for a command to become available, and it is not clear what the
> > IO behaviour - especially for swap - will be in such cases.
> 
> Yes, but we all agreed that we really only needed one guaranteed
> commmand for the system to make forward progress.  It could be argued
> that we only need one globally.  One per host seems a reasonable
> compromise.
> 
> I don't really buy the in flight I/O argument because under normal
> circumstances the slab allocation will cope correctly.  The slab
> allocation only fails when we need to grow the slab *and* we're out of
> readily available pages, so the system tips into paging mode anyway and
> its throughput tanks.
> 
> Just the act of pre-allocating a single command means that we pin a slab
> page and thus have at least a page full of commands to throw about even
> in an out of memory situation.
> 
> James

But we do not have fair arbitration to the scsi_get_command. When there
are many commands in flight, and when one IO finishes, the command is
freed, then we immediately try to send another command to the same device
and in doing so we try to allocate another command, independent of any
other devices trying to send IO.

If a little or not recently used device (device_busy == 0) then tries to
get a command at this point and fails, it will effectively poll at some
interval, and (generally) race with any devices that have IO in flight.
But the device with IO in flight will try and get a command at a point
that is much more likely to succeed. If the little-used device is trying
to free memory (for swap or page cache, using only swap in my previous
examples was wrong), it might not make progress.

Adding the device that failed to get a command to the starved list would
solve the problem (though might be a bit hard now without creating a lock
hiearchy), as does having a per-device spare/cached command.

I do not have time now to code up something using the starved list in such
cases, or to try and actually hit the problem. This does not mean I agree
that the current code is sufficient in all cases.

I will re-work and resend the first patch (of the three) to use sdev_lock,
and to work for !GFP_ATOMIC, and not push for a per-device spare_cmd or
related patch at this time.

-- Patrick Mansfield