From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mT1NC-0003mL-Lb for mharc-grub-devel@gnu.org; Wed, 22 Sep 2021 08:29:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41004) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mT1NB-0003jg-44 for grub-devel@gnu.org; Wed, 22 Sep 2021 08:29:13 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:49387) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1mT1N9-0006Fb-7q for grub-devel@gnu.org; Wed, 22 Sep 2021 08:29:12 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:39596 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2115875AbhIVM3G (ORCPT ); Wed, 22 Sep 2021 14:29:06 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Wed, 22 Sep 2021 14:29:04 +0200 From: Daniel Kiper To: Michael Chang Cc: grub-devel@gnu.org, Olav Reinert Subject: Re: [PATCH v2] diskfilter: use nodes in logical volume's segment as member device Message-ID: <20210922122904.4al7jwru6s7nlye7@tomti.i.net-space.pl> References: <20210909130229.24756-1-mchang@suse.com> <20210915160009.mr6th5ndqabwvpap@tomti.i.net-space.pl> <20210917073431.GA8348@mazu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210917073431.GA8348@mazu> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Sep 2021 12:29:13 -0000 On Fri, Sep 17, 2021 at 03:34:32PM +0800, Michael Chang via Grub-devel wrote: > On Wed, Sep 15, 2021 at 06:00:09PM +0200, Daniel Kiper wrote: > > 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 > > > Tested-by: Olav Reinert > > > --- > > > 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]. > > Sorry, I should have double checked grub-dev document before sending the > patch. > > I'll fix this in next version. Cool! Thanks! > > > + 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. > > Hm. I think it is not necessary because grub_error() has been called by > grub_malloc() when it fails, so we should just return NULL and don't try > to override the error number and message already set. > > Even we can preserve grub_malloc's error via grub_error_push (), still > it makes little sense to me to duplicate the effort here ... > > Would you please shed more light on this ? OK, leave "goto fail;" as is... Daniel