Use rename of directories instead of symbolic links in boot partition. #1967
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: valentindavid The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @valentindavid. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Can one of the admins verify this patch?
|
valentindavid
force-pushed
the
support-vfat
branch
from
c3d34fb
to
c36ee32
Compare
|
The challenge with this is that the ESP is vfat, and therefore rename will always be non-atomic. It's still better than what I'm doing now (manually copying files into /efi) #1951 |
|
☔ The latest upstream changes (presumably #1767) made this pull request unmergeable. Please resolve the merge conflicts. |
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967
|
Is this PR still proposed or abandoned? |
I can rebase and take care of it a bit. But since there was no interaction from maintainers, then I am not sure whether they are considering it. |
|
@valentindavid have you been able to pay some attention to this issue again? The changes are highly anticipated here, in one form or another. Thanks. |
|
@mwleeds @rfairley any chance to take a look to this? We currently need this to make GNOME images work, see https://gitlab.gnome.org/GNOME/gnome-build-meta/-/commit/79fb62e0d243a21ab58dc1dda439c23db5d474ab |
valentindavid
force-pushed
the
support-vfat
branch
from
c36ee32
to
861306d
Compare
|
@cgwalters Hey! Any chance to take a look to this? |
|
Friendly ping. |
|
@valentindavid This PR conflicts with upstream again. GNOME OS uses an ancient version of OSTree so they have yet to run into this issue, but on my OS I just updated to the latest version and I'm about to be downgrading again because this patch is broken. @cgwalters Is there any chance for this patch to be reviewed or considered at all? Without this patch, I have to follow every single |
|
updated.txt |
|
Any news on this? |
|
In case is useful, this patch has been updated in GNOME OS to work with libostree-2021.2: https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/41.rc/files/ostree/no-boot-symlink.patch |
|
carbonOS has been using this patch for a while. It works flawlessly so far! Hopefully this will get merged at some point... @jjardon that same patch works for 2021.3 also |
|
Hi, sorry about the delay. I am OK in principle with the idea and code. But I'd like to have some design to close the atomicity hole longer term. If you (or someone else) doesn't mind rebasing we can try to get this merged. |
| GError **error) | ||
| { | ||
| if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE) == 0) | ||
| return TRUE; |
There was a problem hiding this comment.
You're not setting the is_atomic out parameter here or in the other renameat2 success case below.
There was a problem hiding this comment.
We start within ostree_sysroot_write_deployments_with_options with:
bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);
Then we only mark bootloader_is_atomic as FALSE if we do not manage to do a renameat2. But if we manage, we want to still keep FALSE if that was the value given by _ostree_bootloader_is_atomic.
| /* We generate the symlink on disk, then potentially do a syncfs() to ensure | ||
| * that it (and everything else we wrote) has hit disk. Only after that do we | ||
| * rename it into place. | ||
| */ |
There was a problem hiding this comment.
Please don't remove comments like this. This is some of the most sensitive code in ostree. If anything, it should have a lot more comments and I don't see any in the code you've added.
| if (errno != ENOENT) | ||
| return glnx_throw_errno_prefix (error, "renameat2"); | ||
|
|
||
| if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE) == 0) |
There was a problem hiding this comment.
Nitpicky, but space before parentheses here and in a few other spots.
| g_string_truncate (buf, 0); g_string_append_printf (buf, "boot/loader.%d", self->bootversion); | ||
| if (!glnx_shutil_rm_rf_at (self->sysroot_fd, buf->str, cancellable, error)) | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Is this logic even right? In swap_bootloader, loader.<newversion> is exchanged with loader. That means that the current bootloader directory is now named loader.<newversion>. What's even at loader.<currentversion>? This is also in conflict with the code below that cleans up loader.<currentversion>. So, after this change, there's no more loader.* bootloader directories left. Is that correct?
There was a problem hiding this comment.
Right. Only loader directory is left after deployment is finished successfully. We could just use loader.tmp and loader. But if I remember correctly, we need to support older deployments that are still using symlinks. For that reason we still to need to keep this 0/1 version names. Also maintaining a patch for long term I cannot really afford changing too much the logic.
There was a problem hiding this comment.
Reading the code, I do not think the support for older deployment is working. I will try to add tests for that.
This uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. This is working with SystemD boot on EFI using boot loader specifications. There is still the issue of losing `/loader/loader.conf` with SystemD boot.
valentindavid
force-pushed
the
support-vfat
branch
from
861306d
to
5825205
Compare
|
I'm still not totally sure what the best approach here is, but I think the fundamental change from replacing Since the replacing process needs to be supported in case
|
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Signed-off-by: Ricardo Salveti <ricardo@foundries.io>
@dbnicholson FYI, I worked on a patch series to add |
That's great. Of course, you'd still need to support the fallback since it will be some time until that's commonly available (presuming it gets merged). I've come around a bit on this idea in the sense that I think it's reasonable to assume |
|
I'm currently playing with ricardosalveti@731b73f, which is based on the original patch from this PR, which keeps the current behavior for new systems and systems that are already deployed, but also supports loader as a directory (relying on RENAME_EXCHANGE). That way I can create new deployments (using systemd-boot) with the extra kernel patches without breaking my current deployments that are ok with links (using grub / u-boot). Still validating with my yocto integration, can post the final set after I'm done testing it. |
|
FYI the patch-set got merged by a subsystem maintainer and are heading towards the Linux v6.0 release. |
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Upstream-Status: Pending Signed-off-by: Ricardo Salveti <ricardo@foundries.io> %% original patch: 0003-Add-support-for-directories-instead-of-symbolic-link.patch
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Upstream-Status: Pending Signed-off-by: Ricardo Salveti <ricardo@foundries.io> %% original patch: 0003-Add-support-for-directories-instead-of-symbolic-link.patch
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Upstream-Status: Pending Signed-off-by: Ricardo Salveti <ricardo@foundries.io> %% original patch: 0003-Add-support-for-directories-instead-of-symbolic-link.patch
Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Upstream-Status: Pending Signed-off-by: Ricardo Salveti <ricardo@foundries.io> %% original patch: 0003-Add-support-for-directories-instead-of-symbolic-link.patch
|
Is GNOME OS's patch still the best workaround until this is finished? Edit: looks like it doesn't work with 2023.6 update |
|
I ended up working around this issue by making Here's the commands if anyone else is stuck on |
|
update? |
|
I've thought about this several times as I would really like to have this in Endless to support our systemd-boot systems. What always makes me anxious is trying to figure out how to handle compatibility with the vast majority of our systems that have symlink based deployments. There are 2 main issues I'm concerned with:
So, to me this requires a couple additional pieces of implementation and policy.
|
This implements @AdrianVovk idea from #1719 (comment) to solve issue #1719.
This uses
renameat2to do atomic swap of the loader directory in theboot partition. It fallsback to non-atomic rename. This stays atomic
on filesystems supporting links but also provide a non-atomic behavior
when filesystem does not provide any atomic alternative.
This is working with SystemD boot on EFI using boot loader
specifications.
There is still the issue of losing
/loader/loader.confwith SystemDboot. Maybe we should think about copying other files from previous loader directories.