From: Daniel Kiper <dkiper@net-space.pl>
To: Michael Chang <mchang@suse.com>
Cc: Olav Reinert <seroton10@gmail.com>, grub-devel@gnu.org
Subject: Re: [PATCH v2] diskfilter: use nodes in logical volume's segment as member device
Date: Wed, 15 Sep 2021 18:00:09 +0200 [thread overview]
Message-ID: <20210915160009.mr6th5ndqabwvpap@tomti.i.net-space.pl> (raw)
In-Reply-To: <20210909130229.24756-1-mchang@suse.com>
On Thu, Sep 09, 2021 at 09:02:29PM +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.
>
> localhost:~ # lvs -o lv_name,vg_name,devices /dev/system/root
> LV VG Devices
> root system /dev/vda2(512)
>
> localhost:~ # lvdisplay --maps /dev/system/root
> --- Logical volume ---
> ...
> --- Segments ---
> Logical extents 0 to 4604:
> Type linear
> Physical volume /dev/vda2
> Physical extents 512 to 5116
>
> As shown above, we can know system-root lv uses only /dev/vda2 to
> allocate it's extents, or we can say that /dev/vda2 is the member device
> comprising the system-root lv.
>
> It is important to be precise on the member devices, because that helps
> to avoid pulling in excessive dependency. Let's use an example to
> demonstrate why it is needed.
>
> localhost:~ # findmnt /
> TARGET SOURCE FSTYPE OPTIONS
> / /dev/mapper/system-root ext4 rw,relatime
>
> localhost:~ # pvs
> PV VG Fmt Attr PSize PFree
> /dev/mapper/data system lvm2 a-- 1020.00m 0
> /dev/vda2 system lvm2 a-- 19.99g 0
>
> localhost:~ # cryptsetup status /dev/mapper/data
> /dev/mapper/data is active and is in use.
> type: LUKS1
> cipher: aes-xts-plain64
> keysize: 512 bits
> key location: dm-crypt
> device: /dev/vdb
> sector size: 512
> offset: 4096 sectors
> size: 2093056 sectors
> mode: read/write
>
> localhost:~ # vgs
> VG #PV #LV #SN Attr VSize VFree
> system 2 3 0 wz--n- 20.98g 0
>
> localhost:~ # lvs -o lv_name,vg_name,devices
> LV VG Devices
> data system /dev/mapper/data(0)
> root system /dev/vda2(512)
> swap system /dev/vda2(0)
>
> We can learn from above that /dev/mapper/data is an encrypted volume and
> also gets assigned to volume group 'system' as one of it's physical
> volumes. And also it is not used by root device,
> /dev/mapper/system-root, for allocating extents, so it shouldn't be
> taking part in the process of setting up grub to access root device.
>
> However running grub-install reports error as volume group 'system'
> contains encrypted volume.
>
> 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 be booted
> unattended. Additionally, 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 exact physical volume,
> /dev/vda2, rightly used by system-root from the example above, thus
> grub-install will not error out because the excessive encrypted device
> to boot the root device is not configured.
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> Tested-by: Olav Reinert <seroton10@gmail.com>
> ---
> grub-core/disk/diskfilter.c | 60 ++++++++++++++++++++++++++-----------
> 1 file changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 6eb2349a6..3d02d56ec 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,27 +333,51 @@ 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 (seg->nodes[j].pv != NULL)
> {
> - /* 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;
> + pv = seg->nodes[j].pv;
> +
> + 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. */
Again, please fix formatting of this comment if you are moving it [1].
> + grub_util_warn (_("Couldn't find physical volume `%s'."
> + " Some modules may be missing from core image."),
> + pv->name);
> + continue;
> + }
> +
> + for (tmp = list; tmp != NULL; tmp = tmp->next)
> + if (!grub_strcmp (tmp->disk->name, pv->disk->name))
> + break;
> + if (tmp != NULL)
> + continue;
> +
> + tmp = grub_malloc (sizeof (*tmp));
> + if (!tmp)
> + goto fail;
Please call grub_error() here.
> + 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;
> +
> +fail:
Please add a space before label.
> +
Please drop this empty line.
> + while (list != NULL)
> + {
> + tmp = list;
> + list = list->next;
> + grub_free (tmp);
> + }
> +
> + return NULL;
> }
Daniel
[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments
next prev parent reply other threads:[~2021-09-15 16:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-09 13:02 [PATCH v2] diskfilter: use nodes in logical volume's segment as member device Michael Chang
2021-09-15 16:00 ` Daniel Kiper [this message]
2021-09-17 7:34 ` Michael Chang
2021-09-22 12:29 ` Daniel Kiper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210915160009.mr6th5ndqabwvpap@tomti.i.net-space.pl \
--to=dkiper@net-space.pl \
--cc=grub-devel@gnu.org \
--cc=mchang@suse.com \
--cc=seroton10@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.