* [PATCH] diskfilter: use nodes in logical volume's segment as member device
@ 2021-08-02 9:40 Michael Chang
2021-08-09 15:34 ` Daniel Kiper
0 siblings, 1 reply; 3+ messages in thread
From: Michael Chang @ 2021-08-02 9:40 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: Michael Chang, Olav Reinert
Currently the grub_diskfilter_memberlist function returns all physical
volumes added to a volume group to which a logical volume (LV) belongs.
However this is suboptimal as it doesn't fit the intended behavior of
returning underlying devices that make up the LV. To give a clear
picture, the result should be identical to running commands below to
display the logical volumes with underlying physical volumes in use.
lvs -o +devices /dev/system/root
lvdisplay --maps /dev/system/root
This change is required if any part of the PV not used by root LV is
encrypted. Using this lvm setup as an example:
localhost:~ # lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
vda 253:0 0 20G 0 disk
├─vda1 253:1 0 8M 0 part
└─vda2 253:2 0 20G 0 part
├─system-swap 254:0 0 1.4G 0 lvm [SWAP]
└─system-root 254:1 0 18.6G 0 lvm /
vdb 253:16 0 10M 0 disk
└─data 254:2 0 8M 0 crypt
└─system-data 254:3 0 4M 0 lvm /data
Running grub-install would end up with error because "system" VG
contains /dev/vdb which is encrypted.
error: attempt to install to encrypted disk without cryptodisk
enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'.
Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that
is not always acceptable since the server may need to boot unattended,
in addition typing passphase for every system startup can be a big
hassle of which most users would like to avoid.
This patch solves the problem by returning physical volumes, /dev/vda2,
rightly used by system-root in the example above, thus changing how
grub-install perceives the underlying block device to boot and avoids
the error from happening.
Signed-off-by: Michael Chang <mchang@suse.de>
Tested-by: Olav Reinert <seroton10@gmail.com>
---
grub-core/disk/diskfilter.c | 44 +++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 17 deletions(-)
diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index 6eb2349a6..595f5f70f 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -300,6 +300,8 @@ grub_diskfilter_memberlist (grub_disk_t disk)
grub_disk_dev_t p;
struct grub_diskfilter_vg *vg;
struct grub_diskfilter_lv *lv2 = NULL;
+ struct grub_diskfilter_segment *seg;
+ unsigned int i, j;
if (!lv->vg->pvs)
return NULL;
@@ -331,25 +333,33 @@ grub_diskfilter_memberlist (grub_disk_t disk)
}
}
- for (pv = lv->vg->pvs; pv; pv = pv->next)
- {
- if (!pv->disk)
+ for (i = 0, seg = lv->segments; i < lv->segment_count; i++, seg++)
+ for (j =0; j < seg->node_count; ++j)
+ if ((pv = seg->nodes[j].pv))
{
- /* TRANSLATORS: This message kicks in during the detection of
- which modules needs to be included in core image. This happens
- in the case of degraded RAID and means that autodetection may
- fail to include some of modules. It's an installation time
- message, not runtime message. */
- grub_util_warn (_("Couldn't find physical volume `%s'."
- " Some modules may be missing from core image."),
- pv->name);
- continue;
+
+ if (!pv->disk)
+ {
+ /* TRANSLATORS: This message kicks in during the detection of
+ which modules needs to be included in core image. This happens
+ in the case of degraded RAID and means that autodetection may
+ fail to include some of modules. It's an installation time
+ message, not runtime message. */
+ grub_util_warn (_("Couldn't find physical volume `%s'."
+ " Some modules may be missing from core image."),
+ pv->name);
+ continue;
+ }
+
+ for (tmp = list; tmp; tmp = tmp->next)
+ if (grub_strcmp (tmp->disk->name, pv->disk->name) == 0)
+ continue;
+
+ tmp = grub_malloc (sizeof (*tmp));
+ tmp->disk = pv->disk;
+ tmp->next = list;
+ list = tmp;
}
- tmp = grub_malloc (sizeof (*tmp));
- tmp->disk = pv->disk;
- tmp->next = list;
- list = tmp;
- }
return list;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] diskfilter: use nodes in logical volume's segment as member device
2021-08-02 9:40 [PATCH] diskfilter: use nodes in logical volume's segment as member device Michael Chang
@ 2021-08-09 15:34 ` Daniel Kiper
2021-08-11 5:25 ` Michael Chang
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Kiper @ 2021-08-09 15:34 UTC (permalink / raw)
To: Michael Chang, Michael Chang; +Cc: grub-devel, Olav Reinert
On Mon, Aug 02, 2021 at 05:40:20PM +0800, Michael Chang via Grub-devel wrote:
> Currently the grub_diskfilter_memberlist function returns all physical
> volumes added to a volume group to which a logical volume (LV) belongs.
> However this is suboptimal as it doesn't fit the intended behavior of
> returning underlying devices that make up the LV. To give a clear
> picture, the result should be identical to running commands below to
> display the logical volumes with underlying physical volumes in use.
>
> lvs -o +devices /dev/system/root
> lvdisplay --maps /dev/system/root
May I ask you to provide output from these two commands too? Additionally,
I think it would be useful to have output from "pvs" and "lvs" commands
in the commit message. I know "lsblk" shows you most information but
I think both "pvs" and "lvs" are giving more readable output.
> This change is required if any part of the PV not used by root LV is
> encrypted. Using this lvm setup as an example:
>
> localhost:~ # lsblk
> NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
> vda 253:0 0 20G 0 disk
> ├─vda1 253:1 0 8M 0 part
> └─vda2 253:2 0 20G 0 part
> ├─system-swap 254:0 0 1.4G 0 lvm [SWAP]
> └─system-root 254:1 0 18.6G 0 lvm /
> vdb 253:16 0 10M 0 disk
> └─data 254:2 0 8M 0 crypt
> └─system-data 254:3 0 4M 0 lvm /data
>
> Running grub-install would end up with error because "system" VG
> contains /dev/vdb which is encrypted.
>
> error: attempt to install to encrypted disk without cryptodisk
> enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'.
>
> Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that
> is not always acceptable since the server may need to boot unattended,
s/,/./
> in addition typing passphase for every system startup can be a big
s/in addition/Additionally,/
> hassle of which most users would like to avoid.
>
> This patch solves the problem by returning physical volumes, /dev/vda2,
> rightly used by system-root in the example above, thus changing how
> grub-install perceives the underlying block device to boot and avoids
> the error from happening.
>
> Signed-off-by: Michael Chang <mchang@suse.de>
> Tested-by: Olav Reinert <seroton10@gmail.com>
> ---
> grub-core/disk/diskfilter.c | 44 +++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 6eb2349a6..595f5f70f 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -300,6 +300,8 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> grub_disk_dev_t p;
> struct grub_diskfilter_vg *vg;
> struct grub_diskfilter_lv *lv2 = NULL;
> + struct grub_diskfilter_segment *seg;
> + unsigned int i, j;
>
> if (!lv->vg->pvs)
> return NULL;
> @@ -331,25 +333,33 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> }
> }
>
> - for (pv = lv->vg->pvs; pv; pv = pv->next)
> - {
> - if (!pv->disk)
> + for (i = 0, seg = lv->segments; i < lv->segment_count; i++, seg++)
> + for (j =0; j < seg->node_count; ++j)
s/=0/= 0/
> + if ((pv = seg->nodes[j].pv))
if (seg->nodes[j].pv != NULL)
...and then later...
pv = seg->nodes[j].pv
> {
> - /* TRANSLATORS: This message kicks in during the detection of
> - which modules needs to be included in core image. This happens
> - in the case of degraded RAID and means that autodetection may
> - fail to include some of modules. It's an installation time
> - message, not runtime message. */
> - grub_util_warn (_("Couldn't find physical volume `%s'."
> - " Some modules may be missing from core image."),
> - pv->name);
> - continue;
> +
> + if (!pv->disk)
> + {
> + /* TRANSLATORS: This message kicks in during the detection of
> + which modules needs to be included in core image. This happens
> + in the case of degraded RAID and means that autodetection may
> + fail to include some of modules. It's an installation time
> + message, not runtime message. */
Please fix this comment formatting if you move it.
> + grub_util_warn (_("Couldn't find physical volume `%s'."
> + " Some modules may be missing from core image."),
> + pv->name);
> + continue;
> + }
> +
> + for (tmp = list; tmp; tmp = tmp->next)
tmp != NULL please...
> + if (grub_strcmp (tmp->disk->name, pv->disk->name) == 0)
!grub_strcmp() instead of grub_strcmp() == 0
> + continue;
I know what you mean but this "continue" does not make much sense here...
> +
> + tmp = grub_malloc (sizeof (*tmp));
Please do not ignore grub_malloc() failures.
Daniel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] diskfilter: use nodes in logical volume's segment as member device
2021-08-09 15:34 ` Daniel Kiper
@ 2021-08-11 5:25 ` Michael Chang
0 siblings, 0 replies; 3+ messages in thread
From: Michael Chang @ 2021-08-11 5:25 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: Michael Chang, Olav Reinert
On Mon, Aug 09, 2021 at 05:34:41PM +0200, Daniel Kiper wrote:
> On Mon, Aug 02, 2021 at 05:40:20PM +0800, Michael Chang via Grub-devel wrote:
> > Currently the grub_diskfilter_memberlist function returns all physical
> > volumes added to a volume group to which a logical volume (LV) belongs.
> > However this is suboptimal as it doesn't fit the intended behavior of
> > returning underlying devices that make up the LV. To give a clear
> > picture, the result should be identical to running commands below to
> > display the logical volumes with underlying physical volumes in use.
> >
> > lvs -o +devices /dev/system/root
> > lvdisplay --maps /dev/system/root
>
> May I ask you to provide output from these two commands too? Additionally,
> I think it would be useful to have output from "pvs" and "lvs" commands
> in the commit message. I know "lsblk" shows you most information but
> I think both "pvs" and "lvs" are giving more readable output.
It sounds good as that helps better understanding. I'll add these
information to next version.
>
> > This change is required if any part of the PV not used by root LV is
> > encrypted. Using this lvm setup as an example:
> >
> > localhost:~ # lsblk
> > NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
> > vda 253:0 0 20G 0 disk
> > ├─vda1 253:1 0 8M 0 part
> > └─vda2 253:2 0 20G 0 part
> > ├─system-swap 254:0 0 1.4G 0 lvm [SWAP]
> > └─system-root 254:1 0 18.6G 0 lvm /
> > vdb 253:16 0 10M 0 disk
> > └─data 254:2 0 8M 0 crypt
> > └─system-data 254:3 0 4M 0 lvm /data
> >
> > Running grub-install would end up with error because "system" VG
> > contains /dev/vdb which is encrypted.
> >
> > error: attempt to install to encrypted disk without cryptodisk
> > enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'.
> >
> > Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that
> > is not always acceptable since the server may need to boot unattended,
>
> s/,/./
OK.
>
> > in addition typing passphase for every system startup can be a big
>
> s/in addition/Additionally,/
OK.
>
> > hassle of which most users would like to avoid.
> >
> > This patch solves the problem by returning physical volumes, /dev/vda2,
> > rightly used by system-root in the example above, thus changing how
> > grub-install perceives the underlying block device to boot and avoids
> > the error from happening.
> >
> > Signed-off-by: Michael Chang <mchang@suse.de>
> > Tested-by: Olav Reinert <seroton10@gmail.com>
> > ---
> > grub-core/disk/diskfilter.c | 44 +++++++++++++++++++++++--------------
> > 1 file changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> > index 6eb2349a6..595f5f70f 100644
> > --- a/grub-core/disk/diskfilter.c
> > +++ b/grub-core/disk/diskfilter.c
> > @@ -300,6 +300,8 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> > grub_disk_dev_t p;
> > struct grub_diskfilter_vg *vg;
> > struct grub_diskfilter_lv *lv2 = NULL;
> > + struct grub_diskfilter_segment *seg;
> > + unsigned int i, j;
> >
> > if (!lv->vg->pvs)
> > return NULL;
> > @@ -331,25 +333,33 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> > }
> > }
> >
> > - for (pv = lv->vg->pvs; pv; pv = pv->next)
> > - {
> > - if (!pv->disk)
> > + for (i = 0, seg = lv->segments; i < lv->segment_count; i++, seg++)
> > + for (j =0; j < seg->node_count; ++j)
>
> s/=0/= 0/
OK.
>
> > + if ((pv = seg->nodes[j].pv))
>
> if (seg->nodes[j].pv != NULL)
>
> ...and then later...
>
> pv = seg->nodes[j].pv
OK.
>
> > {
> > - /* TRANSLATORS: This message kicks in during the detection of
> > - which modules needs to be included in core image. This happens
> > - in the case of degraded RAID and means that autodetection may
> > - fail to include some of modules. It's an installation time
> > - message, not runtime message. */
> > - grub_util_warn (_("Couldn't find physical volume `%s'."
> > - " Some modules may be missing from core image."),
> > - pv->name);
> > - continue;
> > +
> > + if (!pv->disk)
> > + {
> > + /* TRANSLATORS: This message kicks in during the detection of
> > + which modules needs to be included in core image. This happens
> > + in the case of degraded RAID and means that autodetection may
> > + fail to include some of modules. It's an installation time
> > + message, not runtime message. */
>
> Please fix this comment formatting if you move it.
May I ask what is wrong as it looks good to the indention that has been
adjusted to the new place ?
>
> > + grub_util_warn (_("Couldn't find physical volume `%s'."
> > + " Some modules may be missing from core image."),
> > + pv->name);
> > + continue;
> > + }
> > +
> > + for (tmp = list; tmp; tmp = tmp->next)
>
> tmp != NULL please...
OK.
>
> > + if (grub_strcmp (tmp->disk->name, pv->disk->name) == 0)
>
> !grub_strcmp() instead of grub_strcmp() == 0
OK.
Well. It appears "grub_strcmp() == 0" more popular as it has more
grep hits . But I am fine in doing either way. :)
grep -R '!grub_strcmp' | wc -l
26
grep -R 'grub_strcmp.*== 0' | wc -l
385
>
> > + continue;
>
> I know what you mean but this "continue" does not make much sense here...
Indeed this looks bogus. I will fix this in next version.
>
> > +
> > + tmp = grub_malloc (sizeof (*tmp));
>
> Please do not ignore grub_malloc() failures.
OK.
Thanks a lot for review.
Michael
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-11 5:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-02 9:40 [PATCH] diskfilter: use nodes in logical volume's segment as member device Michael Chang
2021-08-09 15:34 ` Daniel Kiper
2021-08-11 5:25 ` Michael Chang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.