Skip to content

Conversation

cgwalters
Copy link
Member

This fleshes out a bit more details of the boot links.

This fleshes out a bit more details of the boot links.
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds comprehensive documentation for the ostree boot deployment transaction. The new document, docs/boot-transaction.md, provides a detailed explanation of the boot process, especially in environments with separate root and boot filesystems. My review focuses on ensuring the accuracy and clarity of this new documentation. I've identified a few areas where the explanation could be more precise to avoid confusion, particularly regarding the structure of boot links and the terminology used to describe the update scenarios. Overall, this is a valuable addition to the project's documentation.

Comment on lines +37 to +38
| +-- <hash1>/ | +-- entries/
| +-- <hash2>/ | +-- ...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The vertical bars | in the diagram appear to be remnants of an attempt to visually separate the two filesystem trees, but they are misaligned and make the diagram harder to read. Removing them would improve clarity.

Suggested change
| +-- <hash1>/ | +-- entries/
| +-- <hash2>/ | +-- ...
| +-- <hash1>/ +-- entries/
| +-- <hash2>/ +-- ...

Comment on lines +63 to +66
symlinks (`/ostree/boot.{0,1}`) will also include the `bootcsum` in their
target path, as can be seen in the diagram above. The function
`_ostree_sysroot_parse_bootlink()` is used to parse this information from the
symlink.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This paragraph is slightly inaccurate. The bootcsum is not part of the boot symlink's target path. Instead, it's part of the path to the boot symlink itself, which is what the ostree= kernel argument points to. The _ostree_sysroot_parse_bootlink() function then parses this path (not the symlink target) to extract the bootcsum.

I suggest rephrasing this for clarity and accuracy.

Suggested change
symlinks (`/ostree/boot.{0,1}`) will also include the `bootcsum` in their
target path, as can be seen in the diagram above. The function
`_ostree_sysroot_parse_bootlink()` is used to parse this information from the
symlink.
symlinks are referenced by a path that includes the `bootcsum`. This path is
passed to the kernel via the `ostree=` argument. The function
`_ostree_sysroot_parse_bootlink()` is then used by the initramfs to parse this
path and find the correct deployment.

Comment on lines +86 to +88
1. **Create New Bootlinks**: A new temporary directory of symlinks is created (e.g., `/ostree/boot.0.1`) pointing to the new deployment content.
2. **System Sync**: `full_system_sync()` is called to ensure all filesystem changes are persisted to disk.
3. **Atomic Swap**: The primary boot symlink (e.g., `/ostree/boot.0`) is atomically updated to point to the new temporary directory.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The term "temporary directory" here could be misleading. The directory /ostree/boot.0.1 is not temporary; it's one of the two rotating sub-bootversion directories. The "temporary" aspect is an implementation detail of the atomic symlink swap. Clarifying this would improve understanding.

Suggested change
1. **Create New Bootlinks**: A new temporary directory of symlinks is created (e.g., `/ostree/boot.0.1`) pointing to the new deployment content.
2. **System Sync**: `full_system_sync()` is called to ensure all filesystem changes are persisted to disk.
3. **Atomic Swap**: The primary boot symlink (e.g., `/ostree/boot.0`) is atomically updated to point to the new temporary directory.
1. **Create New Bootlinks**: A new directory for boot symlinks is created for the next sub-bootversion (e.g., `/ostree/boot.0.1`), pointing to the new deployment content.
2. **System Sync**: `full_system_sync()` is called to ensure all filesystem changes are persisted to disk.
3. **Atomic Swap**: The primary boot symlink (e.g., `/ostree/boot.0`) is atomically updated to point to the new directory of boot symlinks.


2. **Prepare for Final Swap**: A temporary symbolic link, `/boot/loader.tmp`, is created. It points to the new bootloader directory from the previous step.

3. **The Critical Sync (`full_system_sync`)**: This is a crucial step for ensuring robustness against crashes. The goal is to ensure all changes from steps 1 & 2 are durably persisted to the physical disk *before* performing the final, non-reversible bootloader switch. To do this, we call `syncfs()` on the root filesystem, and issue an `ioctl(FIFREEZE)` on the `/boot` filesystem. This is a strong synchronization primitive that forces the filesystem to flush all of its data and metadata, including its journal. This is important for filesystems like XFS, as bootloaders often cannot read a "dirty" journal.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
3. **The Critical Sync (`full_system_sync`)**: This is a crucial step for ensuring robustness against crashes. The goal is to ensure all changes from steps 1 & 2 are durably persisted to the physical disk *before* performing the final, non-reversible bootloader switch. To do this, we call `syncfs()` on the root filesystem, and issue an `ioctl(FIFREEZE)` on the `/boot` filesystem. This is a strong synchronization primitive that forces the filesystem to flush all of its data and metadata, including its journal. This is important for filesystems like XFS, as bootloaders often cannot read a "dirty" journal.
3. **The Critical Sync (`full_system_sync`)**: This is a crucial step for ensuring robustness against crashes. The goal is to ensure all changes from steps 1 & 2 are durably persisted to the physical disk *before* performing the final, non-reversible bootloader switch. To do this, we call `syncfs(/ostree)`, and issue an `ioctl(FIFREEZE)` on the `/boot` filesystem. This is a strong synchronization primitive that forces the filesystem to flush all of its data and metadata, including its journal. This is important for filesystems like XFS, as bootloaders often cannot read a "dirty" journal.


4. **The Atomic Switch (`swap_bootloader`)**: With all of the data safely on disk, we can now perform the "point of no return". An atomic `renameat()` is performed to rename `/boot/loader.tmp` to `/boot/loader`. After this point, the bootloader will use the new configuration on the next boot.

5. **Final Sync**: To be fully robust, another `fsfreeze`/`thaw` cycle is performed on `/boot` to ensure the `renameat()` operation from step 4 is also durably persisted to disk.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. cleanup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants