Re: [patch 1/1] md: Software Raid autodetect dev list not array

Re: [patch 1/1] md: Software Raid autodetect dev list not array

[Neil Brown]

On Wednesday August 22, mjevans1983@comcast.net wrote:
> From: Michael J. Evans <mjevans1983@gmail.com>
> 
> In current release kernels the md module (Software RAID) uses a static array
>  (dev_t[128]) to store partition/device info temporarily for autostart.
> 
> This patch replaces that static array with a list.

I must admit that I'm not very keen on this.
I would much rather that in-kernel autodetect were deprecated rather
than enhanced.

Just use 'mdadm' in an initrd, or during normal boot, to assemble all
your arrays.

However.....

> =============================================================
> --- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
> +++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
> @@ -24,4 +24,6 @@
> 
> +   - autodetect dev list not array: Michael J. Evans <mjevans1983@gmail.com>
> +
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>     the Free Software Foundation; either version 2, or (at your option)
> @@ -5752,13 +5754,25 @@ void md_autodetect_dev(dev_t dev)
>   * Searches all registered partitions for autorun RAID arrays
>   * at boot time.
>   */
> -static dev_t detected_devices[128];
> -static int dev_cnt;
> +
> +static LIST_HEAD(all_detected_devices);
> +/* FIXME : Should these 4 lines instead go in to include/linux/raid/md_k.h ? 

No.  No-one outside this file uses them, so they are fine where they
are.

> */
> +struct detected_devices_node {
> +	struct list_head list;
> +	dev_t dev;
> +};
>  
>  void md_autodetect_dev(dev_t dev)
>  {
> -	if (dev_cnt >= 0 && dev_cnt < 127)
> -		detected_devices[dev_cnt++] = dev;
> +	struct detected_devices_node *node_detected_dev;
> +	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
> +	if (node_detected_dev) {
> +		node_detected_dev->dev = dev;
> +		list_add(&node_detected_dev->list, &all_detected_devices);

Probably list_add_tail would be better so the ordering is the same as
it was before?


> +	} else {
> +		printk(KERN_CRIT "md: kzAlloc node failed, skipping device."
> +				 " : 0x%p.\n", node_detected_dev);
> +	}



>  }
>  
>  
> @@ -5765,7 +5779,13 @@ static void autostart_arrays(int part)
>  static void autostart_arrays(int part)
>  {
>  	mdk_rdev_t *rdev;
> -	int i;
> +	struct detected_devices_node *node_detected_dev;
> +	dev_t dev;
> +	int i_scanned, i_passed, i_loops;
> +	signed int i_found;
> +	i_scanned = 0;
> +	i_passed = 0;
> +	i_loops = 0;

i_passed is never used.

And what is the point of i_loops (and i_scanned)?  The comments
at the top of the patch should explain this if there is a good reason.

>  
>  	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
>  
> @@ -5772,3 +5792,11 @@ static void autostart_arrays(int part)
> -	for (i = 0; i < dev_cnt; i++) {
> -		dev_t dev = detected_devices[i];
> -
> +		/* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */
> +	while (!list_empty(&all_detected_devices) && i_loops < 0x7FFFFFFF) {
> +		i_scanned++;
> +		node_detected_dev = NULL;
> +		node_detected_dev = list_entry(all_detected_devices.next,
> +					struct detected_devices_node, list);
> +		if (node_detected_dev) {

list_entry will *never* return NULL.   It simply doesn't pointer
arithmetic.  (Well, I guess it could return NULL if it was passed
NULL, and the struct-offset were 0, but that isn't the case here).

So you don't need this 'if' at all.

> +			list_del(&node_detected_dev->list);
> +			dev = node_detected_dev->dev;
> +			kfree(node_detected_dev);
> +		/* Indent is now off by one, but the old code is after this. */

so you don't need to worrying about indents.


> @@ -5781,8 +5809,16 @@ static void autostart_arrays(int part)
>  			continue;
>  		}
>  		list_add(&rdev->same_set, &pending_raid_disks);
> +		/* Indent is now off by one, but the old code is above this. */
> +
> +		} else {
> +			printk(KERN_CRIT "md: Invalid list node, skipping.\n");
> +		}
> +		i_loops++;
>  	}
> -	dev_cnt = 0;
> +
> +	printk(KERN_INFO "md: Passes %d : Scanned %d and added %d devices.\n",
> +						i_loops, i_scanned, i_passed);
>  
>  	autorun_devices(part);
>  }


I'm not dead-set against the change, and if you tidy it up I'll
probably accept it, but I really think you would be better off skipping
the in-kernel autodetect stuff altogether and use mdadm to assemble
your arrays.

NeilBrown

Re: [patch 1/1] md: Software Raid autodetect dev list not array

[Michael Evans]

I'll look at this again on my next weekend and make the changes.

If it exists I'd rather it functioned without issues.  My initrds are
created by gentoo's genkernel script, which places dmraid on them.
I'm not sure if it supports autodetect or not.

On 8/24/07, Neil Brown <neilb@suse.de> wrote:
> On Wednesday August 22, mjevans1983@comcast.net wrote:
> > From: Michael J. Evans <mjevans1983@gmail.com>
> >
> > In current release kernels the md module (Software RAID) uses a static array
> >  (dev_t[128]) to store partition/device info temporarily for autostart.
> >
> > This patch replaces that static array with a list.
>
> I must admit that I'm not very keen on this.
> I would much rather that in-kernel autodetect were deprecated rather
> than enhanced.
>
> Just use 'mdadm' in an initrd, or during normal boot, to assemble all
> your arrays.
>
> However.....
>
> > =============================================================
> > --- linux/drivers/md/md.c.orig        2007-08-21 03:19:42.511576248 -0700
> > +++ linux/drivers/md/md.c     2007-08-21 04:30:09.775525710 -0700
> > @@ -24,4 +24,6 @@
> >
> > +   - autodetect dev list not array: Michael J. Evans <mjevans1983@gmail.com>
> > +
> >     This program is free software; you can redistribute it and/or modify
> >     it under the terms of the GNU General Public License as published by
> >     the Free Software Foundation; either version 2, or (at your option)
> > @@ -5752,13 +5754,25 @@ void md_autodetect_dev(dev_t dev)
> >   * Searches all registered partitions for autorun RAID arrays
> >   * at boot time.
> >   */
> > -static dev_t detected_devices[128];
> > -static int dev_cnt;
> > +
> > +static LIST_HEAD(all_detected_devices);
> > +/* FIXME : Should these 4 lines instead go in to include/linux/raid/md_k.h ?
>
> No.  No-one outside this file uses them, so they are fine where they
> are.
>
> > */
> > +struct detected_devices_node {
> > +     struct list_head list;
> > +     dev_t dev;
> > +};
> >
> >  void md_autodetect_dev(dev_t dev)
> >  {
> > -     if (dev_cnt >= 0 && dev_cnt < 127)
> > -             detected_devices[dev_cnt++] = dev;
> > +     struct detected_devices_node *node_detected_dev;
> > +     node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
> > +     if (node_detected_dev) {
> > +             node_detected_dev->dev = dev;
> > +             list_add(&node_detected_dev->list, &all_detected_devices);
>
> Probably list_add_tail would be better so the ordering is the same as
> it was before?
>
>
> > +     } else {
> > +             printk(KERN_CRIT "md: kzAlloc node failed, skipping device."
> > +                              " : 0x%p.\n", node_detected_dev);
> > +     }
>
>
>
> >  }
> >
> >
> > @@ -5765,7 +5779,13 @@ static void autostart_arrays(int part)
> >  static void autostart_arrays(int part)
> >  {
> >       mdk_rdev_t *rdev;
> > -     int i;
> > +     struct detected_devices_node *node_detected_dev;
> > +     dev_t dev;
> > +     int i_scanned, i_passed, i_loops;
> > +     signed int i_found;
> > +     i_scanned = 0;
> > +     i_passed = 0;
> > +     i_loops = 0;
>
> i_passed is never used.
>
> And what is the point of i_loops (and i_scanned)?  The comments
> at the top of the patch should explain this if there is a good reason.
>
> >
> >       printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
> >
> > @@ -5772,3 +5792,11 @@ static void autostart_arrays(int part)
> > -     for (i = 0; i < dev_cnt; i++) {
> > -             dev_t dev = detected_devices[i];
> > -
> > +             /* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */
> > +     while (!list_empty(&all_detected_devices) && i_loops < 0x7FFFFFFF) {
> > +             i_scanned++;
> > +             node_detected_dev = NULL;
> > +             node_detected_dev = list_entry(all_detected_devices.next,
> > +                                     struct detected_devices_node, list);
> > +             if (node_detected_dev) {
>
> list_entry will *never* return NULL.   It simply doesn't pointer
> arithmetic.  (Well, I guess it could return NULL if it was passed
> NULL, and the struct-offset were 0, but that isn't the case here).
>
> So you don't need this 'if' at all.
>
> > +                     list_del(&node_detected_dev->list);
> > +                     dev = node_detected_dev->dev;
> > +                     kfree(node_detected_dev);
> > +             /* Indent is now off by one, but the old code is after this. */
>
> so you don't need to worrying about indents.
>
>
> > @@ -5781,8 +5809,16 @@ static void autostart_arrays(int part)
> >                       continue;
> >               }
> >               list_add(&rdev->same_set, &pending_raid_disks);
> > +             /* Indent is now off by one, but the old code is above this. */
> > +
> > +             } else {
> > +                     printk(KERN_CRIT "md: Invalid list node, skipping.\n");
> > +             }
> > +             i_loops++;
> >       }
> > -     dev_cnt = 0;
> > +
> > +     printk(KERN_INFO "md: Passes %d : Scanned %d and added %d devices.\n",
> > +                                             i_loops, i_scanned, i_passed);
> >
> >       autorun_devices(part);
> >  }
>
>
> I'm not dead-set against the change, and if you tidy it up I'll
> probably accept it, but I really think you would be better off skipping
> the in-kernel autodetect stuff altogether and use mdadm to assemble
> your arrays.
>
> NeilBrown
>

[patch v2 1/1] md: Software Raid autodetect dev list not array

[Michael J. Evans]

From: Michael J. Evans <mjevans1983@gmail.com>

In current release kernels the md module (Software RAID) uses a static array
 (dev_t[128]) to store partition/device info temporarily for autostart.

This patch replaces that static array with a list.

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
--- 
Version 2: Following Neil Brown's requests...
using list_add_tail, and corrected missing i_passed++;.
removed sections of code that would never be reached.
- -
The data/structures are only used within md.c, and very close together.
However I wonder if the structural information shouldn't go in to...
../../include/linux/raid/md_k.h instead.


I discovered this (and that the devices are added as disks/partitions are
discovered at boot) while I was debugging why only one of my MD arrays would
come up whole, while all the others were short a disk.

I eventually discovered that it was enumerating through all of 9 of my 11 hds
(2 had only 4 partitions apiece) while the other 9 have 15 partitions
(I wanted 64 per drive...). The last partition of the 8th drive in my 9 drive
raid 5 sets wasn't added, thus making the final md array short both a parity
and data disk, and it was started later, elsewhere.

Subject: [patch 1/1] md: Software Raid autodetect dev list not array

SOFTWARE RAID (Multiple Disks) SUPPORT
P:	Ingo Molnar
M:	mingo@redhat.com
P:	Neil Brown
M:	neilb@suse.de
L:	linux-raid@vger.kernel.org
S:	Supported
Unless you have a reason NOT to do so, CC linux-kernel@vger.kernel.org.

12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
    CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
    CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_SPINLOCK_SLEEP all simultaneously
    enabled.

It has been tested with CONFIG_SMP set and unset (Different x86_64 systems).
It has been tested with CONFIG_PREEMPT set and unset (same system).
CONFIG_LBD isn't even an option in my .config file.

Note: between 2.6.22 and 2.6.23-rc3-git5
                rdev = md_import_device(dev,0, 0);
became
                rdev = md_import_device(dev,0, 90);
So the patch has been edited to patch around that line. (might be fuzzy)

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
=============================================================
--- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
+++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
@@ -24,4 +24,6 @@

+   - autodetect dev list not array: Michael J. Evans <mjevans1983@gmail.com>
+
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 2, or (at your option)
@@ -5752,13 +5754,24 @@ void md_autodetect_dev(dev_t dev)
  * Searches all registered partitions for autorun RAID arrays
  * at boot time.
  */
-static dev_t detected_devices[128];
-static int dev_cnt;
+
+static LIST_HEAD(all_detected_devices);
+struct detected_devices_node {
+	struct list_head list;
+	dev_t dev;
+};
 
 void md_autodetect_dev(dev_t dev)
 {
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	struct detected_devices_node *node_detected_dev;
+	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
+	if (node_detected_dev) {
+		node_detected_dev->dev = dev;
+		list_add_tail(&node_detected_dev->list, &all_detected_devices);
+	} else {
+		printk(KERN_CRIT "md: kzAlloc node failed, skipping device."
+				 " : 0x%p.\n", node_detected_dev);
+	}
 }
 
 
@@ -5765,7 +5778,12 @@ static void autostart_arrays(int part)
 static void autostart_arrays(int part)
 {
 	mdk_rdev_t *rdev;
-	int i;
+	struct detected_devices_node *node_detected_dev;
+	dev_t dev;
+	int i_scanned, i_passed;
+	signed int i_found;
+	i_scanned = 0;
+	i_passed = 0;
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
@@ -5772,3 +5790,8 @@ static void autostart_arrays(int part)
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
-
+		/* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */
+	while (!list_empty(&all_detected_devices) && i_scanned < 0x7FFFFFFF) {
+		i_scanned++;
+		node_detected_dev = list_entry(all_detected_devices.next,
+					struct detected_devices_node, list);
+		list_del(&node_detected_dev->list);
+		dev = node_detected_dev->dev;
+		kfree(node_detected_dev);
@@ -5781,8 +5806,11 @@ static void autostart_arrays(int part)
 			continue;
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
+		i_passed++;
 	}
-	dev_cnt = 0;
+
+	printk(KERN_INFO "md: Scanned %d and added %d devices.\n",
+						i_scanned, i_passed);
 
 	autorun_devices(part);
 }

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Michael Evans]

Also, I forgot to mention, the reason I added the counters was mostly
for debugging.  However they're also as useful in the same way that
listing the partitions when a new disk is added can be (in fact this
augments that and the existing messages the autodetect routines
provide).

As for using autodetect or not... the only way to skip it seems to be
compiling md's raid support as a module.  I checked 2.6.22's
menuconfig and there's no way for me to explicitly turn it on or off
at compile time.  I also feel that forcing the addition of a boot
parameter to de-activate a broken and deprecated system you aren't
even aware you are getting is somehow wrong.  So if you have over 128
devices for it to scan, as I do on one of my PCs, then it can bring up
an array in degraded mode.

... crud.

I also just noticed, while looking to see if there was some existing
way of detecting if debugging were enabled and to be extra-verbose,
that I left in one of my other debugging variables by mistake.
i_found. Since it's signed, it must have been the variable I was using
to detect where my list matched the existing array in my initial
verification runs.


Are there any other things you'd like to see changed before I submit a
third patch version?

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Jan Engelhardt]

On Aug 26 2007 04:51, Michael J. Evans wrote:
> {
>-	if (dev_cnt >= 0 && dev_cnt < 127)
>-		detected_devices[dev_cnt++] = dev;
>+	struct detected_devices_node *node_detected_dev;
>+	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\

What's the \ good for, besides escaping the newline
that is ignored as whitespace anyway? :-)

>@@ -5772,3 +5790,8 @@ static void autostart_arrays(int part)
>-	for (i = 0; i < dev_cnt; i++) {
>-		dev_t dev = detected_devices[i];
>-
>+		/* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */
>+	while (!list_empty(&all_detected_devices) && i_scanned < 0x7FFFFFFF) {

I doubt someone really has _that_ many devices. Of course, to be on the
safer side, make it an unsigned int. That way, people could put in about
0xFFFFFFFE devs (which is even less likely than 0x7FFFFFFF)

>+		i_scanned++;
>+		node_detected_dev = list_entry(all_detected_devices.next,
>+					struct detected_devices_node, list);
>+		list_del(&node_detected_dev->list);
>+		dev = node_detected_dev->dev;
>+		kfree(node_detected_dev);

	Jan
-- 

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Michael Evans]

On 8/26/07, Jan Engelhardt <jengelh@computergmbh.de> wrote:
>
> On Aug 26 2007 04:51, Michael J. Evans wrote:
> > {
> >-      if (dev_cnt >= 0 && dev_cnt < 127)
> >-              detected_devices[dev_cnt++] = dev;
> >+      struct detected_devices_node *node_detected_dev;
> >+      node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
>
> What's the \ good for, besides escaping the newline
> that is ignored as whitespace anyway? :-)
>

I hadn't even noticed that, I suppose I mashed the key above enter at
some time.  Removing from my local file.

> >@@ -5772,3 +5790,8 @@ static void autostart_arrays(int part)
> >-      for (i = 0; i < dev_cnt; i++) {
> >-              dev_t dev = detected_devices[i];
> >-
> >+              /* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */
> >+      while (!list_empty(&all_detected_devices) && i_scanned < 0x7FFFFFFF) {
>
> I doubt someone really has _that_ many devices. Of course, to be on the
> safer side, make it an unsigned int. That way, people could put in about
> 0xFFFFFFFE devs (which is even less likely than 0x7FFFFFFF)
>

There is that, but I'm almost expecting someone to ask me to remove
both the ints and kprint statement.  (I'd like them as part of some
kind of verbose startup that people would actually think to use
however.)  Additionally a though occurred to me earlier, if there are
That many devices, the chance of a UUID namespace collision might
actually be realistic anyway.  Though I'm not short sighted enough to
put it past anyone to have more then 32/64K possible block devices.
Anyone with that much cash today is probably buying hardware raid, but
who knows.

> >+              i_scanned++;
> >+              node_detected_dev = list_entry(all_detected_devices.next,
> >+                                      struct detected_devices_node, list);
> >+              list_del(&node_detected_dev->list);
> >+              dev = node_detected_dev->dev;
> >+              kfree(node_detected_dev);
>
>         Jan
> --
>

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Randy Dunlap]

On Sun, 26 Aug 2007 04:51:24 -0700 Michael J. Evans wrote:

> From: Michael J. Evans <mjevans1983@gmail.com>
> 
> In current release kernels the md module (Software RAID) uses a static array
>  (dev_t[128]) to store partition/device info temporarily for autostart.
> 
> This patch replaces that static array with a list.
> 
> Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
> --- 
> Version 2: Following Neil Brown's requests...
> using list_add_tail, and corrected missing i_passed++;.
> removed sections of code that would never be reached.
> - -
> The data/structures are only used within md.c, and very close together.
> However I wonder if the structural information shouldn't go in to...
> ../../include/linux/raid/md_k.h instead.
> 
> 
> I discovered this (and that the devices are added as disks/partitions are
> discovered at boot) while I was debugging why only one of my MD arrays would
> come up whole, while all the others were short a disk.
> 
> I eventually discovered that it was enumerating through all of 9 of my 11 hds
> (2 had only 4 partitions apiece) while the other 9 have 15 partitions
> (I wanted 64 per drive...). The last partition of the 8th drive in my 9 drive
> raid 5 sets wasn't added, thus making the final md array short both a parity
> and data disk, and it was started later, elsewhere.
> 
> Subject: [patch 1/1] md: Software Raid autodetect dev list not array
> 
> SOFTWARE RAID (Multiple Disks) SUPPORT
> P:	Ingo Molnar
> M:	mingo@redhat.com
> P:	Neil Brown
> M:	neilb@suse.de
> L:	linux-raid@vger.kernel.org
> S:	Supported
> Unless you have a reason NOT to do so, CC linux-kernel@vger.kernel.org.
> 
> 12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
>     CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
>     CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_SPINLOCK_SLEEP all simultaneously
>     enabled.
> 
> It has been tested with CONFIG_SMP set and unset (Different x86_64 systems).
> It has been tested with CONFIG_PREEMPT set and unset (same system).
> CONFIG_LBD isn't even an option in my .config file.

It's not an option 64_BIT builds.

> Note: between 2.6.22 and 2.6.23-rc3-git5
>                 rdev = md_import_device(dev,0, 0);
> became
>                 rdev = md_import_device(dev,0, 90);
> So the patch has been edited to patch around that line. (might be fuzzy)
> 
> Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
> =============================================================
> --- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
> +++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
> @@ -5752,13 +5754,24 @@ void md_autodetect_dev(dev_t dev)
>   * Searches all registered partitions for autorun RAID arrays
>   * at boot time.
>   */
> -static dev_t detected_devices[128];
> -static int dev_cnt;
> +
> +static LIST_HEAD(all_detected_devices);
> +struct detected_devices_node {
> +	struct list_head list;
> +	dev_t dev;
> +};
>  
>  void md_autodetect_dev(dev_t dev)
>  {
> -	if (dev_cnt >= 0 && dev_cnt < 127)
> -		detected_devices[dev_cnt++] = dev;
> +	struct detected_devices_node *node_detected_dev;
> +	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
> +	if (node_detected_dev) {
> +		node_detected_dev->dev = dev;
> +		list_add_tail(&node_detected_dev->list, &all_detected_devices);
> +	} else {
> +		printk(KERN_CRIT "md: kzAlloc node failed, skipping device."
> +				 " : 0x%p.\n", node_detected_dev);

Is there any way to tell the user what device (or partition?) is
bein skipped?  This printk should just print (confirm) that
node_detected_dev is NULL.  Shouldn't it just print <dev> in
major:minor format?

> +	}
>  }
>  
>  
> @@ -5765,7 +5778,12 @@ static void autostart_arrays(int part)
>  static void autostart_arrays(int part)
>  {
>  	mdk_rdev_t *rdev;
> -	int i;
> +	struct detected_devices_node *node_detected_dev;
> +	dev_t dev;
> +	int i_scanned, i_passed;
> +	signed int i_found;

Drop "signed", like the surrounding code.
Leave a blank line between data declarations and beginning of code.

> +	i_scanned = 0;
> +	i_passed = 0;
>  
>  	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
>  
> @@ -5772,3 +5790,8 @@ static void autostart_arrays(int part)
> -	for (i = 0; i < dev_cnt; i++) {
> -		dev_t dev = detected_devices[i];
> -
> +		/* FIXME: max 'int' #DEFINEd somewhere?  not   0x7FFFFFFF ? */

include/linux/kernel.h has INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX,
LLONG_MAX, ULLONG_MAX.

> +	while (!list_empty(&all_detected_devices) && i_scanned < 0x7FFFFFFF) {
> +		i_scanned++;
> +		node_detected_dev = list_entry(all_detected_devices.next,
> +					struct detected_devices_node, list);
> +		list_del(&node_detected_dev->list);
> +		dev = node_detected_dev->dev;
> +		kfree(node_detected_dev);


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Michael Evans]

On 8/26/07, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On Sun, 26 Aug 2007 04:51:24 -0700 Michael J. Evans wrote:
>
> > From: Michael J. Evans <mjevans1983@gmail.com>
> >

>
> Is there any way to tell the user what device (or partition?) is
> bein skipped?  This printk should just print (confirm) that
> node_detected_dev is NULL.  Shouldn't it just print <dev> in
> major:minor format?
>

It would be possible with the MAJOR() and MINOR() macros to do this...
however it doesn't really help out much during troubleshooting. I
tried using the bdevname function like the function that calls this
one uses, however, it wants a struct device_block... which I tried
getting with:

container_of(dev, struct block_device, bd_dev)

Of course this didn't quite work out, I got kernel panics on my two
trial attempts.

Here's a skip from a dmesg where I added a printk right under the line
in question.

[   63.033532] sd 11:0:0:0: [sdk] 976773168 512-byte hardware sectors
(500108 MB)
[   63.039842] sd 11:0:0:0: [sdk] Write Protect is off
[   63.046012] sd 11:0:0:0: [sdk] Mode Sense: 00 3a 00 00
[   63.046025] sd 11:0:0:0: [sdk] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[   63.052309]  sdk: sdk1 sdk2 sdk3 sdk4 sdk5 sdk6 sdk7 sdk8 sdk9
sdk10 sdk11 sdk12 sdk13 sdk14 sdk15
[   63.082546] md: Autodetect-buffering the above device.
[   63.088893] md: Autodetect-buffering the above device.
[   63.095053] md: Autodetect-buffering the above device.
[   63.101082] md: Autodetect-buffering the above device.
[   63.106956] md: Autodetect-buffering the above device.
[   63.112596] md: Autodetect-buffering the above device.
[   63.117998] md: Autodetect-buffering the above device.
[   63.123396] md: Autodetect-buffering the above device.
[   63.128789] md: Autodetect-buffering the above device.
[   63.134182] md: Autodetect-buffering the above device.
[   63.139576] md: Autodetect-buffering the above device.
[   63.144970] md: Autodetect-buffering the above device.
[   63.150360] md: Autodetect-buffering the above device.
[   63.155749] md: Autodetect-buffering the above device.
[   63.161498] sd 11:0:0:0: [sdk] Attached SCSI disk


> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Kyle Moffett]

On Aug 26, 2007, at 08:20:45, Michael Evans wrote:
> Also, I forgot to mention, the reason I added the counters was  
> mostly for debugging.  However they're also as useful in the same  
> way that listing the partitions when a new disk is added can be (in  
> fact this augments that and the existing messages the autodetect  
> routines provide).
>
> As for using autodetect or not... the only way to skip it seems to  
> be compiling md's raid support as a module.  I checked 2.6.22's  
> menuconfig and there's no way for me to explicitly turn it on or  
> off at compile time.  I also feel that forcing the addition of a  
> boot parameter to de-activate a broken and deprecated system you  
> aren't even aware you are getting is somehow wrong.  So if you have  
> over 128 devices for it to scan, as I do on one of my PCs, then it  
> can bring up
> an array in degraded mode.  ... crud.

Well, you could just change the MSDOS disk label to use a different  
"Partition Type" for your raid partitions.  Just pick the standard  
"Linux" type and you will get exactly the same behavior that  
everybody who doesn't use MSDOS partition tables gets.

Cheers,
Kyle Moffett

Re: [patch v2 1/1] md: Software Raid autodetect dev list not array

[Michael Evans]

On 8/26/07, Kyle Moffett <mrmacman_g4@mac.com> wrote:
> On Aug 26, 2007, at 08:20:45, Michael Evans wrote:
> > Also, I forgot to mention, the reason I added the counters was
> > mostly for debugging.  However they're also as useful in the same
> > way that listing the partitions when a new disk is added can be (in
> > fact this augments that and the existing messages the autodetect
> > routines provide).
> >
> > As for using autodetect or not... the only way to skip it seems to
> > be compiling md's raid support as a module.  I checked 2.6.22's
> > menuconfig and there's no way for me to explicitly turn it on or
> > off at compile time.  I also feel that forcing the addition of a
> > boot parameter to de-activate a broken and deprecated system you
> > aren't even aware you are getting is somehow wrong.  So if you have
> > over 128 devices for it to scan, as I do on one of my PCs, then it
> > can bring up
> > an array in degraded mode.  ... crud.
>
> Well, you could just change the MSDOS disk label to use a different
> "Partition Type" for your raid partitions.  Just pick the standard
> "Linux" type and you will get exactly the same behavior that
> everybody who doesn't use MSDOS partition tables gets.
>
> Cheers,
> Kyle Moffett
>
>

I recall most of the guides I referenced during setup having me change
the partition type, additionally parted only calls the flag 'raid' not
'raid autodetect'.  However it would still be confusing to anyone not
intimately familiar with the subsystem.

Also, even though the system has a standard PC BIOS, I liked some of
the specifications of the GUID partition table (GPT) provided.  Namely
a checksum and backup copy, and up to 128 partitions per disk.  Parted
is the only real tool I could find for editing such a disk label.

A problem I experienced that is almost completely unrelated to this
patch are other 'magic number' assumptions.  It is rather unfortunate
that linux allocates a fixed (and very small) number of partitions per
scsi disk.  A better method might be a complete dis-association of
major:minor pair to device name, and instead simply enumerating
partitions as they are detected.  That way if I choose to have 128
drives but each having at most 2 partitions I still easily fit within
one major, or if I choose the opposite (for whatever reason) I still
come to the same conclusion.  Before anyone mentions using LVM
instead, sharing operating systems, and using different partitions for
different raid stripe sizes/rebuilding flexibility/restriping
flexibility are just two good reasons I can think of for supporting
more then 15 partitions per device.

[patch v3 1/1] md: Software Raid autodetect dev list not array

[Michael J. Evans]

From: Michael J. Evans <mjevans1983@gmail.com>

In current release kernels the md module (Software RAID) uses a static array
 (dev_t[128]) to store partition/device info temporarily for autostart.

This patch replaces that static array with a list.

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
--- 
Sorry, it looks like I hit reply instead of reply to all yesterday.

Version 3:
md_autodetect_dev failure message is now more usefully verbose.
removed unused i_found that was leftover from initial verification.
Thank you Randy Dunlap for pointing where INT_MAX was, fixme fixed.

Version 2:
using list_add_tail, and corrected missing i_passed++;.
removed sections of code that would never be reached.

Version 1:
The data/structures are only used within md.c, and very close together.
However I wonder if the structural information shouldn't go in to...
../../include/linux/raid/md_k.h instead.


I discovered this (and that the devices are added as disks/partitions are
discovered at boot) while I was debugging why only one of my MD arrays would
come up whole, while all the others were short a disk.

I eventually discovered that it was enumerating through all of 9 of my 11 hds
(2 had only 4 partitions apiece) while the other 9 have 15 partitions
(I wanted 64 per drive...). The last partition of the 8th drive in my 9 drive
raid 5 sets wasn't added, thus making the final md array short both a parity
and data disk, and it was started later, elsewhere.

Subject: [patch 1/1] md: Software Raid autodetect dev list not array

SOFTWARE RAID (Multiple Disks) SUPPORT
P:	Ingo Molnar
M:	mingo@redhat.com
P:	Neil Brown
M:	neilb@suse.de
L:	linux-raid@vger.kernel.org
S:	Supported
Unless you have a reason NOT to do so, CC linux-kernel@vger.kernel.org.

12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
    CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
    CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_SPINLOCK_SLEEP all simultaneously
    enabled.

It has been tested with CONFIG_SMP set and unset (Different x86_64 systems).
It has been tested with CONFIG_PREEMPT set and unset (same system).
CONFIG_LBD isn't even an option in my .config file.

Note: between 2.6.22 and 2.6.23-rc3-git5
                rdev = md_import_device(dev,0, 0);
became
                rdev = md_import_device(dev,0, 90);
So the patch has been edited to patch around that line. (might be fuzzy)

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
=============================================================
--- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
+++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
@@ -24,4 +24,6 @@

+   - autodetect dev list not array: Michael J. Evans <mjevans1983@gmail.com>
+
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 2, or (at your option)
@@ -5752,13 +5754,26 @@ void md_autodetect_dev(dev_t dev)
  * Searches all registered partitions for autorun RAID arrays
  * at boot time.
  */
-static dev_t detected_devices[128];
-static int dev_cnt;
+
+static LIST_HEAD(all_detected_devices);
+struct detected_devices_node {
+	struct list_head list;
+	dev_t dev;
+};
 
 void md_autodetect_dev(dev_t dev)
 {
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	struct detected_devices_node *node_detected_dev;
+	char strbuf[BDEVNAME_SIZE];
+
+	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\
+	if (node_detected_dev) {
+		node_detected_dev->dev = dev;
+		list_add_tail(&node_detected_dev->list, &all_detected_devices);
+	} else {
+		printk(KERN_CRIT "md: md_autodetect_dev: kzAlloc node failed"
+		" (null return), skipping dev(%d,%d)\n", MAJOR(dev), MINOR(dev));
+	}
 }
 
 
@@ -5765,7 +5760,12 @@ static void autostart_arrays(int part)
 static void autostart_arrays(int part)
 {
 	mdk_rdev_t *rdev;
-	int i;
+	struct detected_devices_node *node_detected_dev;
+	dev_t dev;
+	int i_scanned, i_passed;
+
+	i_scanned = 0;
+	i_passed = 0;
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
@@ -5772,3 +5792,7 @@ static void autostart_arrays(int part)
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
-
+	while (!list_empty(&all_detected_devices) && i_scanned < INT_MAX) {
+		i_scanned++;
+		node_detected_dev = list_entry(all_detected_devices.next,
+					struct detected_devices_node, list);
+		list_del(&node_detected_dev->list);
+		dev = node_detected_dev->dev;
+		kfree(node_detected_dev);
@@ -5781,8 +5807,11 @@ static void autostart_arrays(int part)
 			continue;
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
+		i_passed++;
 	}
-	dev_cnt = 0;
+
+	printk(KERN_INFO "md: Scanned %d and added %d devices.\n",
+						i_scanned, i_passed);
 
 	autorun_devices(part);
 }

Re: [patch v3 1/1] md: Software Raid autodetect dev list not array

[Randy Dunlap]

On Mon, 27 Aug 2007 15:16:21 -0700 Michael J. Evans wrote:

> Note: between 2.6.22 and 2.6.23-rc3-git5
>                 rdev = md_import_device(dev,0, 0);
> became
>                 rdev = md_import_device(dev,0, 90);
> So the patch has been edited to patch around that line. (might be fuzzy)

so you should update the patch to the latest mainline.

It's up to the (RAID) maintainer(s) if they want to merge a patch with fuzz.
Andrew may fix it up.  Linus wouldn't accept it with fuzz.

> Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
> =============================================================
> --- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
> +++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
> @@ -24,4 +24,6 @@
> 
> +   - autodetect dev list not array: Michael J. Evans <mjevans1983@gmail.com>
> +

Nowadays we use an SCM for such comments, not the source file(s).

>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>     the Free Software Foundation; either version 2, or (at your option)
> @@ -5752,13 +5754,26 @@ void md_autodetect_dev(dev_t dev)
>   * Searches all registered partitions for autorun RAID arrays
>   * at boot time.
>   */
> -static dev_t detected_devices[128];
> -static int dev_cnt;
> +
> +static LIST_HEAD(all_detected_devices);
> +struct detected_devices_node {
> +	struct list_head list;
> +	dev_t dev;
> +};
>  
>  void md_autodetect_dev(dev_t dev)
>  {
> -	if (dev_cnt >= 0 && dev_cnt < 127)
> -		detected_devices[dev_cnt++] = dev;
> +	struct detected_devices_node *node_detected_dev;
> +	char strbuf[BDEVNAME_SIZE];
> +
> +	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);\

Drop the trailing '\', as someone has already commented on.

> +	if (node_detected_dev) {
> +		node_detected_dev->dev = dev;
> +		list_add_tail(&node_detected_dev->list, &all_detected_devices);
> +	} else {
> +		printk(KERN_CRIT "md: md_autodetect_dev: kzAlloc node failed"
> +		" (null return), skipping dev(%d,%d)\n", MAJOR(dev), MINOR(dev));

printk() formatting is bad.
Drop the " (null return)" and indent that line more than the
printk line is indented.

> +	}
>  }
>  
>  

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[patch v5 1/1] md: Software Raid autodetect dev list not array

[Michael J. Evans]

From: Michael J. Evans <mjevans1983@gmail.com>

In current release kernels the md module (Software RAID) uses a static array
 (dev_t[128]) to store partition/device info temporarily for autostart.

This patch replaces that static array with a list.

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
--- 
CCing akpm@linux-foundation.org

Version 5:
patched modified to cleanly patch against 2.6.23-rc4 (Offsets only)
Removed credit from head of file since the changelogs are archived.

Version 4:
Minor formatting cleanup for kzAlloc error message.
Should I remove the first patch section in version 5?

Version 3:
md_autodetect_dev failure message is now more usefully verbose.
removed unused i_found that was leftover from initial verification.
Thank you Randy Dunlap for pointing where INT_MAX was, fixme fixed.

Version 2:
using list_add_tail, and corrected missing i_passed++;.
removed sections of code that would never be reached.

Version 1:
The data/structures are only used within md.c, and very close together.
However I wonder if the structural information shouldn't go in to...
../../include/linux/raid/md_k.h instead.


I discovered this (and that the devices are added as disks/partitions are
discovered at boot) while I was debugging why only one of my MD arrays would
come up whole, while all the others were short a disk.

I eventually discovered that it was enumerating through all of 9 of my 11 hds
(2 had only 4 partitions apiece) while the other 9 have 15 partitions
(I wanted 64 per drive...). The last partition of the 8th drive in my 9 drive
raid 5 sets wasn't added, thus making the final md array short both a parity
and data disk, and it was started later, elsewhere.

Subject: [patch 1/1] md: Software Raid autodetect dev list not array

SOFTWARE RAID (Multiple Disks) SUPPORT
P:	Ingo Molnar
M:	mingo@redhat.com
P:	Neil Brown
M:	neilb@suse.de
L:	linux-raid@vger.kernel.org
S:	Supported
Unless you have a reason NOT to do so, CC linux-kernel@vger.kernel.org.

12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
    CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
    CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_SPINLOCK_SLEEP all simultaneously
    enabled.

It has been tested with CONFIG_SMP set and unset (Different x86_64 systems).
It has been tested with CONFIG_PREEMPT set and unset (same system).
CONFIG_LBD isn't even an option in my .config file.

Note: between 2.6.22 and 2.6.23-rc3-git5
                rdev = md_import_device(dev,0, 0);
became
                rdev = md_import_device(dev,0, 90);
So the patch has been edited to patch around that line.

Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
=============================================================
--- linux/drivers/md/md.c.orig	2007-08-21 03:19:42.511576248 -0700
+++ linux/drivers/md/md.c	2007-08-21 04:30:09.775525710 -0700
@@ -5781,13 +5781,26 @@ void md_autodetect_dev(dev_t dev)
  * Searches all registered partitions for autorun RAID arrays
  * at boot time.
  */
-static dev_t detected_devices[128];
-static int dev_cnt;
+
+static LIST_HEAD(all_detected_devices);
+struct detected_devices_node {
+	struct list_head list;
+	dev_t dev;
+};
 
 void md_autodetect_dev(dev_t dev)
 {
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	struct detected_devices_node *node_detected_dev;
+	char strbuf[BDEVNAME_SIZE];
+
+	node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);
+	if (node_detected_dev) {
+		node_detected_dev->dev = dev;
+		list_add_tail(&node_detected_dev->list, &all_detected_devices);
+	} else {
+		printk(KERN_CRIT "md: md_autodetect_dev: kzAlloc node failed"
+			", skipping dev(%d,%d)\n", MAJOR(dev), MINOR(dev));
+	}
 }
 
 
@@ -5794,7 +5787,12 @@ static void autostart_arrays(int part)
 static void autostart_arrays(int part)
 {
 	mdk_rdev_t *rdev;
-	int i;
+	struct detected_devices_node *node_detected_dev;
+	dev_t dev;
+	int i_scanned, i_passed;
+
+	i_scanned = 0;
+	i_passed = 0;
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
@@ -5801,3 +5821,7 @@ static void autostart_arrays(int part)
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
-
+	while (!list_empty(&all_detected_devices) && i_scanned < INT_MAX) {
+		i_scanned++;
+		node_detected_dev = list_entry(all_detected_devices.next,
+					struct detected_devices_node, list);
+		list_del(&node_detected_dev->list);
+		dev = node_detected_dev->dev;
+		kfree(node_detected_dev);
@@ -5810,8 +5834,11 @@ static void autostart_arrays(int part)
 			continue;
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
+		i_passed++;
 	}
-	dev_cnt = 0;
+
+	printk(KERN_INFO "md: Scanned %d and added %d devices.\n",
+						i_scanned, i_passed);
 
 	autorun_devices(part);
 }

Re: [patch v5 1/1] md: Software Raid autodetect dev list not array

[Randy Dunlap]

Michael J. Evans wrote:
> From: Michael J. Evans <mjevans1983@gmail.com>
> 
> In current release kernels the md module (Software RAID) uses a static array
>  (dev_t[128]) to store partition/device info temporarily for autostart.
> 
> This patch replaces that static array with a list.
> 
> Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>
> --- 

So only the text above goes into the permanent changelog (if it's
used as is).  That text tells what the patch does but not why it
does it.  Telling why is more important.  Some of the text below
should be moved up to the changelog area, possibly with some other
editing for readability (see BEGIN/END below).

> CCing akpm@linux-foundation.org
> 
> Version 5:
> patched modified to cleanly patch against 2.6.23-rc4 (Offsets only)
> Removed credit from head of file since the changelogs are archived.
> 
> Version 4:
> Minor formatting cleanup for kzAlloc error message.
> Should I remove the first patch section in version 5?
> 
> Version 3:
> md_autodetect_dev failure message is now more usefully verbose.
> removed unused i_found that was leftover from initial verification.
> Thank you Randy Dunlap for pointing where INT_MAX was, fixme fixed.
> 
> Version 2:
> using list_add_tail, and corrected missing i_passed++;.
> removed sections of code that would never be reached.
> 
> Version 1:
> The data/structures are only used within md.c, and very close together.
> However I wonder if the structural information shouldn't go in to...
> ../../include/linux/raid/md_k.h instead.
> 

BEGIN

> I discovered this (and that the devices are added as disks/partitions are
> discovered at boot) while I was debugging why only one of my MD arrays would
> come up whole, while all the others were short a disk.
> 
> I eventually discovered that it was enumerating through all of 9 of my 11 hds
> (2 had only 4 partitions apiece) while the other 9 have 15 partitions
> (I wanted 64 per drive...). The last partition of the 8th drive in my 9 drive
> raid 5 sets wasn't added, thus making the final md array short both a parity
> and data disk, and it was started later, elsewhere.

END

> Subject: [patch 1/1] md: Software Raid autodetect dev list not array
> 
> SOFTWARE RAID (Multiple Disks) SUPPORT
> P:	Ingo Molnar
> M:	mingo@redhat.com
> P:	Neil Brown
> M:	neilb@suse.de
> L:	linux-raid@vger.kernel.org
> S:	Supported
> Unless you have a reason NOT to do so, CC linux-kernel@vger.kernel.org.
> 
> 12: Has been tested with CONFIG_PREEMPT, CONFIG_DEBUG_PREEMPT,
>     CONFIG_DEBUG_SLAB, CONFIG_DEBUG_PAGEALLOC, CONFIG_DEBUG_MUTEXES,
>     CONFIG_DEBUG_SPINLOCK, CONFIG_DEBUG_SPINLOCK_SLEEP all simultaneously
>     enabled.
> 
> It has been tested with CONFIG_SMP set and unset (Different x86_64 systems).
> It has been tested with CONFIG_PREEMPT set and unset (same system).
> CONFIG_LBD isn't even an option in my .config file.
> 
> Note: between 2.6.22 and 2.6.23-rc3-git5
>                 rdev = md_import_device(dev,0, 0);
> became
>                 rdev = md_import_device(dev,0, 90);
> So the patch has been edited to patch around that line.
> 
> Signed-off-by: Michael J. Evans <mjevans1983@gmail.com>


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***