[PATCH] take 48-bit lba a bit further

[PATCH] take 48-bit lba a bit further

[Jens Axboe]

Hi,

Thanks for taking the previous bit Alan, here's an incremental update to
2.5.66-ac2. Just cleans up the 'when to use 48-bit lba' logic a bit per
Andries suggestion, and also expands the request size for 48-bit lba
capable drives to 512KiB.

Works perfectly in testing here, ext2/3 generates nice big 512KiB
requests and the drive flies.

diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-disk.c linux-2.5.66-ac2/drivers/ide/ide-disk.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-disk.c	2003-04-06 13:15:19.000000000 +0200
+++ linux-2.5.66-ac2/drivers/ide/ide-disk.c	2003-04-06 15:05:07.000000000 +0200
@@ -373,7 +373,7 @@
 
 	nsectors.all		= (u16) rq->nr_sectors;
 
-	if (drive->addressing == 1 && block + rq->nr_sectors > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	if (driver_blocked)
@@ -583,7 +583,7 @@
 {
 	int lba48bit = 0;
 
-	if (drive->addressing == 1 && block > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48bit = 1;
 
 	BUG_ON(drive->blocked);
@@ -611,7 +611,7 @@
 		return ide_started;
 	}
 
-	if (lba48bit && block > 0xfffffff)
+	if (lba48bit)
 		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
 	if (drive->select.b.lba)		/* 28-bit LBA */
 		return lba_28_rw_disk(drive, rq, (unsigned long) block);
@@ -625,7 +625,7 @@
 	int cmd = rq_data_dir(rq);
 	int lba48bit = 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48bit = 1;
 
 	if ((cmd == READ) && drive->using_tcq)
@@ -1504,7 +1504,7 @@
 
 static int set_lba_addressing (ide_drive_t *drive, int arg)
 {
-	return (probe_lba_addressing(drive, arg));
+	return probe_lba_addressing(drive, arg);
 }
 
 static void idedisk_add_settings(ide_drive_t *drive)
@@ -1591,6 +1591,9 @@
 
 	(void) probe_lba_addressing(drive, 1);
 
+	if (drive->addressing)
+		blk_queue_max_sectors(&drive->queue, 1024);
+
 	/* Extract geometry if we did not already have one for the drive */
 	if (!drive->cyl || !drive->head || !drive->sect) {
 		drive->cyl     = drive->bios_cyl  = id->cyls;
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-dma.c linux-2.5.66-ac2/drivers/ide/ide-dma.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-dma.c	2003-04-06 13:15:19.000000000 +0200
+++ linux-2.5.66-ac2/drivers/ide/ide-dma.c	2003-04-06 13:19:34.000000000 +0200
@@ -663,7 +663,7 @@
 	if (drive->media != ide_disk)
 		return 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	command = (lba48) ? WIN_READDMA_EXT : WIN_READDMA;
@@ -698,7 +698,7 @@
 	if (drive->media != ide_disk)
 		return 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	command = (lba48) ? WIN_WRITEDMA_EXT : WIN_WRITEDMA;
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/include/linux/ide.h linux-2.5.66-ac2/include/linux/ide.h
--- /opt/kernel/linux-2.5.66-ac2/include/linux/ide.h	2003-04-06 13:15:20.000000000 +0200
+++ linux-2.5.66-ac2/include/linux/ide.h	2003-04-06 13:20:25.000000000 +0200
@@ -866,6 +866,12 @@
 		bio_kunmap_irq(buffer, flags);
 }
 
+/*
+ * must be addressed with 48-bit lba
+ */
+#define rq_lba48(rq)	\
+	(((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256)
+
 #define IDE_CHIPSET_PCI_MASK	\
     ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))
 #define IDE_CHIPSET_IS_PCI(c)	((IDE_CHIPSET_PCI_MASK >> (c)) & 1)

-- 
Jens Axboe

Re: [PATCH] take 48-bit lba a bit further

[John Bradford]

> Thanks for taking the previous bit Alan, here's an incremental update to
> 2.5.66-ac2. Just cleans up the 'when to use 48-bit lba' logic a bit per
> Andries suggestion, and also expands the request size for 48-bit lba
> capable drives to 512KiB.
> 
> Works perfectly in testing here, ext2/3 generates nice big 512KiB
> requests and the drive flies.

Then, don't we want to be using 48-bit lba all the time on compatible devices
instead of falling back to 28-bit when possible to save a small amount of
instruction overhead?  (Or is that what we're doing already?  I haven't really
had the time to follow this thread).

John.

Re: [PATCH] take 48-bit lba a bit further

[Jens Axboe]

On Sun, Apr 06 2003, John Bradford wrote:
> > Thanks for taking the previous bit Alan, here's an incremental update to
> > 2.5.66-ac2. Just cleans up the 'when to use 48-bit lba' logic a bit per
> > Andries suggestion, and also expands the request size for 48-bit lba
> > capable drives to 512KiB.
> > 
> > Works perfectly in testing here, ext2/3 generates nice big 512KiB
> > requests and the drive flies.
> 
> Then, don't we want to be using 48-bit lba all the time on compatible devices
> instead of falling back to 28-bit when possible to save a small amount of
> instruction overhead?  (Or is that what we're doing already?  I haven't really
> had the time to follow this thread).

The logic in the patch is to enable large requests _if_ the drive can do
48-bit lba. However, we will only use 48-bit lba commands if the request
is either beyond 2^28 sectors _or_ bigger than 256 sectors since neither
of these can be addressed with 28-bit lba.

See rq_lba48 in the patch, it explains it.

-- 
Jens Axboe

Re: [PATCH] take 48-bit lba a bit further

[Alan Cox]

On Sul, 2003-04-06 at 14:32, John Bradford wrote:
> Then, don't we want to be using 48-bit lba all the time on compatible devices
> instead of falling back to 28-bit when possible to save a small amount of
> instruction overhead?  (Or is that what we're doing already?  I haven't really
> had the time to follow this thread).

The overhead of the double load of the command registers is microseconds so it
is actually quite a lot, especially since IDE lacks TCQ so neither end of the
link is doing *anything* useful. SCSI has similar problems on older SCSI with 
command sending being slow, but the drive is at least doing other commands during
this.

Re: [PATCH] take 48-bit lba a bit further

[John Bradford]

> > Then, don't we want to be using 48-bit lba all the time on compatible devices
> > instead of falling back to 28-bit when possible to save a small amount of
> > instruction overhead?  (Or is that what we're doing already?  I haven't really
> > had the time to follow this thread).
> 
> The overhead of the double load of the command registers is microseconds so it
> is actually quite a lot, especially since IDE lacks TCQ so neither end of the
> link is doing *anything* useful. SCSI has similar problems on older SCSI with 
> command sending being slow, but the drive is at least doing other commands during
> this.

So, say you have a choice of either a 256Kb request to a low block number,
which can use the faster 28-bit mode, or a 512Kb request to the same low block
number, which can only be made using 48-bit LBA, which is the best to use?

I originally thought that we might only be honouring 512Kb requests for blocks
over the 28-bit limit, which Jens corrected me on, but maybe we *should* only
do 512Kb requests on high block number, where we have to use 48-bit anyway.

John.

Re: [PATCH] take 48-bit lba a bit further

[Jens Axboe]

On Sun, Apr 06 2003, John Bradford wrote:
> I originally thought that we might only be honouring 512Kb requests
> for blocks over the 28-bit limit, which Jens corrected me on, but
> maybe we *should* only do 512Kb requests on high block number, where
> we have to use 48-bit anyway.

That makes little sense in practice, and is not currently even doable
within the block layer.  You got the limits wrong, btw, it's 128kb max
for 28-bit. A single 512KiB request will have a lower per-kb overhead
with 48-bit lba than a single 128kb on 28-bit would.

-- 
Jens Axboe

Re: [PATCH] take 48-bit lba a bit further

[Jens Axboe]

On Sun, Apr 06 2003, Jens Axboe wrote:
> On Sun, Apr 06 2003, John Bradford wrote:
> > I originally thought that we might only be honouring 512Kb requests
> > for blocks over the 28-bit limit, which Jens corrected me on, but
> > maybe we *should* only do 512Kb requests on high block number, where
> > we have to use 48-bit anyway.
> 
> That makes little sense in practice, and is not currently even doable
> within the block layer.  You got the limits wrong, btw, it's 128kb max
> for 28-bit. A single 512KiB request will have a lower per-kb overhead
> with 48-bit lba than a single 128kb on 28-bit would.

I should mention that the 512KiB number is one I chose, as a good large
request size. You could as high as 32MiB.

-- 
Jens Axboe

Re: [PATCH] take 48-bit lba a bit further

[John Bradford]

> A single 512KiB request will have a lower per-kb overhead
> with 48-bit lba than a single 128kb on 28-bit would.

Ah, OK, that's what I wasn't sure about.

John.

Re: [PATCH] take 48-bit lba a bit further

[Alan Cox]

On Sul, 2003-04-06 at 17:43, Petr Vandrovec wrote:
> > Works perfectly in testing here, ext2/3 generates nice big 512KiB
> > requests and the drive flies.
> > 
> >  	(void) probe_lba_addressing(drive, 1);
> >  
> > +	if (drive->addressing)
> > +		blk_queue_max_sectors(&drive->queue, 1024);
> > +
> 
> Should not you honor host's max queue length? siimage & pdc4030 sets
> max_sectors to 128 (resp. 16 resp. 127), while you overwrite it here
> unconditionally with 1024.

For production code thats required. The actual change required is to
clamp the default/hwif set queue limit to 256 if driver->addressing !=1

Re: [PATCH] take 48-bit lba a bit further

[Petr Vandrovec]

On Sun, Apr 06, 2003 at 03:07:37PM +0200, Jens Axboe wrote:
> Hi,
> 
> Thanks for taking the previous bit Alan, here's an incremental update to
> 2.5.66-ac2. Just cleans up the 'when to use 48-bit lba' logic a bit per
> Andries suggestion, and also expands the request size for 48-bit lba
> capable drives to 512KiB.
> 
> Works perfectly in testing here, ext2/3 generates nice big 512KiB
> requests and the drive flies.
> 
>  	(void) probe_lba_addressing(drive, 1);
>  
> +	if (drive->addressing)
> +		blk_queue_max_sectors(&drive->queue, 1024);
> +

Should not you honor host's max queue length? siimage & pdc4030 sets
max_sectors to 128 (resp. 16 resp. 127), while you overwrite it here
unconditionally with 1024.
					Thanks,
						Petr Vandrovec
						vandrove@vc.cvut.cz

Re: [PATCH] take 48-bit lba a bit further

[Peter Chubb]

>>>>> "John" == John Bradford <john@grabjohn.com> writes:


John> So, say you have a choice of either a 256Kb request to a low
John> block number, which can use the faster 28-bit mode, or a 512Kb
John> request to the same low block number, which can only be made
John> using 48-bit LBA, which is the best to use?

Interrupt overhead as measured here is greater than the IO overhead
(at least on IA64 and alpha machines).  The bigger the transfer for a
single interrupt, the better.

Peter C

Re: [PATCH] take 48-bit lba a bit further

[Jens Axboe]

On Sun, Apr 06 2003, Alan Cox wrote:
> On Sul, 2003-04-06 at 17:43, Petr Vandrovec wrote:
> > > Works perfectly in testing here, ext2/3 generates nice big 512KiB
> > > requests and the drive flies.
> > > 
> > >  	(void) probe_lba_addressing(drive, 1);
> > >  
> > > +	if (drive->addressing)
> > > +		blk_queue_max_sectors(&drive->queue, 1024);
> > > +
> > 
> > Should not you honor host's max queue length? siimage & pdc4030 sets
> > max_sectors to 128 (resp. 16 resp. 127), while you overwrite it here
> > unconditionally with 1024.
> 
> For production code thats required. The actual change required is to
> clamp the default/hwif set queue limit to 256 if driver->addressing !=1

Yes I was aware of this short-cut. I want to see rqsize being generally
set as a maximum, of the host adapter doesn't set its own upper limit.
This patch adds that little piece (I think I caught both legacy and pci,
please double check Alan), and also fixes pdc202xx_old to work with
48-bit again.

diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-disk.c linux-2.5.66-ac2/drivers/ide/ide-disk.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-disk.c	2003-04-06 13:15:19.000000000 +0200
+++ linux-2.5.66-ac2/drivers/ide/ide-disk.c	2003-04-07 09:44:34.000000000 +0200
@@ -373,7 +373,7 @@
 
 	nsectors.all		= (u16) rq->nr_sectors;
 
-	if (drive->addressing == 1 && block + rq->nr_sectors > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	if (driver_blocked)
@@ -583,7 +583,7 @@
 {
 	int lba48bit = 0;
 
-	if (drive->addressing == 1 && block > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48bit = 1;
 
 	BUG_ON(drive->blocked);
@@ -611,7 +611,7 @@
 		return ide_started;
 	}
 
-	if (lba48bit && block > 0xfffffff)
+	if (lba48bit)
 		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
 	if (drive->select.b.lba)		/* 28-bit LBA */
 		return lba_28_rw_disk(drive, rq, (unsigned long) block);
@@ -625,7 +625,7 @@
 	int cmd = rq_data_dir(rq);
 	int lba48bit = 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48bit = 1;
 
 	if ((cmd == READ) && drive->using_tcq)
@@ -1504,7 +1504,7 @@
 
 static int set_lba_addressing (ide_drive_t *drive, int arg)
 {
-	return (probe_lba_addressing(drive, arg));
+	return probe_lba_addressing(drive, arg);
 }
 
 static void idedisk_add_settings(ide_drive_t *drive)
@@ -1591,6 +1591,15 @@
 
 	(void) probe_lba_addressing(drive, 1);
 
+	if (drive->addressing) {
+		int max_sectors = 1024;
+
+		if (max_sectors > HWIF(drive)->rqsize)
+			max_sectors = HWIF(drive)->rqsize;
+
+		blk_queue_max_sectors(&drive->queue, max_sectors);
+	}
+
 	/* Extract geometry if we did not already have one for the drive */
 	if (!drive->cyl || !drive->head || !drive->sect) {
 		drive->cyl     = drive->bios_cyl  = id->cyls;
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-dma.c linux-2.5.66-ac2/drivers/ide/ide-dma.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide-dma.c	2003-04-06 13:15:19.000000000 +0200
+++ linux-2.5.66-ac2/drivers/ide/ide-dma.c	2003-04-06 13:19:34.000000000 +0200
@@ -663,7 +663,7 @@
 	if (drive->media != ide_disk)
 		return 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	command = (lba48) ? WIN_READDMA_EXT : WIN_READDMA;
@@ -698,7 +698,7 @@
 	if (drive->media != ide_disk)
 		return 0;
 
-	if (drive->addressing == 1 && rq->sector > 0xfffffff)
+	if (drive->addressing == 1 && rq_lba48(rq))
 		lba48 = 1;
 
 	command = (lba48) ? WIN_WRITEDMA_EXT : WIN_WRITEDMA;
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide.c linux-2.5.66-ac2/drivers/ide/ide.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/ide.c	2003-04-06 13:15:19.000000000 +0200
+++ linux-2.5.66-ac2/drivers/ide/ide.c	2003-04-07 09:41:01.000000000 +0200
@@ -947,6 +947,9 @@
 #endif
 	}
 
+	if (!hwif->rqsize)
+		hwif->rqsize = 65535;
+
 	if (hwifp)
 		*hwifp = hwif;
 
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/pci/pdc202xx_old.c linux-2.5.66-ac2/drivers/ide/pci/pdc202xx_old.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/pci/pdc202xx_old.c	2003-03-24 23:01:47.000000000 +0100
+++ linux-2.5.66-ac2/drivers/ide/pci/pdc202xx_old.c	2003-04-07 09:13:50.000000000 +0200
@@ -535,8 +535,9 @@
 
 static int pdc202xx_old_ide_dma_begin(ide_drive_t *drive)
 {
-	if (drive->addressing == 1) {
-		struct request *rq	= HWGROUP(drive)->rq;
+	struct request *rq = HWGROUP(drive)->rq;
+
+	if (drive->addressing == 1 && rq_lba48(rq)) {
 		ide_hwif_t *hwif	= HWIF(drive);
 //		struct pci_dev *dev	= hwif->pci_dev;
 //		unsgned long high_16	= pci_resource_start(dev, 4);
@@ -557,7 +558,9 @@
 
 static int pdc202xx_old_ide_dma_end(ide_drive_t *drive)
 {
-	if (drive->addressing == 1) {
+	struct request *rq = HWGROUP(drive)->rq;
+
+	if (drive->addressing == 1 && rq_lba48(rq)) {
 		ide_hwif_t *hwif	= HWIF(drive);
 //		unsigned long high_16	= pci_resource_start(hwif->pci_dev, 4);
 		unsigned long high_16	= hwif->dma_master;
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/drivers/ide/setup-pci.c linux-2.5.66-ac2/drivers/ide/setup-pci.c
--- /opt/kernel/linux-2.5.66-ac2/drivers/ide/setup-pci.c	2003-03-24 23:00:20.000000000 +0100
+++ linux-2.5.66-ac2/drivers/ide/setup-pci.c	2003-04-07 09:40:48.000000000 +0200
@@ -632,7 +632,6 @@
 			index->b.low = hwif->index;
 		}
 
-		
 		if (d->init_iops)
 			d->init_iops(hwif);
 
@@ -658,6 +657,9 @@
 			 */
 			d->init_hwif(hwif);
 
+		if (!hwif->rqsize)
+			hwif->rqsize = 65535;
+
 		/*
 		 *	This is in the wrong place. The driver may
 		 *	do set up based on the autotune value and this
diff -ur -X /home/axboe/cdrom/exclude /opt/kernel/linux-2.5.66-ac2/include/linux/ide.h linux-2.5.66-ac2/include/linux/ide.h
--- /opt/kernel/linux-2.5.66-ac2/include/linux/ide.h	2003-04-06 13:15:20.000000000 +0200
+++ linux-2.5.66-ac2/include/linux/ide.h	2003-04-06 13:20:25.000000000 +0200
@@ -866,6 +866,12 @@
 		bio_kunmap_irq(buffer, flags);
 }
 
+/*
+ * must be addressed with 48-bit lba
+ */
+#define rq_lba48(rq)	\
+	(((rq)->sector + (rq)->nr_sectors) > 0xfffffff || rq->nr_sectors > 256)
+
 #define IDE_CHIPSET_PCI_MASK	\
     ((1<<ide_pci)|(1<<ide_cmd646)|(1<<ide_ali14xx))
 #define IDE_CHIPSET_IS_PCI(c)	((IDE_CHIPSET_PCI_MASK >> (c)) & 1)

-- 
Jens Axboe