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

shutdown: several fixlets #28648

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Aug 2, 2023

Hopefully fixes #28645.

@yuwata yuwata added the shutdown label Aug 2, 2023
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Aug 2, 2023
@bluca
Copy link
Member

bluca commented Aug 2, 2023

uhm doesn't this undo the fix that I did before the release? Where stuff under /run was lost in the transition to initrd?

@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

IIUC, the previous issue is on initramfs -> host transition on boot. But this is for shutting down, and host -> initramfs transition.

@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

At least, v253 did not mount /run/ recursively. git diff v253..v254 src/shutdown/shutdown.c

@@ -162,17 +163,15 @@ static int parse_argv(int argc, char *argv[]) {
 }
 
 static int switch_root_initramfs(void) {
-        if (mount("/run/initramfs", "/run/initramfs", NULL, MS_BIND, NULL) < 0)
-                return log_error_errno(errno, "Failed to mount bind /run/initramfs on /run/initramfs: %m");
-
-        if (mount(NULL, "/run/initramfs", NULL, MS_PRIVATE, NULL) < 0)
-                return log_error_errno(errno, "Failed to make /run/initramfs private mount: %m");
-
-        /* switch_root with MS_BIND, because there might still be processes lurking around, which have open file descriptors.
-         * /run/initramfs/shutdown will take care of these.
-         * Also do not detach the old root, because /run/initramfs/shutdown needs to access it.
-         */
-        return switch_root("/run/initramfs", "/oldroot", false, MS_BIND);
+        /* Do not detach the old root, because /run/initramfs/shutdown needs to access it.
+         *
+         * Disable sync() during switch-root, we after all sync'ed here plenty, and a dumb sync (as opposed
+         * to the "smart" sync() we did here that looks at progress parameters) would defeat much of our
+         * efforts here. */
+        return switch_root(
+                        /* new_root= */ "/run/initramfs",
+                        /* old_root_after= */ "/oldroot",
+                        /* flags= */ SWITCH_ROOT_DONT_SYNC);
 }
 
 /* Read the following fields from /proc/meminfo:
@@ -341,6 +340,12 @@ int main(int argc, char *argv[]) {

Copy link
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

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

Ok, let's give it a shot

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed needs-stable-backport and removed please-review PR is ready for (re-)review by a maintainer labels Aug 2, 2023
@yuwata yuwata added good-to-merge/waiting-for-reporter-feedback 👍 and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Aug 2, 2023
@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

I'd like to wait for reporter feedback for a while, as I have not tested it.

@mrc0mmand
Copy link
Member

Nope, still seeing the error (+ an extra debug message just to confirm I run the correct binary):

image

yuwata added a commit to yuwata/systemd that referenced this pull request Aug 2, 2023
@yuwata yuwata force-pushed the shutdown-skip-recursive-mount-run branch from 722a35e to af4a2f4 Compare August 2, 2023 19:29
@github-actions github-actions bot added util-lib please-review PR is ready for (re-)review by a maintainer labels Aug 2, 2023
@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

@mrc0mmand Could you test again?

@mrc0mmand
Copy link
Member

@mrc0mmand Could you test again?

The computer is still unhappy:
image

@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

@mrc0mmand Does v253 work fine?

@Freyon77
Copy link

Freyon77 commented Aug 2, 2023

@mrc0mmand Does v253 work fine?

Yep

@mrc0mmand
Copy link
Member

@mrc0mmand Does v253 work fine?

It seems so, I don't see any errors even with some debug output sprinkled around:
image

@mrc0mmand
Copy link
Member

And just a note: on v253 it seems to go through the pivot_root() branch, but it works even if I force it to use MS_MOVE instead:
image

@yuwata yuwata force-pushed the shutdown-skip-recursive-mount-run branch from af4a2f4 to b9436d3 Compare August 2, 2023 20:44
@yuwata yuwata marked this pull request as draft August 2, 2023 20:44
@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

@mrc0mmand @Freyon77 Could you test again?

@mrc0mmand
Copy link
Member

@mrc0mmand @Freyon77 Could you test again?

This looks promising:

[  OK  ] Reached target shutdown.target.
[  OK  ] Reached target final.target.
[  OK  ] Finished systemd-halt.service.
[  OK  ] Reached target halt.target.
[  118.668517] reboot: System halted

@yuwata
Copy link
Member Author

yuwata commented Aug 2, 2023

Thank you!

@mrc0mmand
Copy link
Member

@mrc0mmand Could you test again?

Still looks good!

[  OK  ] Reached target shutdown.target.
[  OK  ] Reached target final.target.
[  OK  ] Finished systemd-halt.service.
[  OK  ] Reached target halt.target.
[39099.296592] systemd-shutdown[1]: switch_root: about to pivot_root()
[39099.297278] systemd-shutdown[1]: switch_root: pivot_root() succeeded
[39099.394763] reboot: System halted
[  OK  ] Reached target shutdown.target.
[  OK  ] Reached target final.target.
[  OK  ] Finished systemd-halt.service.
[  OK  ] Reached target halt.target.
[  208.428841] systemd-shutdown[1]: Pivoting root file system failed, moving mounts instead: Operation not permitted
[  208.429875] systemd-shutdown[1]: switch_root: about to move '.' to '/'
[  208.430518] systemd-shutdown[1]: switch_root: about to chroot() into '.'
[  208.431182] systemd-shutdown[1]: switch_root: about to chdir() to '.'
[  208.431922] systemd-shutdown[1]: switch_root: all done!
[  208.458289] reboot: System halted

@Freyon77
Copy link

Freyon77 commented Aug 3, 2023

@mrc0mmand what about pivoting root file system failed?

@mrc0mmand
Copy link
Member

@mrc0mmand what about pivoting root file system failed?

That's just me setting r to -1 to test the fallback branch, probably should've dropped that from the output.

@yuwata yuwata marked this pull request as ready for review August 3, 2023 19:50
@yuwata
Copy link
Member Author

yuwata commented Aug 3, 2023

Thank you for testing so many times!

@bluca Could you take a look again?

@yuwata yuwata changed the title shutdown: disable recursive mount of /run/ on switching root shutdown: several fixlets Aug 3, 2023
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Aug 3, 2023
@YHNdnzj YHNdnzj merged commit 07bd7b3 into systemd:main Aug 4, 2023
44 of 47 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Aug 4, 2023
@yuwata yuwata deleted the shutdown-skip-recursive-mount-run branch August 4, 2023 08:07
@keszybz
Copy link
Member

keszybz commented Aug 9, 2023

I queued up all three commits for v254.1.

@poettering
Copy link
Member

So I am pretty sure the 2nd commit is not right. It's not load-bearing for the bugfix this PR is supposed to be, is it?

I prepped #28793 that reverts that part. Please take a look for an explanation.

mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
This should provide coverage for shutdown initrd related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
This should provide coverage for shutdown initrd related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
This should provide coverage for shutdown initrd related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
And move the initrd related tests from TEST-01-BASIC there.

Additionally, this should provide coverage for recemt shutdown initrd
related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
And move the initrd related tests from TEST-01-BASIC there.

Additionally, this should provide coverage for recemt shutdown initrd
related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 11, 2023
And move the initrd related tests from TEST-01-BASIC there.

Additionally, this should provide coverage for recemt shutdown initrd
related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
mrc0mmand added a commit to mrc0mmand/systemd that referenced this pull request Aug 12, 2023
And move the initrd related tests from TEST-01-BASIC there.

Additionally, this should provide coverage for recemt shutdown initrd
related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
YHNdnzj pushed a commit to YHNdnzj/systemd that referenced this pull request Aug 12, 2023
And move the initrd related tests from TEST-01-BASIC there.

Additionally, this should provide coverage for recemt shutdown initrd
related issues, see:
  - systemd#28645
  - systemd#28648
  - systemd#28793
minimada added a commit to minimada/openbmc that referenced this pull request Sep 20, 2023
Issue:
We cannot perform BMC FW update due to systemd-shutdown error.
systemd/systemd#28645
systemd-shutdown: Failed to move /run/initramfs to /: invalid argument
systemd-shutdown: Failed to switch root to "run/initramfs" invalid argument

Fix:
systemd/systemd#28648
The systemd upstream already fix this issue, but poky not merge it.
So we need manual cherry-pick the fix patches into our repository, and
remove it once poky merge these changes.

Signed-off-by: Brian Ma <chma0@nuvoton.com>
maxdog988 pushed a commit to Nuvoton-Israel/openbmc that referenced this pull request Sep 25, 2023
Issue:
We cannot perform BMC FW update due to systemd-shutdown error.
systemd/systemd#28645
systemd-shutdown: Failed to move /run/initramfs to /: invalid argument
systemd-shutdown: Failed to switch root to "run/initramfs" invalid argument

Fix:
systemd/systemd#28648
The systemd upstream already fix this issue, but poky not merge it.
So we need manual cherry-pick the fix patches into our repository, and
remove it once poky merge these changes.

Signed-off-by: Brian Ma <chma0@nuvoton.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Failed to move /run/initramfs to /
8 participants