From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mR8OW-0003l8-EK for mharc-grub-devel@gnu.org; Fri, 17 Sep 2021 03:34:48 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:35582) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mR8OU-0003kx-Dv for grub-devel@gnu.org; Fri, 17 Sep 2021 03:34:46 -0400 Received: from de-smtp-delivery-102.mimecast.com ([194.104.111.102]:38996) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mR8OR-0007Vj-P7 for grub-devel@gnu.org; Fri, 17 Sep 2021 03:34:45 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=mimecast20200619; t=1631864080; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cTkonapffNcAzl6L529gmj4PvAecB22BA8CNh30xCo4=; b=ZTEi9DjeR3Nmk3qlfdjct7ArsGcQmfAhgcJwehq2lGWFmW6Aucq5SRqzFuw0veGvqH9MdF HgtC/LxGrKLSkluzg8vr0Iu2/7tFrCVtZQOwKlG4bPP74XERGu3jL/7z37EDc0fawbZ9Lw i73OOZYhFg1457ZzKEwn3+jVUIcPClM= Received: from EUR05-AM6-obe.outbound.protection.outlook.com (mail-am6eur05lp2107.outbound.protection.outlook.com [104.47.18.107]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-22-TO3z5xxiNlykfuDRqO6q1g-1; Fri, 17 Sep 2021 09:34:39 +0200 X-MC-Unique: TO3z5xxiNlykfuDRqO6q1g-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=m3Wq4eIIFpdB6o6fgBl5Hrv9gE6pxveeuroiY6KDobb9yRd05gmN8CfwCNR2mMO7VYGn9UwBQVzQlbwz+hK3I2htZXnAgPYI+zTkdM3Yed0rKSbfQs4XxU/D5hIqHapC9hf1hGL3qmSwVN/h+1m4Tsv3MHVt0diCzV4X6M69wZHs1ZNYH0Kk7yZ2BbOG8P4eBsItPfTlOytpkWis/g4qjTRlOKkhuNU1SipnFIEorVtI5JSohvywK3KVVCK5hAl9L8Z3wUyJYBzs+JpKSz9eT48OHzYSBgthojgNQ1jJmdSqrR9/8FTqte3E99Cqh0Bu2yhWpGipeosOk0jvNIttDA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=cTkonapffNcAzl6L529gmj4PvAecB22BA8CNh30xCo4=; b=KqjQ+l3pB15FB2gWX+g4Gd2kEKZ5PTnUhlfeZ23gGMxSVB4wqpkyfHOaUMsQAZVQgWTq55wVBvXz8nrjTdsfTQhUXO/jK+zvi+uI3JO/mZGLCn4xfOp0KpInpy4ZZf0t6hsn1fpvWJxsV2gV+4vLmEDyjMwT5FSTNYpn7jv4xgN5GsdCqUykjGGogzYX0fAgXVZIc93kk/e/I77lkcj1Pbwr1aUtKXEoEHxO/+tL7Yb6sN/v52DKKj0ZfCfpvkwKWBrw857ZbAvN0TZ9e5IGZ0+52Ppz/ZEZsNiyXEfBujp5cWDyfSRVv82FqH4PeYJA1imTyN9lypFfKX914X9wgA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Authentication-Results: net-space.pl; dkim=none (message not signed) header.d=none;net-space.pl; dmarc=none action=none header.from=suse.com; Received: from DU2PR04MB8648.eurprd04.prod.outlook.com (2603:10a6:10:2df::21) by DU2PR04MB8534.eurprd04.prod.outlook.com (2603:10a6:10:2d5::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4523.17; Fri, 17 Sep 2021 07:34:37 +0000 Received: from DU2PR04MB8648.eurprd04.prod.outlook.com ([fe80::7c78:3729:f82b:6802]) by DU2PR04MB8648.eurprd04.prod.outlook.com ([fe80::7c78:3729:f82b:6802%6]) with mapi id 15.20.4523.017; Fri, 17 Sep 2021 07:34:37 +0000 Date: Fri, 17 Sep 2021 15:34:32 +0800 From: Michael Chang To: Daniel Kiper Cc: Olav Reinert , grub-devel@gnu.org Subject: Re: [PATCH v2] diskfilter: use nodes in logical volume's segment as member device Message-ID: <20210917073431.GA8348@mazu> References: <20210909130229.24756-1-mchang@suse.com> <20210915160009.mr6th5ndqabwvpap@tomti.i.net-space.pl> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210915160009.mr6th5ndqabwvpap@tomti.i.net-space.pl> User-Agent: Mutt/1.10.1 (2018-07-13) X-ClientProxiedBy: HK2PR02CA0215.apcprd02.prod.outlook.com (2603:1096:201:20::27) To DU2PR04MB8648.eurprd04.prod.outlook.com (2603:10a6:10:2df::21) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from localhost (2001:b011:30d0:3f43:6600:6aff:fe77:a7be) by HK2PR02CA0215.apcprd02.prod.outlook.com (2603:1096:201:20::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4523.14 via Frontend Transport; Fri, 17 Sep 2021 07:34:36 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7fde6492-53e4-42c8-de93-08d979ad9a1a X-MS-TrafficTypeDiagnostic: DU2PR04MB8534: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:6790; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: DUdMpy8TegoeEZhxKh0KhzKCbEUGwtduRlEvLodrkETCwmNNqUVeV/27M4GxkmVPv2OjXupj+8Tr5GUyVotCVD23FehnLdVIKkDQpEbB/fmYij+Hbv8K23kILTorl0A8gpBGcY/FwJKJhbMQcQPysG/4ORSZvAXgmcBsQrrilPhkdnCur1lvHwkaGQPs1681ZkvEgAHMUIWfGpf2nSubY3LBd/O6J1gflUhBKk5MxlT2npSCAQ+c00g11IbGAOwyXsDmHeO+JhBefL/26WLmpERSX7BpE6kJqx6iPw5QtCWtagzGWi+yvmyf/pKp3vWquyX/QXC3EN2axH/slER5Z7phVo8YwD7WpUgxGqrfoeshT3uAYqwjEikxFvtLXNTYoIe9tzOmBkhn96GoKvs30f8AIm8mAeaWCIzmWX+MyFC43qWIC4apx6gDW8iXjABqktYRuNEmF+bYYrNcMKzXfUP5GvLpbZteSMHKaaFCVL97wPOqvrdS0IphVB+DoCkaWb9l53RJCn+3ulextggs5/Jq0kdIztYx4jLmoDOWgXWQRG4p/7WyOYpUSUdcnqZl6LuveOYGMcHT2zSKrfRxroDVkhZZI+3bDC9dem0chabRsptgihwxLIdy/HhaFONUZHHi3WTLF1rYAu/S9WKVGydNKq6Oy2+EA736yntCVF479r8OWG1unf6KY0dNTytzwaplA7SUAz8F1Ju17UWXfbuM6ft5oC4axPuUPKWRRHk= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DU2PR04MB8648.eurprd04.prod.outlook.com; PTR:; CAT:NONE; SFS:(7916004)(366004)(66556008)(9686003)(6666004)(66946007)(186003)(316002)(6486002)(5660300002)(66476007)(38100700002)(33656002)(6916009)(1076003)(8676002)(8936002)(4326008)(508600001)(966005)(6496006)(2906002)(33716001)(86362001)(83380400001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tr0nhi/tRtY8hEmodwjBJvFG+D/DdW932rmlKU4LAGcBT00WsJ3yndaEGwXZ?= =?us-ascii?Q?kutzXNLaTfhizu79CfPsabgsIIgCU058682OOLiUFlcV6BZ50U1Teo9ZekQn?= =?us-ascii?Q?5ip2LRnBESuhi3qvgD1OnPrTFcqBZm/L7x2NPu1N/stz4b1VpyhvQHba9M+t?= =?us-ascii?Q?4Em28HSbBvJZ80xHmfDj9t0rtxznFf2AHLaIRFVU5qrVPzwqN9wvob4hMYTX?= =?us-ascii?Q?ZCrK53hDKHzOJPszUoy+tW62/1inalmumRXPK395bOBZyuyZgM0IJWNP4Z3R?= =?us-ascii?Q?wuHSEqxia7Ur8BQkykPEMJxFgBY0vt7Y0iSHI4EBIjJyih8yliuKvhsIg+5v?= =?us-ascii?Q?jCb2Bp6/ZPdgEhN3KK3u42B+tjk11f6/OZS6KBjQUL0HQkN8Kep91dd/aIWj?= =?us-ascii?Q?QQ25mrwm18tUeN+FFh/HY5heWk9Dw/sE2nBG2WYZfx0fTw1nOLoojPWuX7Vs?= =?us-ascii?Q?E9GQ7KWLCveMhheS5DA+BvM0KFL58tiikw7zWAGk+RI7QPHPnuWXdJTtcKAt?= =?us-ascii?Q?OTKS65v+JO6QVo9okeX2BD4qgxpZLTUpe3lRM3TrLoaA8OQpsk+Yrf1rLBZp?= =?us-ascii?Q?VskvtRRkNm7kB2CXvbNiFkGGfrZ9P4Ydu1EFJYc+XKfORmNy2t3XMET5hFNe?= =?us-ascii?Q?MzEkY+wBiSzuKVYtIvYUHAwd8FZw4wmrc+hIRkyM1zCnel7DT+x+ACmPemFS?= =?us-ascii?Q?wDxmCsu/momadw1iOdcGX9A2I2gsnDMT6+nkqesm2j1aloEhExTdKVSUagwt?= =?us-ascii?Q?BwujRphyLh3FmhGzUCee31mfi3KL2zv/KisHkBpdg01DbblVwTHXNVLvts5q?= =?us-ascii?Q?B5RcJiMNElXmkuZwAN2AwtkMjezTqdxNIGQu5Z5sI0RRcgCM4hbaoybhllkl?= =?us-ascii?Q?d1rGmnz/F2igdStYgP96X5yObB/tiWKDP/ug1KLywcfmdzhrkZlPSGBrwnOW?= =?us-ascii?Q?2AkBy78IA6o4Ngd/rk8Xj9C1wH7rzoU4DpAIyD3v8VnvJJYSeCWWfTA8Wh2r?= =?us-ascii?Q?y3Z+yuVYlbklIDm61zzVhb6w2bDUiW4LpK7+e8Axg7COS98vUH4jMfSlFm2W?= =?us-ascii?Q?o3AZOUvh7FYZ7N9PY5e6CV2c0kHF3eRdv2lG36DGB3Umd/In4OYJhgwg84l+?= =?us-ascii?Q?h0dcUQlD7Lq2mFhgNRvUiK1q7661hlmUHu7UY2AWRftx8UainPmRAOgtwc1N?= =?us-ascii?Q?45h0Hbb51GuTkdswnkMWQFyj6uOFGF1AbuoffBsSAv9SRo/R1eHD00bqAf2H?= =?us-ascii?Q?Lr9nNEcFO8CfS4v+y20hspnvFv2nGJaADT/HJ80HQ0L/ROJd2pURWjDxMW1/?= =?us-ascii?Q?ckRXJ7fn3GE2gDa9007FCNkgp4Q1j1K1b7UpgoEDPMi535sj65jl44/9BX7a?= =?us-ascii?Q?n0X9lh40japJq6oIHv8A1Y4gjvLr?= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7fde6492-53e4-42c8-de93-08d979ad9a1a X-MS-Exchange-CrossTenant-AuthSource: DU2PR04MB8648.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Sep 2021 07:34:37.2854 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: NMICfG+OY8AuNIllrlFhu5fND1oI0LA15zzM+gmRo9KSvkqgOI/DkHZYGCi8SWVW X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8534 Received-SPF: pass client-ip=194.104.111.102; envelope-from=mchang@suse.com; helo=de-smtp-delivery-102.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, MSGID_FROM_MTA_HEADER=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, 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: Fri, 17 Sep 2021 07:34:46 -0000 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. > > > + 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 ? > > > + 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. OK. > > > + > > Please drop this empty line. OK. > > > + while (list != NULL) > > + { > > + tmp = list; > > + list = list->next; > > + grub_free (tmp); > > + } > > + > > + return NULL; > > } Thanks a lot for review. Michael > > Daniel > > [1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments