serialize access to ide device

serialize access to ide device

[Jens Axboe]

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).

diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide.c linux-2.6.5/drivers/ide/ide.c
--- /opt/kernel/linux-2.6.5/drivers/ide/ide.c	2004-05-25 11:50:14.797583926 +0200
+++ linux-2.6.5/drivers/ide/ide.c	2004-05-25 11:52:40.367855970 +0200
@@ -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
diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide-io.c linux-2.6.5/drivers/ide/ide-io.c
--- /opt/kernel/linux-2.6.5/drivers/ide/ide-io.c	2004-05-25 11:50:15.174543192 +0200
+++ linux-2.6.5/drivers/ide/ide-io.c	2004-05-25 11:53:34.606000019 +0200
@@ -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)
diff -urp /opt/kernel/linux-2.6.5/drivers/ide/ide-lib.c linux-2.6.5/drivers/ide/ide-lib.c
--- /opt/kernel/linux-2.6.5/drivers/ide/ide-lib.c	2004-05-25 11:50:15.204539951 +0200
+++ linux-2.6.5/drivers/ide/ide-lib.c	2004-05-25 11:52:40.433848845 +0200
@@ -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);
diff -urp /opt/kernel/linux-2.6.5/include/linux/ide.h linux-2.6.5/include/linux/ide.h
--- /opt/kernel/linux-2.6.5/include/linux/ide.h	2004-05-25 11:50:29.701973356 +0200
+++ linux-2.6.5/include/linux/ide.h	2004-05-25 11:52:40.457846254 +0200
@@ -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[];
 

-- 
Jens Axboe

Re: serialize access to ide device

[Geert Uytterhoeven]

On Mon, 2 Aug 2004, Jens Axboe wrote:
> +		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 -- geert@linux-m68k.org

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

Re: serialize access to ide device

[Jens Axboe]

On Mon, Aug 02 2004, Geert Uytterhoeven wrote:
> On Mon, 2 Aug 2004, Jens Axboe wrote:
> > +		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.

-- 
Jens Axboe

Re: serialize access to ide device

[Alan Cox]

On Iau, 2004-08-19 at 14:14, Bartlomiej Zolnierkiewicz wrote:
> 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

Re: serialize access to ide device

[Bartlomiej Zolnierkiewicz]

On Monday 02 August 2004 15:11, Jens Axboe wrote:
> Hi Bart,

Hi,

Late reply follows...

> 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.

> 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

Re: serialize access to ide device

[Jens Axboe]

On Thu, Aug 19 2004, Bartlomiej Zolnierkiewicz wrote:
> > 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.

> > 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.

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

-- 
Jens Axboe

Re: serialize access to ide device

[Bartlomiej Zolnierkiewicz]

On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > 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)

Re: serialize access to ide device

[Jens Axboe]

On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > > 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.

-- 
Jens Axboe

Re: serialize access to ide device

[Bartlomiej Zolnierkiewicz]

On Saturday 21 August 2004 18:21, Jens Axboe wrote:
> On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > > > 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.

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

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

Re: serialize access to ide device

[Jens Axboe]

On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 21 August 2004 18:21, Jens Axboe wrote:
> > On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > > On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > > > > 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.
> 
> 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.

-- 
Jens Axboe

Re: serialize access to ide device

[Alan Cox]

On Llu, 2004-08-23 at 16:02, Bartlomiej Zolnierkiewicz wrote:
> 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. 

Re: serialize access to ide device

[Bartlomiej Zolnierkiewicz]

On Monday 23 August 2004 14:15, Jens Axboe wrote:
> On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 21 August 2004 18:21, Jens Axboe wrote:
> > > On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > > > On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > > > > > 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.
> >
> > 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.

Re: serialize access to ide device

[Jens Axboe]

On Mon, Aug 23 2004, Bartlomiej Zolnierkiewicz wrote:
> On Monday 23 August 2004 14:15, Jens Axboe wrote:
> > On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > > On Saturday 21 August 2004 18:21, Jens Axboe wrote:
> > > > On Sat, Aug 21 2004, Bartlomiej Zolnierkiewicz wrote:
> > > > > On Saturday 21 August 2004 12:32, Jens Axboe wrote:
> > > > > > > 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.
> > >
> > > 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.

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

-- 
Jens Axboe

Re: serialize access to ide device

[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).

	Jeff

Re: serialize access to ide device

[Jens Axboe]

On Mon, Aug 23 2004, Jeff Garzik wrote:
> 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 :)

-- 
Jens Axboe