let md auto-detect 128+ raid members, fix potential race condition

let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

I accidentally ran into the 128-devices limit in md.c's
detected_devices.  It doesn't seem like a high-enough limit, and I
don't quite see why we wouldn't use a list for this.

Besides, there appears to be a race condition in it:

         detected_devices[dev_cnt++] = dev;

won't atomically increment dev_cnt and use its previous value, unless
there's something up the call stack that guarantees mutual exclusion.
I don't see that this is the case.

Previously devices that exceeded the array would be silently
discarded.  Now we'll only discard them if we run out of memory, and
we'll report so if we do.

Before I wrap up, a question on style: does it make sense to use
kzmalloc to allocate this newly-created data structure that contains
only a list_head and a dev_t, where the latter is immediately copied
from another dev_t variable, and then the whole thing is added to a
list?  I.e., could any list-checking present or future feature rely on
list_head fields that might hurt if not zero-initialized, or would it
be future-proof to just use kmalloc in this case?

This was tested on an Athlon64 notebook with 2 disks on RAID (internal
and external USB) with the latest Fedora development kernel plus this
patch.

	Signed-off-by: Alexandre Oliva <oliva@lsd.ic.unicamp.br>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: raid-detected-list.patch --]
[-- Type: text/x-patch, Size: 3393 bytes --]

Index: kernel-2.6.17-1.2462.fc6/drivers/md/md.c
===================================================================
--- kernel-2.6.17-1.2462.fc6.orig/drivers/md/md.c	2006-07-30 01:03:28.000000000 -0300
+++ kernel-2.6.17-1.2462.fc6/drivers/md/md.c	2006-07-30 03:40:54.000000000 -0300
@@ -1435,7 +1435,7 @@ static void unlock_rdev(mdk_rdev_t *rdev
 	blkdev_put_partition(bdev);
 }
 
-void md_autodetect_dev(dev_t dev);
+int md_register_autodetect_dev(dev_t dev);
 
 static void export_rdev(mdk_rdev_t * rdev)
 {
@@ -1447,7 +1447,8 @@ static void export_rdev(mdk_rdev_t * rde
 	free_disk_sb(rdev);
 	list_del_init(&rdev->same_set);
 #ifndef MODULE
-	md_autodetect_dev(rdev->bdev->bd_dev);
+	if (md_register_autodetect_dev(rdev->bdev->bd_dev))
+		printk(KERN_ERR "md: out of memory re-registering %s\n", b);
 #endif
 	unlock_rdev(rdev);
 	kobject_put(&rdev->kobj);
@@ -5575,27 +5576,40 @@ static int __init md_init(void)
  * Searches all registered partitions for autorun RAID arrays
  * at boot time.
  */
-static dev_t detected_devices[128];
-static int dev_cnt;
+static LIST_HEAD(detected_devices);
+static DEFINE_SPINLOCK(detected_devices_lock);
 
-void md_autodetect_dev(dev_t dev)
+struct detected_dev_list_t {
+	struct list_head list;
+	dev_t dev;
+};
+
+int md_register_autodetect_dev(dev_t dev)
 {
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	struct detected_dev_list_t *ldev = kzalloc(sizeof (*ldev), GFP_KERNEL);
+
+	if (!ldev)
+	  return -1;
+
+	ldev->dev = dev;
+
+	spin_lock(&detected_devices_lock);
+	list_add_tail(&ldev->list, &detected_devices);
+	spin_unlock(&detected_devices_lock);
+
+	return 0;
 }
 
 
 static void autostart_arrays(int part)
 {
-	mdk_rdev_t *rdev;
-	int i;
+	struct detected_dev_list_t *ldev, *next;
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
-
-		rdev = md_import_device(dev,0, 0);
+	spin_lock(&detected_devices_lock);
+	list_for_each_entry_safe(ldev, next, &detected_devices, list) {
+		mdk_rdev_t *rdev = md_import_device(ldev->dev, 0, 0);
 		if (IS_ERR(rdev))
 			continue;
 
@@ -5604,8 +5618,11 @@ static void autostart_arrays(int part)
 			continue;
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
+		list_del(&ldev->list);
+		kfree(ldev);
 	}
-	dev_cnt = 0;
+	BUG_ON(!list_empty(&detected_devices));
+	spin_unlock(&detected_devices_lock);
 
 	autorun_devices(part);
 }
Index: kernel-2.6.17-1.2462.fc6/fs/partitions/check.c
===================================================================
--- kernel-2.6.17-1.2462.fc6.orig/fs/partitions/check.c	2006-07-30 01:03:34.000000000 -0300
+++ kernel-2.6.17-1.2462.fc6/fs/partitions/check.c	2006-07-30 03:41:02.000000000 -0300
@@ -36,7 +36,7 @@
 #include "karma.h"
 
 #ifdef CONFIG_BLK_DEV_MD
-extern void md_autodetect_dev(dev_t dev);
+extern int md_register_autodetect_dev(dev_t dev);
 #endif
 
 int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/
@@ -471,8 +471,10 @@ int rescan_partitions(struct gendisk *di
 		}
 		add_partition(disk, p, from, size);
 #ifdef CONFIG_BLK_DEV_MD
-		if (state->parts[p].flags)
-			md_autodetect_dev(bdev->bd_dev+p);
+		if (state->parts[p].flags
+		    && md_register_autodetect_dev(bdev->bd_dev+p))
+			printk(KERN_ERR "md: out of memory registering %s%d\n",
+			       disk->disk_name, p);
 #endif
 	}
 	kfree(state);

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Andrew Morton]

On Sun, 30 Jul 2006 03:56:21 -0300
Alexandre Oliva <aoliva@redhat.com> wrote:

> I accidentally ran into the 128-devices limit in md.c's
> detected_devices.  It doesn't seem like a high-enough limit, and I
> don't quite see why we wouldn't use a list for this.
> 
> Besides, there appears to be a race condition in it:
> 
>          detected_devices[dev_cnt++] = dev;
> 
> won't atomically increment dev_cnt and use its previous value, unless
> there's something up the call stack that guarantees mutual exclusion.
> I don't see that this is the case.
> 
> Previously devices that exceeded the array would be silently
> discarded.  Now we'll only discard them if we run out of memory, and
> we'll report so if we do.

Neil cc'ed.

> Before I wrap up, a question on style: does it make sense to use
> kzmalloc to allocate this newly-created data structure that contains
> only a list_head and a dev_t, where the latter is immediately copied
> from another dev_t variable, and then the whole thing is added to a
> list?  I.e., could any list-checking present or future feature rely on
> list_head fields that might hurt if not zero-initialized, or would it
> be future-proof to just use kmalloc in this case?

I'd say that if all fields are going to be initialised, kzalloc() shouldn't
be used.

nits:

Index: kernel-2.6.17-1.2462.fc6/drivers/md/md.c
> ===================================================================
> --- kernel-2.6.17-1.2462.fc6.orig/drivers/md/md.c	2006-07-30 01:03:28.000000000 -0300
> +++ kernel-2.6.17-1.2462.fc6/drivers/md/md.c	2006-07-30 03:40:54.000000000 -0300
> @@ -1435,7 +1435,7 @@ static void unlock_rdev(mdk_rdev_t *rdev
>  	blkdev_put_partition(bdev);
>  }
>  
> -void md_autodetect_dev(dev_t dev);
> +int md_register_autodetect_dev(dev_t dev);

Put it in a header file, please.
 
> +int md_register_autodetect_dev(dev_t dev)
>  {
> -	if (dev_cnt >= 0 && dev_cnt < 127)
> -		detected_devices[dev_cnt++] = dev;
> +	struct detected_dev_list_t *ldev = kzalloc(sizeof (*ldev), GFP_KERNEL);
> +
> +	if (!ldev)
> +	  return -1;

whitespace broke.

>  }
> Index: kernel-2.6.17-1.2462.fc6/fs/partitions/check.c
> ===================================================================
> --- kernel-2.6.17-1.2462.fc6.orig/fs/partitions/check.c	2006-07-30 01:03:34.000000000 -0300
> +++ kernel-2.6.17-1.2462.fc6/fs/partitions/check.c	2006-07-30 03:41:02.000000000 -0300
> @@ -36,7 +36,7 @@
>  #include "karma.h"
>  
>  #ifdef CONFIG_BLK_DEV_MD
> -extern void md_autodetect_dev(dev_t dev);
> +extern int md_register_autodetect_dev(dev_t dev);
>  #endif

Again, let's find a header for this.

>  int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/
> @@ -471,8 +471,10 @@ int rescan_partitions(struct gendisk *di
>  		}
>  		add_partition(disk, p, from, size);
>  #ifdef CONFIG_BLK_DEV_MD
> -		if (state->parts[p].flags)
> -			md_autodetect_dev(bdev->bd_dev+p);
> +		if (state->parts[p].flags
> +		    && md_register_autodetect_dev(bdev->bd_dev+p))
> +			printk(KERN_ERR "md: out of memory registering %s%d\n",
> +			       disk->disk_name, p);
>  #endif

What happens if CONFIG_BLK_DEV_MD=m?

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

On Jul 30, 2006, Andrew Morton <akpm@osdl.org> wrote:

> On Sun, 30 Jul 2006 03:56:21 -0300
> Alexandre Oliva <aoliva@redhat.com> wrote:

>> -void md_autodetect_dev(dev_t dev);
>> +int md_register_autodetect_dev(dev_t dev);

> Put it in a header file, please.
 
AFAICT it really isn't supposed to be used elsewhere.  I suppose I
could add it to either blkdev.h, fs.h or raid/md.h, since it's more of
glue code between two modules than something that belongs to one
specific module.  E.g., if it goes in raid/md.h, where it feels the
most appropriate, then fs/parititions/check.c has to include it, which
doesn't sound right.  OTOH, if it goes in blkdev.h or fs.h, then a lot
of code ends up seeing the declaration that shouldn't be available.
Thoughts?

Maybe we could replace this with some register/unregister notifier
interface, such that add_partitions() could then notify multiple
watchers when a new partition is configured.  This would remove the
backwards dependency here, but I feel it should be done in a separate
patch.  I don't mind if they're integrated at once, but I don't feel
that changing two unrelated issues at once is a good approach.

>> +	if (!ldev)
>> +	  return -1;

> whitespace broke.

Oops, thanks, fixed below.

>> #ifdef CONFIG_BLK_DEV_MD
>> -		if (state->parts[p].flags)
>> -			md_autodetect_dev(bdev->bd_dev+p);
>> +		if (state->parts[p].flags
>> +		    && md_register_autodetect_dev(bdev->bd_dev+p))
>> +			printk(KERN_ERR "md: out of memory registering %s%d\n",
>> +			       disk->disk_name, p);
>> #endif

> What happens if CONFIG_BLK_DEV_MD=m?

AFAIK then you'd get a link failure.  One more reason to go with the
notifier approach, I guess.  It wouldn't quite enable md to
auto-detect from partitions set up before the module was loaded, but
it would at least remove this presumed link error.

Another approach would be to split the autodetect stuff out of md.c
into a separate file that goes in the main kernel image (if
CONFIG_MD=y, it's never m) even if CONFIG_BLK_DEV_MD=m.  Would this be
a desirable arrangement?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: raid-detected-list.patch --]
[-- Type: text/x-patch, Size: 3393 bytes --]

Index: kernel-2.6.17-1.2462.fc6/drivers/md/md.c
===================================================================
--- kernel-2.6.17-1.2462.fc6.orig/drivers/md/md.c	2006-07-30 01:03:28.000000000 -0300
+++ kernel-2.6.17-1.2462.fc6/drivers/md/md.c	2006-07-30 03:40:54.000000000 -0300
@@ -1435,7 +1435,7 @@ static void unlock_rdev(mdk_rdev_t *rdev
 	blkdev_put_partition(bdev);
 }
 
-void md_autodetect_dev(dev_t dev);
+int md_register_autodetect_dev(dev_t dev);
 
 static void export_rdev(mdk_rdev_t * rdev)
 {
@@ -1447,7 +1447,8 @@ static void export_rdev(mdk_rdev_t * rde
 	free_disk_sb(rdev);
 	list_del_init(&rdev->same_set);
 #ifndef MODULE
-	md_autodetect_dev(rdev->bdev->bd_dev);
+	if (md_register_autodetect_dev(rdev->bdev->bd_dev))
+		printk(KERN_ERR "md: out of memory re-registering %s\n", b);
 #endif
 	unlock_rdev(rdev);
 	kobject_put(&rdev->kobj);
@@ -5575,27 +5576,40 @@ static int __init md_init(void)
  * Searches all registered partitions for autorun RAID arrays
  * at boot time.
  */
-static dev_t detected_devices[128];
-static int dev_cnt;
+static LIST_HEAD(detected_devices);
+static DEFINE_SPINLOCK(detected_devices_lock);
 
-void md_autodetect_dev(dev_t dev)
+struct detected_dev_list_t {
+	struct list_head list;
+	dev_t dev;
+};
+
+int md_register_autodetect_dev(dev_t dev)
 {
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	struct detected_dev_list_t *ldev = kzalloc(sizeof (*ldev), GFP_KERNEL);
+
+	if (!ldev)
+	  return -1;
+
+	ldev->dev = dev;
+
+	spin_lock(&detected_devices_lock);
+	list_add_tail(&ldev->list, &detected_devices);
+	spin_unlock(&detected_devices_lock);
+
+	return 0;
 }
 
 
 static void autostart_arrays(int part)
 {
-	mdk_rdev_t *rdev;
-	int i;
+	struct detected_dev_list_t *ldev, *next;
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
-
-		rdev = md_import_device(dev,0, 0);
+	spin_lock(&detected_devices_lock);
+	list_for_each_entry_safe(ldev, next, &detected_devices, list) {
+		mdk_rdev_t *rdev = md_import_device(ldev->dev, 0, 0);
 		if (IS_ERR(rdev))
 			continue;
 
@@ -5604,8 +5618,11 @@ static void autostart_arrays(int part)
 			continue;
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
+		list_del(&ldev->list);
+		kfree(ldev);
 	}
-	dev_cnt = 0;
+	BUG_ON(!list_empty(&detected_devices));
+	spin_unlock(&detected_devices_lock);
 
 	autorun_devices(part);
 }
Index: kernel-2.6.17-1.2462.fc6/fs/partitions/check.c
===================================================================
--- kernel-2.6.17-1.2462.fc6.orig/fs/partitions/check.c	2006-07-30 01:03:34.000000000 -0300
+++ kernel-2.6.17-1.2462.fc6/fs/partitions/check.c	2006-07-30 03:41:02.000000000 -0300
@@ -36,7 +36,7 @@
 #include "karma.h"
 
 #ifdef CONFIG_BLK_DEV_MD
-extern void md_autodetect_dev(dev_t dev);
+extern int md_register_autodetect_dev(dev_t dev);
 #endif
 
 int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/
@@ -471,8 +471,10 @@ int rescan_partitions(struct gendisk *di
 		}
 		add_partition(disk, p, from, size);
 #ifdef CONFIG_BLK_DEV_MD
-		if (state->parts[p].flags)
-			md_autodetect_dev(bdev->bd_dev+p);
+		if (state->parts[p].flags
+		    && md_register_autodetect_dev(bdev->bd_dev+p))
+			printk(KERN_ERR "md: out of memory registering %s%d\n",
+			       disk->disk_name, p);
 #endif
 	}
 	kfree(state);

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Andrew Morton]

On Sun, 30 Jul 2006 17:56:31 -0300
Alexandre Oliva <aoliva@redhat.com> wrote:

> On Jul 30, 2006, Andrew Morton <akpm@osdl.org> wrote:
> 
> > On Sun, 30 Jul 2006 03:56:21 -0300
> > Alexandre Oliva <aoliva@redhat.com> wrote:
> 
> >> -void md_autodetect_dev(dev_t dev);
> >> +int md_register_autodetect_dev(dev_t dev);
> 
> > Put it in a header file, please.
>  
> AFAICT it really isn't supposed to be used elsewhere.  I suppose I
> could add it to either blkdev.h, fs.h or raid/md.h, since it's more of
> glue code between two modules than something that belongs to one
> specific module.  E.g., if it goes in raid/md.h, where it feels the
> most appropriate, then fs/parititions/check.c has to include it, which
> doesn't sound right.  OTOH, if it goes in blkdev.h or fs.h, then a lot
> of code ends up seeing the declaration that shouldn't be available.
> Thoughts?

If the function is exported by md then md.h would be an appropriate place
for the declaration.

> Maybe we could replace this with some register/unregister notifier
> interface, such that add_partitions() could then notify multiple
> watchers when a new partition is configured.  This would remove the
> backwards dependency here, but I feel it should be done in a separate
> patch.  I don't mind if they're integrated at once, but I don't feel
> that changing two unrelated issues at once is a good approach.

If we went that way then patch #1 would be "add a notifier" and patch #2
would be "use it in md".

Do we anticipate that there would ever be other users of this notifier
callback API?

> >> #ifdef CONFIG_BLK_DEV_MD
> >> -		if (state->parts[p].flags)
> >> -			md_autodetect_dev(bdev->bd_dev+p);
> >> +		if (state->parts[p].flags
> >> +		    && md_register_autodetect_dev(bdev->bd_dev+p))
> >> +			printk(KERN_ERR "md: out of memory registering %s%d\n",
> >> +			       disk->disk_name, p);
> >> #endif
> 
> > What happens if CONFIG_BLK_DEV_MD=m?
> 
> AFAIK then you'd get a link failure.  One more reason to go with the
> notifier approach, I guess.  It wouldn't quite enable md to
> auto-detect from partitions set up before the module was loaded, but
> it would at least remove this presumed link error.
> 
> Another approach would be to split the autodetect stuff out of md.c
> into a separate file that goes in the main kernel image (if
> CONFIG_MD=y, it's never m) even if CONFIG_BLK_DEV_MD=m.  Would this be
> a desirable arrangement?

I guess it sounds logical, but I'm not sure what the end result would look
like.  Which amounts to noncommittal Sunday afternoon waffling ;)

The notifier certainly makes sense if we anticipate other users of it.  If
we think it'll always be an md-special thing then yeah, I guess we can code
it that way.

Re: let md auto-detect 128+ raid members, fix potential race condition

[Neil Brown]

[linux-raid added to cc.
 Background: patch was submitted to remove the current hard limit
 of 127 partitions that can be auto-detected - limit set by 
 'detected_devices array in md.c.
]

My first inclination is not to fix this problem.

I consider md auto-detect to be a legacy feature.
I don't use it and I recommend that other people don't use it.
However I cannot justify removing it, so it stays there.
Having this limitation could be seen as a good motivation for some
more users to stop using it.

Why not use auto-detect?
I have three issues with it.

 1/
    It just isn't "right".  We don't mount filesystems from partitions
    just because they have type 'Linux'.  We don't enable swap on
    partitions just because they have type 'Linux swap'.  So why do we
    assemble md/raid from partitions that have type 'Linux raid
    autodetect'? 

 2/ 
    It can cause problems when moving devices.  If you have two
    machines, both with an 'md0' array and you move the drives from one
    on to the other - say because the first lost a powersupply - and
    then reboot the machine that received the drives, which array gets
    assembled as 'md0' ?? You might be lucky, you might not. This
    isn't purely theoretical - there have been pleas for help on
    linux-raid resulting from exactly this - though they have been
    few. 

 3/ 
    The information redundancy can cause a problem when it gets out of
    sync.  i.e. you add a partition to a raid array without setting
    the partition type to 'fd'.  This works, but on the next reboot
    the partition doesn't get added back into the array and you have
    to manually add it yourself.
    This too is not purely theory - it has been reported slightly more
    often than '2'.

So my preferred solution to the problem is to tell people not to use
autodetect.  Quite possibly this should be documented in the code, and
maybe even have a KERN_INFO message if more than 64 devices are
autodetected. 


Now one doesn't always go with one's first inclination so I should
discuss the approaches to fixing the problem, should a fix really be
the right thing to do.

The idea of having a generic notifier is, I think, missing the point -
so I'd better explain what the point is.....

The kernel already has the hotplug mechanism for alerting user-space
about new partitions, and I'm sure kernel clients could hook into this
some how.  But even if they could, md wouldn't want to.
md doesn't really want to know about new partitions.  It simply wants a
list of all partitions which are of type 'autodetect'.   Getting a
list of all partitions is quite easy (/proc/partitions does it, so it
cannot be hard).  But 'struct hd_struct' doesn't record what the
partition type is at all.  It spare bits, so it could. (->policy is
currently a 'ro' flag.  A little work would allow multiple flags there).

The point of the current detected_devices array is precisely to find
partitions which have this flag set.

If we were to 'fix' this problem, I think the cleanest approach (which
I haven't actually coded, so it might not work...) would be to define
a new flag to go in hd_struct->policy to say if the partition type
suggested auto-detect, and get partitions/check.c to set this.
Then have md iterate all partitions looking for this flag.

That could be considered to be intrusive to bits of the kernel which
don't need to be intruded in to, and could run the risk of someone
wanting to expose that flag to user-space (with in /proc/partitions or
/sys) and I would be against that.  But otherwise it would be a
fairly clean approach.

The minimal (non-empty) approach of replacing the array by a linked
list as the original patch did would also be quite reasonable.  Maybe
not as clean as a flag in hd_struct, but maybe we don't need this
code to be particularly clean(?).

So:  Do you *really* need to *fix* this, or can you just use 'mdadm'
to assemble you arrays instead?

NeilBrown

Re: let md auto-detect 128+ raid members, fix potential race condition

[Helge Hafting]

On Mon, Jul 31, 2006 at 09:20:58AM +1000, Neil Brown wrote:
> 
> My first inclination is not to fix this problem.
> 
> I consider md auto-detect to be a legacy feature.
> I don't use it and I recommend that other people don't use it.
> However I cannot justify removing it, so it stays there.
> Having this limitation could be seen as a good motivation for some
> more users to stop using it.
> 
> Why not use auto-detect?
[Arguments deleted]

Well, if autodetection is removed, what is then the preferred
way of booting off a raid-1 device?

Kernel parameters?

An initrd with mdadm just for this?  Some people want to do even
partition detection from initrd, in order to have a smaller
kernel.  We aren't there yet though.

Autotetect is nice from an administrator viewpoint - compile
it in and it "just works".  The trouble when you connect
an array from some other machine is to be expected, but 
that isn't exactly everyday stuff.  

Helge Hafting

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Jul 30, 2006, Neil Brown <neilb@suse.de> wrote:

>  1/
>     It just isn't "right".  We don't mount filesystems from partitions
>     just because they have type 'Linux'.  We don't enable swap on
>     partitions just because they have type 'Linux swap'.  So why do we
>     assemble md/raid from partitions that have type 'Linux raid
>     autodetect'? 

Similar reason to why vgscan finds and attempts to use any partitions
that have the appropriate type/signature (difference being that raid
auto-detect looks at the actual partition type, whereas vgscan looks
at the actual data, just like mdadm, IIRC): when you have to bootstrap
from an initrd, you don't want to be forced to have the correct data
in the initrd image, since then any reconfiguration requires the info
to be introduced in the initrd image before the machine goes down.
Sometimes, especially in case of disk failures, you just can't do
that.

>  2/ 
>     It can cause problems when moving devices.

It can, indeed, and it has caused such problems to me before, but
they're the exception, not the rule, and one should optimize for the
rule, not the exception.

>  3/ 
>     The information redundancy can cause a problem when it gets out of
>     sync.  i.e. you add a partition to a raid array without setting
>     the partition type to 'fd'.  This works, but on the next reboot
>     the partition doesn't get added back into the array and you have
>     to manually add it yourself.
>     This too is not purely theory - it has been reported slightly more
>     often than '2'.

This has happened to me as well, and I remember it was extremely
confusing when it first happened :-)  But that's an argument to change
the behavior so as to look for the superblock instead of trusting the
partition type, not an argument to remove the auto-detection feature.

And then, the reliance on partition type has been useful at times as
well, when I explicitly did *not* want a certain raid device or raid
member to be brought up on boot.

> So my preferred solution to the problem is to tell people not to use
> autodetect.  Quite possibly this should be documented in the code, and
> maybe even have a KERN_INFO message if more than 64 devices are
> autodetected. 

I wouldn't have a problem with that, since then distros would probably
switch to a more recommended mechanism that works just as well, i.e.,
ideally without requiring initrd-regeneration after reconfigurations
such as adding one more raid device to the logical volume group
containing the root filesystem.

> If we were to 'fix' this problem, I think the cleanest approach (which
> I haven't actually coded, so it might not work...) would be to define
> a new flag to go in hd_struct->policy to say if the partition type
> suggested auto-detect, and get partitions/check.c to set this.
> Then have md iterate all partitions looking for this flag.

AFAICT we'd still need a list or an array, since we add stuff back to
the list in various situations.

> So:  Do you *really* need to *fix* this, or can you just use 'mdadm'
> to assemble you arrays instead?

I'm not sure.  I'd expect not to need it, but the limited feature
currently in place, that initrd uses to bring up the raid1 devices
containing the physical volumes that form the volume group where the
logical volume with my root filesystem is also brings up various raid6
physical volumes that form an unrelated volume group, and it does so
in such a way that the last of them, containing the 128th fd-type
partition in the box, ends up being left out, so the raid device it's
a member of is brought up either degraded or missing the spare member,
none of which are good.

I don't know that I can easily get initrd to replace nash's
raidautorun for mdadm unless mdadm has a mode to bring up any arrays
it can find, as opposed to bringing up a specific array out of a given
list of members or scanning for members.  Either way, this won't fix
the problem 2) that you mentioned, but requiring initrd-regeneration
after extending the volume group containing the root device is another
problem that the current modes of operation of mdadm AFAIK won't
contemplate, so switching to it will trade one problem for another,
and the latter is IMHO more common than the former.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[David Greaves]

Alexandre Oliva wrote:
> On Jul 30, 2006, Neil Brown <neilb@suse.de> wrote:
> 
>>  1/
>>     It just isn't "right".  We don't mount filesystems from partitions
>>     just because they have type 'Linux'.  We don't enable swap on
>>     partitions just because they have type 'Linux swap'.  So why do we
>>     assemble md/raid from partitions that have type 'Linux raid
>>     autodetect'? 
> 
> Similar reason to why vgscan finds and attempts to use any partitions
> that have the appropriate type/signature (difference being that raid
> auto-detect looks at the actual partition type, whereas vgscan looks
> at the actual data, just like mdadm, IIRC): when you have to bootstrap
> from an initrd, you don't want to be forced to have the correct data
> in the initrd image, since then any reconfiguration requires the info
> to be introduced in the initrd image before the machine goes down.
> Sometimes, especially in case of disk failures, you just can't do
> that.
> 
This debate is not about generic autodetection - a good thing (tm) - but
 in-kernel vs userspace autodetection.

Your example supports Neil's case - the proposal is to use initrd to run
mdadm which thne (kinda) does what vgscan does.


> 
>> So my preferred solution to the problem is to tell people not to use
(in kernel)
>> autodetect.  Quite possibly this should be documented in the code, and
>> maybe even have a KERN_INFO message if more than 64 devices are
>> autodetected. 
> 
> I wouldn't have a problem with that, since then distros would probably
> switch to a more recommended mechanism that works just as well, i.e.,
> ideally without requiring initrd-regeneration after reconfigurations
> such as adding one more raid device to the logical volume group
> containing the root filesystem.
That's supported in today's mdadm.

look at --uuid and --name

>> So:  Do you *really* need to *fix* this, or can you just use 'mdadm'
>> to assemble you arrays instead?
> 
> I'm not sure.  I'd expect not to need it, but the limited feature
> currently in place, that initrd uses to bring up the raid1 devices
> containing the physical volumes that form the volume group where the
> logical volume with my root filesystem is also brings up various raid6
> physical volumes that form an unrelated volume group, and it does so
> in such a way that the last of them, containing the 128th fd-type
> partition in the box, ends up being left out, so the raid device it's
> a member of is brought up either degraded or missing the spare member,
> none of which are good.
> 
> I don't know that I can easily get initrd to replace nash's
> raidautorun for mdadm unless mdadm has a mode to bring up any arrays
> it can find, as opposed to bringing up a specific array out of a given
> list of members or scanning for members.  Either way, this won't fix
> the problem 2) that you mentioned, but requiring initrd-regeneration
> after extending the volume group containing the root device is another
> problem that the current modes of operation of mdadm AFAIK won't
> contemplate, so switching to it will trade one problem for another,
> and the latter is IMHO more common than the former.
> 

I think you should name your raid1 (maybe "hostname-root") and use
initrd to bring it up by --name using:
 mdadm --assemble --scan --config partitions --name hostname-root


It could also, later in the boot process, bring up "hostname-raid6" by
--name too.
 mdadm --assemble --scan --config partitions --name hostname-raid6

David


-- 

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Jul 31, 2006, David Greaves <david@dgreaves.com> wrote:

> Alexandre Oliva wrote:

>> in the initrd image, since then any reconfiguration requires the info
>> to be introduced in the initrd image before the machine goes down.
>> Sometimes, especially in case of disk failures, you just can't do
>> that.

> Your example supports Neil's case - the proposal is to use initrd to run
> mdadm which thne (kinda) does what vgscan does.

If mdadm can indeed scan all partitions to bring up all raid devices
in them, like nash's raidautorun does, great.  I'll give that a try,
since Neil suggested it should already work in the version of mdadm
that I got here.  I didn't get that impression while skimming through
the man page, but upon closer inspection now I see it's all there.
Oops :-)

>> I wouldn't have a problem with that, since then distros would probably
>> switch to a more recommended mechanism that works just as well, i.e.,
>> ideally without requiring initrd-regeneration after reconfigurations
>> such as adding one more raid device to the logical volume group
>> containing the root filesystem.

> That's supported in today's mdadm.

> look at --uuid and --name

--uuid and --name won't help at all.  I'm talking about adding raid
physical volumes to a volume group, which means new uuid and name, so
whatever already is in initrd won't get it.  Neil's-posted command
line should take care of that though.

Even if the root device doesn't use the newly-added physical volume,
initrd's vgscan needs to find *all* physical volumes in the volume
group, otherwise the volume group will be started in `degraded' mode,
i.e., with the missing physical volumes mapped to a device mapper node
that will produce I/O errors on access IIRC, and everything else
read-only, without any way to switch to read-write when the remaining
devices are made available, which is arguably a missing feature in the
LVM subsystem.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Michael Tokarev]

Alexandre Oliva wrote:
[]
> If mdadm can indeed scan all partitions to bring up all raid devices
> in them, like nash's raidautorun does, great.  I'll give that a try,

Never, ever, try to do that (again).  Mdadm (or vgscan, or whatever)
should NOT assemble ALL arrays found, but only those which it has
been told to assemble.  This is it again: you bring another disk into
a system (disk which comes from another machine), and mdadm finds
FOREIGN arrays and brings them up as /dev/md0, where YOUR root
filesystem should be.  That's what 'homehost' option is for, for
example.

If initrd should be reconfigured after some changes (be it raid
arrays, LVM volumes, hostname, whatever), -- I for one am fine
with that.  Hopefully no one will argue that if you forgot to
install an MBR into your replacement drive, it was entirely your
own fault that your system become unbootable, after all ;)

/mjt

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Aug  1, 2006, Michael Tokarev <mjt@tls.msk.ru> wrote:

> Alexandre Oliva wrote:
> []
>> If mdadm can indeed scan all partitions to bring up all raid devices
>> in them, like nash's raidautorun does, great.  I'll give that a try,

> Never, ever, try to do that (again).  Mdadm (or vgscan, or whatever)
> should NOT assemble ALL arrays found, but only those which it has
> been told to assemble.  This is it again: you bring another disk into
> a system (disk which comes from another machine), and mdadm finds
> FOREIGN arrays and brings them up as /dev/md0, where YOUR root
> filesystem should be.  That's what 'homehost' option is for, for
> example.

Exactly.  So make it /all/all local/, if you must.  It's the same as
far as I'm concerned.

> If initrd should be reconfigured after some changes (be it raid
> arrays, LVM volumes, hostname, whatever), -- I for one am fine
> with that.

Feel free to be fine with it, as long as you also let me be free to
not be fine with it and try to cut a better deal :-)

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Neil Brown]

On Monday July 31, aoliva@redhat.com wrote:
> On Jul 30, 2006, Neil Brown <neilb@suse.de> wrote:
> 
> >  1/
> >     It just isn't "right".  We don't mount filesystems from partitions
> >     just because they have type 'Linux'.  We don't enable swap on
> >     partitions just because they have type 'Linux swap'.  So why do we
> >     assemble md/raid from partitions that have type 'Linux raid
> >     autodetect'? 
> 
> Similar reason to why vgscan finds and attempts to use any partitions
> that have the appropriate type/signature (difference being that raid
> auto-detect looks at the actual partition type, whereas vgscan looks
> at the actual data, just like mdadm, IIRC): when you have to bootstrap
> from an initrd, you don't want to be forced to have the correct data
> in the initrd image, since then any reconfiguration requires the info
> to be introduced in the initrd image before the machine goes down.
> Sometimes, especially in case of disk failures, you just can't do
> that.

The initrd need to 'know' how to find the root filesystem, whether by
devnum or uuid or whatever.
In exactly the same way it needs to know how to find the components
for the root md array - uuid is the best.  There is no need to
reconfigure this in the case of a disk failure.

Current mdadm will assemble arrays for you given only a hostname.  You
still need to get the hostname into the initrd, but that is no
different from a root device number.


> 
> >  2/ 
> >     It can cause problems when moving devices.
> 
> It can, indeed, and it has caused such problems to me before, but
> they're the exception, not the rule, and one should optimize for the
> rule, not the exception.

We aren't talking about optimisation.  We are talking about whether it
actually works or not.  A system that stops booting just because you
plugged a couple of extra drives in is a badly configured system.

> 
> >  3/ 
> >     The information redundancy can cause a problem when it gets out of
> >     sync.  i.e. you add a partition to a raid array without setting
> >     the partition type to 'fd'.  This works, but on the next reboot
> >     the partition doesn't get added back into the array and you have
> >     to manually add it yourself.
> >     This too is not purely theory - it has been reported slightly more
> >     often than '2'.
> 
> This has happened to me as well, and I remember it was extremely
> confusing when it first happened :-)  But that's an argument to change
> the behavior so as to look for the superblock instead of trusting the
> partition type, not an argument to remove the auto-detection
> feature.

As has been said, I don't want to remove auto-detection.  I want to do
it right, and do it from userspace.  It is in-kernel autodetection
that I have no interest in improving.

> 
> And then, the reliance on partition type has been useful at times as
> well, when I explicitly did *not* want a certain raid device or raid
> member to be brought up on boot.

Well, at boot it should only bring up the raid array containing the
root filesystem.  Everything else is best done by /etc/init.d
scripts.  And you can stop those from running by booting with -s (or
whatever it is to get single-user).

> 
> > So my preferred solution to the problem is to tell people not to use
> > autodetect.  Quite possibly this should be documented in the code, and
> > maybe even have a KERN_INFO message if more than 64 devices are
> > autodetected. 
> 
> I wouldn't have a problem with that, since then distros would probably
> switch to a more recommended mechanism that works just as well, i.e.,
> ideally without requiring initrd-regeneration after reconfigurations
> such as adding one more raid device to the logical volume group
> containing the root filesystem.
> 
> > If we were to 'fix' this problem, I think the cleanest approach (which
> > I haven't actually coded, so it might not work...) would be to define
> > a new flag to go in hd_struct->policy to say if the partition type
> > suggested auto-detect, and get partitions/check.c to set this.
> > Then have md iterate all partitions looking for this flag.
> 
> AFAICT we'd still need a list or an array, since we add stuff back to
> the list in various situations.

No.  We just need a list of partitions of the appropriate type.
Taking items of the list and putting them back on later is simply a
non-essential optimisation.

> 
> > So:  Do you *really* need to *fix* this, or can you just use 'mdadm'
> > to assemble you arrays instead?
> 
> I'm not sure.  I'd expect not to need it, but the limited feature
> currently in place, that initrd uses to bring up the raid1 devices
> containing the physical volumes that form the volume group where the
> logical volume with my root filesystem is also brings up various raid6
> physical volumes that form an unrelated volume group, and it does so
> in such a way that the last of them, containing the 128th fd-type
> partition in the box, ends up being left out, so the raid device it's
> a member of is brought up either degraded or missing the spare member,
> none of which are good.
> 
> I don't know that I can easily get initrd to replace nash's
> raidautorun for mdadm unless mdadm has a mode to bring up any arrays
> it can find, as opposed to bringing up a specific array out of a given
> list of members or scanning for members.  Either way, this won't fix
> the problem 2) that you mentioned, but requiring initrd-regeneration
> after extending the volume group containing the root device is another
> problem that the current modes of operation of mdadm AFAIK won't
> contemplate, so switching to it will trade one problem for another,
> and the latter is IMHO more common than the former.

Get mdadm 2.5.2 (or 2.5.3 if I get that out soon enough) and try

  mdadm --assemble --scan --homehost='<system>' --auto-update-homehost \
  --auto=yes --run

in your initrd, having set the hostname correctly first.  It might do
exactly what you want.

NeilBrown

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Jul 31, 2006, Neil Brown <neilb@suse.de> wrote:

> The initrd need to 'know' how to find the root filesystem, whether by
> devnum or uuid or whatever.

Yeah, the tricky bit is the `whatever' alternative, when / is a
logical volume, and you need to bring up all of the physical volumes
in order for vgscan to bring up the volume group in a usable way.

> In exactly the same way it needs to know how to find the components
> for the root md array - uuid is the best.  There is no need to
> reconfigure this in the case of a disk failure.

When you add physical volumes to the volume group, you'd have to
reconfigure initrd if it wasn't for mdadm's ability to scan all
partitions.

> Current mdadm will assemble arrays for you given only a hostname.  You
> still need to get the hostname into the initrd, but that is no
> different from a root device number.

Yep, this should work, at least until someone changes the hostname,
creates a new array with the new option and then gets puzzled because
only that array isn't brought up.

Or, worse, does all of the above and then rebuilds initrd ``just in
case'', and then ends up unable to reboot because the root device
won't be brought up.  Oops :-)

>> 
>> >  2/ 
>> >     It can cause problems when moving devices.

>> It can, indeed, and it has caused such problems to me before, but
>> they're the exception, not the rule, and one should optimize for the
>> rule, not the exception.

> We aren't talking about optimisation.  We are talking about whether it
> actually works or not.

Yes, I'm talking about getting it to work most often in the most
common case.  Obviously we can't get it to work in every possible
case, since there are the various corner cases involving moving disks
around, renaming hosts and creating arrays, some of which must
necessarily fail in order for others to work.  It's finding the right
balance between them that is tricky, and some people will always be
unhappy because their particularly rare case failed, even without
realizing that this was in order to enable a more common case they
happened to rely on to work.

> A system that stops booting just because you plugged a couple of
> extra drives in is a badly configured system.

I tend to agree, although I used to exercise a case that wouldn't be
covered by this new policy: I used to move a pair of raid 1 external
disks between two hosts, and have them configured to be optionally
mounted on boot, depending on whether the raid devices were in place
or not.  With hostname identification, this wouldn't quite work :-)

> Well, at boot it should only bring up the raid array containing the
> root filesystem.

If all you have is in a single LVM volume group, then that must be
everything :-/

> Everything else is best done by /etc/init.d scripts.  And you can
> stop those from running by booting with -s (or whatever it is to get
> single-user).

Booting into single user mode actually attempts to mount everything
that is local, after bringing up raid devices et al, so that would be
too late.  But there's always init=/bin/bash :-)

> Get mdadm 2.5.2 (or 2.5.3 if I get that out soon enough) and try

>   mdadm --assemble --scan --homehost='<system>' --auto-update-homehost \
>   --auto=yes --run

> in your initrd, having set the hostname correctly first.  It might do
> exactly what you want.

Awesome, thanks, I'd missed that in the docs.  It might make sense to
spell it out as an example, instead of requiring someone to figure out
all of the bits and pieces from the extensive documentation.  Not
complaining about the extent of the documentation, BTW :-)

I'll give it a try some time tomorrow, since I won't turn on that
noisy box today any more; my daughter is already asleep :-)

Anyhow, unless there's a good reason to keep the code the way it is,
wasting valuable bytes of memory in the fixed-size array, I guess it
would make more sense to just merge the patch in, no? :-)

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Jul 31, 2006, Alexandre Oliva <aoliva@redhat.com> wrote:

>> mdadm --assemble --scan --homehost='<system>' --auto-update-homehost \
>> --auto=yes --run

>> in your initrd, having set the hostname correctly first.  It might do
>> exactly what you want.

> I'll give it a try some time tomorrow, since I won't turn on that
> noisy box today any more; my daughter is already asleep :-)

But then, I could use my own desktop to test it :-)

FWIW, here's the patch for Fedora rawhide's mkinitrd that worked for
me.  I figured even without --homehost it worked fine, even without
HOMEHOST set in mdadm.conf.

I hope copying mdadm.conf to initrd won't ever hurt, can you think of
any case in which it would?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mkinitrd-mdadm.patch --]
[-- Type: text/x-patch, Size: 650 bytes --]

--- /sbin/mkinitrd	2006-07-26 15:43:41.000000000 -0300
+++ /tmp/mkinitrd	2006-08-01 00:06:14.000000000 -0300
@@ -1240,10 +1240,19 @@
 emitdms
 
 if [ -n "$raiddevices" ]; then
+  if test -f /sbin/mdadm.static; then
+    if test -f /etc/mdadm.conf; then
+      inst /etc/mdadm.conf "$MNTIMAGE/etc/mdadm.conf"
+    fi
+    inst /sbin/mdadm.static "$MNTIMAGE/sbin/mdadm"
+    emit "mkdir /dev/md"
+    emit "mdadm --quiet --assemble --scan --auto-update-homehost --auto=yes --run"
+  else
     for dev in $raiddevices; do
         cp -a /dev/${dev} $MNTIMAGE/dev
         emit "raidautorun /dev/${dev}"
     done
+  fi
 fi
 
 if [ -n "$vg_list" ]; then

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Aug  1, 2006, Alexandre Oliva <aoliva@redhat.com> wrote:

>> I'll give it a try some time tomorrow, since I won't turn on that
>> noisy box today any more; my daughter is already asleep :-)

> But then, I could use my own desktop to test it :-)

But then, I wouldn't be testing quite the same scenario.

My boot-required RAID devices were all raid 1, whereas the larger,
separate volume group was all raid 6.

Using the mkinitrd patch that I posted before, the result was that
mdadm did try to bring up all raid devices but, because the raid456
module was not loaded in initrd, the raid devices were left inactive.

Then, when rc.sysinit tried to bring them up with mdadm -A -s, that
did nothing to the inactive devices, since they didn't have to be
assembled.  Adding --run didn't help.

My current work-around is to add raid456 to initrd, but that's ugly.
Scanning /proc/mdstat for inactive devices in rc.sysinit and doing
mdadm --run on them is feasible, but it looks ugly and error-prone.

Would it be reasonable to change mdadm so as to, erhm, disassemble ;-)
the raid devices it tried to bring up but that, for whatever reason,
it couldn't activate?  (say, missing module, not enough members,
whatever)

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Luca Berra]

On Tue, Aug 01, 2006 at 05:46:38PM -0300, Alexandre Oliva wrote:
>Using the mkinitrd patch that I posted before, the result was that
>mdadm did try to bring up all raid devices but, because the raid456
>module was not loaded in initrd, the raid devices were left inactive.

probably your initrd is broken, it should not have even tried to bring
up an md array that was not needed to mount root.

>Then, when rc.sysinit tried to bring them up with mdadm -A -s, that
>did nothing to the inactive devices, since they didn't have to be
>assembled.  Adding --run didn't help.
>
>My current work-around is to add raid456 to initrd, but that's ugly.
>Scanning /proc/mdstat for inactive devices in rc.sysinit and doing
>mdadm --run on them is feasible, but it looks ugly and error-prone.
>
>Would it be reasonable to change mdadm so as to, erhm, disassemble ;-)
>the raid devices it tried to bring up but that, for whatever reason,
>it couldn't activate?  (say, missing module, not enough members,
>whatever)

this would make sense if it were an option, patches welcome :)

L.

-- 
Luca Berra -- bluca@comedia.it
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \

Re: let md auto-detect 128+ raid members, fix potential race condition

[Bill Davidsen]

Neil Brown wrote:

>[linux-raid added to cc.
> Background: patch was submitted to remove the current hard limit
> of 127 partitions that can be auto-detected - limit set by 
> 'detected_devices array in md.c.
>]
>
>My first inclination is not to fix this problem.
>
>I consider md auto-detect to be a legacy feature.
>I don't use it and I recommend that other people don't use it.
>However I cannot justify removing it, so it stays there.
>Having this limitation could be seen as a good motivation for some
>more users to stop using it.
>
>Why not use auto-detect?
>I have three issues with it.
>
> 1/
>    It just isn't "right".  We don't mount filesystems from partitions
>    just because they have type 'Linux'.  We don't enable swap on
>    partitions just because they have type 'Linux swap'.  So why do we
>    assemble md/raid from partitions that have type 'Linux raid
>    autodetect'? 
>  
>

I rarely think you are totally wrong about anything RAID, but I do 
believe you have missed the point of autodetect. It is intended to work 
as it does now, building the array without depending on some user level 
functionality. The name "autodetect" clearly differentiates this type 
from the others you mentioned, there is no implication that swap or 
Linux partitions should do anything automatically.

This is not a case of my using a feature and defending it, I don't use 
it currently. for all of the reasons you enumerate. That doesn't mean 
that I haven't used the autodetect in the past or that I won't in the 
future, particularly with embedded systems.

> 2/ 
>    It can cause problems when moving devices.  If you have two
>    machines, both with an 'md0' array and you move the drives from one
>    on to the other - say because the first lost a powersupply - and
>    then reboot the machine that received the drives, which array gets
>    assembled as 'md0' ?? You might be lucky, you might not. This
>    isn't purely theoretical - there have been pleas for help on
>    linux-raid resulting from exactly this - though they have been
>    few. 
>
> 3/ 
>    The information redundancy can cause a problem when it gets out of
>    sync.  i.e. you add a partition to a raid array without setting
>    the partition type to 'fd'.  This works, but on the next reboot
>    the partition doesn't get added back into the array and you have
>    to manually add it yourself.
>    This too is not purely theory - it has been reported slightly more
>    often than '2'.
>
>So my preferred solution to the problem is to tell people not to use
>autodetect.  Quite possibly this should be documented in the code, and
>maybe even have a KERN_INFO message if more than 64 devices are
>autodetected. 
>  
>
I don't personally see the value of autodetect for putting together the 
huge number of drives people configure. I see this as a way to improve 
boot reliability, if someone needs 64 drives for root and boot, they 
need to read a few essays on filesystem configuration. However, I'm 
aware that there are some really bizarre special cases out there.

Maybe the limit should be in KCONFIG, with a default of 16 or so.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: let md auto-detect 128+ raid members, fix potential race condition

[Alexandre Oliva]

On Aug  1, 2006, Bill Davidsen <davidsen@tmr.com> wrote:

> I rarely think you are totally wrong about anything RAID, but I do
> believe you have missed the point of autodetect. It is intended to
> work as it does now, building the array without depending on some user
> level functionality.

Well, it clearly depends on at least some user level functionality
(the ioctl that triggers autodetect).  Going from that to a
full-fledged mdadm doesn't sound like such a big deal to me.

> I don't personally see the value of autodetect for putting together
> the huge number of drives people configure. I see this as a way to
> improve boot reliability, if someone needs 64 drives for root and
> boot, they need to read a few essays on filesystem
> configuration. However, I'm aware that there are some really bizarre
> special cases out there.

There's LVM.  If you have to keep root out of the VG just because
people say so, you lose lots of benefits from LVM, such as being able
to grow root with the system running, take snapshots of root, etc.

Sure enough the LVM subsystem could make things better for one to not
need all of the PVs in the root-containing VG in order to be able to
mount root read-write, or at all, but if you think about it, if initrd
is set up such that you only bring up the devices that hold the actual
root device within the VG and then you change that, say by taking a
snapshot of root, moving it around, growing it, etc, you'd be better
off if you could still boot.  So you do want all of the VG members to
be around, just in case.

This is trivially-accomplished for regular disks whose drivers are
loaded by initrd, but for raid devices, you need to tentatively bring
up every raid member you can, just in case some piece of root is
there, otherwise you may end up unable to boot.

Yes, this is an argument against root on LVM, but there are arguments
*for* root on LVM as well, and there's no reason to not support both
behaviors equally well and let people figure out what works best for
them.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Secretary for FSF Latin America        http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: let md auto-detect 128+ raid members, fix potential race condition

[Luca Berra]

On Tue, Aug 01, 2006 at 06:32:33PM -0300, Alexandre Oliva wrote:
>Sure enough the LVM subsystem could make things better for one to not
>need all of the PVs in the root-containing VG in order to be able to
>mount root read-write, or at all, but if you think about it, if initrd
it shouldn't need all of the PVs you just need all the pv where the
rootfs is.

>is set up such that you only bring up the devices that hold the actual
>root device within the VG and then you change that, say by taking a
>snapshot of root, moving it around, growing it, etc, you'd be better
>off if you could still boot.  So you do want all of the VG members to
>be around, just in case.
in this case just regenerate the initramfs after modifying the vg that
contains root. I am fairly sure that kernel upgrades are far more
frequent than the addirion of PVs to the root VG.

>Yes, this is an argument against root on LVM, but there are arguments
>*for* root on LVM as well, and there's no reason to not support both
>behaviors equally well and let people figure out what works best for
>them.

No, this is just an argument against misusing root on lvm.

L.

-- 
Luca Berra -- bluca@comedia.it
        Communication Media & Services S.r.l.
 /"\
 \ /     ASCII RIBBON CAMPAIGN
  X        AGAINST HTML MAIL
 / \

Re: let md auto-detect 128+ raid members, fix potential race condition

[Bill Davidsen]

Alexandre Oliva wrote:
> On Aug  1, 2006, Bill Davidsen <davidsen@tmr.com> wrote:
> 
>> I rarely think you are totally wrong about anything RAID, but I do
>> believe you have missed the point of autodetect. It is intended to
>> work as it does now, building the array without depending on some user
>> level functionality.
> 
> Well, it clearly depends on at least some user level functionality
> (the ioctl that triggers autodetect).  Going from that to a
> full-fledged mdadm doesn't sound like such a big deal to me.
> 
>> I don't personally see the value of autodetect for putting together
>> the huge number of drives people configure. I see this as a way to
>> improve boot reliability, if someone needs 64 drives for root and
>> boot, they need to read a few essays on filesystem
>> configuration. However, I'm aware that there are some really bizarre
>> special cases out there.
> 
> There's LVM.  If you have to keep root out of the VG just because
> people say so, you lose lots of benefits from LVM, such as being able
> to grow root with the system running, take snapshots of root, etc.
> 
But it's MY system. I don't have to anything. More to the point, growing 
root while the system is running is done a lot less than booting. In 
general the root f/s has very little in it, and that's a good thing.

-- 
Bill Davidsen <davidsen@tmr.com>
   Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one errors occurs during
wildcard (glob) expansion.