LinuxLists.cc - Re: [patch 1/1] md: Software Raid autodetect dev list not array

[permalink] [raw]

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

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 <[email protected]> wrote:
> On Wednesday August 22, [email protected] wrote:
> > From: Michael J. Evans <[email protected]>
> >
> > 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 <[email protected]>
> > +
> > 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
>

2007-08-26 11:57:21

[permalink] [raw]

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

From: Michael J. Evans <[email protected]>

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 <[email protected]>
---
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: [email protected]
P: Neil Brown
M: [email protected]
L: [email protected]
S: Supported
Unless you have a reason NOT to do so, CC [email protected].

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 <[email protected]>
=============================================================
--- 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 <[email protected]>
+
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);
}

2007-08-26 12:20:58

[permalink] [raw]

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

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?

2007-08-26 12:56:36

by Jan Engelhardt

[permalink] [raw]

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

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
--

2007-08-26 15:58:37

[permalink] [raw]

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

On 8/26/07, Jan Engelhardt <[email protected]> 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
> --
>

2007-08-26 16:58:36

[permalink] [raw]

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

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

> From: Michael J. Evans <[email protected]>
>
> 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 <[email protected]>
> ---
> 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: [email protected]
> P: Neil Brown
> M: [email protected]
> L: [email protected]
> S: Supported
> Unless you have a reason NOT to do so, CC [email protected].
>
> 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 <[email protected]>
> =============================================================
> --- 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 ***

2007-08-26 19:19:15

[permalink] [raw]

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

On 8/26/07, Randy Dunlap <[email protected]> wrote:
> On Sun, 26 Aug 2007 04:51:24 -0700 Michael J. Evans wrote:
>
> > From: Michael J. Evans <[email protected]>
> >

>
> 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 ***
>

2007-08-27 03:42:10

by Kyle Moffett

[permalink] [raw]

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

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

2007-08-27 07:56:43

[permalink] [raw]

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

On 8/26/07, Kyle Moffett <[email protected]> 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.

2007-08-27 22:17:22

[permalink] [raw]

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

From: Michael J. Evans <[email protected]>

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 <[email protected]>
---
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: [email protected]
P: Neil Brown
M: [email protected]
L: [email protected]
S: Supported
Unless you have a reason NOT to do so, CC [email protected].

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 <[email protected]>
=============================================================
--- 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 <[email protected]>
+
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);
}

2007-08-27 22:31:41

[permalink] [raw]

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

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 <[email protected]>
> =============================================================
> --- 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 <[email protected]>
> +

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 ***

2007-08-28 04:40:28

[permalink] [raw]

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

From: Michael J. Evans <[email protected]>

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 <[email protected]>
---
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: [email protected]
P: Neil Brown
M: [email protected]
L: [email protected]
S: Supported
Unless you have a reason NOT to do so, CC [email protected].

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 <[email protected]>
=============================================================
--- 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 <[email protected]>
+
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"
+ ", 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);
}

2007-08-28 04:40:48

[permalink] [raw]

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

On Monday 27 August 2007, Randy Dunlap wrote:
> On Mon, 27 Aug 2007 15:16:21 -0700 Michael J. Evans wrote:
>
> > =============================================================
> > --- 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
<[email protected]>
> > +
>
> Nowadays we use an SCM for such comments, not the source file(s).

SCM? Is this automatic, where do I go to see it?

>
> > 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.
>

I thought I had fixed that, I must have copied the unfixed version out of the
way when I made the other changes to it.

> > + 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 ***
>

2007-08-28 04:48:21

[permalink] [raw]

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

Michael J. Evans wrote:
> On Monday 27 August 2007, Randy Dunlap wrote:
>> On Mon, 27 Aug 2007 15:16:21 -0700 Michael J. Evans wrote:
>>
>>> =============================================================
>>> --- 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
> <[email protected]>
>>> +
>> Nowadays we use an SCM for such comments, not the source file(s).
>
> SCM? Is this automatic, where do I go to see it?

See Documentation/SubmittingPatches:
14) The canonical patch format:

The canonical patch message body contains the following:

- A "from" line specifying the patch author.

- An empty line.

- The body of the explanation, which will be copied to the
permanent changelog to describe this patch.

- The "Signed-off-by:" lines, described above, which will
also go in the changelog.

- A marker line containing simply "---".

- Any additional comments not suitable for the changelog.

- The actual patch (diff output).

so just put whatever you want to be in the permanent SCM logs
into the "body of the explanation" part of the email.

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

2007-08-28 13:09:04

[permalink] [raw]

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

On 8/27/07, Randy Dunlap <[email protected]> wrote:
> Michael J. Evans wrote:
> > On Monday 27 August 2007, Randy Dunlap wrote:
> >> On Mon, 27 Aug 2007 15:16:21 -0700 Michael J. Evans wrote:
> >>
> >>> =============================================================
> >>> --- 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
> > <[email protected]>
> >>> +
> >> Nowadays we use an SCM for such comments, not the source file(s).
> >
> > SCM? Is this automatic, where do I go to see it?
>
> See Documentation/SubmittingPatches:
> 14) The canonical patch format:
>
> The canonical patch message body contains the following:
>
> - A "from" line specifying the patch author.
>
> - An empty line.
>
> - The body of the explanation, which will be copied to the
> permanent changelog to describe this patch.
>
> - The "Signed-off-by:" lines, described above, which will
> also go in the changelog.
>
> - A marker line containing simply "---".
>
> - Any additional comments not suitable for the changelog.
>
> - The actual patch (diff output).
>
>
> so just put whatever you want to be in the permanent SCM logs
> into the "body of the explanation" part of the email.
>
>
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>

Oh, I see. I forgot about the changelogs. I'd send out version 5
now, but I'm not sure what kernel version to make the patch against.
2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
Additionally I never could tell what git tree was the 'mainline' as it
isn't labeled with such a keyword (at least in the list of git trees I
saw).

2007-08-28 13:14:18

by Jan Engelhardt

[permalink] [raw]

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

On Aug 28 2007 06:08, Michael Evans wrote:
>
>Oh, I see. I forgot about the changelogs. I'd send out version 5
>now, but I'm not sure what kernel version to make the patch against.
>2.6.23-rc4 is on kernel.org and I don't see any git snapshots.

2.6.23-rc4 is a snapshot in itself, a tagged one at that.
Just use git pull to get the latest, which is always good.
Or git fetch; git checkout 2.6.23-rc4; if you need that particular one.

>Additionally I never could tell what git tree was the 'mainline' as it
>isn't labeled with such a keyword (at least in the list of git trees I
>saw).

/torvalds/linux-2.6.git or so; yes, it's not clearly marked. Then again, why?
/Mainline is tarballs for most people, and they don't want to go deeper ;-)

Jan
--

2007-08-28 13:24:48

by Bill Davidsen

[permalink] [raw]

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

Michael Evans wrote:
> Oh, I see. I forgot about the changelogs. I'd send out version 5
> now, but I'm not sure what kernel version to make the patch against.
> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
> Additionally I never could tell what git tree was the 'mainline' as it
> isn't labeled with such a keyword (at least in the list of git trees I
> saw).

I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
in an "-mm" kernel. That's probably desirable, anyway.

--
bill davidsen <[email protected]>
CTO TMR Associates, Inc
Doing interesting things with small computers since 1979

2007-08-28 13:27:38

[permalink] [raw]

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

On Tuesday 28 August 2007, Jan Engelhardt wrote:
>
> On Aug 28 2007 06:08, Michael Evans wrote:
> >
> >Oh, I see. I forgot about the changelogs. I'd send out version 5
> >now, but I'm not sure what kernel version to make the patch against.
> >2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
>
> 2.6.23-rc4 is a snapshot in itself, a tagged one at that.
> Just use git pull to get the latest, which is always good.
> Or git fetch; git checkout 2.6.23-rc4; if you need that particular one.
>
> >Additionally I never could tell what git tree was the 'mainline' as it
> >isn't labeled with such a keyword (at least in the list of git trees I
> >saw).
>
> /torvalds/linux-2.6.git or so; yes, it's not clearly marked. Then again,
why?
> /Mainline is tarballs for most people, and they don't want to go deeper ;-)
>
>
> Jan
> --
>

Thank you, I'll try to keep that in mind the next time I try to use git.

2007-08-28 13:28:42

[permalink] [raw]

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

From: Michael J. Evans <[email protected]>

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 <[email protected]>
---
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: [email protected]
P: Neil Brown
M: [email protected]
L: [email protected]
S: Supported
Unless you have a reason NOT to do so, CC [email protected].

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 <[email protected]>
=============================================================
--- 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);
}

2007-08-28 13:32:19

[permalink] [raw]

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

On 8/28/07, Bill Davidsen <[email protected]> wrote:
> Michael Evans wrote:
> > Oh, I see. I forgot about the changelogs. I'd send out version 5
> > now, but I'm not sure what kernel version to make the patch against.
> > 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
> > Additionally I never could tell what git tree was the 'mainline' as it
> > isn't labeled with such a keyword (at least in the list of git trees I
> > saw).
>
> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
> in an "-mm" kernel. That's probably desirable, anyway.
>
> --
> bill davidsen <[email protected]>
> CTO TMR Associates, Inc
> Doing interesting things with small computers since 1979
>
>

There's another list I should CC to? Or does the section maintainer
do that when they're happy with the patch?

2007-08-28 16:15:59

[permalink] [raw]

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

Michael Evans wrote:
> On 8/28/07, Bill Davidsen <[email protected]> wrote:
>> Michael Evans wrote:
>>> Oh, I see. I forgot about the changelogs. I'd send out version 5
>>> now, but I'm not sure what kernel version to make the patch against.
>>> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
>>> Additionally I never could tell what git tree was the 'mainline' as it
>>> isn't labeled with such a keyword (at least in the list of git trees I
>>> saw).
>>
>> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
>> in an "-mm" kernel. That's probably desirable, anyway.
>>
>> --
>
> There's another list I should CC to? Or does the section maintainer
> do that when they're happy with the patch?

Not another list; just cc: [email protected] so that he can merge
it into the -mm kernel patches for testing. Most patches cook in the -mm
kernel(s) before they are merged into mainline.

Then generally the subsystem maintainer(s) are responsible for sending
patches on to Linus for merging into mainline, if/when they are happy
with the patch and they think that it has been tested enough.

Mainline progresses (roughly, e.g.) like so:

2.6.22
then patches are applied and we get (possibly) daily snapshots from git, like
2.6.22-git1, 2.6.22-git2, ...

After the primary 2-week merge window for new code (i.e., not just bug fixes),
we get 2.6.23-rc1.
Then as more patches are merged, we get 2.6.23-rc1-gitN...

Then 2.6.23-rc2 ... 2.6.23-rc2-gitN...

2.6.23-rc3 ... 2.6.23-rc3-gitN...

blah blah blah

You can use git to download and track the mainline kernel changes,
or you can download tarballs + patches (patches for each -rc as well as
for each -gitN snapshot) and apply the patches yourself or use a
script to do it. (like 'ketchup': http://www.selenic.com/ketchup/wiki/;
or like http://www.xenotime.net/linux/scripts/grab_kernel)

In general, patches should be made against latest/recent kernels, either
current git (cloned or updated via pull), or against recent patches
+ git snapshot if there is a git snapshot that is applicable.
Immediately after 2.6.23-rcN, there is no applicable git snapshot to
be applied (well, a few hours later there may be).

Sometimes it is appropriate to make patches against the -mm kernel
patchset, but only if the patch is for code that is only in that
kernel patchset (or if Andrew asks for a patch to be updated against
-mm). [This is a mostly rough generalization.] Andrew normally
takes patches against mainline and coerces them into -mm if coercion
is needed.

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

2007-08-28 17:10:23

[permalink] [raw]

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

On 8/28/07, Randy Dunlap <[email protected]> wrote:
> Michael Evans wrote:
> > On 8/28/07, Bill Davidsen <[email protected]> wrote:
> >> Michael Evans wrote:
> >>> Oh, I see. I forgot about the changelogs. I'd send out version 5
> >>> now, but I'm not sure what kernel version to make the patch against.
> >>> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
> >>> Additionally I never could tell what git tree was the 'mainline' as it
> >>> isn't labeled with such a keyword (at least in the list of git trees I
> >>> saw).
> >>
> >> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
> >> in an "-mm" kernel. That's probably desirable, anyway.
> >>
> >> --
> >
> > There's another list I should CC to? Or does the section maintainer
> > do that when they're happy with the patch?
>
> Not another list; just cc: [email protected] so that he can merge
> it into the -mm kernel patches for testing. Most patches cook in the -mm
> kernel(s) before they are merged into mainline.
>
> Then generally the subsystem maintainer(s) are responsible for sending
> patches on to Linus for merging into mainline, if/when they are happy
> with the patch and they think that it has been tested enough.
>

So I should look at the git documentation again, try to pull down
Andrew's latest -mm, and see what I need to change (if anything) to
patch against it? (I would probably only verify that boots once my
self given the other testing this has already seen and how often mm
breaks things my mythtv box likes, such as nvidia-drivers, etc,)

> Mainline progresses (roughly, e.g.) like so:
>

I knew about the progression order (but not timing) of the non-git
side of it, but it was helpful to have the whole thing spelled out,
thank you.

2007-08-28 17:23:43

[permalink] [raw]

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

Michael Evans wrote:
> On 8/28/07, Randy Dunlap <[email protected]> wrote:
>> Michael Evans wrote:
>>> On 8/28/07, Bill Davidsen <[email protected]> wrote:
>>>> Michael Evans wrote:
>>>>> Oh, I see. I forgot about the changelogs. I'd send out version 5
>>>>> now, but I'm not sure what kernel version to make the patch against.
>>>>> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
>>>>> Additionally I never could tell what git tree was the 'mainline' as it
>>>>> isn't labeled with such a keyword (at least in the list of git trees I
>>>>> saw).
>>>> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
>>>> in an "-mm" kernel. That's probably desirable, anyway.
>>>>
>>>> --
>>> There's another list I should CC to? Or does the section maintainer
>>> do that when they're happy with the patch?
>> Not another list; just cc: [email protected] so that he can merge
>> it into the -mm kernel patches for testing. Most patches cook in the -mm
>> kernel(s) before they are merged into mainline.
>>
>> Then generally the subsystem maintainer(s) are responsible for sending
>> patches on to Linus for merging into mainline, if/when they are happy
>> with the patch and they think that it has been tested enough.
>>
>
> So I should look at the git documentation again, try to pull down
> Andrew's latest -mm, and see what I need to change (if anything) to
> patch against it? (I would probably only verify that boots once my
> self given the other testing this has already seen and how often mm
> breaks things my mythtv box likes, such as nvidia-drivers, etc,)

-mm is available only as a patchset, not via git.
There has been a git version of it, but it's not currently working and
hasn't been for 2 months or so.

I mostly use the grab_kernel script that I mentioned. It knows how
to download linux-2.6.M and how to apply -rcN, -gitN, and/or -mmN
patches to the base (linux-2.6.M).

E.g.:
grab_kernel 2.6.23-rc3-git6 $PWD
or
grab_kernel 2.6.23-rc3-mm1 $PWD

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

2007-08-28 19:27:23

[permalink] [raw]

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

On 8/28/07, Randy Dunlap <[email protected]> wrote:
> Michael Evans wrote:
> > On 8/28/07, Randy Dunlap <[email protected]> wrote:
> >> Michael Evans wrote:
> >>> On 8/28/07, Bill Davidsen <[email protected]> wrote:
> >>>> Michael Evans wrote:
> >>>>> Oh, I see. I forgot about the changelogs. I'd send out version 5
> >>>>> now, but I'm not sure what kernel version to make the patch against.
> >>>>> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
> >>>>> Additionally I never could tell what git tree was the 'mainline' as it
> >>>>> isn't labeled with such a keyword (at least in the list of git trees I
> >>>>> saw).
> >>>> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
> >>>> in an "-mm" kernel. That's probably desirable, anyway.
> >>>>
> >>>> --
> >>> There's another list I should CC to? Or does the section maintainer
> >>> do that when they're happy with the patch?
> >> Not another list; just cc: [email protected] so that he can merge
> >> it into the -mm kernel patches for testing. Most patches cook in the -mm
> >> kernel(s) before they are merged into mainline.
> >>
> >> Then generally the subsystem maintainer(s) are responsible for sending
> >> patches on to Linus for merging into mainline, if/when they are happy
> >> with the patch and they think that it has been tested enough.
> >>
> >
> > So I should look at the git documentation again, try to pull down
> > Andrew's latest -mm, and see what I need to change (if anything) to
> > patch against it? (I would probably only verify that boots once my
> > self given the other testing this has already seen and how often mm
> > breaks things my mythtv box likes, such as nvidia-drivers, etc,)
>
> -mm is available only as a patchset, not via git.
> There has been a git version of it, but it's not currently working and
> hasn't been for 2 months or so.
>
> I mostly use the grab_kernel script that I mentioned. It knows how
> to download linux-2.6.M and how to apply -rcN, -gitN, and/or -mmN
> patches to the base (linux-2.6.M).
>
> E.g.:
> grab_kernel 2.6.23-rc3-git6 $PWD
> or
> grab_kernel 2.6.23-rc3-mm1 $PWD
>
>
> --
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>

Gentoo's mm-sources is version 2.6.23-rc3-mm1, which I'd guess is
close enough for an area like this. Given that I didn't need to make
any changes to the patch should I just submit it to the mm for testing
without rebooting in to an mm-sources kernel? I need to schedule when
I do that so it would take me overnight to do so.

2007-08-28 19:31:32

[permalink] [raw]

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

Michael Evans wrote:
> On 8/28/07, Randy Dunlap <[email protected]> wrote:
>> Michael Evans wrote:
>>> On 8/28/07, Randy Dunlap <[email protected]> wrote:
>>>> Michael Evans wrote:
>>>>> On 8/28/07, Bill Davidsen <[email protected]> wrote:
>>>>>> Michael Evans wrote:
>>>>>>> Oh, I see. I forgot about the changelogs. I'd send out version 5
>>>>>>> now, but I'm not sure what kernel version to make the patch against.
>>>>>>> 2.6.23-rc4 is on kernel.org and I don't see any git snapshots.
>>>>>>> Additionally I never could tell what git tree was the 'mainline' as it
>>>>>>> isn't labeled with such a keyword (at least in the list of git trees I
>>>>>>> saw).
>>>>>> I suspect you wait for 2.5.23 release, or send it to AKPM for inclusion
>>>>>> in an "-mm" kernel. That's probably desirable, anyway.
>
> Gentoo's mm-sources is version 2.6.23-rc3-mm1, which I'd guess is
> close enough for an area like this. Given that I didn't need to make
> any changes to the patch should I just submit it to the mm for testing
> without rebooting in to an mm-sources kernel? I need to schedule when
> I do that so it would take me overnight to do so.

Yes, I think that you can go ahead with that, but it's up to the
subsystem maintainers where/when it goes from there.

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

2007-08-29 08:07:15

[permalink] [raw]

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

From: Michael J. Evans <[email protected]>

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 <[email protected]>
---
CCing [email protected]

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: [email protected]
P: Neil Brown
M: [email protected]
L: [email protected]
S: Supported
Unless you have a reason NOT to do so, CC [email protected].

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 <[email protected]>
=============================================================
--- 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);
}

2007-08-29 15:21:06