Re: [PATCH] Install to LVM PVs

[Dryden Personalis]

Andrei Borzenkov schreef op 08-05-2016 8:05:

> 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?

Hi Andrei,

I must say I have for the most part just copied the patch as it was, and 
just tried to fix it so that the patch would actually apply in full. I 
have no experience yet with Grub code, so I may not be able to answer 
any or all of your questions.

I must say that I did try the former, but not the latter yet. My system 
is not installed though so I can try at any point. What I haven't tried 
yet is booting FROM the PV in the sense of loading a system from it (and 
not just the boot loader) so at this point I have only booted a 
different system from the boot loader on my /dev/sda residing in a 
partitionless PV.

Testing a PV with partitions is trivial, I will do that shortly.

I plan on readily installing a system on that disk after I have my 
scheme sorted out, so then I can test for real the real use case (but 
only in the case of partitionless).


> 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.
> > === 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
> > @@ -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?

That was just in the original patch. You can see the dates. I put some 
artificial times on the latest hunk, from yesterday. I didn't do 
anything really to the hunks from 2012/2013.

So I don't know the rationale, we'd have to ask the original author, it 
probably makes no sense from my POV, but it does seem nicer lol.

> > -  /* 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"?

I don't know but I think the guy wanted to simplify by only having one 
block? Or something of the kind?

> >       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.

Alright this is beyond me at this point, I'd prefer if you gave feedback 
on the other items first.

But do you mean not to use grub_lvm_detect?

Are you saying that "array->driver" would already be loaded before we do 
grub_lvm_detect? (Or rather, before grub_util_is_lvm)? You are meaning 
to imply to just use a different identification measure right, because 
apparently grub_lvm_detect has already been done in that case.


> > +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?

Just for my recollection:

* The function "grub_util_info" now calls "grub_util_is_lvm"
* grub_util_is_lvm only scans the first 4 sectors of a disk, so it will 
not find anything on a partitioned /dev/sda
* The above function grub_util_lvm_embed is only called when it does 
return true.

* If you don't have a VG in the PV, the error never gets triggered.

I don't know, the guy designed the feature for partitionless. Does it 
make sense to use if you don't have partitions? If it is all equivalent 
to Grub, I don't see why it shouldn't be possible.

It means you'd want to embed grub in a partition that has a PV. Don't 
see why it shouldn't be possible.

I can remove it but I will have to find out how to boot from an actual 
partition to test it :p.

Maybe easiest is to clear the MBR and set bootable flag? Don't know if 
that works for grub. If default msdos label has such code....

> > +  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.

Yes, can do. If I understand get_pvh, but that shouldn't be hard.

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

Aye, it even reports as a warning while being fatal. Can do.

> > +    return grub_error (GRUB_ERR_BUG, "LVM PV doesn't have a 
> > bootloader area");
> > +
>
> Again, this is not a bug.

Roger. Will find some other error if I can.

> > -    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.

You mean you want to change it to:

...something that gives a different error as in:

if (fs_probe) {
  if (is_lvm && fs ) {
    grub_util_error (_("a filesystem signature is present on %s, but 
embedding in an LVM PV was requested"), dest_dev->disk->name);
  } else if (!is_lvm && !fs && !ctx.dest_partmap) {
    grub_util_error (_("unable to identify a filesystem in %s; safety 
check can't be performed"), dest_dev->disk->name);
  }
  <therest>
}

That is just my coding style, I understand yours.

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

I take it you mean:

if (!is_lvm && (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || 
(is_ldm && fs)))

?

Or do you mean:

if (ctx.multiple_partmaps || (ctx.dest_partmap && fs) || (is_ldm && fs) 
|| is_lvm))

?

I take it the former: with is_lvm there should be no warning.

I will work on the changes and post a new patch (not git format, but 
shouldn't matter).