From owner-dev-commits-src-all@freebsd.org Wed Mar 3 17:21:50 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 431755494CC; Wed, 3 Mar 2021 17:21:50 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DrLQQ1QXrz4bKJ; Wed, 3 Mar 2021 17:21:50 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from comporellon.tachypleus.net (unknown [IPv6:2601:405:4a00:acd:cc7b:682b:f804:9afd]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: nwhitehorn/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id D42C523B6D; Wed, 3 Mar 2021 17:21:49 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) To: Warner Losh Cc: Brandon Bergren , "Rodney W. Grimes" , Ed Maste , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202103031253.123CrxKG051357@gndrsh.dnsmgr.net> <14d09680-1036-4a7e-8a0e-c3063cac8bc9@www.fastmail.com> <6e52fee6-a2fd-584f-757e-e77a8f8ea8eb@freebsd.org> From: Nathan Whitehorn Subject: Re: git: 2c26d77d989a - main - Remove /boot/efi from mtree, missed in 0b7472b3d8d2. Message-ID: <91a51b75-9872-d202-53c0-fa1a21dc9cb3@freebsd.org> Date: Wed, 3 Mar 2021 12:21:49 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Content-Language: en-US X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Mar 2021 17:21:50 -0000 On 3/3/21 11:53 AM, Warner Losh wrote: > [clipping non-technical pre-history] > > The installer *does* mount the partition in advance, so checking > whether > there is a mounted file system is a perfectly reasonable test to > do. We > could also check fstab. I would like to understand what is actually= > wrong here first, though. Especially after this misfire -- which is= > problematic for reasons that are still not clear to me, since > there are > a number of standard directories in hier(7) not in mtree -- I want = to > make sure we actually do have consensus about what is changing and > why. > > > At the top level, we default to having directories in mtree unless=20 > there's a good reason not to. We disagree as to whether the installer=20 > should take the presence or absence of the directory as a strong=20 > enough reason to do something. I don't think that's a good reason. > > But leaving that aside, let's say we=C2=A0wanted to reuse the install b= oot=20 > part of the installer to update boot blocks as part of installworld.=20 > If we can talk through that example w/o it in mtree, then we can leave = > it out. The last time I worked through this, though, I thought I'd=20 > talked myself into needing it. > Looking at bootconfig, we could use machdep.bootmethod to determine if = > we need to update the ESP. If we didn't use that, then the ESP=20 > shouldn't be touched. This is, at the moment, x86 centric, but could=20 > trivially be added to architectures (I'm happy to add it). This would=20 > prevent the 'false positive' that's possible in cases where we've=20 > installed UEFI then downgraded to BIOS because of some problem (though = > purely in the context of the installer, I guess this isn't an issue).=20 > Even with your approach, we'd bogusly update an ESP (though one could=20 > argue you might want that). We could also change the code so that=20 > 'unsupported' architectures just didn't update. This is why I think=20 > it's a bit fragile to rely only on the directory being present. It=20 > should have something mounted there. If you wanted to mkfs_dos=C2=A0+ m= kdir=20 > efi at the top level, you could check for that directory if you were=20 > looking for a flag, though that would still update on a BIOS boot the=20 > ESP, and prevent false positives if run as part of an update. I think we would *want* to update an ESP that is mounted but not=20 currently being used. If I set up a dual BIOS/EFI-boot system for some=20 reason and happened to install an update while booted from BIOS, I would = be deeply astonished if my configured-by-the-installer EFI bootloader=20 did not also get updated. (As an aside, I would also much rather the installer use an update=20 utility to set up the ESP than have the update utility use the installer.= ) So here's a proposal, now that everyone is in the CC list: - We add /boot/efi back to mtree, even though I find it kind of weird to = have it there I think we're too close to the release to have a=20 conclusion on this. - We have the installer check for either the ESP directory being an=20 active mountpoint or being in the in-progress fstab, whichever is=20 easiest to implement (they are equivalent for the installer). If that seems OK, I'll post another review for the change. > A long-term project I've had has been to try to update the boot blocks = > as part of installworld or maybe as part of installboot. We have=20 > really poor reuse as a project in this area. Every little=20 > orchestration thing wrote its own thing, and all of them have done it=20 > badly. I was hoping to be able to reuse this code, or modify the=20 > installer to use whatever we come up with there. As part of that, I=20 > had talked myself into thinking we always needed /boot/efi, but I'm=20 > having trouble reconstructing why that is now though I know it had to=20 > do with installed systems and bootstrapping issues... I know I was=20 > worried about questions about 'why isn't /boot/efi on the system by=20 > default so I can mount it' for people that have upgraded, but I recall = > there was more to it than that. With it in mtree, an installworld > (even w/o an ESP update) would create it and people could mount it w/o = > having to mkdir which they might make as $SOMETHING_ELSE. So I guess=20 > that's a bit of a weak reason to absolutely require it in mtree. Thanks a lot for the explanation. I'm agreed entirely about the problem=20 and the difficulty -- hopefully this set of changes helps at least. As for mtree, I was imagining this as something like /home, which is a=20 standard part of the system but isn't part of mtree since it depends on=20 local-system policy. It's also different from /home in that we *do* want = it to be a standard place for updates, of course. I think there's really = not a ton of precedent either way: we don't have any other mount points=20 in there for file systems that may or may not exist depending on=20 circumstances, as far as I can tell. My worry with having it in mtree is = that having it exist but potentially be a directory rather than an=20 actual ESP requires that update tools be a little smarter and errors=20 will be a little less obvious, since updates that don't pay enough=20 attention will be a bit more likely to splat files there assuming there=20 is an ESP even if makes no sense. It's a weak consideration either way,=20 I think. -Nathan > > Warner