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

[Daniel Kahn Gillmor]

On Thu, 23 Feb 2012 06:43:23 +0100, Vladimir 'φ-coder/phcoder' Serbinenko 
<address@hidden> 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 address@hidden:/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 address@hidden:/home/dkg/src/grub/grub# ./grub-probe --target=device /
/dev/mapper/vg_trash0-root
0 address@hidden:/home/dkg/src/grub/grub# 

I think it's worth applying.

Thanks for your work on this, phcoder!

       --dkg

=== 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),

pgpGbeubeWYrL.pgp
Description: PGP signature