serialize access to ide device

PATCH Jens Axboe 5 participants 15 messages Aug 2, 2004 – Aug 23, 2004

Share & tools

Jens Axboe

author

2004-08-02 13:12 UTC #0 lore raw

Hi Bart,

2.6 breaks really really easily if you have any traffic on a device and issue a hdparm (or similar) command to it. Things like set_using_dma() and ide_set_xfer_rate() just stomp all over the drive regardless of what it's doing right now.

I hacked something up for the SUSE kernel to fix this _almost_, it still doesn't handle cases where you want to serialize across more than a single channel. Not a common case, but I think there is such hardware out there (which?).

Clearly something needs to be done about this, it's extremely frustrating not to be able to reliably turn on dma on a drive at all. I'm just tossing this one out there to solve 99% of the case, I'd like some input from you on what you feel we should do.

(Note patch is for the SLES9 base kernel, should be easy to adapt. I didn't do that, since I'm just sparking conversation - I will if you want it, though).

drivers/ide/ide.c renamed from opt/kernel/linux-2.6.5/drivers/ide/ide.c +16 -6

diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide.c linux-2.6.5/drivers/ide/ide.c
@@ -1289,18 +1289,28 @@ static int set_io_32bit(ide_drive_t *dri
 static int set_using_dma (ide_drive_t *drive, int arg)
 {
 #ifdef CONFIG_BLK_DEV_IDEDMA
+	int ret = -EPERM;
+
+	ide_pin_hwgroup(drive);
+
 	if (!drive->id || !(drive->id->capability & 1))
-		return -EPERM;
+		goto out;
 	if (HWIF(drive)->ide_dma_check == NULL)
-		return -EPERM;
+		goto out;
+	ret = -EIO;
 	if (arg) {
-		if (HWIF(drive)->ide_dma_check(drive)) return -EIO;
-		if (HWIF(drive)->ide_dma_on(drive)) return -EIO;
+		if (HWIF(drive)->ide_dma_check(drive))
+			goto out;
+		if (HWIF(drive)->ide_dma_on(drive))
+			goto out;
 	} else {
 		if (__ide_dma_off(drive))
-			return -EIO;
+			goto out;
 	}
-	return 0;
+	ret = 0;
+out:
+	ide_unpin_hwgroup(drive);
+	return ret;
 #else
 	return -EPERM;
 #endif

drivers/ide/ide-io.c renamed from opt/kernel/linux-2.6.5/drivers/ide/ide-io.c +40 -0

diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide-io.c linux-2.6.5/drivers/ide/ide-io.c
@@ -881,6 +881,46 @@ void ide_stall_queue (ide_drive_t *drive
 	drive->sleep = timeout + jiffies;
 }
 
+void ide_unpin_hwgroup(ide_drive_t *drive)
+{
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+
+	if (hwgroup) {
+		spin_lock_irq(&ide_lock);
+		HWGROUP(drive)->busy = 0;
+		drive->blocked = 0;
+		do_ide_request(drive->queue);
+		spin_unlock_irq(&ide_lock);
+	}
+}
+
+void ide_pin_hwgroup(ide_drive_t *drive)
+{
+	ide_hwgroup_t *hwgroup = HWGROUP(drive);
+
+	/*
+	 * should only happen very early, so not a problem
+	 */
+	if (!hwgroup)
+		return;
+
+	spin_lock_irq(&ide_lock);
+	do {
+		if (!hwgroup->busy && !drive->blocked && !drive->doing_barrier)
+			break;
+		spin_unlock_irq(&ide_lock);
+		schedule_timeout(HZ/100);
+		spin_lock_irq(&ide_lock);
+	} while (hwgroup->busy || drive->blocked || drive->doing_barrier);
+
+	/*
+	 * we've now secured exclusive access to this hwgroup
+	 */
+	hwgroup->busy = 1;
+	drive->blocked = 1;
+	spin_unlock_irq(&ide_lock);
+}
+
 EXPORT_SYMBOL(ide_stall_queue);
 
 #define WAKEUP(drive)	((drive)->service_start + 2 * (drive)->service_time)

drivers/ide/ide-lib.c renamed from opt/kernel/linux-2.6.5/drivers/ide/ide-lib.c +7 -3

diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide-lib.c linux-2.6.5/drivers/ide/ide-lib.c
@@ -436,13 +436,17 @@ EXPORT_SYMBOL(ide_toggle_bounce);
  
 int ide_set_xfer_rate(ide_drive_t *drive, u8 rate)
 {
+	int ret;
 #ifndef CONFIG_BLK_DEV_IDEDMA
 	rate = min(rate, (u8) XFER_PIO_4);
 #endif
-	if(HWIF(drive)->speedproc)
-		return HWIF(drive)->speedproc(drive, rate);
+	ide_pin_hwgroup(drive);
+	if (HWIF(drive)->speedproc)
+		ret = HWIF(drive)->speedproc(drive, rate);
 	else
-		return -1;
+		ret = -1;
+	ide_unpin_hwgroup(drive);
+	return ret;
 }
 
 EXPORT_SYMBOL_GPL(ide_set_xfer_rate);

include/linux/ide.h renamed from opt/kernel/linux-2.6.5/include/linux/ide.h +3 -0

diff -urp /opt/kernel/linux-2.6.5/include/linux/ide.h linux-2.6.5/include/linux/ide.h
@@ -1474,6 +1474,9 @@ extern irqreturn_t ide_intr(int irq, voi
 extern void do_ide_request(request_queue_t *);
 extern void ide_init_subdrivers(void);
 
+extern void ide_pin_hwgroup(ide_drive_t *);
+extern void ide_unpin_hwgroup(ide_drive_t *);
+
 extern struct block_device_operations ide_fops[];
 extern ide_proc_entry_t generic_subdriver_entries[];

Signature · 1 line

Jens Axboe

Geert Uytterhoeven

2004-08-02 13:41 UTC #1 lore raw

drivers/ide/ide-io.c void ide_stall_queue (ide_drive_t *drive · line 912

Quoted 1 line · Jens Axboe

drivers/ide/ide-io.c renamed from opt/kernel/linux-2.6.5/drivers/ide/ide-io.c +1 -0
+		schedule_timeout(HZ/100);

Hmm, we still have a few platforms where HZ < 100.

Probably safer to use `schedule_timeout((HZ+99)/100);'?

BTW, did anyone ever audit the kernel for such usages?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Jens Axboe

author

2004-08-02 13:51 UTC #2 lore raw

Quoted 8 lines · Geert Uytterhoeven

drivers/ide/ide-io.c void ide_stall_queue (ide_drive_t *drive · line 912
Quoted 1 line · Jens Axboe
drivers/ide/ide-io.c renamed from opt/kernel/linux-2.6.5/drivers/ide/ide-io.c +1 -0
+		schedule_timeout(HZ/100);
Hmm, we still have a few platforms where HZ < 100.
Probably safer to use `schedule_timeout((HZ+99)/100);'?
BTW, did anyone ever audit the kernel for such usages?

Good point, or perhaps just 1 instead.

Signature · 1 line

Jens Axboe

Bartlomiej Zolnierkiewicz

2004-08-19 13:14 UTC #3 lore raw

In reply to Jens Axboe

Quoted 1 line · Jens Axboe

Hi Bart,

Hi,

Late reply follows...

Quoted 4 lines

2.6 breaks really really easily if you have any traffic on a device and issue a hdparm (or similar) command to it. Things like set_using_dma() and ide_set_xfer_rate() just stomp all over the drive regardless of what it's doing right now.

Yep, known problem.

Quoted 9 lines

I hacked something up for the SUSE kernel to fix this _almost_, it still doesn't handle cases where you want to serialize across more than a single channel. Not a common case, but I think there is such hardware out there (which?).
Clearly something needs to be done about this, it's extremely frustrating not to be able to reliably turn on dma on a drive at all. I'm just tossing this one out there to solve 99% of the case, I'd like some input from you on what you feel we should do.

What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?

This should be a lot cleaner and will cover 100% cases.

Bartlomiej

Alan Cox

2004-08-19 13:34 UTC #4 lore raw

Quoted 3 lines · Bartlomiej Zolnierkiewicz

What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?

Definitely the right direction but it doesn't ultimately fix the bigger problem which is that in some cases you need to issue multiple commands in order to set up the drive and cannot do that in one sequence if the drive is active. This could also be done with some kind of ATA_IO SG_IO like functionality for the most part (the drive side).

The reset() logic also has the same problem when it occurs from process context. The newer 2.4 code in 2.6 as well is only a bandaid that normally works.

Serialization is indeed needed for some controllers (CMD64x in some cases, RZ1000 and others). Certain controllers have drive pair constraints and in some cases BIOSes know a lot about DMA suitability of their configurations. Thats more an argument that using hdparm -d1 at random is playing with fire.

Alan

Jens Axboe

author

2004-08-21 10:32 UTC #5 lore raw

In reply to Bartlomiej Zolnierkiewicz

Quoted 6 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines
2.6 breaks really really easily if you have any traffic on a device and issue a hdparm (or similar) command to it. Things like set_using_dma() and ide_set_xfer_rate() just stomp all over the drive regardless of what it's doing right now.
Yep, known problem.

Something should be done about it, it's pretty critical imho.

Quoted 10 lines

Quoted 9 lines
I hacked something up for the SUSE kernel to fix this _almost_, it still doesn't handle cases where you want to serialize across more than a single channel. Not a common case, but I think there is such hardware out there (which?).
Clearly something needs to be done about this, it's extremely frustrating not to be able to reliably turn on dma on a drive at all. I'm just tossing this one out there to solve 99% of the case, I'd like some input from you on what you feel we should do.

Quoted 5 lines

What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.

That will still only serialize per-channel. But yes, a lot cleaner.

Signature · 1 line

Jens Axboe

Bartlomiej Zolnierkiewicz

2004-08-21 14:46 UTC #6 lore raw

Quoted 7 lines · Jens Axboe

Quoted 5 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.
That will still only serialize per-channel. But yes, a lot cleaner.

per hwgroup not per channel
(serializing per host device will be better but requires even more work)

Jens Axboe

author

2004-08-21 16:22 UTC #7 lore raw

Quoted 11 lines · Bartlomiej Zolnierkiewicz

Quoted 7 lines · Jens Axboe
Quoted 5 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.
That will still only serialize per-channel. But yes, a lot cleaner.
per hwgroup not per channel
(serializing per host device will be better but requires even more work)

Sorry yes hwgroup, that's what I meant. The case I worried about in my patch (and noted) is that it doesn't cover per-hwif and neither would a special request.

Signature · 1 line

Jens Axboe

Bartlomiej Zolnierkiewicz

2004-08-21 17:15 UTC #8 lore raw

Quoted 8 lines · Jens Axboe

Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jens Axboe
Quoted 5 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
That will still only serialize per-channel. But yes, a lot cleaner.
per hwgroup not per channel
(serializing per host device will be better but requires even more work)
Sorry yes hwgroup, that's what I meant. The case I worried about in my patch (and noted) is that it doesn't cover per-hwif and neither would a special request.

I guess you meant 'per-host' because hwif == channel.

[ You are of course right for about 'per-host' case. ]

Jens Axboe

author

2004-08-23 12:17 UTC #9 lore raw

Quoted 14 lines · Bartlomiej Zolnierkiewicz

Quoted 13 lines · Jens Axboe
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 7 lines · Jens Axboe
Quoted 5 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.
That will still only serialize per-channel. But yes, a lot cleaner.
per hwgroup not per channel
(serializing per host device will be better but requires even more work)

Quoted 7 lines

Quoted 3 lines
Sorry yes hwgroup, that's what I meant. The case I worried about in my patch (and noted) is that it doesn't cover per-hwif and neither would a special request.
I guess you meant 'per-host' because hwif == channel.
[ You are of course right for about 'per-host' case. ]

Yep, per host. So REQ_SPECIAL-like approach is cleaner, but doesn't cover more cases than a simple hwif pinning would anyways. You'd need some code to quisce the host in any case.

Signature · 1 line

Jens Axboe

Bartlomiej Zolnierkiewicz

2004-08-23 15:30 UTC #10 lore raw

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Bartlomiej Zolnierkiewicz
Quoted 17 lines · Jens Axboe
Quoted 5 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines · Jens Axboe
Quoted 4 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
This should be a lot cleaner and will cover 100% cases.
That will still only serialize per-channel. But yes, a lot cleaner.
per hwgroup not per channel
(serializing per host device will be better but requires even more work)
Sorry yes hwgroup, that's what I meant. The case I worried about in my patch (and noted) is that it doesn't cover per-hwif and neither would a special request.

Quoted 7 lines

Quoted 3 lines
I guess you meant 'per-host' because hwif == channel.
[ You are of course right for about 'per-host' case. ]
Yep, per host. So REQ_SPECIAL-like approach is cleaner, but doesn't cover more cases than a simple hwif pinning would anyways. You'd need some code to quisce the host in any case.

No, REQ_SPECIAL-like approach would serialize per ide_hwgroup_t and ide_hwgroup_t may serialize more then one ide_hwif_t. See ide-probe.c.

Jens Axboe

author

2004-08-23 15:34 UTC #11 lore raw

Quoted 23 lines · Bartlomiej Zolnierkiewicz

Quoted 22 lines · Jens Axboe
Quoted 21 lines · Bartlomiej Zolnierkiewicz
Quoted 10 lines · Jens Axboe
Quoted 9 lines · Bartlomiej Zolnierkiewicz
Quoted 7 lines · Jens Axboe
Quoted 5 lines
What about adding new kind of REQ_SPECIAL request and converting set_using_dma(), set_xfer_rate(), ..., to be callback functions for this request?
This should be a lot cleaner and will cover 100% cases.
That will still only serialize per-channel. But yes, a lot cleaner.
Quoted 7 lines
Quoted 3 lines
per hwgroup not per channel
(serializing per host device will be better but requires even more work)
Sorry yes hwgroup, that's what I meant. The case I worried about in my patch (and noted) is that it doesn't cover per-hwif and neither would a special request.
I guess you meant 'per-host' because hwif == channel.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
[ You are of course right for about 'per-host' case. ]
Yep, per host. So REQ_SPECIAL-like approach is cleaner, but doesn't cover more cases than a simple hwif pinning would anyways. You'd need some code to quisce the host in any case.
No, REQ_SPECIAL-like approach would serialize per ide_hwgroup_t and ide_hwgroup_t may serialize more then one ide_hwif_t. See ide-probe.c.

I see, that would work. So you would need to doctor some message type system for these requests. Are you going to do this?

Signature · 1 line

Jens Axboe

Alan Cox

2004-08-23 15:54 UTC #12 lore raw

In reply to Bartlomiej Zolnierkiewicz

Quoted 2 lines · Bartlomiej Zolnierkiewicz

No, REQ_SPECIAL-like approach would serialize per ide_hwgroup_t and ide_hwgroup_t may serialize more then one ide_hwif_t. See ide-probe.c.

This is probably appropriately paranoid behaviour anyway.

Jeff Garzik

2004-08-23 16:11 UTC #13 lore raw

In reply to Bartlomiej Zolnierkiewicz

As a tangent...

Per-host synchronization is REQUIRED for controllers which indicate "simplex" BMDMA. And I think for some strange non-simplex controllers too (though I guess they could be classified as simplex).

Jeff

Jens Axboe

author

2004-08-23 17:00 UTC #14 lore raw

Quoted 6 lines · Jeff Garzik

As a tangent...
Per-host synchronization is REQUIRED for controllers which indicate "simplex" BMDMA. And I think for some strange non-simplex controllers too (though I guess they could be classified as simplex).

I think this is exactly the case mentioned (where that level of serialization is required), so not a tangent :)

Signature · 1 line

Jens Axboe