From: Arjan van de Ven <[email protected]>
Date: Sun, 20 Jul 2008 13:07:09 -0700
Subject: [PATCH] fastboot: make the raid autodetect code wait for all devices to init
The raid autodetect code really needs to have all devices probed before
it can detect raid arrays; not doing so would give rather messy situations
where arrays would get detected as degraded while they shouldn't be etc.
This is in preparation of removing the "wait for everything to init"
code that makes everyone pay, not just raid users.
Signed-off-by: Arjan van de Ven <[email protected]>
---
init/do_mounts_md.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/init/do_mounts_md.c b/init/do_mounts_md.c
index 693d246..c0412a9 100644
--- a/init/do_mounts_md.c
+++ b/init/do_mounts_md.c
@@ -267,9 +267,16 @@ __setup("md=", md_setup);
void __init md_run_setup(void)
{
create_dev("/dev/md0", MKDEV(MD_MAJOR, 0));
+
if (raid_noautodetect)
printk(KERN_INFO "md: Skipping autodetection of RAID arrays. (raid=noautodetect)\n");
else {
+ /*
+ * Since we don't want to detect and use half a raid array, we
+ * need to wait for the known devices to complete their probing
+ */
+ while (driver_probe_done() != 0)
+ msleep(100);
int fd = sys_open("/dev/md0", 0, 0);
if (fd >= 0) {
sys_ioctl(fd, RAID_AUTORUN, raid_autopart);
--
1.5.5.1
(wondering why I wasn't Cc:ed on this...)
On Sunday July 20, [email protected] wrote:
>
> From: Arjan van de Ven <[email protected]>
> Date: Sun, 20 Jul 2008 13:07:09 -0700
> Subject: [PATCH] fastboot: make the raid autodetect code wait for all devices to init
>
> The raid autodetect code really needs to have all devices probed before
> it can detect raid arrays; not doing so would give rather messy situations
> where arrays would get detected as degraded while they shouldn't be etc.
>
> This is in preparation of removing the "wait for everything to init"
> code that makes everyone pay, not just raid users.
>
> Signed-off-by: Arjan van de Ven <[email protected]>
> ---
> init/do_mounts_md.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/init/do_mounts_md.c b/init/do_mounts_md.c
> index 693d246..c0412a9 100644
> --- a/init/do_mounts_md.c
> +++ b/init/do_mounts_md.c
> @@ -267,9 +267,16 @@ __setup("md=", md_setup);
> void __init md_run_setup(void)
> {
> create_dev("/dev/md0", MKDEV(MD_MAJOR, 0));
> +
> if (raid_noautodetect)
> printk(KERN_INFO "md: Skipping autodetection of RAID arrays. (raid=noautodetect)\n");
> else {
> + /*
> + * Since we don't want to detect and use half a raid array, we
> + * need to wait for the known devices to complete their probing
> + */
> + while (driver_probe_done() != 0)
> + msleep(100);
> int fd = sys_open("/dev/md0", 0, 0);
> if (fd >= 0) {
> sys_ioctl(fd, RAID_AUTORUN, raid_autopart);
I must say that I think this is pretty horrible. But then it is a
pretty horrible problem and I don't think there is a clean solution.
If md in a module, this code won't run so there will be no change. If
md is compiled in, this code will silently slow down boot even if
there are no raid arrays to assemble. I think the "silently" is a
problem. I'm not looking forward to "my computer boots slower if I
compile md into the kernel" reports on linux-raid@vger.
What would you think of
if (driver_probe_done() != 0) {
printk("md: Waiting for all devices to be available before autodetect\n"
"md: If you don't boot off raid, use raid=noautodetect\n");
do
msleep(100);
while (driver_probe_done() != 0);
}
??
Also, the "driver_probe_done() != 0" bothers me.
If driver_probe_done is a boolean, it should be
while (! drive_probe_done() ) msleep ....
and if it returns a negative error, then it should be
while ( drive_probe_done() < 0) msleep ....
The != 0 confuses me about the expected return type.
The "real" solution here involves assembling arrays in userspace using
"mdadm --incremental" from udevd, and using write-intent-bitmaps so
that writing to an array before all the component devices are
available can be done without requiring a full resync. There is still
a bit more code needed to make that work really smoothly.
NeilBrown
On Mon, 21 Jul 2008, Neil Brown wrote:
> I must say that I think this is pretty horrible. But then it is a
> pretty horrible problem and I don't think there is a clean solution.
>
> If md in a module, this code won't run so there will be no change. If
> md is compiled in, this code will silently slow down boot even if
> there are no raid arrays to assemble. I think the "silently" is a
> problem. I'm not looking forward to "my computer boots slower if I
> compile md into the kernel" reports on linux-raid@vger.
>
> What would you think of
>
> if (driver_probe_done() != 0) {
> printk("md: Waiting for all devices to be available before autodetect\n"
> "md: If you don't boot off raid, use raid=noautodetect\n");
> do
> msleep(100);
> while (driver_probe_done() != 0);
> }
>
> ??
even if you don't boot off of raid, if the boot process will mount a raid
that's autodetected this will be a problem. so I think the message needs
to allow for this.
David Lang
On Mon, 21 Jul 2008 09:06:36 +1000
Neil Brown <[email protected]> wrote:
> > + /*
> > + * Since we don't want to detect and use half a
> > raid array, we
> > + * need to wait for the known devices to complete
> > their probing
> > + */
> > + while (driver_probe_done() != 0)
> > + msleep(100);
> > int fd = sys_open("/dev/md0", 0, 0);
> > if (fd >= 0) {
> > sys_ioctl(fd, RAID_AUTORUN, raid_autopart);
>
> I must say that I think this is pretty horrible. But then it is a
> pretty horrible problem and I don't think there is a clean solution.
it's also the existing solution, just moved around to the only place
that needs it.
>
> If md in a module, this code won't run so there will be no change. If
> md is compiled in, this code will silently slow down boot even if
> there are no raid arrays to assemble.
it's not slower than it is is today for *everyone*. All that this does
is move it to the MD code rather than having it 1 line before calling
the MD code (the removal of the global wait is in the next patch)
Waiting twice is the same cost as waiting once; the moment everything
is done it'll stay done.
> I think the "silently" is a
> problem. I'm not looking forward to "my computer boots slower if I
> compile md into the kernel" reports on linux-raid@vger.
I *am* looking forward to "my computer boots faster" reports though ;-)
>
> What would you think of
>
> if (driver_probe_done() != 0) {
> printk("md: Waiting for all devices to be available before
> autodetect\n" "md: If you don't boot off raid, use
> raid=noautodetect\n"); do
> msleep(100);
> while (driver_probe_done() != 0);
> }
not sure we need the outer if for this (it'll basically always hit, and
even if by some miracle, everything is done, it's no big deal, you
tried to wait and were done immediately).
Adding a printk like this makes total sense to me; I'll add a patch for
that.
Maybe it's worth introducing a config option for the raid autodetect
thing; but I'll leave that up to you.
> Also, the "driver_probe_done() != 0" bothers me.
it's the existing form just moved; I wanted to make the change to be as
simple as possible.
>
> The "real" solution here involves assembling arrays in userspace using
> "mdadm --incremental" from udevd, and using write-intent-bitmaps so
> that writing to an array before all the component devices are
> available can be done without requiring a full resync. There is still
> a bit more code needed to make that work really smoothly.
if you use an initrd, the code above doesn't run...
totally different method of booting.
--
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Mon, 21 Jul 2008 09:06:36 +1000
Neil Brown <[email protected]> wrote:
> if (driver_probe_done() != 0) {
> printk("md: Waiting for all devices to be available before
> autodetect\n" "md: If you don't boot off raid, use
> raid=noautodetect\n"); do
how about this patch?
From: Arjan van de Ven <[email protected]>
Date: Sun, 20 Jul 2008 16:30:29 -0700
Subject: [PATCH] fastboot: make the RAID autostart code print a message just before waiting
As requested/suggested by Neil Brown: make the raid code print that it's
about to wait for probing to be done as well as give a suggestion on how
to disable the probing if the user doesn't use raid.
Signed-off-by: Arjan van de Ven <[email protected]
---
init/do_mounts_md.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/init/do_mounts_md.c b/init/do_mounts_md.c
index c0412a9..2ab122a 100644
--- a/init/do_mounts_md.c
+++ b/init/do_mounts_md.c
@@ -275,7 +275,9 @@ void __init md_run_setup(void)
* Since we don't want to detect and use half a raid array, we need to
* wait for the known devices to complete their probing
*/
- while (driver_probe_done() != 0)
+ printk(KERN_INFO "md: Waiting for all devices to be available before autodetect\n");
+ printk(KERN_INFO "md: If you don't use raid, use raid=noautodetect\n");
+ while (driver_probe_done())
msleep(100);
int fd = sys_open("/dev/md0", 0, 0);
if (fd >= 0) {
--
1.5.5.1
On Sunday July 20, [email protected] wrote:
> On Mon, 21 Jul 2008 09:06:36 +1000
> Neil Brown <[email protected]> wrote:
>
> > if (driver_probe_done() != 0) {
> > printk("md: Waiting for all devices to be available before
> > autodetect\n" "md: If you don't boot off raid, use
> > raid=noautodetect\n"); do
>
>
> how about this patch?
Well,
> - while (driver_probe_done() != 0)
> + printk(KERN_INFO "md: Waiting for all devices to be available before autodetect\n");
> + printk(KERN_INFO "md: If you don't use raid, use raid=noautodetect\n");
that's better thanks. I'm quite happy with that.
> + while (driver_probe_done())
> msleep(100);
That's worse. Now it really looks like a boolean that is being used
wrongly. It seems that driver_probe_done either returns 0 or -EBUSY,
so I'd prefer
> + while (driver_probe_done() < 0)
> msleep(100);
or even
> + while (driver_probe_done() == -EBUSY)
> msleep(100);
though that is less robust.
Thanks,
NeilBrown
On Mon, 21 Jul 2008 14:30:12 +1000
Neil Brown <[email protected]> wrote:
>
> > + while (driver_probe_done())
> > msleep(100);
>
> That's worse. Now it really looks like a boolean that is being used
> wrongly. It seems that driver_probe_done either returns 0 or -EBUSY,
> so I'd prefer
>
> > + while (driver_probe_done() < 0)
> > msleep(100);
>
> or even
ok I've done this one.
I'll be the first to say that this driver_probe_done() isn't very
elegant.. but it's out of my current scope to rewrite that whole
thing ... I'll leave that to GregKH.
--
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings,
visit http://www.lesswatts.org