Hello,
When I first booted my 3.5.0-rc3 config, I've some :
Default I/O scheduler not found. Using noop.
which surprised me. so, after the boot completed, I had a look at the
system messages, and found :
...
io scheduler noop registered
io scheduler deadline registered
------------[ cut here ]------------
WARNING: at block/blk-cgroup.c:867 blkcg_policy_register+0xb5/0xc0()
Hardware name: Vostro 1520
Modules linked in:
Pid: 1, comm: swapper/0 Not tainted 3.5.0-rc3 #1
Call Trace:
[<ffffffff8103a59a>] warn_slowpath_common+0x7a/0xb0
[<ffffffff81cafbed>] ? deadline_init+0x12/0x12
[<ffffffff8103a5e5>] warn_slowpath_null+0x15/0x20
[<ffffffff8131a6f5>] blkcg_policy_register+0xb5/0xc0
[<ffffffff81cafc2f>] cfq_init+0x42/0x9f
[<ffffffff810001ba>] do_one_initcall+0x3a/0x160
[<ffffffff81c8ed08>] kernel_init+0x137/0x1bb
[<ffffffff81c8e5c5>] ? do_early_param+0x87/0x87
[<ffffffff8169e3d4>] kernel_thread_helper+0x4/0x10
[<ffffffff81c8ebd1>] ? start_kernel+0x3b2/0x3b2
[<ffffffff8169e3d0>] ? gs_change+0xb/0xb
---[ end trace 82bc55f036371117 ]---
So, at the time cfq should have registered, something went wrong.
It looks that this comes from my config defining CFQ, CGROUP but no
CFQ_CGROUP_IOSCHED.
...
CONFIG_CGROUPS=y
CONFIG_IOSCHED_CFQ=y
# CONFIG_CFQ_GROUP_IOSCHED is not set
...
as the code is :
static struct blkcg_policy blkcg_policy_cfq __maybe_unused;
...
#ifdef CONFIG_CFQ_GROUP_IOSCHED
static struct blkcg_policy blkcg_policy_cfq = {
.pd_size = sizeof(struct cfq_group),
.cftypes = cfq_blkcg_files,
.pd_init_fn = cfq_pd_init,
.pd_reset_stats_fn = cfq_pd_reset_stats,
};
#endif
...
static int __init cfq_init(void)
{
...
ret = blkcg_policy_register(&blkcg_policy_cfq);
Not sure what's the best fix is... Could someone shed some light on this ?
Paul
PS: Just checked 3.5-rc4, source code there is the same.
Hello,
On Wed, Jun 27, 2012 at 07:58:08PM +0200, Paul Rolland wrote:
> Hello,
>
> When I first booted my 3.5.0-rc3 config, I've some :
> Default I/O scheduler not found. Using noop.
> which surprised me. so, after the boot completed, I had a look at the
> system messages, and found :
>
> ...
> io scheduler noop registered
> io scheduler deadline registered
> ------------[ cut here ]------------
> WARNING: at block/blk-cgroup.c:867 blkcg_policy_register+0xb5/0xc0()
> Hardware name: Vostro 1520
> Modules linked in:
> Pid: 1, comm: swapper/0 Not tainted 3.5.0-rc3 #1
> Call Trace:
> [<ffffffff8103a59a>] warn_slowpath_common+0x7a/0xb0
> [<ffffffff81cafbed>] ? deadline_init+0x12/0x12
> [<ffffffff8103a5e5>] warn_slowpath_null+0x15/0x20
> [<ffffffff8131a6f5>] blkcg_policy_register+0xb5/0xc0
> [<ffffffff81cafc2f>] cfq_init+0x42/0x9f
> [<ffffffff810001ba>] do_one_initcall+0x3a/0x160
> [<ffffffff81c8ed08>] kernel_init+0x137/0x1bb
> [<ffffffff81c8e5c5>] ? do_early_param+0x87/0x87
> [<ffffffff8169e3d4>] kernel_thread_helper+0x4/0x10
> [<ffffffff81c8ebd1>] ? start_kernel+0x3b2/0x3b2
> [<ffffffff8169e3d0>] ? gs_change+0xb/0xb
> ---[ end trace 82bc55f036371117 ]---
Can you please test the following branch?
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-linus
Thanks.
--
tejun
Hello,
On Wed, 27 Jun 2012 11:22:02 -0700
Tejun Heo <[email protected]> wrote:
> Hello,
>
> > io scheduler noop registered
> Can you please test the following branch?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
> for-linus
Well code is the same.... so is the result :
[root@tux ~]# uname -a
Linux tux.DEF.witbe.net 3.5.0-rc4-g6b16351 #1 SMP PREEMPT Thu Jun 28
09:11:07 CEST 2012 x86_64 x86_64 x86_64 GNU/Linux
and
------------[ cut here ]------------
WARNING: at block/blk-cgroup.c:867 blkcg_policy_register+0xb5/0xc0()
Hardware name: Vostro 1520
Modules linked in:
Pid: 1, comm: swapper/0 Not tainted 3.5.0-rc4-g6b16351 #1
Call Trace:
[<ffffffff8103a59a>] warn_slowpath_common+0x7a/0xb0
[<ffffffff81cafbee>] ? deadline_init+0x12/0x12
[<ffffffff8103a5e5>] warn_slowpath_null+0x15/0x20
[<ffffffff8131aab5>] blkcg_policy_register+0xb5/0xc0
[<ffffffff81cafc30>] cfq_init+0x42/0x9f
[<ffffffff810001ba>] do_one_initcall+0x3a/0x160
[<ffffffff81c8ed08>] kernel_init+0x137/0x1bb
[<ffffffff81c8e5c5>] ? do_early_param+0x87/0x87
[<ffffffff8169e954>] kernel_thread_helper+0x4/0x10
[<ffffffff81c8ebd1>] ? start_kernel+0x3b2/0x3b2
[<ffffffff8169e950>] ? gs_change+0xb/0xb
---[ end trace 82bc55f036371117 ]---
and of course cfq is missing in :
[root@tux ~]# cat /sys/block/sda/queue/scheduler
[noop] deadline
Looking at the code, it seems strange that cfq_init requires
blkcg_policy_register to be successfull, wether you have defined
CFQ_CGROUP_IOSCHED or not.
So, I applied the following patch, and I now have:
[root@tux ~]# cat /sys/block/sda/queue/scheduler
noop deadline [cfq]
Not sure this is the correct patch, but at least it works for me (tm) ;)
Signed-Off-by: Paul Rolland <[email protected]>
diff -urN block/cfq-iosched.orig block/cfq-iosched.c
--- block/cfq-iosched.orig 2012-06-28 09:39:06.000000000 +0200
+++ block/cfq-iosched.c 2012-06-28 09:39:31.000000000 +0200
@@ -4198,9 +4198,11 @@
cfq_group_idle = 0;
#endif
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
ret = blkcg_policy_register(&blkcg_policy_cfq);
if (ret)
return ret;
+#endif
cfq_pool = KMEM_CACHE(cfq_queue, 0);
if (!cfq_pool)
@@ -4215,7 +4217,9 @@
err_free_pool:
kmem_cache_destroy(cfq_pool);
err_pol_unreg:
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
blkcg_policy_unregister(&blkcg_policy_cfq);
+#endif
return ret;
}
Paul
Hello,
On Thu, Jun 28, 2012 at 09:51:03AM +0200, Paul Rolland wrote:
> > git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
> > for-linus
>
> Well code is the same.... so is the result :
The code is not the same. block/for-linus has several fixes which
haven't been pushed to mainline yet including the following commit.
Thanks.
>From ffea73fc723a12fdde4c9fb3fcce5d154d1104a1 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Mon, 4 Jun 2012 10:02:29 +0200
Subject: [PATCH] block: blkcg_policy_cfq shouldn't be used if
!CONFIG_CFQ_GROUP_IOSCHED
cfq may be built w/ or w/o blkcg support depending on
CONFIG_CFQ_CGROUP_IOSCHED. If blkcg support is disabled, most of
related code is ifdef'd out but some part is left dangling -
blkcg_policy_cfq is left zero-filled and blkcg_policy_[un]register()
calls are made on it.
Feeding zero filled policy to blkcg_policy_register() is incorrect and
triggers the following WARN_ON() if CONFIG_BLK_CGROUP &&
!CONFIG_CFQ_GROUP_IOSCHED.
------------[ cut here ]------------
WARNING: at block/blk-cgroup.c:867
Modules linked in:
Modules linked in:
CPU: 3 Not tainted 3.4.0-09547-gfb21aff #1
Process swapper/0 (pid: 1, task: 000000003ff80000, ksp: 000000003ff7f8b8)
Krnl PSW : 0704100180000000 00000000003d76ca (blkcg_policy_register+0xca/0xe0)
R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:1 PM:0 EA:3
Krnl GPRS: 0000000000000000 00000000014b85ec 00000000014b85b0 0000000000000000
000000000096fb60 0000000000000000 00000000009a8e78 0000000000000048
000000000099c070 0000000000b6f000 0000000000000000 000000000099c0b8
00000000014b85b0 0000000000667580 000000003ff7fd98 000000003ff7fd70
Krnl Code: 00000000003d76be: a7280001 lhi %r2,1
00000000003d76c2: a7f4ffdf brc 15,3d7680
#00000000003d76c6: a7f40001 brc 15,3d76c8
>00000000003d76ca: a7c8ffea lhi %r12,-22
00000000003d76ce: a7f4ffce brc 15,3d766a
00000000003d76d2: a7f40001 brc 15,3d76d4
00000000003d76d6: a7c80000 lhi %r12,0
00000000003d76da: a7f4ffc2 brc 15,3d765e
Call Trace:
([<0000000000b6f000>] initcall_debug+0x0/0x4)
[<0000000000989e8a>] cfq_init+0x62/0xd4
[<00000000001000ba>] do_one_initcall+0x3a/0x170
[<000000000096fb60>] kernel_init+0x214/0x2bc
[<0000000000623202>] kernel_thread_starter+0x6/0xc
[<00000000006231fc>] kernel_thread_starter+0x0/0xc
no locks held by swapper/0/1.
Last Breaking-Event-Address:
[<00000000003d76c6>] blkcg_policy_register+0xc6/0xe0
---[ end trace b8ef4903fcbf9dd3 ]---
This patch fixes the problem by ensuring all blkcg support code is
inside CONFIG_CFQ_GROUP_IOSCHED.
* blkcg_policy_cfq declaration and blkg_to_cfqg() definition are moved
inside the first CONFIG_CFQ_GROUP_IOSCHED block. __maybe_unused is
dropped from blkcg_policy_cfq decl.
* blkcg_deactivate_poilcy() invocation is moved inside ifdef. This
also makes the activation logic match cfq_init_queue().
* All blkcg_policy_[un]register() invocations are moved inside ifdef.
Signed-off-by: Tejun Heo <[email protected]>
Reported-by: Heiko Carstens <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
block/cfq-iosched.c | 29 +++++++++++++++++------------
1 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index ae5113d..fb52df9 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -17,8 +17,6 @@
#include "blk.h"
#include "blk-cgroup.h"
-static struct blkcg_policy blkcg_policy_cfq __maybe_unused;
-
/*
* tunables
*/
@@ -418,11 +416,6 @@ static inline struct cfq_group *pd_to_cfqg(struct blkg_policy_data *pd)
return pd ? container_of(pd, struct cfq_group, pd) : NULL;
}
-static inline struct cfq_group *blkg_to_cfqg(struct blkcg_gq *blkg)
-{
- return pd_to_cfqg(blkg_to_pd(blkg, &blkcg_policy_cfq));
-}
-
static inline struct blkcg_gq *cfqg_to_blkg(struct cfq_group *cfqg)
{
return pd_to_blkg(&cfqg->pd);
@@ -572,6 +565,13 @@ static inline void cfqg_stats_update_avg_queue_size(struct cfq_group *cfqg) { }
#ifdef CONFIG_CFQ_GROUP_IOSCHED
+static struct blkcg_policy blkcg_policy_cfq;
+
+static inline struct cfq_group *blkg_to_cfqg(struct blkcg_gq *blkg)
+{
+ return pd_to_cfqg(blkg_to_pd(blkg, &blkcg_policy_cfq));
+}
+
static inline void cfqg_get(struct cfq_group *cfqg)
{
return blkg_get(cfqg_to_blkg(cfqg));
@@ -3951,10 +3951,11 @@ static void cfq_exit_queue(struct elevator_queue *e)
cfq_shutdown_timer_wq(cfqd);
-#ifndef CONFIG_CFQ_GROUP_IOSCHED
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
+ blkcg_deactivate_policy(q, &blkcg_policy_cfq);
+#else
kfree(cfqd->root_group);
#endif
- blkcg_deactivate_policy(q, &blkcg_policy_cfq);
kfree(cfqd);
}
@@ -4194,13 +4195,13 @@ static int __init cfq_init(void)
#ifdef CONFIG_CFQ_GROUP_IOSCHED
if (!cfq_group_idle)
cfq_group_idle = 1;
-#else
- cfq_group_idle = 0;
-#endif
ret = blkcg_policy_register(&blkcg_policy_cfq);
if (ret)
return ret;
+#else
+ cfq_group_idle = 0;
+#endif
ret = -ENOMEM;
cfq_pool = KMEM_CACHE(cfq_queue, 0);
@@ -4216,13 +4217,17 @@ static int __init cfq_init(void)
err_free_pool:
kmem_cache_destroy(cfq_pool);
err_pol_unreg:
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
blkcg_policy_unregister(&blkcg_policy_cfq);
+#endif
return ret;
}
static void __exit cfq_exit(void)
{
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
blkcg_policy_unregister(&blkcg_policy_cfq);
+#endif
elv_unregister(&iosched_cfq);
kmem_cache_destroy(cfq_pool);
}
--
1.7.7.3
Hello,
On Thu, 28 Jun 2012 08:57:54 -0700
Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Thu, Jun 28, 2012 at 09:51:03AM +0200, Paul Rolland wrote:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
> > > for-linus
> >
> > Well code is the same.... so is the result :
>
> The code is not the same. block/for-linus has several fixes which
> haven't been pushed to mainline yet including the following commit.
Then something is wrong, because the following commit doesn't seem to be
applied in the working copy I have :(
For example, line 418, the code is :
static inline struct cfq_group *blkg_to_cfqg(struct blkcg_gq *blkg)
{
return pd_to_cfqg(blkg_to_pd(blkg, &blkcg_policy_cfq));
}
which shouldn't be possible had the following commit been applied :(
What I did :
mkdir tejun
cd tejun
git init
git clone
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-linus
and then I chdir'ed into for-linus and rebuild a kernel locally...
Should I apply manually the patch below and retest ?
Paul
> Thanks.
>
> >From ffea73fc723a12fdde4c9fb3fcce5d154d1104a1 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <[email protected]>
> Date: Mon, 4 Jun 2012 10:02:29 +0200
> Subject: [PATCH] block: blkcg_policy_cfq shouldn't be used if
> !CONFIG_CFQ_GROUP_IOSCHED
>
> cfq may be built w/ or w/o blkcg support depending on
> CONFIG_CFQ_CGROUP_IOSCHED. If blkcg support is disabled, most of
> related code is ifdef'd out but some part is left dangling -
> blkcg_policy_cfq is left zero-filled and blkcg_policy_[un]register()
> calls are made on it.
>
> Feeding zero filled policy to blkcg_policy_register() is incorrect and
> triggers the following WARN_ON() if CONFIG_BLK_CGROUP &&
> !CONFIG_CFQ_GROUP_IOSCHED.
>
> ------------[ cut here ]------------
> WARNING: at block/blk-cgroup.c:867
> Modules linked in:
> Modules linked in:
> CPU: 3 Not tainted 3.4.0-09547-gfb21aff #1
> Process swapper/0 (pid: 1, task: 000000003ff80000, ksp: 000000003ff7f8b8)
> Krnl PSW : 0704100180000000 00000000003d76ca
> (blkcg_policy_register+0xca/0xe0) R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0
> AS:0 CC:1 PM:0 EA:3 Krnl GPRS: 0000000000000000 00000000014b85ec
> 00000000014b85b0 0000000000000000 000000000096fb60 0000000000000000
> 00000000009a8e78 0000000000000048 000000000099c070 0000000000b6f000
> 0000000000000000 000000000099c0b8 00000000014b85b0 0000000000667580
> 000000003ff7fd98 000000003ff7fd70 Krnl Code: 00000000003d76be:
> a7280001 lhi %r2,1 00000000003d76c2: a7f4ffdf
> brc 15,3d7680 #00000000003d76c6: a7f40001 brc 15,3d76c8
> >00000000003d76ca: a7c8ffea lhi %r12,-22
> 00000000003d76ce: a7f4ffce brc 15,3d766a
> 00000000003d76d2: a7f40001 brc 15,3d76d4
> 00000000003d76d6: a7c80000 lhi %r12,0
> 00000000003d76da: a7f4ffc2 brc 15,3d765e
> Call Trace:
> ([<0000000000b6f000>] initcall_debug+0x0/0x4)
> [<0000000000989e8a>] cfq_init+0x62/0xd4
> [<00000000001000ba>] do_one_initcall+0x3a/0x170
> [<000000000096fb60>] kernel_init+0x214/0x2bc
> [<0000000000623202>] kernel_thread_starter+0x6/0xc
> [<00000000006231fc>] kernel_thread_starter+0x0/0xc
> no locks held by swapper/0/1.
> Last Breaking-Event-Address:
> [<00000000003d76c6>] blkcg_policy_register+0xc6/0xe0
> ---[ end trace b8ef4903fcbf9dd3 ]---
>
> This patch fixes the problem by ensuring all blkcg support code is
> inside CONFIG_CFQ_GROUP_IOSCHED.
>
> * blkcg_policy_cfq declaration and blkg_to_cfqg() definition are moved
> inside the first CONFIG_CFQ_GROUP_IOSCHED block. __maybe_unused is
> dropped from blkcg_policy_cfq decl.
>
> * blkcg_deactivate_poilcy() invocation is moved inside ifdef. This
> also makes the activation logic match cfq_init_queue().
>
> * All blkcg_policy_[un]register() invocations are moved inside ifdef.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Reported-by: Heiko Carstens <[email protected]>
> LKML-Reference: <[email protected]>
> Signed-off-by: Jens Axboe <[email protected]>
> ---
> block/cfq-iosched.c | 29 +++++++++++++++++------------
> 1 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index ae5113d..fb52df9 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -17,8 +17,6 @@
> #include "blk.h"
> #include "blk-cgroup.h"
>
> -static struct blkcg_policy blkcg_policy_cfq __maybe_unused;
> -
> /*
> * tunables
> */
> @@ -418,11 +416,6 @@ static inline struct cfq_group *pd_to_cfqg(struct
> blkg_policy_data *pd) return pd ? container_of(pd, struct cfq_group,
> pd) : NULL; }
>
> -static inline struct cfq_group *blkg_to_cfqg(struct blkcg_gq *blkg)
> -{
> - return pd_to_cfqg(blkg_to_pd(blkg, &blkcg_policy_cfq));
> -}
> -
> static inline struct blkcg_gq *cfqg_to_blkg(struct cfq_group *cfqg)
> {
> return pd_to_blkg(&cfqg->pd);
> @@ -572,6 +565,13 @@ static inline void
> cfqg_stats_update_avg_queue_size(struct cfq_group *cfqg) { }
> #ifdef CONFIG_CFQ_GROUP_IOSCHED
>
> +static struct blkcg_policy blkcg_policy_cfq;
> +
> +static inline struct cfq_group *blkg_to_cfqg(struct blkcg_gq *blkg)
> +{
> + return pd_to_cfqg(blkg_to_pd(blkg, &blkcg_policy_cfq));
> +}
> +
> static inline void cfqg_get(struct cfq_group *cfqg)
> {
> return blkg_get(cfqg_to_blkg(cfqg));
> @@ -3951,10 +3951,11 @@ static void cfq_exit_queue(struct elevator_queue
> *e)
> cfq_shutdown_timer_wq(cfqd);
>
> -#ifndef CONFIG_CFQ_GROUP_IOSCHED
> +#ifdef CONFIG_CFQ_GROUP_IOSCHED
> + blkcg_deactivate_policy(q, &blkcg_policy_cfq);
> +#else
> kfree(cfqd->root_group);
> #endif
> - blkcg_deactivate_policy(q, &blkcg_policy_cfq);
> kfree(cfqd);
> }
>
> @@ -4194,13 +4195,13 @@ static int __init cfq_init(void)
> #ifdef CONFIG_CFQ_GROUP_IOSCHED
> if (!cfq_group_idle)
> cfq_group_idle = 1;
> -#else
> - cfq_group_idle = 0;
> -#endif
>
> ret = blkcg_policy_register(&blkcg_policy_cfq);
> if (ret)
> return ret;
> +#else
> + cfq_group_idle = 0;
> +#endif
>
> ret = -ENOMEM;
> cfq_pool = KMEM_CACHE(cfq_queue, 0);
> @@ -4216,13 +4217,17 @@ static int __init cfq_init(void)
> err_free_pool:
> kmem_cache_destroy(cfq_pool);
> err_pol_unreg:
> +#ifdef CONFIG_CFQ_GROUP_IOSCHED
> blkcg_policy_unregister(&blkcg_policy_cfq);
> +#endif
> return ret;
> }
>
> static void __exit cfq_exit(void)
> {
> +#ifdef CONFIG_CFQ_GROUP_IOSCHED
> blkcg_policy_unregister(&blkcg_policy_cfq);
> +#endif
> elv_unregister(&iosched_cfq);
> kmem_cache_destroy(cfq_pool);
> }
--
Paul Rolland E-Mail : rol(at)witbe.net
CTO - Witbe.net SA Tel. +33 (0)1 47 67 77 77
Les Collines de l'Arche Fax. +33 (0)1 47 67 77 99
F-92057 Paris La Defense RIPE : PR12-RIPE
Please no HTML, I'm not a browser - Pas d'HTML, je ne suis pas un
navigateur "Some people dream of success... while others wake up and work
hard at it"
"I worry about my child and the Internet all the time, even though she's
too young to have logged on yet. Here's what I worry about. I worry that 10
or 15 years from now, she will come to me and say 'Daddy, where were you
when they took freedom of the press away from the Internet?'"
--Mike Godwin, Electronic Frontier Foundation
On Thu, Jun 28, 2012 at 06:04:43PM +0200, Paul Rolland wrote:
> What I did :
> mkdir tejun
> cd tejun
> git init
> git clone
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
> for-linus
You cloned block tree into for-linus directory and then implicitly
used the master branch of the block tree, which is mainline. What
should have been done are...
* git clone git://git.kernel....block.git block
* cd block
* git checkout for-linus
Thanks.
--
tejun
Hello,
On Thu, 28 Jun 2012 09:08:55 -0700
Tejun Heo <[email protected]> wrote:
> You cloned block tree into for-linus directory and then implicitly
> used the master branch of the block tree, which is mainline. What
> should have been done are...
>
> * git clone git://git.kernel....block.git block
> * cd block
> * git checkout for-linus
OK, just boot that version and it's fixing the issue (BTW, your patch is
nicer than mine ;)
Thanks,
Paul