Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mkinitcpio: fix mounting root with mountpoint=legacy #54391

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rrveex
Copy link

@rrveex rrveex commented Feb 18, 2025

${node} was /new_root (logged out with msg) - there's nothing in that directory at this point. findmnt finds nothing (in absence of a fstab) so boot stops.

Copying fstab to the ramfs and removing ${node} from path fixes this.

Testing the changes

  • I tested the changes in this PR: briefly

Local build testing

  • I built this PR locally for my native architecture, x86_64-glibc

@classabbyamp classabbyamp self-assigned this Feb 18, 2025
@ahesford
Copy link
Member

I'm not sure that this is the right fix.

We know what the root is and where it should be mounted, because it was provided on the command line. It can be mounted without /etc/fstab in all cases. The only issue is that we need to determine whether to pass zfsutil as a mount option, which can be determined based on the mountpoint property.

After the root is mounted at ${node}, the rest of the child mounts will work with the current logic, pulling the data from fstab on the new root rather than requiring an up-to-date version in the initramfs. This avoids two problems:

  1. The possibility of a de-sync, for example when somebody restructures the root filesystem and its children but forgets to regenerate the initramfs image.
  2. The notional invalidity of /etc/fstab, which doesn't actually represent the mounts done in the early boot environment. (Indeed, all of the mountpoints in /etc/fstab will be wrong, because they all need to live within ${node}.)

@ahesford
Copy link
Member

Thinking about this some more, I hate the entire loop and think it should go away. None of this belongs in this hook, which is not generally correct in the first place.

The standard practice in the initramfs is to mount only the root filesystem. The default mkinitcpio root mount looks to command-line arguments like rootfstype and rootflags to know how the root should be mounted without needing and fstab. The final init process, after jumping into the new root, should also be prepared to remount anything in /etc/fstab that is already mounted (especially the root!) to correct any disparate mount options.

The zfs hook instead makes a poor assumption that it should mount all children of the root, when this should be the purview of the final init. I suppose the thinking was that /usr may live somewhere else and need to be mounted, but

  1. The necessary filesystem may be anywhere, not just as a child of the root; the hook breaks without this arbitrary restriction.
  2. mkinitcpio already includes a /usr hook that will scrape the fstab for a /usr mount. That hook should work just fine even with ZFS mounts, as long as /usr is legacy mounted.

I think the right behavior here is to drop the loop entirely, and mount only the root (omitting the zfsutil option for legacy mounts) without regard for the fstab. (We can even respect rootflags here!) Users can rely on the existing usr hook if they need mounting of /usr in legacy mode.

The final question, then, is how to handle automatic mounts of /usr if necessary. Again, this hook is already broken because it requires that the dependent mount be a child of the root. I submit that it would be more generally correct to walk the entire pool looking for the first filesystem with mountpoint=/usr canmount=auto and use that. Trying to reconcile the right option if there are multiples is too much magic. If people want separate environment-specific mounts and have more than one boot environment, they should be using legacy mounts anyway.

@rrveex
Copy link
Author

rrveex commented Feb 19, 2025

I reverted the first commit and now just handle rootfs when fstab is not yet available.

Rootfs is mounted as expected. I don't know how to test child mounts. Should work though, as zfs lists the parent first, this gets mounted and then fstab is available in the next iterations of the while loop.

@ahesford
Copy link
Member

See #54402, where I tried to clean up the process considerably.

@rrveex
Copy link
Author

rrveex commented Feb 19, 2025

See #54402, where I tried to clean up the process considerably.

I looked. Sorry, I can't really follow what you did there. You wrote you don't like the loop but then replace the easy-readable while with another loop using read, which, in my opinion, is harder to comprehend.

I tried to "test" your implementation in a shell and got this:

zfs list -H -o fs,mountpoint,canmount -r -t filesystem z2/os/void
bad property list: invalid property 'fs'
usage: [...]

As I don't understand your implementation and it doesn't seem to work, I decided to fix mine. Also, in my opinion, for something that is not easily tested, fewer changes is better.

But I don't want to start an argument, I only care that it works on my machine without needing to keep yet another package separate from the default :) So if you make yours work, I'll be equally happy as with this one (which I tested for my use-case)

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.

3 participants