Re: [PATCH] Install to LVM PVs

[Andrei Borzenkov]

08.05.2016 07:53, Dryden Personalis пишет:
> I updated this guy's patch to work with 2.02 beta 2, at least.
> 
> I've just worked on the version that gets downloaded and unpacked on
> Kubuntu / Ubuntu 16.04.
> 
> One part of the patch didn't apply because apparently some tabs have
> entered a file, namely grub-core/disk/lvm.c.
> 
> Another part didn't apply because code had been refactored from
> grub-setup.c to setup.c.
> 
> I've only compiled with these extra packages:
> 
> - bison
> - flex
> - libdevmapper-dev
> 
> and automake was also required. I've had to symlink the 1.15 executables
> to 1.14.
> 
> 

Did you actually test it by installing GRUB on PV? Does it boot? Did you
test it both with unpartitioned disk and PV in partition?

> I'm not sure why the patch never made it into mainline grub, but back
> then (2013) the argument was that it was doing something to an area that
> was not meant for that.
> 

Neither am I :) I understand that Vladimir had plans to stuff more
information in this area (personally I'd like to use it for environment
block as example) but just guessing. In any case, we really always can
cut off part of this area by artificially reducing size.

> However LVM contains the --bootloaderareasize flag that seems to be
> meant for this, and if you put it to 2048 sectors (or 1M) it ought to
> work cq. just works.
> 
> ie. pvcreate --bootloaderareasize 2048s /dev/sda, followed by,
> grub-install /dev/sda, will just work and result in a bootable system
> from that harddisk.
> 
> I don't know if I can attach here, and I don't know what will happen
> with tabs if I inline it.
> 
> So I will attach first.
> 

That's fine.

> Regards.
> 
> 
> ps. I did not test booting from the harddisk to a system on the
> harddisk, but it boots a system on another device just fine, so I see no
> reason why it wouldn't work.
> 
> reference:
> https://lists.gnu.org/archive/html/grub-devel/2013-09/msg00113.html
> 

> === modified file 'grub-core/disk/lvm.c'
> --- grub-core/disk/lvm.c        2012-06-25 15:52:20 +0000
> +++ grub-core/disk/lvm.c        2013-09-25 11:03:21 +0000
> @@ -95,6 +95,38 @@
>      }
>  }
>  
> +static struct grub_lvm_pv_header *
> +grub_lvm_get_pvh(grub_disk_t disk, char buf[static GRUB_LVM_LABEL_SIZE])
> +{
> +  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
> +  unsigned int i;
> +
> +  /* Search for label. */
> +  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> +    {
> +      if (grub_disk_read (disk, i, 0, GRUB_LVM_LABEL_SIZE, buf))
> +        return NULL;
> +
> +      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> +                          sizeof (lh->id)))
> +         && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> +                             sizeof (lh->type))))
> +       break;
> +    }
> +
> +  /* Return if we didn't find a label. */
> +  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> +    {
> +#ifdef GRUB_UTIL
> +      grub_util_info ("no LVM signature found");
> +#endif
> +      return NULL;
> +    }
> +
> +  return (struct grub_lvm_pv_header *) (buf + 
> grub_le_to_cpu32(lh->offset_xl));
> +}
> +
> +
>  static struct grub_diskfilter_vg * 
>  grub_lvm_detect (grub_disk_t disk,
>                  struct grub_diskfilter_pv_id *id,
> @@ -102,11 +134,10 @@
>  {
>    grub_err_t err;
>    grub_uint64_t mda_offset, mda_size;
> -  char buf[GRUB_LVM_LABEL_SIZE];
>    char vg_id[GRUB_LVM_ID_STRLEN+1];
>    char pv_id[GRUB_LVM_ID_STRLEN+1];
> +  char buf[GRUB_LVM_LABEL_SIZE];

Why reordering buf definition?

>    char *metadatabuf, *p, *q, *vgname;
> -  struct grub_lvm_label_header *lh = (struct grub_lvm_label_header *) buf;
>    struct grub_lvm_pv_header *pvh;
>    struct grub_lvm_disk_locn *dlocn;
>    struct grub_lvm_mda_header *mdah;
> @@ -115,30 +146,9 @@
>    struct grub_diskfilter_vg *vg;
>    struct grub_diskfilter_pv *pv;
>  
> -  /* Search for label. */
> -  for (i = 0; i < GRUB_LVM_LABEL_SCAN_SECTORS; i++)
> -    {
> -      err = grub_disk_read (disk, i, 0, sizeof(buf), buf);
> -      if (err)
> -     goto fail;
> -
> -      if ((! grub_strncmp ((char *)lh->id, GRUB_LVM_LABEL_ID,
> -                        sizeof (lh->id)))
> -       && (! grub_strncmp ((char *)lh->type, GRUB_LVM_LVM2_LABEL,
> -                           sizeof (lh->type))))
> -     break;
> -    }
> -
> -  /* Return if we didn't find a label. */
> -  if (i == GRUB_LVM_LABEL_SCAN_SECTORS)
> -    {
> -#ifdef GRUB_UTIL
> -      grub_util_info ("no LVM signature found");
> -#endif
> -      goto fail;
> -    }
> -
> -  pvh = (struct grub_lvm_pv_header *) (buf + 
> grub_le_to_cpu32(lh->offset_xl));
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    goto fail;
>  
>    for (i = 0, j = 0; i < GRUB_LVM_ID_LEN; i++)
>      {
> @@ -151,7 +161,7 @@
>    dlocn = pvh->disk_areas_xl;
>  
>    dlocn++;
> -  /* Is it possible to have multiple data/metadata areas? I haven't
> +  /* Is it possible to have multiple data areas? I haven't

Could you explain why you deleted "metadata"?

>       seen devices that have it. */
>    if (dlocn->offset)
>      {
> @@ -746,6 +756,88 @@
>    return NULL;
>  }
>  
> +#ifdef GRUB_UTIL
> +int
> +grub_util_is_lvm(grub_disk_t disk)
> +{
> +  struct grub_diskfilter_pv_id id;
> +  struct grub_diskfilter_vg *vg;
> +  grub_disk_addr_t start_sector;
> +  vg = grub_lvm_detect(disk, &id, &start_sector);
> +  if (! vg)
> +    return 0;
> +  /* don't free the vg, it's held by grub_diskfilter_vg_register */
> +  grub_free(id.uuid);
> +  return 1;
> +}
> +

This has side effect of adding duplicate VG definitions; this may later
confuse grub. What about just checking array->driver for LVM? Go through
registered arrays, find disk match and check array driver. See
scan_disk_partition_iter () for example.

> +grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +                    unsigned int max_nsectors,
> +                    grub_embed_type_t embed_type,
> +                    grub_disk_addr_t **sectors)
> +{
> +  char buf[GRUB_LVM_LABEL_SIZE];
> +  struct grub_lvm_pv_header *pvh;
> +  struct grub_lvm_pv_header_ext *pvh_ext;
> +  struct grub_diskfilter_pv *pv = NULL;
> +  struct grub_diskfilter_vg *vg = NULL;
> +  struct grub_lvm_disk_locn *dlocn;
> +  grub_uint64_t ba_offset, ba_size, ba_start_sector;
> +  unsigned int i;
> +
> +  if (embed_type != GRUB_EMBED_PCBIOS)
> +    return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> +                      "LVM curently supports only PC-BIOS embedding");
> +  if (disk->partition)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");

Why? What's wrong with PV on partition?

> +  pv = grub_diskfilter_get_pv_from_disk (disk, &vg);
> +  if (! pv)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +
> +  pvh = grub_lvm_get_pvh(disk, buf);
> +  if (! pvh)
> +    return grub_error (GRUB_ERR_BUG, "disk isn't LVM");
> +

Please, more specific error message, so they are actually useful to
identify what happened.

None of those is bug; this should probably be GRUB_ERR_BAD_DEVICE.

> +  dlocn = pvh->disk_areas_xl;
> +
> +  /* skip past the data area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +  /* and the metadata area list */
> +  while (dlocn->offset)
> +    dlocn++;
> +  dlocn++;
> +
> +  pvh_ext = (struct grub_lvm_pv_header_ext*)dlocn;
> +  if (! pvh_ext->version_xl)
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader 
> area");
> +

Again, this is not a bug.

> +  dlocn = pvh_ext->disk_areas_xl;
> +  ba_offset = grub_le_to_cpu64 (dlocn->offset);
> +  ba_size = grub_le_to_cpu64 (dlocn->size);
> +  if (! (ba_offset && ba_size))
> +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a bootloader 
> area");

And this.

> +  /* could be worked around with extra arithmetic if this actually happens */
> +  if (ba_offset % GRUB_DISK_SECTOR_SIZE)
> +    return grub_error (
> +      GRUB_ERR_BUG, "LVM bootloader area is improperly aligned");
> +  ba_start_sector = ba_offset / GRUB_DISK_SECTOR_SIZE;
> +
> +  *nsectors = ba_size / GRUB_DISK_SECTOR_SIZE;
> +  if (*nsectors > max_nsectors)
> +    *nsectors = max_nsectors;
> +
> +  *sectors = grub_malloc (*nsectors * sizeof (**sectors));
> +  if (!*sectors)
> +    return grub_errno;
> +  for (i = 0; i < *nsectors; i++)
> +    (*sectors)[i] = ba_start_sector + i;
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
>  
>  
>  static struct grub_diskfilter grub_lvm_dev = {
> 
> === modified file 'include/grub/diskfilter.h'
> --- include/grub/diskfilter.h   2012-06-25 15:36:50 +0000
> +++ include/grub/diskfilter.h   2013-09-25 08:59:05 +0000
> @@ -75,6 +75,9 @@
>  #ifdef GRUB_UTIL
>    char **partmaps;
>  #endif
> +  /* Optional bootloader embedding area */
> +  grub_uint64_t ba_offset;
> +  grub_uint64_t ba_size;
>  };
>  
>  struct grub_diskfilter_lv {
> 
> === modified file 'include/grub/emu/hostdisk.h'
> --- include/grub/emu/hostdisk.h 2013-08-22 14:50:12 +0000
> +++ include/grub/emu/hostdisk.h 2013-09-25 07:59:06 +0000
> @@ -44,9 +44,16 @@
>  char *
>  grub_util_get_ldm (grub_disk_t disk, grub_disk_addr_t start);
>  int
> +grub_util_is_lvm (grub_disk_t disk);
> +int
>  grub_util_is_ldm (grub_disk_t disk);
>  #ifdef GRUB_UTIL
>  grub_err_t
> +grub_util_lvm_embed (struct grub_disk *disk, unsigned int *nsectors,
> +                    unsigned int max_nsectors,
> +                    grub_embed_type_t embed_type,
> +                    grub_disk_addr_t **sectors);
> +grub_err_t
>  grub_util_ldm_embed (struct grub_disk *disk, unsigned int *nsectors,
>                      unsigned int max_nsectors,
>                      grub_embed_type_t embed_type,
> 
> === modified file 'include/grub/lvm.h'
> --- include/grub/lvm.h  2012-01-29 13:28:01 +0000
> +++ include/grub/lvm.h  2013-09-25 08:32:31 +0000
> @@ -62,6 +62,13 @@
>    struct grub_lvm_disk_locn disk_areas_xl[0];  /* Two lists */
>  } __attribute__ ((packed));
>  
> +struct grub_lvm_pv_header_ext {
> +  grub_uint32_t version_xl;
> +  grub_uint32_t flags_xl;
> +  /* NULL-terminated list of bootloader embedding areas */
> +  struct grub_lvm_disk_locn disk_areas_xl[0];
> +} __attribute__ ((packed));
> +
>  #define GRUB_LVM_FMTT_MAGIC 
> "\040\114\126\115\062\040\170\133\065\101\045\162\060\116\052\076"
>  #define GRUB_LVM_FMTT_VERSION 1
>  #define GRUB_LVM_MDA_HEADER_SIZE 512
> 
> === modified file 'util/setup.c'
> --- util/setup.c      2016-05-08 01:00:00.000000000 +0200
> +++ util/setup.c      2016-05-08 02:00:00.000000000 +0200
> @@ -390,7 +390,7 @@
>        .container = dest_dev->disk->partition,
>        .multiple_partmaps = 0
>      };
> -    int is_ldm;
> +    int is_ldm, is_lvm;
>      grub_err_t err;
>      grub_disk_addr_t *sectors;
>      int i;
> @@ -423,8 +423,9 @@
>        grub_errno = GRUB_ERR_NONE;
>  
>      is_ldm = grub_util_is_ldm (dest_dev->disk);
> +    is_lvm = grub_util_is_lvm (dest_dev->disk);
>  
> -    if (fs_probe)
> +    if (!is_lvm && fs_probe)

No, we want to probe for FS here to eliminate corner case of both PV and
FS metadata.

>        {
>       if (!fs && !ctx.dest_partmap)
>         grub_util_error (_("unable to identify a filesystem in %s; safety 
> check can't be performed"),
> @@ -462,7 +463,7 @@
>  
>        }
>  
> -    if (! ctx.dest_partmap && ! fs && !is_ldm)
> +    if (! ctx.dest_partmap && ! fs && !is_ldm && !is_lvm)
>        {
>       grub_util_warn ("%s", _("Attempting to install GRUB to a partitionless 
> disk or to a partition.  This is a BAD idea."));
>       goto unable_to_embed;

And you need to add check for LVM here

    if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && fs))
      {
        grub_util_warn ("%s", _("Attempting to install GRUB to a disk
with multiple partition labels.  This is not supported yet."));
        goto unable_to_embed;
      }


> @@ -499,7 +500,10 @@
>        maxsec = ((0x78000 - GRUB_KERNEL_I386_PC_LINK_ADDR)
>               >> GRUB_DISK_SECTOR_BITS);
>  
> -    if (is_ldm)
> +    if (is_lvm)
> +      err = grub_util_lvm_embed (dest_dev->disk, &nsec, maxsec,
> +                                GRUB_EMBED_PCBIOS, &sectors);
> +    else if (is_ldm)
>        err = grub_util_ldm_embed (dest_dev->disk, &nsec, maxsec,
>                                GRUB_EMBED_PCBIOS, &sectors);
>      else if (ctx.dest_partmap)
> @@ -602,7 +606,7 @@
>  
>    if (dest_dev->disk->dev->id != root_dev->disk->dev->id)
>      grub_util_error ("%s", _("embedding is not possible, but this is 
> required for "
> -                          "RAID and LVM install"));
> +                          "RAID install"));
>  
>    {
>      grub_fs_t fs;