[PATCH] Support RAID on virtio devices, and others

[PATCH] Support RAID on virtio devices, and others

[Colin Watson]

GRUB only supports RAID on a relatively small number of device types, as
implemented by grub_util_getdiskname. I received a bug report noting
that this doesn't work for RAID arrays with virtio block devices (often
used in kvm) as components. This is difficult to support using the
approach taken by grub_util_getdiskname, as virtio devices use dynamic
major numbers.

find_root_device in util/getroot.c seemed to be exactly what I wanted:
it just trawls /dev for the appropriate major and minor numbers. This
code is not performance-critical, so that should be fine. I made the
function naming more consistent, added support for a default directory
in its interface (this may have problems on Cygwin; does anyone care
about RAID on Cygwin? If so, perhaps they can propose improvements), and
changed the RAID code to use it.

Bazaar users can merge this from:

  bzr+ssh://bzr.sv.gnu.org/grub/people/cjwatson/raid-virtio/

=== modified file 'ChangeLog'
--- ChangeLog	2009-10-21 12:22:05 +0000
+++ ChangeLog	2009-10-25 01:32:02 +0000
@@ -1,3 +1,22 @@
+2009-10-25  Colin Watson  <cjwatson@ubuntu.com>
+
+	Support RAID on virtio devices, and others.
+
+	* util/getroot.c [__MINGW32__] (find_root_device): Rename to ...
+	[__MINGW32__] (grub_find_device): ... this.
+	[! __MINGW32__ && ! __CYGWIN__] (find_root_device): Rename to ...
+	[! __MINGW32__ && ! __CYGWIN__] (grub_find_device): ... this.  Use a
+	reasonable default if dir is NULL.
+	[! __MINGW32__ && __CYGWIN__] (find_cygwin_root_device): Rename to
+	...
+	[! __MINGW32__ && __CYGWIN__] (grub_find_device): ... this.
+	(grub_guess_root_device): Update callers.
+	* include/grub/util/getroot.h (grub_find_device): Add prototype.
+
+	* util/raid.c (grub_util_getdiskname): Remove.
+	(grub_util_raid_getmembers): Use grub_find_device rather than
+	grub_util_getdiskname.
+
 2009-10-21  Felix Zielcke  <fzielcke@z-51.de>
 
 	* config.guess: Update to latest version from config git

=== modified file 'include/grub/util/getroot.h'
--- include/grub/util/getroot.h	2009-04-11 09:40:39 +0000
+++ include/grub/util/getroot.h	2009-10-25 01:22:15 +0000
@@ -19,12 +19,15 @@
 #ifndef GRUB_UTIL_GETROOT_HEADER
 #define GRUB_UTIL_GETROOT_HEADER	1
 
+#include <sys/types.h>
+
 enum grub_dev_abstraction_types {
   GRUB_DEV_ABSTRACTION_NONE,
   GRUB_DEV_ABSTRACTION_LVM,
   GRUB_DEV_ABSTRACTION_RAID,
 };
 
+char *grub_find_device (const char *dir, dev_t dev);
 char *grub_guess_root_device (const char *dir);
 char *grub_get_prefix (const char *dir);
 int grub_util_get_dev_abstraction (const char *os_dev);

=== modified file 'util/getroot.c'
--- util/getroot.c	2009-07-20 20:03:18 +0000
+++ util/getroot.c	2009-10-25 01:22:15 +0000
@@ -172,8 +172,8 @@ grub_get_prefix (const char *dir)
 
 #ifdef __MINGW32__
 
-static char *
-find_root_device (const char *dir __attribute__ ((unused)),
+char *
+grub_find_device (const char *dir __attribute__ ((unused)),
                   dev_t dev __attribute__ ((unused)))
 {
   return 0;
@@ -181,13 +181,22 @@ find_root_device (const char *dir __attr
 
 #elif ! defined(__CYGWIN__)
 
-static char *
-find_root_device (const char *dir, dev_t dev)
+char *
+grub_find_device (const char *dir, dev_t dev)
 {
   DIR *dp;
   char *saved_cwd;
   struct dirent *ent;
 
+  if (! dir)
+    {
+#ifdef __CYGWIN__
+      return NULL;
+#else
+      dir = "/dev";
+#endif
+    }
+
   dp = opendir (dir);
   if (! dp)
     return 0;
@@ -225,7 +234,7 @@ find_root_device (const char *dir, dev_t
 	  /* Find it recursively.  */
 	  char *res;
 
-	  res = find_root_device (ent->d_name, dev);
+	  res = grub_find_device (ent->d_name, dev);
 
 	  if (res)
 	    {
@@ -328,8 +337,8 @@ get_bootsec_serial (const char *os_dev, 
   return serial;
 }
 
-static char *
-find_cygwin_root_device (const char *path, dev_t dev)
+char *
+grub_find_device (const char *path, dev_t dev)
 {
   /* No root device for /cygdrive.  */
   if (dev == (DEV_CYGDRIVE_MAJOR << 16))
@@ -350,7 +359,7 @@ find_cygwin_root_device (const char *pat
 
   /* Cygwin returns the partition serial number in stat.st_dev.
      This is never identical to the device number of the emulated
-     /dev/sdXN device, so above find_root_device () does not work.
+     /dev/sdXN device, so above grub_find_device () does not work.
      Search the partition with the same serial in boot sector instead.  */
   char devpath[sizeof ("/dev/sda15") + 13]; /* Size + Paranoia.  */
   int d;
@@ -386,12 +395,12 @@ grub_guess_root_device (const char *dir)
 
 #ifdef __CYGWIN__
   /* Cygwin specific function.  */
-  os_dev = find_cygwin_root_device (dir, st.st_dev);
+  os_dev = grub_find_device (dir, st.st_dev);
 
 #else
 
   /* This might be truly slow, but is there any better way?  */
-  os_dev = find_root_device ("/dev", st.st_dev);
+  os_dev = grub_find_device ("/dev", st.st_dev);
 #endif
 
   return os_dev;

=== modified file 'util/raid.c'
--- util/raid.c	2009-06-10 21:04:23 +0000
+++ util/raid.c	2009-10-25 01:22:15 +0000
@@ -21,40 +21,19 @@
 #ifdef __linux__
 #include <grub/util/misc.h>
 #include <grub/util/raid.h>
+#include <grub/util/getroot.h>
 
 #include <string.h>
 #include <fcntl.h>
 #include <sys/ioctl.h>
 #include <errno.h>
+#include <sys/types.h>
 
 #include <linux/types.h>
 #include <linux/major.h>
 #include <linux/raid/md_p.h>
 #include <linux/raid/md_u.h>
 
-static char *
-grub_util_getdiskname (int major, int minor)
-{
-  char *name = xmalloc (15);
-
-  if (major == LOOP_MAJOR)
-    sprintf (name, "/dev/loop%d", minor);
-  else if (major == IDE0_MAJOR)
-    sprintf (name, "/dev/hd%c", 'a' + minor / 64);
-  else if (major == IDE1_MAJOR)
-    sprintf (name, "/dev/hd%c", 'c' + minor / 64);
-  else if (major == IDE2_MAJOR)
-    sprintf (name, "/dev/hd%c", 'e' + minor / 64);
-  else if (major == IDE3_MAJOR)
-    sprintf (name, "/dev/hd%c", 'g' + minor / 64);
-  else if (major == SCSI_DISK0_MAJOR)
-    sprintf (name, "/dev/sd%c", 'a' + minor / 16);
-  else
-    grub_util_error ("Unknown device number: %d, %d", major, minor);
-
-  return name;
-}
-
 char **
 grub_util_raid_getmembers (char *name)
 {
@@ -99,7 +78,8 @@ grub_util_raid_getmembers (char *name)
 
       if (disk.state & (1 << MD_DISK_ACTIVE))
 	{
-	  devicelist[j] = grub_util_getdiskname (disk.major, disk.minor);
+	  devicelist[j] = grub_find_device (NULL,
+					    makedev (disk.major, disk.minor));
 	  j++;
 	}
     }


-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Support RAID on virtio devices, and others

[Vladimir 'phcoder' Serbinenko]

Colin Watson wrote:
> GRUB only supports RAID on a relatively small number of device types, as
> implemented by grub_util_getdiskname. I received a bug report noting
> that this doesn't work for RAID arrays with virtio block devices (often
> used in kvm) as components. This is difficult to support using the
> approach taken by grub_util_getdiskname, as virtio devices use dynamic
> major numbers.
>
> find_root_device in util/getroot.c seemed to be exactly what I wanted:
> it just trawls /dev for the appropriate major and minor numbers. This
> code is not performance-critical, so that should be fine. 
Not true. Even in current state grub-mkconfig is taking considerable
time to complete on my system with numerous kernels. Unless someone
implements a cache (it can be invalidated after 5 minutes) not to go
through the same probing procedure on every grub-probe call I object
against adding any additional delay in probing procedure
> I made the
> function naming more consistent, added support for a default directory
> in its interface (this may have problems on Cygwin; does anyone care
> about RAID on Cygwin? If so, perhaps they can propose improvements), and
> changed the RAID code to use it.
>
> Bazaar users can merge this from:
>
>   bzr+ssh://bzr.sv.gnu.org/grub/people/cjwatson/raid-virtio/
>
> === modified file 'ChangeLog'
> --- ChangeLog	2009-10-21 12:22:05 +0000
> +++ ChangeLog	2009-10-25 01:32:02 +0000
> @@ -1,3 +1,22 @@
> +2009-10-25  Colin Watson  <cjwatson@ubuntu.com>
> +
> +	Support RAID on virtio devices, and others.
> +
> +	* util/getroot.c [__MINGW32__] (find_root_device): Rename to ...
> +	[__MINGW32__] (grub_find_device): ... this.
> +	[! __MINGW32__ && ! __CYGWIN__] (find_root_device): Rename to ...
> +	[! __MINGW32__ && ! __CYGWIN__] (grub_find_device): ... this.  Use a
> +	reasonable default if dir is NULL.
> +	[! __MINGW32__ && __CYGWIN__] (find_cygwin_root_device): Rename to
> +	...
> +	[! __MINGW32__ && __CYGWIN__] (grub_find_device): ... this.
> +	(grub_guess_root_device): Update callers.
> +	* include/grub/util/getroot.h (grub_find_device): Add prototype.
> +
> +	* util/raid.c (grub_util_getdiskname): Remove.
> +	(grub_util_raid_getmembers): Use grub_find_device rather than
> +	grub_util_getdiskname.
> +
>  2009-10-21  Felix Zielcke  <fzielcke@z-51.de>
>  
>  	* config.guess: Update to latest version from config git
>
> === modified file 'include/grub/util/getroot.h'
> --- include/grub/util/getroot.h	2009-04-11 09:40:39 +0000
> +++ include/grub/util/getroot.h	2009-10-25 01:22:15 +0000
> @@ -19,12 +19,15 @@
>  #ifndef GRUB_UTIL_GETROOT_HEADER
>  #define GRUB_UTIL_GETROOT_HEADER	1
>  
> +#include <sys/types.h>
> +
>  enum grub_dev_abstraction_types {
>    GRUB_DEV_ABSTRACTION_NONE,
>    GRUB_DEV_ABSTRACTION_LVM,
>    GRUB_DEV_ABSTRACTION_RAID,
>  };
>  
> +char *grub_find_device (const char *dir, dev_t dev);
>  char *grub_guess_root_device (const char *dir);
>  char *grub_get_prefix (const char *dir);
>  int grub_util_get_dev_abstraction (const char *os_dev);
>
> === modified file 'util/getroot.c'
> --- util/getroot.c	2009-07-20 20:03:18 +0000
> +++ util/getroot.c	2009-10-25 01:22:15 +0000
> @@ -172,8 +172,8 @@ grub_get_prefix (const char *dir)
>  
>  #ifdef __MINGW32__
>  
> -static char *
> -find_root_device (const char *dir __attribute__ ((unused)),
> +char *
> +grub_find_device (const char *dir __attribute__ ((unused)),
>                    dev_t dev __attribute__ ((unused)))
>  {
>    return 0;
> @@ -181,13 +181,22 @@ find_root_device (const char *dir __attr
>  
>  #elif ! defined(__CYGWIN__)
>  
> -static char *
> -find_root_device (const char *dir, dev_t dev)
> +char *
> +grub_find_device (const char *dir, dev_t dev)
>  {
>    DIR *dp;
>    char *saved_cwd;
>    struct dirent *ent;
>  
> +  if (! dir)
> +    {
> +#ifdef __CYGWIN__
> +      return NULL;
> +#else
> +      dir = "/dev";
> +#endif
> +    }
> +
>    dp = opendir (dir);
>    if (! dp)
>      return 0;
> @@ -225,7 +234,7 @@ find_root_device (const char *dir, dev_t
>  	  /* Find it recursively.  */
>  	  char *res;
>  
> -	  res = find_root_device (ent->d_name, dev);
> +	  res = grub_find_device (ent->d_name, dev);
>  
>  	  if (res)
>  	    {
> @@ -328,8 +337,8 @@ get_bootsec_serial (const char *os_dev, 
>    return serial;
>  }
>  
> -static char *
> -find_cygwin_root_device (const char *path, dev_t dev)
> +char *
> +grub_find_device (const char *path, dev_t dev)
>  {
>    /* No root device for /cygdrive.  */
>    if (dev == (DEV_CYGDRIVE_MAJOR << 16))
> @@ -350,7 +359,7 @@ find_cygwin_root_device (const char *pat
>  
>    /* Cygwin returns the partition serial number in stat.st_dev.
>       This is never identical to the device number of the emulated
> -     /dev/sdXN device, so above find_root_device () does not work.
> +     /dev/sdXN device, so above grub_find_device () does not work.
>       Search the partition with the same serial in boot sector instead.  */
>    char devpath[sizeof ("/dev/sda15") + 13]; /* Size + Paranoia.  */
>    int d;
> @@ -386,12 +395,12 @@ grub_guess_root_device (const char *dir)
>  
>  #ifdef __CYGWIN__
>    /* Cygwin specific function.  */
> -  os_dev = find_cygwin_root_device (dir, st.st_dev);
> +  os_dev = grub_find_device (dir, st.st_dev);
>  
>  #else
>  
>    /* This might be truly slow, but is there any better way?  */
> -  os_dev = find_root_device ("/dev", st.st_dev);
> +  os_dev = grub_find_device ("/dev", st.st_dev);
>  #endif
>  
>    return os_dev;
>
> === modified file 'util/raid.c'
> --- util/raid.c	2009-06-10 21:04:23 +0000
> +++ util/raid.c	2009-10-25 01:22:15 +0000
> @@ -21,40 +21,19 @@
>  #ifdef __linux__
>  #include <grub/util/misc.h>
>  #include <grub/util/raid.h>
> +#include <grub/util/getroot.h>
>  
>  #include <string.h>
>  #include <fcntl.h>
>  #include <sys/ioctl.h>
>  #include <errno.h>
> +#include <sys/types.h>
>  
>  #include <linux/types.h>
>  #include <linux/major.h>
>  #include <linux/raid/md_p.h>
>  #include <linux/raid/md_u.h>
>  
> -static char *
> -grub_util_getdiskname (int major, int minor)
> -{
> -  char *name = xmalloc (15);
> -
> -  if (major == LOOP_MAJOR)
> -    sprintf (name, "/dev/loop%d", minor);
> -  else if (major == IDE0_MAJOR)
> -    sprintf (name, "/dev/hd%c", 'a' + minor / 64);
> -  else if (major == IDE1_MAJOR)
> -    sprintf (name, "/dev/hd%c", 'c' + minor / 64);
> -  else if (major == IDE2_MAJOR)
> -    sprintf (name, "/dev/hd%c", 'e' + minor / 64);
> -  else if (major == IDE3_MAJOR)
> -    sprintf (name, "/dev/hd%c", 'g' + minor / 64);
> -  else if (major == SCSI_DISK0_MAJOR)
> -    sprintf (name, "/dev/sd%c", 'a' + minor / 16);
> -  else
> -    grub_util_error ("Unknown device number: %d, %d", major, minor);
> -
> -  return name;
> -}
> -
>  char **
>  grub_util_raid_getmembers (char *name)
>  {
> @@ -99,7 +78,8 @@ grub_util_raid_getmembers (char *name)
>  
>        if (disk.state & (1 << MD_DISK_ACTIVE))
>  	{
> -	  devicelist[j] = grub_util_getdiskname (disk.major, disk.minor);
> +	  devicelist[j] = grub_find_device (NULL,
> +					    makedev (disk.major, disk.minor));
>  	  j++;
>  	}
>      }
>
>
>   


-- 
Regards
Vladimir 'phcoder' Serbinenko
Personal git repository: http://repo.or.cz/w/grub2/phcoder.git 

Re: [PATCH] Support RAID on virtio devices, and others

[Felix Zielcke]

Am Sonntag, den 25.10.2009, 10:27 +0100 schrieb Vladimir 'phcoder'
Serbinenko:
> > find_root_device in util/getroot.c seemed to be exactly what I
> wanted:
> > it just trawls /dev for the appropriate major and minor numbers.
> This
> > code is not performance-critical, so that should be fine. 
> Not true. Even in current state grub-mkconfig is taking considerable
> time to complete on my system with numerous kernels. Unless someone
> implements a cache (it can be invalidated after 5 minutes) not to go
> through the same probing procedure on every grub-probe call I object
> against adding any additional delay in probing procedure 

We have a bug report in Debian[0] open that grub-probe is very slow due
to the use of ioctl(BLKFLSBUF)
A comment above it says this is needed due to a bug in the Linux kernel.
Is there a safe way to check if this is still needed with a recent
Kernel?
This seems to be added 2003, so things could have changed now a bit.

[0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508834

-- 
Felix Zielcke
Proud Debian Maintainer and GNU GRUB developer

Re: [PATCH] Support RAID on virtio devices, and others

[Colin Watson]

On Sun, Oct 25, 2009 at 10:27:49AM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Colin Watson wrote:
> > GRUB only supports RAID on a relatively small number of device types, as
> > implemented by grub_util_getdiskname. I received a bug report noting
> > that this doesn't work for RAID arrays with virtio block devices (often
> > used in kvm) as components. This is difficult to support using the
> > approach taken by grub_util_getdiskname, as virtio devices use dynamic
> > major numbers.
> >
> > find_root_device in util/getroot.c seemed to be exactly what I wanted:
> > it just trawls /dev for the appropriate major and minor numbers. This
> > code is not performance-critical, so that should be fine. 
> 
> Not true. Even in current state grub-mkconfig is taking considerable
> time to complete on my system with numerous kernels. Unless someone
> implements a cache (it can be invalidated after 5 minutes) not to go
> through the same probing procedure on every grub-probe call I object
> against adding any additional delay in probing procedure

But this code is not called from grub-probe; it is only called from
grub-setup (grep for grub_util_raid_getmembers to confirm for yourself).
A grub-mkconfig run shouldn't go anywhere near this.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: [PATCH] Support RAID on virtio devices, and others

[Robert Millan]

On Sun, Oct 25, 2009 at 10:37:01AM +0100, Felix Zielcke wrote:
> 
> We have a bug report in Debian[0] open that grub-probe is very slow due
> to the use of ioctl(BLKFLSBUF)
> A comment above it says this is needed due to a bug in the Linux kernel.
> Is there a safe way to check if this is still needed with a recent
> Kernel?
> This seems to be added 2003, so things could have changed now a bit.
> 
> [0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508834

I think this is only an issue in very specific situations.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Support RAID on virtio devices, and others

[Robert Millan]

On Sun, Oct 25, 2009 at 10:27:49AM +0100, Vladimir 'phcoder' Serbinenko wrote:
> Not true. Even in current state grub-mkconfig is taking considerable
> time to complete on my system with numerous kernels. Unless someone
> implements a cache (it can be invalidated after 5 minutes) not to go
> through the same probing procedure on every grub-probe call I object
> against adding any additional delay in probing procedure

I've been thinking for a while that grub-probe does a few too many things,
perhaps if it is split in separate commands, and the upper layer makes
appropiate use of them (e.g. by remembering results so they don't need to
be re-obtained), we could optimize this without need for a cache in the
lower layer.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."