cciss updates for 2.6.6xxx [1/2]

cciss updates for 2.6.6xxx [1/2]

[mikem]

This patch adds per logical device queues to the HP cciss driver. It currently only implements a single lock but when time permits I will provide that funtionality. Thanks to Jeff Garzik for providing some sample code.
This patch built against 2.6.5. Please consider this for inclusion.

mikem
-------------------------------------------------------------------------------

diff -burpN lx265.orig/drivers/block/cciss.c lx265.save/drivers/block/cciss.c
--- lx265.orig/drivers/block/cciss.c	2004-04-03 21:38:20.000000000 -0600
+++ lx265.save/drivers/block/cciss.c	2004-04-06 10:22:08.000000000 -0500
@@ -988,7 +988,7 @@ static int revalidate_allvol(ctlr_info_t
 		drive_info_struct *drv = &(host->drv[i]);
 		if (!drv->nr_blocks)
 			continue;
-		blk_queue_hardsect_size(host->queue, drv->block_size);
+		blk_queue_hardsect_size(drv->queue, drv->block_size);
 		set_capacity(disk, drv->nr_blocks);
 		add_disk(disk);
 	}
@@ -2013,7 +2013,7 @@ static irqreturn_t do_cciss_intr(int irq
 	CommandList_struct *c;
 	unsigned long flags;
 	__u32 a, a1;
-
+	int j;
 
 	/* Is this interrupt for us? */
 	if (( h->access.intr_pending(h) == 0) || (h->interrupts_enabled == 0))
@@ -2059,11 +2059,18 @@ static irqreturn_t do_cciss_intr(int irq
 			}
 		}
 	}
-
 	/*
 	 * See if we can queue up some more IO
+	 * check every disk that exists on this controller 
+	 * and start it's IO
 	 */
-	blk_start_queue(h->queue);
+	for(j=0;j < NWD; j++) {
+		/* make sure the disk has been added and the drive is real */
+		/* because this can be called from the middle of init_one */
+		if(!(h->gendisk[j]->queue) || !(h->drv[j].nr_blocks) )
+			continue;
+		blk_start_queue(h->gendisk[j]->queue);
+	}
 	spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
 	return IRQ_HANDLED;
 }
@@ -2510,7 +2517,6 @@ static void free_hba(int i)
 static int __devinit cciss_init_one(struct pci_dev *pdev,
 	const struct pci_device_id *ent)
 {
-	request_queue_t *q;
 	int i;
 	int j;
 
@@ -2568,13 +2574,6 @@ static int __devinit cciss_init_one(stru
 	}
 
 	spin_lock_init(&hba[i]->lock);
-	q = blk_init_queue(do_cciss_request, &hba[i]->lock);
-	if (!q)
-		goto clean4;
-
-	q->backing_dev_info.ra_pages = READ_AHEAD;
-	hba[i]->queue = q;
-	q->queuedata = hba[i];
 
 	/* Initialize the pdev driver private data. 
 		have it point to hba[i].  */
@@ -2596,6 +2595,19 @@ static int __devinit cciss_init_one(stru
 
 	cciss_procinit(i);
 
+	for(j=0; j<NWD; j++) {
+		drive_info_struct *drv = &(hba[i]->drv[j]);
+		struct gendisk *disk = hba[i]->gendisk[j];
+		request_queue_t *q;
+
+	        q = blk_init_queue(do_cciss_request, &hba[i]->lock);
+		if (!q) {
+			printk(KERN_ERR 
+			   "cciss:  unable to allocate queue for disk %d\n",
+			   j);
+			break;
+		}
+		drv->queue = q;
 	blk_queue_bounce_limit(q, hba[i]->pdev->dma_mask);
 
 	/* This is a hardware imposed limit. */
@@ -2606,21 +2618,17 @@ static int __devinit cciss_init_one(stru
 
 	blk_queue_max_sectors(q, 512);
 
-
-	for(j=0; j<NWD; j++) {
-		drive_info_struct *drv = &(hba[i]->drv[j]);
-		struct gendisk *disk = hba[i]->gendisk[j];
-
+		q->queuedata = hba[i];
 		sprintf(disk->disk_name, "cciss/c%dd%d", i, j);
 		sprintf(disk->devfs_name, "cciss/host%d/target%d", i, j);
 		disk->major = COMPAQ_CISS_MAJOR + i;
 		disk->first_minor = j << NWD_SHIFT;
 		disk->fops = &cciss_fops;
-		disk->queue = hba[i]->queue;
+		disk->queue = q;
 		disk->private_data = drv;
 		if( !(drv->nr_blocks))
 			continue;
-		blk_queue_hardsect_size(hba[i]->queue, drv->block_size);
+		blk_queue_hardsect_size(q, drv->block_size);
 		set_capacity(disk, drv->nr_blocks);
 		add_disk(disk);
 	}
@@ -2690,9 +2698,9 @@ static void __devexit cciss_remove_one (
 		struct gendisk *disk = hba[i]->gendisk[j];
 		if (disk->flags & GENHD_FL_UP)
 			del_gendisk(disk);
+			blk_cleanup_queue(disk->queue);
 	}
 
-	blk_cleanup_queue(hba[i]->queue);
 	pci_free_consistent(hba[i]->pdev, NR_CMDS * sizeof(CommandList_struct),
 			    hba[i]->cmd_pool, hba[i]->cmd_pool_dhandle);
 	pci_free_consistent(hba[i]->pdev, NR_CMDS * sizeof( ErrorInfo_struct),
diff -burpN lx265.orig/drivers/block/cciss.h lx265.save/drivers/block/cciss.h
--- lx265.orig/drivers/block/cciss.h	2004-04-03 21:36:13.000000000 -0600
+++ lx265.save/drivers/block/cciss.h	2004-04-06 10:22:08.000000000 -0500
@@ -27,6 +27,7 @@ typedef struct _drive_info_struct
 {
  	__u32   LunID;	
 	int 	usage_count;
+	struct request_queue *queue;
 	sector_t nr_blocks;
 	int	block_size;
 	int 	heads;
@@ -69,7 +70,6 @@ struct ctlr_info 
 	unsigned int maxQsinceinit;
 	unsigned int maxSG;
 	spinlock_t lock;
-	struct request_queue *queue;
 
 	//* pointers to command and error info pool */ 
 	CommandList_struct 	*cmd_pool;
@@ -252,7 +252,7 @@ struct board_type {
 	struct access_method *access;
 };
 
-#define CCISS_LOCK(i)	(hba[i]->queue->queue_lock)
+#define CCISS_LOCK(i)	(&(hba[i]->lock))
 
 #endif /* CCISS_H */
 

Re: cciss updates for 2.6.6xxx [1/2]

[Jeff Garzik]

mikem@beardog.cca.cpqcorp.net wrote:
> This patch adds per logical device queues to the HP cciss driver. It currently only implements a single lock but when time permits I will provide that funtionality. Thanks to Jeff Garzik for providing some sample code.
> This patch built against 2.6.5. Please consider this for inclusion.


I appreciate the credit but I don't see that it addressed my original 
objection -- the starvation issue.

Do you cap the number of per-array requests a "1024 / n_arrays", or 
something like that?  You mentioned that the hardware has a maximum of 
1024 outstanding commands, for all devices.  The two typical solutions 
are a round-robin queue (see carmel.c) or limiting each array such that 
if all arrays are full of commands, the total outstanding never exceeds 
1024.

This patch may be OK for -mm, I would rather not see it go upstream -- 
it seems to me you are choosing to decrease stability to obtain a 
performance increase.  I think you can increase performance without 
decreasing stability.

	Jeff

Re: cciss updates for 2.6.6xxx [1/2]

[Jeff Garzik]

Miller, Mike (OS Dev) wrote:
> I like the idea of capping max commands based on the number of arrays. One problem is that we can add or remove a logical drive during runtime. How would Linux handle us reshuffling the max commands for each queue?


What is the maximum number of logical drives?

If you always assume the maximum number of logical drives are configured 
(let's say 32), then you can calculate based on that:
	1024 / 32 logical drives == 32 commands per logical drive

It certainly gets a bit more complex, if you truly want to handle 
something other than "hardcode max # of logical drives"...  I would 
suggest going to a round-robin anti-starvation approach at that point, 
like what I do in carmel.c.  The Carmel hardware has a single hardware 
queue for all of its 8 SATA ports, which is similar to cciss having 1024 
commands for all of its logical drives (if I understand things correctly).

	Jeff

RE: cciss updates for 2.6.6xxx [1/2]

[Miller, Mike (OS Dev)]

I like the idea of capping max commands based on the number of arrays. One problem is that we can add or remove a logical drive during runtime. How would Linux handle us reshuffling the max commands for each queue?

mikem

-----Original Message-----
From: Jeff Garzik [mailto:jgarzik@pobox.com]
Sent: Wednesday, April 07, 2004 11:34 AM
To: Miller, Mike (OS Dev)
Cc: alpm@odsl.org; axboe@suse.de; linux-kernel@vger.kernel.org
Subject: Re: cciss updates for 2.6.6xxx [1/2]


Miller, Mike (OS Dev) wrote:
> Yep, you're right. I just regurgitated the same code. I'll pull my head out and try again :(


The easiest thing to do may be to take your patch #1, and then add code 
to clamp the per-queue outstanding-command (tag) depth to
	1024 / n_arrays_found

at initialization time.  Or perhaps s/n_arrays_found/max_arrays_per_hba/

I bet that's just a few additional lines of code, and should work...

Regards,

	Jeff

Re: cciss updates for 2.6.6xxx [1/2]

[Jeff Garzik]

Miller, Mike (OS Dev) wrote:
> Yep, you're right. I just regurgitated the same code. I'll pull my head out and try again :(


The easiest thing to do may be to take your patch #1, and then add code 
to clamp the per-queue outstanding-command (tag) depth to
	1024 / n_arrays_found

at initialization time.  Or perhaps s/n_arrays_found/max_arrays_per_hba/

I bet that's just a few additional lines of code, and should work...

Regards,

	Jeff

RE: cciss updates for 2.6.6xxx [1/2]

[Miller, Mike (OS Dev)]

Yep, you're right. I just regurgitated the same code. I'll pull my head out and try again :(

mikem

-----Original Message-----
From: Jeff Garzik [mailto:jgarzik@pobox.com]
Sent: Wednesday, April 07, 2004 11:15 AM
To: Miller, Mike (OS Dev)
Cc: alpm@odsl.org; axboe@suse.de; linux-kernel@vger.kernel.org
Subject: Re: cciss updates for 2.6.6xxx [1/2]


mikem@beardog.cca.cpqcorp.net wrote:
> This patch adds per logical device queues to the HP cciss driver. It currently only implements a single lock but when time permits I will provide that funtionality. Thanks to Jeff Garzik for providing some sample code.
> This patch built against 2.6.5. Please consider this for inclusion.


I appreciate the credit but I don't see that it addressed my original 
objection -- the starvation issue.

Do you cap the number of per-array requests a "1024 / n_arrays", or 
something like that?  You mentioned that the hardware has a maximum of 
1024 outstanding commands, for all devices.  The two typical solutions 
are a round-robin queue (see carmel.c) or limiting each array such that 
if all arrays are full of commands, the total outstanding never exceeds 
1024.

This patch may be OK for -mm, I would rather not see it go upstream -- 
it seems to me you are choosing to decrease stability to obtain a 
performance increase.  I think you can increase performance without 
decreasing stability.

	Jeff