From: Neil Brown <neilb@suse.de>
To: Arjan van de Ven <arjan@infradead.org>
Date: Mon, 21 Jul 2008 09:06:36 +1000
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-ID: <18563.50428.659101.868745@notabene.brown>
Cc: linux-kernel@vger.kernel.org, mingo@elte.hu,
       Simon Arlott <simon@fire.lp0.eu>, Daniel Walker <dwalker@mvista.com>,
       Rene Herman <rene.herman@keyaccess.nl>
Subject: Re: [patch 3/4] fastboot: make the raid autodetect code wait for all
 devices to init
In-Reply-To: message from Arjan van de Ven on Sunday July 20
References: <20080720151140.4aa7c682@infradead.org>
	<20080720151339.6f4a83d8@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3173
Lines: 87


(wondering why I wasn't Cc:ed on this...)

On Sunday July 20, arjan@infradead.org wrote:
> 
> From: Arjan van de Ven <arjan@linux.intel.com>
> 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 <arjan@linux.intel.com>
> ---
>  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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/