[PATCH 7/7] initdev:kernel:Await block device discovery

[PATCH 7/7] initdev:kernel:Await block device discovery

[David VomLehn]

Use the initdev infrastructure to wait for a root device to become available.
This should make most uses of the kernel rootwait parameter unnecessary. The
only time it should be necessary is when the root device might not be attached
at boot time.

Signed-off-by: David VomLehn <dvomlehn@cisco>
---
 init/do_mounts.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index dd7ee5f..36a92ae 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -358,6 +358,18 @@ void __init mount_root(void)
 #endif
 }
 
+/**
+ * root_present - determine whether the root device is available yet
+ *
+ * Returns true if the root device is available, false if not. The check to
+ * see if the root device is available is done by check to see whether it
+ * has been assigned a major/minor device number.
+ */
+static bool root_present(void)
+{
+	return name_to_dev_t(saved_root_name) != 0;
+}
+
 /*
  * Prepare the namespace - decide what/where to mount, load ramdisks, etc.
  */
@@ -398,12 +410,21 @@ void __init prepare_namespace(void)
 		goto out;
 
 	/* wait for any asynchronous scanning to complete */
-	if ((ROOT_DEV == 0) && root_wait) {
+	if (ROOT_DEV == 0) {
 		printk(KERN_INFO "Waiting for root device %s...\n",
 			saved_root_name);
-		while (driver_probe_done() != 0 ||
-			(ROOT_DEV = name_to_dev_t(saved_root_name)) == 0)
-			msleep(100);
+		if (root_wait) {
+			while (driver_probe_done() != 0 ||
+				(ROOT_DEV = name_to_dev_t(saved_root_name)) ==
+					0)
+				msleep(100);
+		}
+
+		else {
+			initdev_wait(INITDEV_BLOCK_TYPE, root_present);
+			ROOT_DEV = name_to_dev_t(saved_root_name);
+		}
+
 		async_synchronize_full();
 	}
 

Re: [PATCH 7/7] initdev:kernel:Await block device discovery

[Stefan Richter]

David VomLehn wrote:
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -358,6 +358,18 @@ void __init mount_root(void)
>  #endif
>  }
>  
> +/**
> + * root_present - determine whether the root device is available yet
> + *
> + * Returns true if the root device is available, false if not. The check to
> + * see if the root device is available is done by check to see whether it
> + * has been assigned a major/minor device number.
> + */
> +static bool root_present(void)
> +{
> +	return name_to_dev_t(saved_root_name) != 0;
> +}
> +
>  /*
>   * Prepare the namespace - decide what/where to mount, load ramdisks, etc.
>   */
> @@ -398,12 +410,21 @@ void __init prepare_namespace(void)
>  		goto out;
>  
>  	/* wait for any asynchronous scanning to complete */
> -	if ((ROOT_DEV == 0) && root_wait) {
> +	if (ROOT_DEV == 0) {
>  		printk(KERN_INFO "Waiting for root device %s...\n",
>  			saved_root_name);
> -		while (driver_probe_done() != 0 ||
> -			(ROOT_DEV = name_to_dev_t(saved_root_name)) == 0)
> -			msleep(100);
> +		if (root_wait) {
> +			while (driver_probe_done() != 0 ||
> +				(ROOT_DEV = name_to_dev_t(saved_root_name)) ==
> +					0)
> +				msleep(100);
> +		}
> +
> +		else {
> +			initdev_wait(INITDEV_BLOCK_TYPE, root_present);
> +			ROOT_DEV = name_to_dev_t(saved_root_name);
> +		}
> +
>  		async_synchronize_full();
>  	}
>  

This together with 1/7, 5/7, 6/7 looks seriously flawed, if not broken.

1.)  It depends on the rootdelay so big that it is guaranteed that one 
or more initdev_found() have been called before prepare_namespace()'s 
ssleep(root_delay) is over.  This is because initdev_wait() does not 
wait at all if called when the number of pending initdevs is 0.

Hence the whole thing, as currently implemented, is quite useless:  The 
user/admin has to guess what a safe rootdelay value is, and then the 
kernel will always be delayed for >= rootdelay.

2.)  The fact that initdev_wait() does not wait at all when there are no 
pending initdevs causes another failure.  If there are sequences of the 
following type:

	initdev_found(INITDEV_BLOCK_TYPE);
	initdev_probe_done(INITDEV_BLOCK_TYPE); /* found other dev */
	initdev_found(INITDEV_BLOCK_TYPE);
	initdev_probe_done(INITDEV_BLOCK_TYPE); /* found root dev */

then prepare_namespace() will fall through without ROOT_DEV found and 
booting will fail, won't it?

I suspect that there are similar problems with the console and 
networking parts of the patch series.
-- 
Stefan Richter
-=====-==--= -==- --==-
http://arcgraph.de/sr/

Re: [PATCH 7/7] initdev:kernel:Await block device discovery

[David VomLehn]

On Sat, Jun 06, 2009 at 06:04:38AM -0400, Stefan Richter wrote:
> David VomLehn wrote:
> > --- a/init/do_mounts.c
> > +++ b/init/do_mounts.c
> > @@ -358,6 +358,18 @@ void __init mount_root(void)
> >  #endif
> >  }
> >  
> > +/**
> > + * root_present - determine whether the root device is available yet
> > + *
> > + * Returns true if the root device is available, false if not. The check to
> > + * see if the root device is available is done by check to see whether it
> > + * has been assigned a major/minor device number.
> > + */
> > +static bool root_present(void)
> > +{
> > +	return name_to_dev_t(saved_root_name) != 0;
> > +}
> > +
> >  /*
> >   * Prepare the namespace - decide what/where to mount, load ramdisks, etc.
> >   */
> > @@ -398,12 +410,21 @@ void __init prepare_namespace(void)
> >  		goto out;
> >  
> >  	/* wait for any asynchronous scanning to complete */
> > -	if ((ROOT_DEV == 0) && root_wait) {
> > +	if (ROOT_DEV == 0) {
> >  		printk(KERN_INFO "Waiting for root device %s...\n",
> >  			saved_root_name);
> > -		while (driver_probe_done() != 0 ||
> > -			(ROOT_DEV = name_to_dev_t(saved_root_name)) == 0)
> > -			msleep(100);
> > +		if (root_wait) {
> > +			while (driver_probe_done() != 0 ||
> > +				(ROOT_DEV = name_to_dev_t(saved_root_name)) ==
> > +					0)
> > +				msleep(100);
> > +		}
> > +
> > +		else {
> > +			initdev_wait(INITDEV_BLOCK_TYPE, root_present);
> > +			ROOT_DEV = name_to_dev_t(saved_root_name);
> > +		}
> > +
> >  		async_synchronize_full();
> >  	}
> >  
> 
> This together with 1/7, 5/7, 6/7 looks seriously flawed, if not broken.
> 
> 1.)  It depends on the rootdelay so big that it is guaranteed that one 
> or more initdev_found() have been called before prepare_namespace()'s 
> ssleep(root_delay) is over.  This is because initdev_wait() does not 
> wait at all if called when the number of pending initdevs is 0.
> 
> Hence the whole thing, as currently implemented, is quite useless:  The 
> user/admin has to guess what a safe rootdelay value is, and then the 
> kernel will always be delayed for >= rootdelay.

I think you misunderstood.  Buses are enumerated before this code is
reached and thus initdev_found() has already been called for all attached
block devices. If there are devices found, but not probed, the number
of pending BLOCK device initdevs will be greater than zero.

Also, this code doesn't depend on rootdelay at all. In fact, avoiding
unnecessary delays both in finding devices and in finding out that you don't
have a device is the whole reason for this patch. I really can't stand
things like rootdelay so long as the hardware supports proper device
discovery.

> 2.)  The fact that initdev_wait() does not wait at all when there are no 
> pending initdevs causes another failure.  If there are sequences of the 
> following type:
> 
> 	initdev_found(INITDEV_BLOCK_TYPE);
> 	initdev_probe_done(INITDEV_BLOCK_TYPE); /* found other dev */
> 	initdev_found(INITDEV_BLOCK_TYPE);
> 	initdev_probe_done(INITDEV_BLOCK_TYPE); /* found root dev */
> 
> then prepare_namespace() will fall through without ROOT_DEV found and 
> booting will fail, won't it?

If this were to happen, then you would be correct, but since initdev_found()
is called for all block devices before reaching this code, you will only
continue once initdev_probe_done() has been called for all of those devices.

> I suspect that there are similar problems with the console and 
> networking parts of the patch series.

I'm less confident in the block code than the console and networking parts, but
don't think there is a problem with any of these.  It's also worth mentioning
that this is not a theoretical exercise--without this patch, I have no USB
console, network device, or root filesystem. With the patch, I have all three.
That is no guarantee of correctness, but it implies that it's on the right
path.

I would appreciate it if you could take a closer look at the locations where
initdev_found() and initdev_probe_done() are called and see if you still
think there is a problem.

> Stefan Richter

David VomLehn

Re: [PATCH 7/7] initdev:kernel:Await block device discovery

[Stefan Richter]

David VomLehn wrote:
> On Sat, Jun 06, 2009 at 06:04:38AM -0400, Stefan Richter wrote:
>> 1.)  It depends on the rootdelay so big that it is guaranteed that one 
>> or more initdev_found() have been called before prepare_namespace()'s 
>> ssleep(root_delay) is over.
...
> I think you misunderstood.  Buses are enumerated before this code is
> reached and thus initdev_found() has already been called for all attached
> block devices.
...
> I would appreciate it if you could take a closer look at the locations where
> initdev_found() and initdev_probe_done() are called and see if you still
> think there is a problem.

I obviously need to do that.

However, one issue remains:  If somebody wants to use your API for 
another bus, he needs to know at which point he can successfully use it, 
and after what point (or context) it cannot, even must not, be used 
anymore.  This isn't documented --- or is it?

(By 'buses are enumerated before...' you surely mean only the buses for 
which you implemented this API usage already.)
-- 
Stefan Richter
-=====-==--= -==- -=--=
http://arcgraph.de/sr/

Re: [PATCH 7/7] initdev:kernel:Await block device discovery

[David VomLehn]

On Tue, Jun 09, 2009 at 12:55:01AM +0200, Stefan Richter wrote:
> David VomLehn wrote:
>> On Sat, Jun 06, 2009 at 06:04:38AM -0400, Stefan Richter wrote:
>>> 1.)  It depends on the rootdelay so big that it is guaranteed that 
>>> one or more initdev_found() have been called before 
>>> prepare_namespace()'s ssleep(root_delay) is over.
> ...
>> I think you misunderstood.  Buses are enumerated before this code is
>> reached and thus initdev_found() has already been called for all attached
>> block devices.
> ...
>> I would appreciate it if you could take a closer look at the locations where
>> initdev_found() and initdev_probe_done() are called and see if you still
>> think there is a problem.
>
> I obviously need to do that.
>
> However, one issue remains:  If somebody wants to use your API for  
> another bus, he needs to know at which point he can successfully use it,  
> and after what point (or context) it cannot, even must not, be used  
> anymore.  This isn't documented --- or is it?

Hmm, I tried to make this clear in Documentation/driver-model/initdev.txt,
but I don't claim to be much of a writer and am open to suggestions.

> (By 'buses are enumerated before...' you surely mean only the buses for  
> which you implemented this API usage already.)

Yes, definitely. In an ideal world, every bus which might have a console,
network, or block device would be using this API. USB is a big problem for
me, so that's the itch I scratched. Alan Stern also gave a rub to SCSI.
If people are using devices on, say, Firewire, they will have whatever
behavior they had before the introduction of the API.

> Stefan Richter

David VomLehn