[patch 3/4] fastboot: make the raid autodetect code wait for all devices to init

(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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/