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

Fix boot in other cases #4558

Closed
wants to merge 14 commits into from
Closed

Fix boot in other cases #4558

wants to merge 14 commits into from

Conversation

Rudd-O
Copy link
Contributor

@Rudd-O Rudd-O commented Apr 25, 2016

Older distros need these fixes, and there's a race condition fix between pool importers here. Finally, there's a problem that users can be led into, by setting the supported rootflags= parameters to a value that precludes booting from ZFS.

@prometheanfire
Copy link
Contributor

I'll add this to my local review queue, at a conf this week so tight on time.

@prometheanfire
Copy link
Contributor

it looks good as a onceover though

@ghost
Copy link

ghost commented Apr 25, 2016

I've added a check for grep and made the generator more in line with the dmsquash one here: #4562 My boot was failing because grep was missing.

I have a couple comments on these changes:

I don't think there's a race in the import services. One will run based on whether the cache is there or not.

I don't think 'Before=dracut-mount.service' should be added to the import services, as they will be used on systems that have systemd, but not dracut.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented Apr 26, 2016

I would prefer if you did not submit #4562 .

One thing: both the check and the requirement for grep are necessary. Can you introduce them in a separate PR? I would approve that right away.

Edit: actually, haha, I can do that! :-)

@@ -4,6 +4,7 @@ DefaultDependencies=no
Requires=systemd-udev-settle.service
After=systemd-udev-settle.service
After=cryptsetup.target
Before=dracut-mount.service
Copy link

Choose a reason for hiding this comment

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

Won't this cause issues with the service for people who have systemd, but not dracut?

Copy link
Contributor Author

@Rudd-O Rudd-O Apr 26, 2016

Choose a reason for hiding this comment

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

No. Before=X.unit is ignored if X.unit is nonexistent / cannot be loaded. This stanza just ensures the ordering.

For the record: dracut-mount.service is nonexistent / cannot be loaded in normal non-initrd environments. So this has no effect at all on the boot stage after pivot_root.

true
elif test -n "${rootflags}" ; then
rootflags="zfsutil,${rootflags}"
else
rootflags=zfsutil
fi
Copy link

@ghost ghost Apr 26, 2016

Choose a reason for hiding this comment

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

Not specific to this change, but you could add a few lines here to handle the zfs:AUTO case, where no root was specified, like in mount-zfs.sh. I assume we don't need to do the check twice, since the pool should already be imported. Something like this:

if [ "${root}" = "zfs:AUTO" ] ; then
  root="$(find_bootfs)"
  if [ $? -ne 0 ] ; then
    exit 0
  fi
fi

Copy link
Contributor Author

@Rudd-O Rudd-O Apr 26, 2016

Choose a reason for hiding this comment

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

I would like that to be the case, but, unfortunately for us, this is far too early in the boot process to be running anything like find_bootfs. Unlike during the execution of mount-zfs.sh, the pools are not imported at this time — you gotta understand, this is the EARLIEST part of the boot process, not even block devices exist yet. And, after the generators have executed, there is no way to alter the mount unit any further.

So, it looks like there is no way to support that feature (at least not without major rethinking of how it works). The only way I see to make zfs:AUTO work, would be to require the execution of mount-zfs.sh unconditionally, as well as to require the complete suppression of sysroot.mount — and who knows what other hellish effects that may have! PERHAPS if there was a way to neutralize sysroot.mount by making it into a noop, while keeping it in the transaction, and making sure that mount-zfs.sh executes right before sysroot.mount, PERHAPS that would work. But we can't just kill sysroot.mount, as modern Dracut effectively depends on it.

To be honest, it's not a big deal for people who are using https://github.com/Rudd-O/zfs-fedora-installer/tree/master/grub-zfs-fixer — the root dataset and mountpoint are properly deduced from /etc/fstab in that case, so there is zero need to specify anything on any config file, as the right thing happens upon generation of the grub.cfg file.

I await until brighter minds find a solution to this problem.

Choose a reason for hiding this comment

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

So will it work if I just add my /etc/fstab to dracut-created initramfs? Actually, my datasets are all set to legacy type mountpoint, so I DO have /etc/fstab.
(It's because I have different OSes on different datasets in the same rpool, that's the whole thing about my system...)

Copy link
Contributor Author

@Rudd-O Rudd-O Apr 26, 2016

Choose a reason for hiding this comment

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

Nothing about what I said has anything to do with adding fstab to your initrd. Additionally, nothing in this changeset changes anything about the way that file systems are mounted after the system is done with the initial RAM disk.

The program I linked above is simply an enhancement that inspects /etc/fstab (<- this is an error, see below) at the time of the creation of grub.cfg, and then adds the proper root=ZFS/dataset parameter to the kernel command line. This only happens when the system is already fully booted and you (or something else) run grub2-mkconfig.

Edit: correction, grub-zfs-fixer doesn't even look at fstab. It just checks /proc/mounts of the running system to deduce which root dataset to use.

Choose a reason for hiding this comment

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

Sorry, my mistake :). I never use these dracut or grub functionalities and always edit things manually, but now I recall having read about this in the past.

Copy link

Choose a reason for hiding this comment

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

Sorry, you're right about the generator script, but I realized there is one way that we can do the auto boot. If root is unset in the kernel command line and the initramfs, systemd-fstab-generator will not create sysroot.mount. If zfs-generator bails when it sees "zfs:AUTO", there will no sysroot.mount, so mount-zfs.sh will run and do the auto mount. Just need this in zfs-generator.sh:

if [ "${root}" = "zfs:AUTO" ] ; then
  exit 0
fi

I just booted using this patch set and that change, and it works with root unspecified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am inclined to believe you, but, have you verified that there will be no sysroot.mount in all supported distributions and versions? At the very least, a test for this behaviour would include Fedora 20 and 23, and CentOS 7 and 6, verifying that systemctl show sysroot.mount's LoadState is empty (or, alternatively, that they boot).

Where should that snippet go?

Copy link

Choose a reason for hiding this comment

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

I've only tested on my Gentoo install (systemd 229 & dracut 044). I think this is the first commit where systemd-fstab-generator started creating root mounts (systemd/systemd@5e398e5). There and in the current code, it doesn't create a mount if there is no root option on the kernel command line.

Dracut may add it's own root option in the initrd, but I use the --no-hostonly-cmdline to strip that off.

That snippet goes just after the if statement that this comment thread belongs to in zfs-generator, just before the two string manipulation commands to remove zfs:/ZFS:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any diff you can come up with, I will add to the patchset if it doesn't interfere with the boot process of anyone else. Happy to accommodate zfs:AUTO users.

@kostyaberger
Copy link

kostyaberger commented Apr 28, 2016

OK, it boots fine on one of my machines with Fedora 23 (kernel version is 4.1.13-100.fc21.x86_64). And since it has the same configuration as do the other 2, I assume it will work on those as well... But I'll test it.

dracut-043-63.git20151211.fc23.x86_64
systemd-222-14.fc23.x86_64

@kostyaberger
Copy link

kostyaberger commented Apr 29, 2016

Doesn't work well on my other, newer machine. Boot stops with sysroot.mount failure -- before ZFS mounting scripts even start, I guess. Because then from the shell I call /usr/lib/dracut/hooks/mount/mount-zfs.sh (it's there) and zpool IS imported and mounted, so after that I resume the boot process.
What I did was installing the RPMs built on the other machine on which I reported above all boots well.

The version of the zfs-related packages is:
zfs-dracut-0.6.5-227_g51316ee.fc23.x86_64

I'm not sure if this naming reflects somehow the version of the git repo I'm using... But I did my best to make sure it was pull request 4558 by Rudd-O.

@behlendorf behlendorf added the Component: Dracut dracut integration label Apr 29, 2016
@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 2, 2016

@kostyaberger need an actual error log. Please submit error log from systemctl status sysroot.mount so I can begin to understand what the problem is.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 4, 2016

Seems the failed tests have nothing to do with my patches.

@kostyaberger
Copy link

kostyaberger commented May 4, 2016

Not much logging can I report from that failure. The most meaningful part of `systemctl status sysroot.mount is:
filesystem mypool/ROOT/dataset cannot be mounted, unable to open dataset.

But calling /usr/lib/dracut/hooks/mount/mount-zfs.sh from the emergency shell both imports and mounts the pool all right.
My guess is sysroot.mount starts on this particular machine before /dev/disk/* appears. I remember having this kind of problem about 1 year ago, then I added this loop at the beginning of mount-zfs.sh:

while [ -z "$(ls /dev/disk/by-id)" ]
do
sleep 3
done

and that had fixed it. And the same, the problem existed on one, but not the other machine.
EDIT: must be the HDD controller driver slowness or something? The problematic machine is Gigabyte z77n-wifi based.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 5, 2016

sysroot.mount is coded to happen after the zpool imports. Perhaps the zpool imports are happening too soon.

systemctl --failed in the dracut shell, discover what other units failed. systemctl status zpool-import-{scan,cache}.service too.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 5, 2016

Please speed review of this up. This is preventing people from booting off of ZFS roots.

Also, the failure above has nothing to do with this pull request. You can check the details link if you so choose.

@behlendorf
Copy link
Contributor

@prometheanfire @jyxent @kostyaberger can you please post your review / testing comments for this PR. Including your signed-off-by if you're comfortable with these changes.

@Rudd-O if you can flesh out the commit comments a little more a describe why a chance is need it'll help me review this. Please rebase this on master and force update the branch while you're at it.

@prometheanfire
Copy link
Contributor

ya, I'm updating my kernel tonight to 4.4.9, I'll include this test in the update.

@prometheanfire
Copy link
Contributor

Works for me. 👍

However, I'm going to open a bug to try and get it so it only modprobes if it needs to. Since I build unikernels I have to modify the systemd service files to remove the modprobe line or it will fail.

Signed-off-by: Matthew Thode mthode@mthode.org

@prometheanfire
Copy link
Contributor

Also, as far as the static kernel thing, since afaik, Gentoo is the only one support installs based on that I'll likely just submit a patch to the ebuild to sed out those lines if the kernel-builtin use flag is enabled.

@ryao does that sound good?

@kostyaberger
Copy link

No problem. I was just thinking it was worth reporting, but actually I CAN boot. So you can ignore it, because I don't have time right now for a lot of testing.

Sent from Yahoo Mail on Android

On Thu, May 5, 2016 at 19:02, Rudd-Onotifications@github.com wrote:
Please speed review of this up. This is preventing people from booting off of ZFS roots.

Also, the failure above has nothing to do with this pull request. You can check the details link if you so choose.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@kostyaberger
Copy link

Ok, it's zfs-import-cache which fails. For the obvious reason there is no zpool.cache file in /etc/zfs or elsewhere. 

Sent from Yahoo Mail on Android

On Thu, May 5, 2016 at 18:59, Rudd-Onotifications@github.com wrote:
sysroot.mount is coded to happen after the zpool imports. Perhaps the zpool imports are happening too soon.

systemctl --failed in the dracut shell, discover what other units failed. systemctl status zpool-import-{scan,cache}.service too.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@kostyaberger
Copy link

kostyaberger commented May 6, 2016

Well, I deleted the /etc/zfs/zpool.cache to exclude the import by cache service, now it seems there is a bug in /usr/lib/dracut/modules.d/90zfs/mount-zfs.sh:
loadstate="$(systemctl --system --show -p LoadState sysroot.mount)"
returns error, because, if I read it correctly, it should be something like:
systemctl --system --show -p LoadState | grep sysroot.mount

@kostyaberger
Copy link

kostyaberger commented May 6, 2016

And now that I've fixed the mount-zfs.sh error sysroor.mount fails with this message:
filesystem ... cannot be mounted using 'zfs mount'
Use 'zfs set mountpoint=/sysroot' or mount -t zfs ... etc

And this time no other failed units show up...
At that it must be noted that my root filesystem is "legacy" type, so it truly cannot be mounted using 'zfs mount'. Still, /usr/lib/dracut/hooks/98mount-zfs.sh handles it all right when called from the emergency shell after this failure.
Sent from Yahoo Mail on Android

On Fri, May 6, 2016 at 21:22, Kostya Bergerbergerkos@yahoo.co.uk wrote: Ok, it's zfs-import-cache which fails. For the obvious reason there is no zpool.cache file in /etc/zfs or elsewhere. 

Sent from Yahoo Mail on Android

On Thu, May 5, 2016 at 18:59, Rudd-Onotifications@github.com wrote:
sysroot.mount is coded to happen after the zpool imports. Perhaps the zpool imports are happening too soon.

systemctl --failed in the dracut shell, discover what other units failed. systemctl status zpool-import-{scan,cache}.service too.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@kostyaberger
Copy link

But still, if what I'm talking about has nothing to do with the patches in question, please, make it a separate bug report. Or don't pay attention to it, please.

@ryao
Copy link
Contributor

ryao commented May 6, 2016

@prometheanfire I am fine with using sed on those things when USE=kernel-builtin is specified.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 10, 2016

@kostyaberger we want the systemctl loadstate stuff to return error, not grepping anything at all. I have added a || true in case this causes some explosion.

@jyxent I have added the "bail if zfs:AUTO" patch to the pull request.

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 10, 2016

This works locally. Please test on your rigs.

We also need to develop a proper automated testing rig. I could throw some funds to an Amazon account designated to run tests like zfs-fedora-installer image boots (heck, we could use mine own with Amazon IAM), but someone else would need to write the automation to (a) get the program to run (b) boot the image (c) verify that the image booted (d) tear all that shit down when not in use. Otherwise I would be paying thousands of dollars a month for idle instances, and I really do not want to do that.

@@ -17,27 +17,22 @@ get_pool_devices() {
local prefix
local resolved
poolconfigtemp=`mktemp`
@sbindir@/zpool list -v -H "$1" > "$poolconfigtemp" 2>&1
@sbindir@/zpool list -v -H -P "$1" > "$poolconfigtemp" 2>&1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

great work @b333z

@Rudd-O
Copy link
Contributor Author

Rudd-O commented May 12, 2016

Hi folks. This is preventing people from using ZFS in at least Fedora if not CentOS as well. If anything, this can be merged as is, and later bugs that may be discovered can be fixed on top of that. It really should go in.

@behlendorf
Copy link
Contributor

Merged as:

d402c18 A collection of dracut fixes

ryao pushed a commit to ClusterHQ/zfs that referenced this pull request Jun 7, 2016
- In older systems without sysroot.mount, import before dracut-mount,
  and re-enable old dracut mount hook
- rootflags MUST be present even if the administrator neglected to
  specify it explicitly
- Check that mount.zfs exists in sbindir
- Remove awk and head as (now unused) requirements, add grep, and
  install the right mount.zfs
- Eliminate one use of grep in Dracut
- Use a more accurate grepping statement to identify zfsutil in rootflags
- Ensure that pooldev is nonempty
- Properly handle /dev/sd* devices and more
- Use new -P to get list of zpool devices
- Bail out of the generator when zfs:AUTO is on the root command line
- Ignore errors from systemctl trying to load sysroot.mount, we only
  care about the output
- Determine which one is the correct initqueuedir at run time.
- Add a compatibility getargbool for our detection / setup script.
- Update dracut .gitignore files

Signed-off-by: <Matthew Thode mthode@mthode.org>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#4558
Closes openzfs#4562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Dracut dracut integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants