: ide dma timeout retry in pio

Hi,

We have the current problem of ide dma possibly tossing out a complete request, when we hit a dma timout. In this case, what we really want to do is retry the request in pio mode and revert to normal dma operations later again.

This patch catches the dma timout. It clears the dma engine, turns dma off, sanity checks the request, and makes sure that the ide request handler restarts the request (now in pio mode). When the first chunk of the request is finished, return to dma mode. If the dma timeouts keep happening, stay in pio mode.

Patch is untested for obvious reason, against 2.4.5-ac3

Jens Axboe

@@ -543,10 +543,20 @@
 {
 	struct request *rq;
 	unsigned long flags;
+	ide_drive_t *drive = hwgroup->drive;
 
 	spin_lock_irqsave(&io_request_lock, flags);
 	rq = hwgroup->rq;
 
+	/*
+	 * decide whether to reenable DMA -- 3 is a random magic for now,
+	 * if we DMA timeout more than 3 times, just stay in PIO
+	 */
+	if (drive->state == DMA_PIO_RETRY && drive->retry_pio <= 3) {
+		drive->state = 0;
+		hwgroup->hwif->dmaproc(ide_dma_on, drive);
+	}
+
 	if (!end_that_request_first(rq, uptodate, hwgroup->drive->name)) {
 		add_blkdev_randomness(MAJOR(rq->rq_dev));
 		blkdev_dequeue_request(rq);

@@ -1419,6 +1429,49 @@
 }
 
 /*
+ * un-busy the hwgroup etc, and clear any pending DMA status. we want to
+ * retry the current request in pio mode instead of risking tossing it
+ * all away
+ */
+void ide_dma_timeout_retry(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = HWIF(drive);
+	struct request *rq;
+
+	/*
+	 * end current dma transaction
+	 */
+	(void) hwif->dmaproc(ide_dma_end, drive);
+
+	/*
+	 * complain a little, later we might remove some of this verbosity
+	 */
+	printk("%s: timeout waiting for DMA\n", drive->name);
+	(void) hwif->dmaproc(ide_dma_timeout, drive);
+
+	/*
+	 * disable dma for now, but remember that we did so because of
+	 * a timeout -- we'll reenable after we finish this next request
+	 * (or rather the first chunk of it) in pio.
+	 */
+	drive->retry_pio++;
+	drive->state = DMA_PIO_RETRY;
+	(void) hwif->dmaproc(ide_dma_off_quietly, drive);
+
+	/*
+	 * un-busy drive etc (hwgroup->busy is cleared on return) and
+	 * make sure request is sane
+	 */
+	rq = HWGROUP(drive)->rq;
+	HWGROUP(drive)->rq = NULL;
+
+	rq->errors = 0;
+	rq->sector = rq->bh->b_rsector;
+	rq->current_nr_sectors = rq->bh->b_size >> 9;
+	rq->buffer = rq->bh->b_data;
+}
+
+/*
  * ide_timer_expiry() is our timeout function for all drive operations.
  * But note that it can also be invoked as a result of a "sleep" operation
  * triggered by the mod_timer() call in ide_do_request.

@@ -1491,11 +1544,10 @@
 				startstop = handler(drive);
 			} else {
 				if (drive->waiting_for_dma) {
-					(void) hwgroup->hwif->dmaproc(ide_dma_end, drive);
-					printk("%s: timeout waiting for DMA\n", drive->name);
-					(void) hwgroup->hwif->dmaproc(ide_dma_timeout, drive);
-				}
-				startstop = ide_error(drive, "irq timeout", GET_STAT());
+					startstop = ide_stopped;
+					ide_dma_timeout_retry(drive);
+				} else
+					startstop = ide_error(drive, "irq timeout", GET_STAT());
 			}
 			set_recovery_timer(hwif);
 			drive->service_time = jiffies - drive->service_start;

@@ -87,6 +87,11 @@
 #define ERROR_RECAL	1	/* Recalibrate every 2nd retry */
 
 /*
+ * state flags
+ */
+#define DMA_PIO_RETRY	1	/* retrying in PIO */
+
+/*
  * Ensure that various configuration flags have compatible settings
  */
 #ifdef REALLY_SLOW_IO

@@ -299,6 +304,8 @@
 	special_t	special;	/* special action flags */
 	byte     keep_settings;		/* restore settings after drive reset */
 	byte     using_dma;		/* disk is using dma for read/write */
+	byte	 retry_pio;		/* retrying dma capable host in pio */
+	byte	 state;			/* retry state */
 	byte     waiting_for_dma;	/* dma currently in progress */
 	byte     unmask;		/* flag: okay to unmask other irqs */
 	byte     slow;			/* flag: slow data port */

really? do we know the nature of the DMA engine problem well enough? is there a reason to believe that it'll work better "later"? I guess I was surprised at resorting to PIO - couldn't we just break the request up into smaller chunks, still using DMA?

I seem to recall Andre saying that the problem arises when the ide DMA engine looses PCI arbitration during a burst. shorter bursts would seem like the best workaround if this is the problem...

resorting to PIO would be such a shame, not only because it eats CPU so badly, but also because it has no checksum like UDMA...

thanks, mark hahn.

I think that this may be an issue with ACPI processor power saving... I have documented issues with ide DMA timeouts when the processor is put into the C3 power state. One of the things that happens in this state is that buss master arbitration is *disabled*..... bus master activity is *supposed* to transition the system back to a C0 power state. I'll bet there are some issues with the Linux IDE dma and disabling bus master arbitration...... ideas? thoughts? patches? ;-)

-- Chris

I can categorise some of them:

1. Hardware that just doesnt support it.
2. Timeouts that are false positives caused by disks having problems
and being very slow to recover
3. Bad cabling
4. Stalls caused by heavy PCI traffic

#1 will go fail, fail, fail -> PIO now (or should do Im about to try it) #2 and #4 will be transient
#3 could go either way

----- Original Message -----
From: "Alan Cox" <[email protected]>
To: "Mark Hahn" <[email protected]>
Cc: "Jens Axboe" <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Monday, May 28, 2001 5:12 PM
Subject: Re: [patch]: ide dma timeout retry in pio

enough?

it)

Where does the "'DMA Timeout -> disable DMA' then lose all responsiveness when I issue 'hdparm -d1' while it tries and fails to re-enable DMA" fit in? The disk will happily run for several days in UDMA33 and then at some point it craps out with a DMA timeout which results 1) DMA being turned off and 2) all attempts to re-enable DMA failing?

And what's up with this:

[root@MoveAlong james]# hdparm /dev/hda

/dev/hda:

 multcount    =  0 (off)
 I/O support  =  1 (32-bit)
 unmaskirq    =  1 (on)
 using_dma    =  1 (on)
 keepsettings =  1 (on)
 nowerr       =  0 (off)
 readonly     =  0 (off)
 readahead    =  8 (on)

geometry = 19590/16/63, sectors = 19746720, start = 0
[root@MoveAlong james]# hdparm -tT /dev/hda

/dev/hda:
Timing buffer-cache reads: 128 MB in 6.98 seconds = 18.34 MB/sec
Timing buffered disk reads: 64 MB in 5.77 seconds = 11.09 MB/sec
Hmm.. suspicious results: probably not enough free memory for a proper test.
[root@MoveAlong james]# free
total used free shared buffers
cached
Mem: 126800 123460 3340 0 67284
41572

-/+ buffers/cache:      14604     112196
Swap:       394624      32816     361808

I used to get ~33MB/sec on buffer-cache and ~10MB/sec on buffered disk reads in 2.2...

--JT

For what it is worth, in the recent postings I made about this topic, you suggested that it was bad cabling, I swapped the cabling, same problem. I swapped the mother board from Abit K7T to ASUS A7V and all cables worked fine.

I really think there is a software problem in there with certain chipsets, those from VIA seem to be problematic.

---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

Well given the catalogue of VIA chipset problems popping up on news sites right now that would not suprise me. Also the non -ac tree has a very out of date VIA ide driver although I don't think that impacts this case.

Alan

LM> For what it is worth, in the recent postings I made about this topic, you LM> suggested that it was bad cabling, I swapped the cabling, same problem. LM> I swapped the mother board from Abit K7T to ASUS A7V and all cables worked LM> fine.

Similar info about KT7 - changing cables (both 30 and 80 wire) on Abit KT7 did not help, still CRC errors (with all disks tried). So it looks like some KT7 boards have problems with IDE interface cabling or smth. like that.

Meelis Roos ([email protected])

I don't think it is a cabling problem, I think it is that motherboard. I suspect that the chipset on that motherboard is not well supported by Linux.

As an aside, I am less than impressed with the IDE support in Linux. It's been a constant source of problems for the last couple of years and it doesn't seem to get fixed. We seem to get lots of chip sets almost working and then move on to the next one.

---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

That is indeed possible, it will require some surgery to the dma request path though. IDE has no concept of doing part of a request for dma currently, it's an all-or-nothing approach. That's why it falls back to pio right now.

It's worth a shot. My patch was not meant as the end-all solution, however we need something _now_. Loosing sectors is not funny. Dynamically limiting general request size for to make dma work is a piece of cake, that'll be about a one-liner addition to the current patch. So the logic could be something of the order of:

- 1st dma timeout
- scale max size down from 128kB (127.5kB really) to half that
...
- things aren't working, 2nd dma timeout. Scale down to 32kB.

and so forth, revert to pio and reset full size if it's really no good. If limiting transfer sizes solves the problem, this would be the way to go. I'll do another version that does this.

Testers? Who has frequent ide dma timeout problems??

Look at the patch -- we resort to pio for _one_ hunk. That's 8 sectors tops, then back to dma. Hardly a big issue.

Jens Axboe

Unless we reissue the entire request from scratch you have no idea what if anything is on the platters. Since one can generally only get control over the device with a soft reset, you have to assume that anything and everything about that request was lost at the device level and begin again.

<RANT>
This is why it is so important to change to TFAM, because we carry a copy of the setup-seek operations with the request, and not unless we error out do we change that content. Thus is a timeout fault not a error case we have all the info to re-issue or copy into a retry queue. But as we all know the proper fix can not be even attempted until 2.5... </RANT>

One thing that I have been trying is to pop the local ISR at a timeout and that looks to handle much if the problem; however, I need to reassemble my one test box that I can reproduce the fault 100% of the time.

As I recall, there is a way to reinsert the faulted request, but that means the request_struct needs fault counter. If it is truly a DMA error because of re-seeks then the timeout value for that request must be expanded.

Now the final issue could be that the value of the calculated get-out timer may be to short for other reason and the driver is jumping the gun to notify and recover.

Cheers,

Andre Hedrick
Linux ATA Development
ASL Kernel Development
-----------------------------------------------------------------------------

ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

Look at the patch, that's what it does. For ide dma, it's all or nothing. So if it times out, no part of the request is ended ide_dma_timeout_retry does the sanity re-setup of the request for good measure, and it might be needed in the future when ide dma can do partial requests (2.5, not now). The request _is_ reissued from scratch.

This is bull shit. If IDE didn't muck around with the request so much in the first place, the info could always be trusted. Even so, we have the hard_* numbers to go by. So this argument does not hold.

Again, look at the patch. The request is never off the list, so there is never a reason to reinsert. hwgroup->busy is cleared (and, again for good measure, hwgroup->rq), so ide_do_request/start_request will get the same request that we just handled.

->errors, it's already there.

Yep

Jens Axboe

Maybe if you looked at the new code model as a whole you would see that the request-forking is gone. The object is to preserve a copy of the io instructions out to the registers to not have to repeat the do_request call unless it is a do or die thing. Also it is good to carry a copy of the local request even if it is never used. Also are you resetting the pointer (back to the geginning) on rq->buffer on the retry?

You first flush the DMA engine and issue a device soft reset not using the current drive reset, is presevers the hwgroup->busy state and allows the request to be retried without reinserting.

I will have to poke in a few flags to verify this but if you say so.

Wrong location to poke and by that time it requires a full retry. The new code would have had the task structs filled with the error.

In some cases yes, but it would be better if I had a standard counter that meant something. Also changing the jiffie counter in ide_delay_50ms to a mdelay may have done more harm than good.

Andre Hedrick
Linux ATA Development
ASL Kernel Development
-----------------------------------------------------------------------------

ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

There's only a 'fork' (I'm assuming you mean copy?) for the pio multwrite path, which I don't consider a big issue. I'm not saying that TFAM is not needed, I'm saying don't go touting that as the big saviour when it really has no relation to this topic at all.

Retry is by no means a performance thing, and I would indeed prefer to just pretend this is a brand new request rather than count on anything being in a sane state.

Of course

Again, _there is no reinsertion_. And why would we want to preserve hwgroup->busy? In fact, we need to clear it at all times to start the request over sanely.

Ok, this is how it goes: a queue list is build of requests. IDE grabs the very first request on the list, the path:

ide_do_request:

	hwgroup->rq = blkdev_entry_next_request(&drive->queue.queue_head)

start_request:

	struct request *rq = blkdev_entry_next_request(&drive->queue.queue_head)

There is no deletion going on. Request is started, runs, and is done. At this point the low level driver calls ide_end_request:

ide_end_request:

	if (last_part_of_request) {
		blkdev_dequeue_request(rq);
		hwgroup->rq = NULL;
		...
	}

_here_ we take the request off the list, and we better be completely done with it at this point. So as long as the request is not ended completely, it will always be the at the head of the pending queue.

->errors is just meant as a simple counter, so you can do stuff like

	if (io_error)
		if (rq->errors++ > ERROR_MAX)
			/* uh oh */

if you need more than this, then yes a dedicated 'ide command' pointer is what we want. You would need that for queueing anyway.

Jens Axboe