2.5.3-pre1 ide updates

PATCH In review Jens Axboe 4 participants 10 messages Jan 17, 2002 – Jan 18, 2002 Block layer

lore

Share & tools

lore

Jens Axboe

author

2002-01-17 13:47 UTC #0 lore raw

PATCH 1/1 2.5.3-pre1 ide updates

Hi,

The following patch makes IDE pio and multi mode work again. Changes:

- Properly map in requests, nr_sectors hack was very wrong (me)

- Fix pio interrupt handlers to handle requests properly (me)

- Fix task_mulout_intr to handle requests in chunks of current_nr_sectors, regardless of set multi count. This both makes it work, but also ensures we do not have to fall back to same hackish 'copy-of-rq' approach that 2.4 uses. It comes at the cost of handling fs block size number of sectors at the time instead of max multi count, but I think this is a price worth paying. It's miniscule anyway. (me)

- Fix task_mulin_intr to properly loop and handle as many sectors as we set up (multi count or nr_sectors, whichever is smallest) (me)

- Fix wrong isr for multi mode enable, caused hdparm hang (Andre)

- ide-disk.c:do_rw_disk() had wrong check of rq flags, couldn't catch anything (either the bit is set or not, checking both makes no sense). Check for fs request there instead. (me)

- ide_dma_timeout_retry(), set correct next current_nr_sectors and only for bio request. (me)

- ll_rw_blk, blk_dump_rq_flags wasn't properly adjusted for new IDE flag (me)

drivers/block/ll_rw_blk.c renamed from ata/linux-2.5.3-pre1/drivers/block/ll_rw_blk.c +2 -2

diff -urN -X exclude /ata/linux-2.5.3-pre1/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
@@ -302,8 +303,8 @@
 static char *rq_flags[] = { "REQ_RW", "REQ_RW_AHEAD", "REQ_BARRIER",
 			   "REQ_CMD", "REQ_NOMERGE", "REQ_STARTED",
 			   "REQ_DONTPREP", "REQ_DRIVE_CMD", "REQ_DRIVE_TASK",
-			   "REQ_PC", "REQ_BLOCK_PC", "REQ_SENSE",
-			   "REQ_SPECIAL" };
+			   "REQ_DRIVE_ACB", "REQ_PC", "REQ_BLOCK_PC",
+			   "REQ_SENSE", "REQ_SPECIAL" };
 
 void blk_dump_rq_flags(struct request *rq, char *msg)
 {

drivers/ide/ide-disk.c renamed from ata/linux-2.5.3-pre1/drivers/ide/ide-disk.c +42 ~~-25~~

133 lines not on this page · raw message

drivers/ide/ide-taskfile.c renamed from ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c +72 ~~-29~~

221 lines not on this page · raw message

drivers/ide/ide.c renamed from ata/linux-2.5.3-pre1/drivers/ide/ide.c +4 -7

diff -urN -X exclude /ata/linux-2.5.3-pre1/drivers/ide/ide.c linux/drivers/ide/ide.c
@@ -1458,14 +1458,11 @@
 	HWGROUP(drive)->rq = NULL;
 
 	rq->errors = 0;
-	rq->sector = rq->bio->bi_sector;
-	rq->current_nr_sectors = bio_sectors(rq->bio);
-
-	/*
-	 * just to make sure...
-	 */
-	if (rq->bio)
+	if (rq->bio) {
+		rq->sector = rq->bio->bi_sector;
+		rq->current_nr_sectors = bio_iovec(rq->bio)->bv_len >> 9;
 		rq->buffer = NULL;
+	}
 }
 
 /*

include/linux/ide.h renamed from ata/linux-2.5.3-pre1/include/linux/ide.h +21 -0

diff -urN -X exclude /ata/linux-2.5.3-pre1/include/linux/ide.h linux/include/linux/ide.h
@@ -884,6 +884,9 @@
  */
 #define ide_rq_offset(rq) (((rq)->hard_cur_sectors - (rq)->current_nr_sectors) << 9)
 
+#define task_rq_offset(rq) \
+	(((rq)->nr_sectors - (rq)->current_nr_sectors) * SECTOR_SIZE)
+
 extern inline void *ide_map_buffer(struct request *rq, unsigned long *flags)
 {
 	return bio_kmap_irq(rq->bio, flags) + ide_rq_offset(rq);

@@ -892,6 +895,24 @@
 extern inline void ide_unmap_buffer(char *buffer, unsigned long *flags)
 {
 	bio_kunmap_irq(buffer, flags);
+}
+
+/*
+ * for now, taskfile requests are special :/
+ */
+extern inline char *ide_map_rq(struct request *rq, unsigned long *flags)
+{
+	if (rq->bio)
+		return ide_map_buffer(rq, flags);
+	else
+		return rq->buffer + task_rq_offset(rq);
+}
+
+extern inline void ide_unmap_rq(struct request *rq, char *buf,
+				unsigned long *flags)
+{
+	if (rq->bio)
+		ide_unmap_buffer(buf, flags);
 }
 
 /*

Signature · 1 line

Jens Axboe

Anton Altaparmakov

2002-01-17 14:27 UTC #1 lore raw

Hi Jens,

I think there is a bug here...

drivers/ide/ide-taskfile.c line 1036

Quoted 11 lines · Jens Axboe

diff -urN -X exclude /ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- /ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c Thu Jan 17
06:32:54 2002
+++ linux/drivers/ide/ide-taskfile.c Thu Jan 17 06:29:13 2002
drivers/ide/ide-taskfile.c renamed from ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c +1 -1
@@ -994,10 +1032,11 @@
        if (!msect) {
                nsect = 1;
                while (rq->current_nr_sectors) {
-                       pBuf = rq->buffer + ((rq->nr_sectors - 
 rq->current_nr_sectors) * SECTOR_SIZE);
+                       pBuf = ide_unmap_buffer(rq, &flags);

That should be: pBuf = ide_map_buffer(rq, &flags);

But I think it actually ought to be:

pBuf = ide_map_rq(rq, &flags)

and the below unmap should consequently be:

ide_unmap_rq(rq, pBuf, &flags)

In either case it's wrong at the moment AFAICS...

Quoted 9 lines

(quoted diff) +1 -0

                        DTF("Multiwrite: %p, nsect: %d, 
 rq->current_nr_sectors: %ld\n", pBuf, nsect, rq->current_nr_sectors);
                        drive->io_32bit = 0;
                        taskfile_output_data(drive, pBuf, nsect * 
 SECTOR_WORDS);
+                       ide_unmap_buffer(pBuf, &flags);

                        drive->io_32bit = io_32bit;
                        rq->errors = 0;
                        rq->current_nr_sectors -= nsect;

Best regards,

Anton

Signature · 5 lines

  "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Jens Axboe

author

2002-01-17 14:28 UTC #2 lore raw

In reply to Jens Axboe

Quoted 22 lines · Anton Altaparmakov

Hi Jens,
I think there is a bug here...
drivers/ide/ide-taskfile.c line 1036
Quoted 12 lines · Jens Axboe
diff -urN -X exclude /ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- /ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c Thu Jan 17
06:32:54 2002
+++ linux/drivers/ide/ide-taskfile.c Thu Jan 17 06:29:13 2002
drivers/ide/ide-taskfile.c renamed from ata/linux-2.5.3-pre1/drivers/ide/ide-taskfile.c +1 -1
@@ -994,10 +1032,11 @@
       if (!msect) {
               nsect = 1;
               while (rq->current_nr_sectors) {
-                       pBuf = rq->buffer + ((rq->nr_sectors - 
 rq->current_nr_sectors) * SECTOR_SIZE);
+                       pBuf = ide_unmap_buffer(rq, &flags);
That should be: pBuf = ide_map_buffer(rq, &flags);
But I think it actually ought to be:

Quoted 7 lines

pBuf = ide_map_rq(rq, &flags)
and the below unmap should consequently be:
ide_unmap_rq(rq, pBuf, &flags)
In either case it's wrong at the moment AFAICS...

Yeah you are right, typo... There's another mixup in there too, but only a very minor race. I'll update the patch asap.

Signature · 1 line

Jens Axboe

Jens Axboe

author

2002-01-17 14:30 UTC #3 lore raw

In reply to Jens Axboe

Quoted 3 lines · Anton Altaparmakov

Hi Jens,
I think there is a bug here...

BTW, should have mentioned that this code doesn't trigger right now anyways so it's only a source bug, not a binary one.

Signature · 1 line

Jens Axboe

Jens Axboe

author

2002-01-17 14:34 UTC #4 lore raw

PATCH 1/1 2.5.3-pre1 ata-253p1-2

In reply to Jens Axboe

Quoted 7 lines · Jens Axboe

Quoted 3 lines · Anton Altaparmakov
Hi Jens,
I think there is a bug here...
BTW, should have mentioned that this code doesn't trigger right now anyways so it's only a source bug, not a binary one.

Since the bug was cosmetic only, I won't include it here. Find the updated version:

*.kernel.org/pub/linux/kernel/people/axboe/patches/v2.5/2.5.3-pre1/ata-253p1-2.bz2

Signature · 1 line

Jens Axboe

Robert Love

2002-01-17 22:13 UTC #5 lore raw

Re: [PATCH] 2.5.3-pre1 ata-253p1-2

Quoted 4 lines · Jens Axboe

Since the bug was cosmetic only, I won't include it here. Find the updated version:
*.kernel.org/pub/linux/kernel/people/axboe/patches/v2.5/2.5.3-pre1/ata-253p1-2.bz2

I haven't seen any status reports, so here is a heads up: it works on my SMP machine, both with and without taskfile enabled. Good job.

Robert Love

Andre Hedrick

2002-01-17 23:18 UTC #6 lore raw

Re: [PATCH] 2.5.3-pre1 ata-253p1-2

Quoted 9 lines · Robert Love

Quoted 4 lines · Jens Axboe
Since the bug was cosmetic only, I won't include it here. Find the updated version:
*.kernel.org/pub/linux/kernel/people/axboe/patches/v2.5/2.5.3-pre1/ata-253p1-2.bz2
I haven't seen any status reports, so here is a heads up: it works on my SMP machine, both with and without taskfile enabled. Good job.

This kernel is totally ACB-IO or Taskfile Driven, the Config.in Option is to allow user-space access for diagnostics, forensics and OEM feature sets.

You have to give Jens the credit for gluing it togather, because there was no way I would have figured out the suttle issues of BIO. There are serveral additions need to fix all the archs so hope to have something today.

Regard,

Andre Hedrick
Linux Disk Certification Project Linux ATA Development

Robert Love

2002-01-17 23:10 UTC #7 lore raw

Re: [PATCH] 2.5.3-pre1 ata-253p1-2

Quoted 3 lines · Andre Hedrick

This kernel is totally ACB-IO or Taskfile Driven, the Config.in Option is to allow user-space access for diagnostics, forensics and OEM feature sets.

Oh, OK -- 2.5.3-pre1 doesn't have Configure.help entries ;) But I think Anton sent them, so hopefully we will see them.

I guess the configure option just enables the ioctl, then, eh? Gotcha.

Quoted 4 lines

You have to give Jens the credit for gluing it togather, because there was no way I would have figured out the suttle issues of BIO. There are serveral additions need to fix all the archs so hope to have something today.

Well, good work to both of you. It is an excellent driver. Please submit it for pre2.

FYI, I am having no problems with the above and preempt-kernel, which I've heard reported otherwise. If it works with preemption and SMP ...

Robert Love

Anton Altaparmakov

2002-01-18 01:00 UTC #8 lore raw

Re: [PATCH] 2.5.3-pre1 ata-253p1-2

Quoted 7 lines · Robert Love

Quoted 3 lines · Andre Hedrick
This kernel is totally ACB-IO or Taskfile Driven, the Config.in Option is to allow user-space access for diagnostics, forensics and OEM feature sets.
Oh, OK -- 2.5.3-pre1 doesn't have Configure.help entries ;) But I think Anton sent them, so hopefully we will see them.

Yes I sent them to Linus.

Quoted 1 line

I guess the configure option just enables the ioctl, then, eh? Gotcha.

Yup.

Quoted 7 lines

Quoted 4 lines
You have to give Jens the credit for gluing it togather, because there was no way I would have figured out the suttle issues of BIO. There are serveral additions need to fix all the archs so hope to have something today.
Well, good work to both of you. It is an excellent driver. Please submit it for pre2.

I second that. 2.5.3-pre1 + 2nd patch from Jens + daft compile fixes is running fine on my Athlon/VIA chipset both UDMA100 and PIO4 on my 60GXP drive! Brilliant work guys!

There is only one odd thing and that is that PIO transfers are slower in 2.5.3-pre1 + 2nd patch from Jens + daft compile fixes compared to in 2.5.2-pre11-vanilla.

PIO mode, hdparm -t /dev/hda:

        on 2.5.3-pre1 + 2nd Jens patch: 4.62MB/s
        on 2.5.2-pre11 vanilla:         7.36MB/s

DMA transfers are the same with both kernels, peaking at 38-39MB/s which is I believe the theoretical upper limit for the 60GXP so there was not much room for visible improvement (2.5.3-pre1 is slightly faster in that it gave in three tests 38.79MB/s while 2.5.2-pre11 gave in three tests 38.32MB/s, so if you believe those ACB is .4 MB/s faster).

I don't care that PIO has become slower, all modern devices are happy with UDMA, and so am I. (-: But I thought I should mention it.

Note that 2.5.3-pre1 now survives find . -type f -exec md5sum "{}" \; both in PIO and UDMA mode so it can really be considered stable on UP system.

Best regards,

Anton

Signature · 5 lines

  "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Jens Axboe

author

2002-01-18 07:59 UTC #9 lore raw

Re: [PATCH] 2.5.3-pre1 ata-253p1-2

In reply to Andre Hedrick

Quoted 6 lines · Anton Altaparmakov

Quoted 6 lines
Quoted 1 line
You have to give Jens the credit for gluing it togather, because there
was
Quoted 3 lines
no way I would have figured out the suttle issues of BIO. There are serveral additions need to fix all the archs so hope to have something today.

Quoted 6 lines

Quoted 2 lines
Well, good work to both of you. It is an excellent driver. Please submit it for pre2.
I second that. 2.5.3-pre1 + 2nd patch from Jens + daft compile fixes is running fine on my Athlon/VIA chipset both UDMA100 and PIO4 on my 60GXP drive! Brilliant work guys!

Thanks!

Quoted 9 lines

There is only one odd thing and that is that PIO transfers are slower in 2.5.3-pre1 + 2nd patch from Jens + daft compile fixes compared to in 2.5.2-pre11-vanilla.
PIO mode, hdparm -t /dev/hda:
        on 2.5.3-pre1 + 2nd Jens patch: 4.62MB/s
        on 2.5.2-pre11 vanilla:         7.36MB/s

Quoted 8 lines

DMA transfers are the same with both kernels, peaking at 38-39MB/s which is I believe the theoretical upper limit for the 60GXP so there was not much room for visible improvement (2.5.3-pre1 is slightly faster in that it gave in three tests 38.79MB/s while 2.5.2-pre11 gave in three tests 38.32MB/s, so if you believe those ACB is .4 MB/s faster).
I don't care that PIO has become slower, all modern devices are happy with UDMA, and so am I. (-: But I thought I should mention it.

Interesting, I dunno where this performance decrease comes from right now. But I'm sure we can make it go at least as fast as 2.5.2-pre11, the primary concern right now was solid PIO + mult mode again, and I think we've achieved that.

Quoted 2 lines

Note that 2.5.3-pre1 now survives find . -type f -exec md5sum "{}" \; both in PIO and UDMA mode so it can really be considered stable on UP system.

Great, thanks for testing.

Signature · 1 line

Jens Axboe