Re: grub RAID heuristics (how can we avoid "superfluous RAID member (2 found)")

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]

On Thu, 23 Feb 2012 06:43:23 +0100, Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> wrote:
> [dkg wrote:]
> > Select the smallest known block device that can completely enclose the
> > RAID member.  The larger block device(s) should not be considered to be
> > exporting that RAID.

phcoder pointed me to the attached patch for improved heuristics for
this case, which applies cleanly to the current head of bzr.  Applied,
it removes the spurious error messages from grub-probe, as seen here
compared against debian's grub-probe 1.99-14:

0 root@trash:/home/dkg/src/grub/grub# grub-probe --target=device /
error: found two disks with the index 1 for RAID md0.
error: superfluous RAID member (2 found).
/dev/mapper/vg_trash0-root
0 root@trash:/home/dkg/src/grub/grub# ./grub-probe --target=device /
/dev/mapper/vg_trash0-root
0 root@trash:/home/dkg/src/grub/grub# 

I think it's worth applying.

Thanks for your work on this, phcoder!

       --dkg


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: grub-raid-heuristics.patch --]
[-- Type: text/x-diff, Size: 3067 bytes --]

=== modified file 'grub-core/disk/diskfilter.c'
--- grub-core/disk/diskfilter.c	2012-02-12 14:25:25 +0000
+++ grub-core/disk/diskfilter.c	2012-02-24 21:06:10 +0000
@@ -974,33 +974,33 @@
 	struct grub_diskfilter_lv *lv;
 	/* FIXME: Check whether the update time of the superblocks are
 	   the same.  */
+	if (pv->disk && grub_disk_get_size (disk) >= pv->part_size)
+	  return GRUB_ERR_NONE;
+	pv->disk = grub_disk_open (disk->name);
+	if (!pv->disk)
+	  return grub_errno;
 	/* This could happen to LVM on RAID, pv->disk points to the
 	   raid device, we shouldn't change it.  */
-	if (! pv->disk)
-	  {
-	    pv->disk = grub_disk_open (disk->name);
-	    if (! pv->disk)
-	      return grub_errno;
-	    pv->part_start = grub_partition_get_start (disk->partition);
-	    pv->part_size = grub_disk_get_size (disk);
+	pv->start_sector -= pv->part_start;
+	pv->part_start = grub_partition_get_start (disk->partition);
+	pv->part_size = grub_disk_get_size (disk);
 
 #ifdef GRUB_UTIL
-	    {
-	      grub_size_t s = 1;
-	      grub_partition_t p;
-	      for (p = disk->partition; p; p = p->parent)
-		s++;
-	      pv->partmaps = xmalloc (s * sizeof (pv->partmaps[0]));
-	      s = 0;
-	      for (p = disk->partition; p; p = p->parent)
-		pv->partmaps[s++] = xstrdup (p->partmap->name);
-	      pv->partmaps[s++] = 0;
-	    }
+	{
+	  grub_size_t s = 1;
+	  grub_partition_t p;
+	  for (p = disk->partition; p; p = p->parent)
+	    s++;
+	  pv->partmaps = xmalloc (s * sizeof (pv->partmaps[0]));
+	  s = 0;
+	  for (p = disk->partition; p; p = p->parent)
+	    pv->partmaps[s++] = xstrdup (p->partmap->name);
+	  pv->partmaps[s++] = 0;
+	}
 #endif
-	    if (start_sector != (grub_uint64_t)-1)
-	      pv->start_sector = start_sector;
-	    pv->start_sector += pv->part_start;
-	  }
+	if (start_sector != (grub_uint64_t)-1)
+	  pv->start_sector = start_sector;
+	pv->start_sector += pv->part_start;
 	/* Add the device to the array. */
 	for (lv = array->lvs; lv; lv = lv->next)
 	  if (!lv->became_readable_at && lv->fullname && is_lv_readable (lv))

=== modified file 'grub-core/disk/mdraid_linux.c'
--- grub-core/disk/mdraid_linux.c	2012-01-29 13:28:01 +0000
+++ grub-core/disk/mdraid_linux.c	2012-02-24 21:11:29 +0000
@@ -201,6 +201,11 @@
       return NULL;
     }
 
+  /* No need for explicit check that sb.size is 0 (unspecified) since
+     0 >= non-0 is false.  */
+  if (((grub_disk_addr_t) grub_le_to_cpu32 (sb.size)) * 2 >= size)
+    return NULL;
+
   /* FIXME: Check the checksum.  */
 
   level = grub_le_to_cpu32 (sb.level);
@@ -241,7 +246,8 @@
   grub_snprintf (buf, sizeof (buf), "md%d", grub_le_to_cpu32 (sb.md_minor));
   return grub_diskfilter_make_raid (16, (char *) uuid,
 				    grub_le_to_cpu32 (sb.raid_disks), buf,
-				    (sb.size) ? grub_le_to_cpu32 (sb.size) * 2 
+				    (sb.size) ? ((grub_disk_addr_t)
+						 grub_le_to_cpu32 (sb.size)) * 2
 				    : sector,
 				    grub_le_to_cpu32 (sb.chunk_size) >> 9,
 				    grub_le_to_cpu32 (sb.layout),


[-- Attachment #2: Type: application/pgp-signature, Size: 965 bytes --]