Re: [GIT PULL] Core block IO bits for 2.6.39

Re: [GIT PULL] Core block IO bits for 2.6.39

[Geert Uytterhoeven]

Hi Jens,

On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
> Jens Axboe (20):
>      block: remove per-queue plugging

This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
on Atari/m68k under ARAnyM. It hangs on:

| ide: Falcon IDE controller
| Probing IDE interface ide0...
| hda: Sarge m68k, ATA DISK drive
| ide0 at 0xfff00000 on irq 15 (serialized)
| ide-gd driver 1.18
| hda: max request size: 128KiB
| hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63

The next expected line is the partition parsing:

| hda: AHDI hda1 hda2

I tried reverting it, but it caused too many conflicts.
The parent of 7eaceaccab5f40bbfda044629a6298616aeaed50 works.

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: [GIT PULL] Core block IO bits for 2.6.39

[Jens Axboe]

On 2011-03-25 22:35, Geert Uytterhoeven wrote:
> Hi Jens,
> 
> On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
>> Jens Axboe (20):
>>      block: remove per-queue plugging
> 
> This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
> on Atari/m68k under ARAnyM. It hangs on:
> 
> | ide: Falcon IDE controller
> | Probing IDE interface ide0...
> | hda: Sarge m68k, ATA DISK drive
> | ide0 at 0xfff00000 on irq 15 (serialized)
> | ide-gd driver 1.18
> | hda: max request size: 128KiB
> | hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63
> 
> The next expected line is the partition parsing:
> 
> | hda: AHDI hda1 hda2

Geert, does this work for you?

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index f407784..381017c 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -549,6 +549,9 @@ plug_device_2:
 
 	if (rq)
 		blk_requeue_request(q, rq);
+
+	/* Use 3ms as that was the old plug delay */
+	blk_delay_queue(q, msecs_to_jiffies(3));
 }
 
 void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
@@ -561,6 +564,8 @@ void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
 	if (rq)
 		blk_requeue_request(q, rq);
 
+	/* Use 3ms as that was the old plug delay */
+	blk_delay_queue(q, msecs_to_jiffies(3));
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 

-- 
Jens Axboe

Re: [GIT PULL] Core block IO bits for 2.6.39

[Geert Uytterhoeven]

On Sat, Mar 26, 2011 at 07:29, Jens Axboe <jaxboe@fusionio.com> wrote:
> On 2011-03-25 22:35, Geert Uytterhoeven wrote:
>> On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
>>> Jens Axboe (20):
>>>      block: remove per-queue plugging
>>
>> This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
>> on Atari/m68k under ARAnyM. It hangs on:
>>
>> | ide: Falcon IDE controller
>> | Probing IDE interface ide0...
>> | hda: Sarge m68k, ATA DISK drive
>> | ide0 at 0xfff00000 on irq 15 (serialized)
>> | ide-gd driver 1.18
>> | hda: max request size: 128KiB
>> | hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63
>>
>> The next expected line is the partition parsing:
>>
>> | hda: AHDI hda1 hda2
>
> Geert, does this work for you?

Yep.Thanks!

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index f407784..381017c 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -549,6 +549,9 @@ plug_device_2:
>
>        if (rq)
>                blk_requeue_request(q, rq);
> +
> +       /* Use 3ms as that was the old plug delay */
> +       blk_delay_queue(q, msecs_to_jiffies(3));
>  }
>
>  void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
> @@ -561,6 +564,8 @@ void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
>        if (rq)
>                blk_requeue_request(q, rq);
>
> +       /* Use 3ms as that was the old plug delay */
> +       blk_delay_queue(q, msecs_to_jiffies(3));
>        spin_unlock_irqrestore(q->queue_lock, flags);
>  }
>

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: [GIT PULL] Core block IO bits for 2.6.39

[Jens Axboe]

On 2011-03-26 08:21, Geert Uytterhoeven wrote:
> On Sat, Mar 26, 2011 at 07:29, Jens Axboe <jaxboe@fusionio.com> wrote:
>> On 2011-03-25 22:35, Geert Uytterhoeven wrote:
>>> On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
>>>> Jens Axboe (20):
>>>>      block: remove per-queue plugging
>>>
>>> This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
>>> on Atari/m68k under ARAnyM. It hangs on:
>>>
>>> | ide: Falcon IDE controller
>>> | Probing IDE interface ide0...
>>> | hda: Sarge m68k, ATA DISK drive
>>> | ide0 at 0xfff00000 on irq 15 (serialized)
>>> | ide-gd driver 1.18
>>> | hda: max request size: 128KiB
>>> | hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63
>>>
>>> The next expected line is the partition parsing:
>>>
>>> | hda: AHDI hda1 hda2
>>
>> Geert, does this work for you?
> 
> Yep.Thanks!

Great! I think we should place those blk_delay_queue() calls under the
if (rq), that should workd to and be more optimal. Can I ask you to
check that, too?

So:

if (rq) {
        blk_requeue_request(q, rq);
        blk_delay_queue(q, msecs_to_jiffies(3));
}

in both locations.

-- 
Jens Axboe

Re: [GIT PULL] Core block IO bits for 2.6.39

[Geert Uytterhoeven]

On Sat, Mar 26, 2011 at 09:25, Jens Axboe <jaxboe@fusionio.com> wrote:
> On 2011-03-26 08:21, Geert Uytterhoeven wrote:
>> On Sat, Mar 26, 2011 at 07:29, Jens Axboe <jaxboe@fusionio.com> wrote:
>>> On 2011-03-25 22:35, Geert Uytterhoeven wrote:
>>>> On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
>>>>> Jens Axboe (20):
>>>>>      block: remove per-queue plugging
>>>>
>>>> This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
>>>> on Atari/m68k under ARAnyM. It hangs on:
>>>>
>>>> | ide: Falcon IDE controller
>>>> | Probing IDE interface ide0...
>>>> | hda: Sarge m68k, ATA DISK drive
>>>> | ide0 at 0xfff00000 on irq 15 (serialized)
>>>> | ide-gd driver 1.18
>>>> | hda: max request size: 128KiB
>>>> | hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63
>>>>
>>>> The next expected line is the partition parsing:
>>>>
>>>> | hda: AHDI hda1 hda2
>>>
>>> Geert, does this work for you?
>>
>> Yep.Thanks!
>
> Great! I think we should place those blk_delay_queue() calls under the
> if (rq), that should workd to and be more optimal. Can I ask you to
> check that, too?
>
> So:
>
> if (rq) {
>        blk_requeue_request(q, rq);
>        blk_delay_queue(q, msecs_to_jiffies(3));
> }
>
> in both locations.

Works, too.

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: [GIT PULL] Core block IO bits for 2.6.39

[Jens Axboe]

On 2011-03-26 09:34, Geert Uytterhoeven wrote:
> On Sat, Mar 26, 2011 at 09:25, Jens Axboe <jaxboe@fusionio.com> wrote:
>> On 2011-03-26 08:21, Geert Uytterhoeven wrote:
>>> On Sat, Mar 26, 2011 at 07:29, Jens Axboe <jaxboe@fusionio.com> wrote:
>>>> On 2011-03-25 22:35, Geert Uytterhoeven wrote:
>>>>> On Thu, Mar 24, 2011 at 14:43, Jens Axboe <jaxboe@fusionio.com> wrote:
>>>>>> Jens Axboe (20):
>>>>>>      block: remove per-queue plugging
>>>>>
>>>>> This one (commit 7eaceaccab5f40bbfda044629a6298616aeaed50) breaks IDE
>>>>> on Atari/m68k under ARAnyM. It hangs on:
>>>>>
>>>>> | ide: Falcon IDE controller
>>>>> | Probing IDE interface ide0...
>>>>> | hda: Sarge m68k, ATA DISK drive
>>>>> | ide0 at 0xfff00000 on irq 15 (serialized)
>>>>> | ide-gd driver 1.18
>>>>> | hda: max request size: 128KiB
>>>>> | hda: 2118816 sectors (1084 MB) w/256KiB Cache, CHS=2102/16/63
>>>>>
>>>>> The next expected line is the partition parsing:
>>>>>
>>>>> | hda: AHDI hda1 hda2
>>>>
>>>> Geert, does this work for you?
>>>
>>> Yep.Thanks!
>>
>> Great! I think we should place those blk_delay_queue() calls under the
>> if (rq), that should workd to and be more optimal. Can I ask you to
>> check that, too?
>>
>> So:
>>
>> if (rq) {
>>        blk_requeue_request(q, rq);
>>        blk_delay_queue(q, msecs_to_jiffies(3));
>> }
>>
>> in both locations.
> 
> Works, too.

Great, thanks for testing! I'll queue this up and submit it so it makes
2.6.39-rc1.

-- 
Jens Axboe

Re: [GIT PULL] Core block IO bits for 2.6.39

[Linus Torvalds]

On Fri, Mar 25, 2011 at 11:29 PM, Jens Axboe <jaxboe@fusionio.com> wrote:
> +
> +       /* Use 3ms as that was the old plug delay */
> +       blk_delay_queue(q, msecs_to_jiffies(3));

That's bogus. blk_delay_queue() already takes msecs, not jiffies.

Also, do we really need to delay every unplug like this? It seems sad.
A 3ms delay is a long time these days - admittedly most people
hopefully use ATA these days if they have an SSD or something, but
still..

                  Linus

Re: [GIT PULL] Core block IO bits for 2.6.39

[Jens Axboe]

On 2011-03-26 17:48, Linus Torvalds wrote:
> On Fri, Mar 25, 2011 at 11:29 PM, Jens Axboe <jaxboe@fusionio.com> wrote:
>> +
>> +       /* Use 3ms as that was the old plug delay */
>> +       blk_delay_queue(q, msecs_to_jiffies(3));
> 
> That's bogus. blk_delay_queue() already takes msecs, not jiffies.

You are right, braino.

> Also, do we really need to delay every unplug like this? It seems sad.
> A 3ms delay is a long time these days - admittedly most people
> hopefully use ATA these days if they have an SSD or something, but
> still..

Depends on whether you want the 'call me back, I ran into busy this
time' or the 'recall me immediately'.

I'll take a look at the IDE case tonight and submit a real fix.

-- 
Jens Axboe

Re: [GIT PULL] Core block IO bits for 2.6.39

[Jens Axboe]

On 2011-03-26 17:53, Jens Axboe wrote:
> On 2011-03-26 17:48, Linus Torvalds wrote:
>> On Fri, Mar 25, 2011 at 11:29 PM, Jens Axboe <jaxboe@fusionio.com> wrote:
>>> +
>>> +       /* Use 3ms as that was the old plug delay */
>>> +       blk_delay_queue(q, msecs_to_jiffies(3));
>>
>> That's bogus. blk_delay_queue() already takes msecs, not jiffies.
> 
> You are right, braino.
> 
>> Also, do we really need to delay every unplug like this? It seems sad.
>> A 3ms delay is a long time these days - admittedly most people
>> hopefully use ATA these days if they have an SSD or something, but
>> still..
> 
> Depends on whether you want the 'call me back, I ran into busy this
> time' or the 'recall me immediately'.
> 
> I'll take a look at the IDE case tonight and submit a real fix.

It's not completely clear cut what the delays should be in the various
old replug cases. Only for the drive sleeping can I make an educated
guess since we now the timeout period.

So something like this. Retains the same recall delay as we had before.
Someone who cares is free to go and optimize this, if the queue rerun
delay should be shorter in some cases.

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index f407784..0e406d73 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -440,6 +440,7 @@ void do_ide_request(struct request_queue *q)
 	struct ide_host *host = hwif->host;
 	struct request	*rq = NULL;
 	ide_startstop_t	startstop;
+	unsigned long queue_run_ms = 3; /* old plug delay */
 
 	spin_unlock_irq(q->queue_lock);
 
@@ -459,6 +460,9 @@ repeat:
 		prev_port = hwif->host->cur_port;
 		if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
 		    time_after(drive->sleep, jiffies)) {
+			unsigned long left = jiffies - drive->sleep;
+
+			queue_run_ms = jiffies_to_msecs(left + 1);
 			ide_unlock_port(hwif);
 			goto plug_device;
 		}
@@ -547,8 +551,10 @@ plug_device:
 plug_device_2:
 	spin_lock_irq(q->queue_lock);
 
-	if (rq)
+	if (rq) {
 		blk_requeue_request(q, rq);
+		blk_delay_queue(q, queue_run_ms);
+	}
 }
 
 void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
@@ -562,6 +568,10 @@ void ide_requeue_and_plug(ide_drive_t *drive, struct request *rq)
 		blk_requeue_request(q, rq);
 
 	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/* Use 3ms as that was the old plug delay */
+	if (rq)
+		blk_delay_queue(q, 3);
 }
 
 static int drive_is_ready(ide_drive_t *drive)


-- 
Jens Axboe

Re: [GIT PULL] Core block IO bits for 2.6.39

[Alan Cox]

On Sat, 26 Mar 2011 09:48:37 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Mar 25, 2011 at 11:29 PM, Jens Axboe <jaxboe@fusionio.com> wrote:
> > +
> > +       /* Use 3ms as that was the old plug delay */
> > +       blk_delay_queue(q, msecs_to_jiffies(3));
> 
> That's bogus. blk_delay_queue() already takes msecs, not jiffies.
> 
> Also, do we really need to delay every unplug like this? It seems sad.
> A 3ms delay is a long time these days - admittedly most people
> hopefully use ATA these days if they have an SSD or something, but
> still..

The ATA code contains several other bogus delays btw. So out of the box
unless you are using AHCI you are continually running small pointless
delays every command issued.