Discussion:
[RFC] [PATCHv3 0/3] resume: implement support for resuming from hibernation
(too old to reply)
Ivan Shapovalov
2014-08-24 22:16:16 UTC
Permalink
This patchset allows systemd to parse resume= kernel command line parameter
and initiate resume from the specified device.

It adds:
- a 'systemd-resume' tool which takes path to a device node and
writes its major:minor to /sys/power/state;
- a corresponding 'systemd-***@.service' templated unit;
- a 'systemd-resume-generator' generator which parses the kernel command line
and instantiates the unit as necessary.

This functionality already exists in-kernel, but only for "/dev/sdXY"-style
pathes. Implementing it in userspace allows to use arbitrary udev-created
symlinks, e. g. persistent block device pathes ("/dev/disk/by-foo/bar").

Userspace parsing of resume= kernel command line parameter has been
traditionally done in initramfs via shell scripts (for Arch Linux, this is
"resume" mkinitcpio hook), so I feel that this feature has its place within
systemd.

Due to the nature of hibernation, the resume unit must be activated before
any modifications to filesystems take place. This can happen only in initramfs
before mounting anything.

So, first patch orders all non-root fsck after local-fs-pre.target, which in
turn allows to order the resume unit before those fsck instances.

Second and third patches add the tool, the unit and the generator.

There are some issues with this implementation.

- legacy usr.mount is not automatically ordered after local-fs-pre.target,
so systemd-***@.service has to be manually ordered before it;
- systemd-udevd.service, which is needed for creating persistent block device
symlinks, is transitively ordered after systemd-remount-fs.service via at
least systemd-udev-hwdb-update.service and systemd-sysusers.service.
Hence, if these units are present, an ordering cycle happens and resume is
impossible.

Encountering these conditions is very unlikely inside of an initramfs (who
would make the in-initramfs /usr external? who would add systemd-sysusers
into initramfs?), but there can still be pathological cases.

So, I would like someone to comment on these issues.

Thanks for reviewing!

v2: fix issues pointed out by Andrei:
- don't RemainAfterExit because it's useless
- don't attempt to resume outside of initramfs because it's unsafe
(reiserfs replays journal even if mounted RO)

v3: fix mistakes spotted by Thomas:
- return 0 in main path of resume.c:process_resume()
- fix type and add missing cleanup attribute in resume-generator.c:main()

Ivan Shapovalov (3):
units: order systemd-***@.service after local-fs-pre.target.
resume: add a tool to write a device node's major:minor to /sys/power/resume.
resume-generator: add a generator for instantiating the resume unit.

Makefile-man.am | 9 ++++
Makefile.am | 28 ++++++++--
man/kernel-command-line.xml | 13 ++++-
man/systemd-resume-generator.xml | 91 +++++++++++++++++++++++++++++++
man/systemd-***@.service.xml | 81 ++++++++++++++++++++++++++++
src/resume-generator/Makefile | 1 +
src/resume-generator/resume-generator.c | 95 +++++++++++++++++++++++++++++++++
src/resume/Makefile | 1 +
src/resume/resume.c | 83 ++++++++++++++++++++++++++++
units/systemd-***@.service.in | 2 +-
units/systemd-***@.service.in | 20 +++++++
11 files changed, 418 insertions(+), 6 deletions(-)
create mode 100644 man/systemd-resume-generator.xml
create mode 100644 man/systemd-***@.service.xml
create mode 120000 src/resume-generator/Makefile
create mode 100644 src/resume-generator/resume-generator.c
create mode 120000 src/resume/Makefile
create mode 100644 src/resume/resume.c
create mode 100644 units/systemd-***@.service.in
--
2.1.0
Ivan Shapovalov
2014-08-24 22:16:17 UTC
Permalink
With this change, it becomes possible to order a unit to activate before any
modifications to the file systems. This is especially useful for supporting
resume from hibernation.
---
units/systemd-***@.service.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/units/systemd-***@.service.in b/units/systemd-***@.service.in
index c12efa8..d2cda6a 100644
--- a/units/systemd-***@.service.in
+++ b/units/systemd-***@.service.in
@@ -10,7 +10,7 @@ Description=File System Check on %f
Documentation=man:systemd-***@.service(8)
DefaultDependencies=no
BindsTo=%i.device
-After=systemd-readahead-collect.service systemd-readahead-replay.service %i.device systemd-fsck-root.service
+After=systemd-readahead-collect.service systemd-readahead-replay.service %i.device systemd-fsck-root.service local-fs-pre.target
Before=shutdown.target

[Service]
--
2.1.0
Ivan Shapovalov
2014-08-24 22:16:19 UTC
Permalink
resume-generator understands resume= kernel command line parameter and
instantiates the systemd-***@.service accordingly if it is passed.

This enables resume from hibernation using device specified on the kernel
command line, where the device path may point to an arbitrary udev-created
symlink, not only "/dev/sdXY" which is understood by the in-kernel
implementation.
---
Makefile-man.am | 2 +
Makefile.am | 11 +++-
man/kernel-command-line.xml | 13 ++++-
man/systemd-resume-generator.xml | 91 +++++++++++++++++++++++++++++++
src/resume-generator/Makefile | 1 +
src/resume-generator/resume-generator.c | 95 +++++++++++++++++++++++++++++++++
6 files changed, 211 insertions(+), 2 deletions(-)
create mode 100644 man/systemd-resume-generator.xml
create mode 120000 src/resume-generator/Makefile
create mode 100644 src/resume-generator/resume-generator.c

diff --git a/Makefile-man.am b/Makefile-man.am
index be19905..00daae2 100644
--- a/Makefile-man.am
+++ b/Makefile-man.am
@@ -76,6 +76,7 @@ MANPAGES += \
man/systemd-nspawn.1 \
man/systemd-path.1 \
man/systemd-remount-fs.service.8 \
+ man/systemd-resume-generator.8 \
man/systemd-***@.service.8 \
man/systemd-run.1 \
man/systemd-shutdownd.service.8 \
@@ -1632,6 +1633,7 @@ EXTRA_DIST += \
man/systemd-readahead-replay.service.xml \
man/systemd-remount-fs.service.xml \
man/systemd-resolved.service.xml \
+ man/systemd-resume-generator.xml \
man/systemd-***@.service.xml \
man/systemd-***@.service.xml \
man/systemd-run.xml \
diff --git a/Makefile.am b/Makefile.am
index c4327ca..820d082 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -378,7 +378,8 @@ systemgenerator_PROGRAMS = \
systemd-getty-generator \
systemd-fstab-generator \
systemd-system-update-generator \
- systemd-debug-generator
+ systemd-debug-generator \
+ systemd-resume-generator

dist_bashcompletion_DATA = \
shell-completion/bash/busctl \
@@ -1973,6 +1974,14 @@ systemd_system_update_generator_LDADD = \
libsystemd-label.la \
libsystemd-shared.la

+# ------------------------------------------------------------------------------
+systemd_resume_generator_SOURCES = \
+ src/resume-generator/resume-generator.c
+
+systemd_resume_generator_LDADD = \
+ libsystemd-label.la \
+ libsystemd-shared.la
+
if ENABLE_EFI
# ------------------------------------------------------------------------------
systemgenerator_PROGRAMS += \
diff --git a/man/kernel-command-line.xml b/man/kernel-command-line.xml
index f244bfc..4bc6cee 100644
--- a/man/kernel-command-line.xml
+++ b/man/kernel-command-line.xml
@@ -351,6 +351,16 @@
</listitem>
</varlistentry>

+ <varlistentry>
+ <term><varname>resume=</varname></term>
+
+ <listitem>
+ <para>Enables resume from hibernation
+ using the specified device. For
+ details, see
+ <citerefentry><refentrytitle>systemd-resume-generator</refentrytitle><manvolnum>8</manvolnum></citerefentry>.</para>
+ </listitem>
+ </varlistentry>
</variablelist>

</refsect1>
@@ -373,7 +383,8 @@
<citerefentry><refentrytitle>systemd-gpt-auto-generator</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
<citerefentry><refentrytitle>systemd-modules-load.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
<citerefentry><refentrytitle>systemd-***@.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
- <citerefentry><refentrytitle>systemd-***@.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>
+ <citerefentry><refentrytitle>systemd-***@.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-resume-generator</refentrytitle><manvolnum>8</manvolnum></citerefentry>
</para>
</refsect1>

diff --git a/man/systemd-resume-generator.xml b/man/systemd-resume-generator.xml
new file mode 100644
index 0000000..7962534
--- /dev/null
+++ b/man/systemd-resume-generator.xml
@@ -0,0 +1,91 @@
+<?xml version="1.0"?>
+<!--*-nxml-*-->
+<!DOCTYPE refentry PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN" "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd">
+<!--
+ This file is part of systemd.
+
+ Copyright 2014 Ivan Shapovalov
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+-->
+<refentry id="systemd-resume-generator">
+
+ <refentryinfo>
+ <title>systemd-resume-generator</title>
+ <productname>systemd</productname>
+
+ <authorgroup>
+ <author>
+ <contrib>Developer</contrib>
+ <firstname>Ivan</firstname>
+ <surname>Shapovalov</surname>
+ <email>***@gmail.com</email>
+ </author>
+ </authorgroup>
+ </refentryinfo>
+
+ <refmeta>
+ <refentrytitle>systemd-resume-generator</refentrytitle>
+ <manvolnum>8</manvolnum>
+ </refmeta>
+
+ <refnamediv>
+ <refname>systemd-resume-generator</refname>
+ <refpurpose>Unit generator for resume= kernel parameter</refpurpose>
+ </refnamediv>
+
+ <refsynopsisdiv>
+ <para><filename>/usr/lib/systemd/system-generators/systemd-resume-generator</filename></para>
+ </refsynopsisdiv>
+
+ <refsect1>
+ <title>Description</title>
+
+ <para><filename>systemd-resume-generator</filename> is
+ a generator that instantiates
+ <citerefentry><refentrytitle>systemd-***@.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>
+ unit according to the value of <option>resume=</option>
+ parameter specified on the kernel command line.</para>
+ </refsect1>
+
+ <refsect1>
+ <title>Kernel Command Line</title>
+
+ <para><filename>systemd-resume-generator</filename> understands
+ the following kernel command line parameters:</para>
+
+ <variablelist class='kernel-commandline-options'>
+
+ <varlistentry>
+ <term><varname>resume=</varname></term>
+
+ <listitem><para>Takes a path to the resume
+ device. May point to any udev-created symlink,
+ including persistent block device pathes like
+ <filename>/dev/disk/by-label/foo</filename>.</para></listitem>
+ </varlistentry>
+
+ </variablelist>
+ </refsect1>
+
+ <refsect1>
+ <title>See Also</title>
+ <para>
+ <citerefentry><refentrytitle>systemd</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-***@.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>kernel-command-line</refentrytitle><manvolnum>7</manvolnum></citerefentry>
+ </para>
+ </refsect1>
+
+</refentry>
diff --git a/src/resume-generator/Makefile b/src/resume-generator/Makefile
new file mode 120000
index 0000000..d0b0e8e
--- /dev/null
+++ b/src/resume-generator/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
diff --git a/src/resume-generator/resume-generator.c b/src/resume-generator/resume-generator.c
new file mode 100644
index 0000000..38179ff
--- /dev/null
+++ b/src/resume-generator/resume-generator.c
@@ -0,0 +1,95 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright 2014 Ivan Shapovalov
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdio.h>
+#include <errno.h>
+
+#include "log.h"
+#include "util.h"
+#include "special.h"
+#include "mkdir.h"
+#include "unit-name.h"
+
+static const char *arg_dest = "/tmp";
+static char *arg_resume_dev = NULL;
+
+static int parse_proc_cmdline_item(const char *key, const char *value) {
+ if (streq(key, "resume") && value) {
+ free(arg_resume_dev);
+ arg_resume_dev = strdup(value);
+ if (!arg_resume_dev)
+ return log_oom();
+ }
+
+ return 0;
+}
+
+static int process_resume(void) {
+ _cleanup_free_ char *name = NULL, *lnk = NULL;
+
+ name = unit_name_from_path_instance("systemd-resume", arg_resume_dev, ".service");
+ if (!name)
+ return log_oom();
+
+ lnk = strjoin(arg_dest, "/" SPECIAL_SYSINIT_TARGET ".wants/", name, NULL);
+ if (!lnk)
+ return log_oom();
+
+ mkdir_parents_label(lnk, 0755);
+ if (symlink(SYSTEM_DATA_UNIT_PATH "/systemd-***@.service", lnk) < 0) {
+ log_error("Failed to create symlink %s: %m", lnk);
+ return -errno;
+ }
+
+ return 0;
+}
+
+int main(int argc, char *argv[]) {
+ int r = 0;
+
+ if (argc > 1 && argc != 4) {
+ log_error("This program takes three or no arguments.");
+ return EXIT_FAILURE;
+ }
+
+ if (argc > 1)
+ arg_dest = argv[1];
+
+ log_set_target(LOG_TARGET_SAFE);
+ log_parse_environment();
+ log_open();
+
+ umask(0022);
+
+ /* Don't even consider resuming outside of initramfs. */
+ if (!in_initrd())
+ return EXIT_SUCCESS;
+
+ if (parse_proc_cmdline(parse_proc_cmdline_item) < 0)
+ return EXIT_FAILURE;
+
+ if (arg_resume_dev != NULL)
+ r = process_resume();
+
+ free(arg_resume_dev);
+
+ return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+}
--
2.1.0
Lennart Poettering
2014-08-25 17:58:52 UTC
Permalink
Post by Ivan Shapovalov
+
+static const char *arg_dest = "/tmp";
+static char *arg_resume_dev = NULL;
+
+static int parse_proc_cmdline_item(const char *key, const char *value) {
+ if (streq(key, "resume") && value) {
+ free(arg_resume_dev);
+ arg_resume_dev = strdup(value);
+ if (!arg_resume_dev)
+ return log_oom();
+ }
+
+ return 0;
There's something wrong with the indentation here...
Post by Ivan Shapovalov
+}
+
+static int process_resume(void) {
+ _cleanup_free_ char *name = NULL, *lnk = NULL;
+
+ name = unit_name_from_path_instance("systemd-resume", arg_resume_dev, ".service");
+ if (!name)
+ return log_oom();
+
+ lnk = strjoin(arg_dest, "/" SPECIAL_SYSINIT_TARGET ".wants/", name, NULL);
+ if (!lnk)
+ return log_oom();
+
+ mkdir_parents_label(lnk, 0755);
+ log_error("Failed to create symlink %s: %m", lnk);
+ return -errno;
+ }
+
+ return 0;
+}
+
+int main(int argc, char *argv[]) {
+ int r = 0;
+
+ if (argc > 1 && argc != 4) {
+ log_error("This program takes three or no arguments.");
+ return EXIT_FAILURE;
+ }
+
+ if (argc > 1)
+ arg_dest = argv[1];
+
+ log_set_target(LOG_TARGET_SAFE);
+ log_parse_environment();
+ log_open();
+
+ umask(0022);
+
+ /* Don't even consider resuming outside of initramfs. */
+ if (!in_initrd())
+ return EXIT_SUCCESS;
+
+ if (parse_proc_cmdline(parse_proc_cmdline_item) < 0)
+ return EXIT_FAILURE;
+
+ if (arg_resume_dev != NULL)
+ r = process_resume();
+
+ free(arg_resume_dev);
+
+ return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
+}
Looks good otherwise!

One more question though, regarding the terminology. So far we used the
following terms:

"suspend" → means suspend-to-ram
"hibernate" → means suspend-to-disk
"hybrid-sleep" → means both STR + STD combined
"sleep" → a generic term for all of the above.

Now, I do wonder how we should call the operation when we come back from
the sleep states.

To me "resume" would probably most clearly be the reverse of "suspend",
but you actually are looking for the reverse of "hibernate" here.

The reverse of "sleep" would be "wake" I figure...

On the kernel side the terminology for all of this is completely
random. Especially given that the input layer names some keys with the
precise opposite of what the PM layer calls the operations... But we
really should try to clean this up a bit when exposing this in
userspace...

So, dunno, what's the antonym of "hibernate"? SOmething like "thaw"
maybe? Any native english speakers with ideas?

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-25 18:27:40 UTC
Permalink
Post by Lennart Poettering
[...]
One more question though, regarding the terminology. So far we used the
"suspend" → means suspend-to-ram
"hibernate" → means suspend-to-disk
"hybrid-sleep" → means both STR + STD combined
"sleep" → a generic term for all of the above.
Now, I do wonder how we should call the operation when we come back from
the sleep states.
To me "resume" would probably most clearly be the reverse of "suspend",
but you actually are looking for the reverse of "hibernate" here.
The reverse of "sleep" would be "wake" I figure...
On the kernel side the terminology for all of this is completely
random. Especially given that the input layer names some keys with the
precise opposite of what the PM layer calls the operations... But we
really should try to clean this up a bit when exposing this in
userspace...
So, dunno, what's the antonym of "hibernate"? SOmething like "thaw"
maybe? Any native english speakers with ideas?
Lennart
I've called it "resume" just because the kernel command line parameter
is named "resume=", kernel messages use the term "resume", the arch initramfs
hook is also named "resume" and so on... This may be inconsistent globally,
but the opposite of hibernation is called "resume" pretty much everywhere.

However, suggestions welcome.
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-26 01:03:57 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
[...]
One more question though, regarding the terminology. So far we used the
"suspend" → means suspend-to-ram
"hibernate" → means suspend-to-disk
"hybrid-sleep" → means both STR + STD combined
"sleep" → a generic term for all of the above.
Now, I do wonder how we should call the operation when we come back from
the sleep states.
To me "resume" would probably most clearly be the reverse of "suspend",
but you actually are looking for the reverse of "hibernate" here.
The reverse of "sleep" would be "wake" I figure...
On the kernel side the terminology for all of this is completely
random. Especially given that the input layer names some keys with the
precise opposite of what the PM layer calls the operations... But we
really should try to clean this up a bit when exposing this in
userspace...
So, dunno, what's the antonym of "hibernate"? SOmething like "thaw"
maybe? Any native english speakers with ideas?
I've called it "resume" just because the kernel command line parameter
is named "resume=", kernel messages use the term "resume", the arch initramfs
hook is also named "resume" and so on... This may be inconsistent globally,
but the opposite of hibernation is called "resume" pretty much everywhere.
However, suggestions welcome.
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...

But maybe call it "systemd-hibernate-***@.service" and
systemd-hibernate-resume-generator or so?

Longer to type but more precise, and I figure nobody has to type this
ever anyway...

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 11:49:13 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
[...]
One more question though, regarding the terminology. So far we used the
"suspend" → means suspend-to-ram
"hibernate" → means suspend-to-disk
"hybrid-sleep" → means both STR + STD combined
"sleep" → a generic term for all of the above.
Now, I do wonder how we should call the operation when we come back from
the sleep states.
To me "resume" would probably most clearly be the reverse of "suspend",
but you actually are looking for the reverse of "hibernate" here.
The reverse of "sleep" would be "wake" I figure...
On the kernel side the terminology for all of this is completely
random. Especially given that the input layer names some keys with the
precise opposite of what the PM layer calls the operations... But we
really should try to clean this up a bit when exposing this in
userspace...
So, dunno, what's the antonym of "hibernate"? SOmething like "thaw"
maybe? Any native english speakers with ideas?
I've called it "resume" just because the kernel command line parameter
is named "resume=", kernel messages use the term "resume", the arch initramfs
hook is also named "resume" and so on... This may be inconsistent globally,
but the opposite of hibernation is called "resume" pretty much everywhere.
However, suggestions welcome.
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...
systemd-hibernate-resume-generator or so?
Longer to type but more precise, and I figure nobody has to type this
ever anyway...
That's what will be reflected in manpages. I'd argue that "systemd-resume"
is how one expects this feature to be named -- support from "systemd" side
for the kernel feature called "resume".
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-26 17:49:08 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...
systemd-hibernate-resume-generator or so?
Longer to type but more precise, and I figure nobody has to type this
ever anyway...
That's what will be reflected in manpages. I'd argue that "systemd-resume"
is how one expects this feature to be named -- support from "systemd" side
for the kernel feature called "resume".
Well, I am mostly thinking about somebody who wonders what that service
is about if he doesn't know that the kernel details and the kernel
cmdline option yet.

I think systemd-hibernate-resume has the benefit that both folks who
already know the kernel side of things, and people who don't know
anything about this at all yet, have both a chance to grok what this is
about.

If I don#t now the kernel side of things, then I would always have
assume that "systemd-resume" is something that is involved with STR, not
STD... "systemd-hibernate-resume.service" otoh makes this very clear, and also
pairs this up nicely to the existing "systemd-hibernate.service".

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 19:37:40 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...
systemd-hibernate-resume-generator or so?
Longer to type but more precise, and I figure nobody has to type this
ever anyway...
That's what will be reflected in manpages. I'd argue that "systemd-resume"
is how one expects this feature to be named -- support from "systemd" side
for the kernel feature called "resume".
Well, I am mostly thinking about somebody who wonders what that service
is about if he doesn't know that the kernel details and the kernel
cmdline option yet.
I think systemd-hibernate-resume has the benefit that both folks who
already know the kernel side of things, and people who don't know
anything about this at all yet, have both a chance to grok what this is
about.
If I don#t now the kernel side of things, then I would always have
assume that "systemd-resume" is something that is involved with STR, not
STD... "systemd-hibernate-resume.service" otoh makes this very clear, and also
pairs this up nicely to the existing "systemd-hibernate.service".
I don't completely agree, but OK, bikeshedding protection activated -- let's
do it this way. :)

I've renamed everything to systemd-hibernate-resume.* and will send a v5 shortly.

--
Ivan Shapovalov / intelfx /

Lennart Poettering
2014-08-26 17:49:08 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...
systemd-hibernate-resume-generator or so?
Longer to type but more precise, and I figure nobody has to type this
ever anyway...
That's what will be reflected in manpages. I'd argue that "systemd-resume"
is how one expects this feature to be named -- support from "systemd" side
for the kernel feature called "resume".
Well, I am mostly thinking about somebody who wonders what that service
is about if he doesn't know that the kernel details and the kernel
cmdline option yet.

I think systemd-hibernate-resume has the benefit that both folks who
already know the kernel side of things, and people who don't know
anything about this at all yet, have both a chance to grok what this is
about.

If I don#t now the kernel side of things, then I would always have
assume that "systemd-resume" is something that is involved with STR, not
STD... "systemd-hibernate-resume.service" otoh makes this very clear, and also
pairs this up nicely to the existing "systemd-hibernate.service".

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 11:49:13 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
[...]
One more question though, regarding the terminology. So far we used the
"suspend" → means suspend-to-ram
"hibernate" → means suspend-to-disk
"hybrid-sleep" → means both STR + STD combined
"sleep" → a generic term for all of the above.
Now, I do wonder how we should call the operation when we come back from
the sleep states.
To me "resume" would probably most clearly be the reverse of "suspend",
but you actually are looking for the reverse of "hibernate" here.
The reverse of "sleep" would be "wake" I figure...
On the kernel side the terminology for all of this is completely
random. Especially given that the input layer names some keys with the
precise opposite of what the PM layer calls the operations... But we
really should try to clean this up a bit when exposing this in
userspace...
So, dunno, what's the antonym of "hibernate"? SOmething like "thaw"
maybe? Any native english speakers with ideas?
I've called it "resume" just because the kernel command line parameter
is named "resume=", kernel messages use the term "resume", the arch initramfs
hook is also named "resume" and so on... This may be inconsistent globally,
but the opposite of hibernation is called "resume" pretty much everywhere.
However, suggestions welcome.
Hmm, the fact that the kernel cmdline option is named "resume" is
probably a strong indication we should stick to that nomenclature, after
all that's UI already in away...
systemd-hibernate-resume-generator or so?
Longer to type but more precise, and I figure nobody has to type this
ever anyway...
That's what will be reflected in manpages. I'd argue that "systemd-resume"
is how one expects this feature to be named -- support from "systemd" side
for the kernel feature called "resume".
--
Ivan Shapovalov / intelfx /
Ivan Shapovalov
2014-08-26 12:10:24 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
+
+static const char *arg_dest = "/tmp";
+static char *arg_resume_dev = NULL;
+
+static int parse_proc_cmdline_item(const char *key, const char *value) {
+ if (streq(key, "resume") && value) {
+ free(arg_resume_dev);
+ arg_resume_dev = strdup(value);
+ if (!arg_resume_dev)
+ return log_oom();
+ }
+
+ return 0;
There's something wrong with the indentation here...
Yeah, the closing brace is at the wrong level. Fixed for v4.
--
Ivan Shapovalov / intelfx /
Ivan Shapovalov
2014-08-24 22:16:18 UTC
Permalink
This can be used to initiate a resume from hibernation by path to a swap
device containing the hibernation image.

The respective templated unit is also added. It is instantiated using
path to the desired resume device.
---
Makefile-man.am | 7 ++++
Makefile.am | 17 ++++++--
man/systemd-***@.service.xml | 81 +++++++++++++++++++++++++++++++++++++++
src/resume/Makefile | 1 +
src/resume/resume.c | 83 ++++++++++++++++++++++++++++++++++++++++
units/systemd-***@.service.in | 20 ++++++++++
6 files changed, 206 insertions(+), 3 deletions(-)
create mode 100644 man/systemd-***@.service.xml
create mode 120000 src/resume/Makefile
create mode 100644 src/resume/resume.c
create mode 100644 units/systemd-***@.service.in

diff --git a/Makefile-man.am b/Makefile-man.am
index 5c289dd..be19905 100644
--- a/Makefile-man.am
+++ b/Makefile-man.am
@@ -76,6 +76,7 @@ MANPAGES += \
man/systemd-nspawn.1 \
man/systemd-path.1 \
man/systemd-remount-fs.service.8 \
+ man/systemd-***@.service.8 \
man/systemd-run.1 \
man/systemd-shutdownd.service.8 \
man/systemd-sleep.conf.5 \
@@ -206,6 +207,7 @@ MANPAGES_ALIAS += \
man/systemd-poweroff.service.8 \
man/systemd-reboot.service.8 \
man/systemd-remount-fs.8 \
+ man/systemd-resume.8 \
man/systemd-shutdown.8 \
man/systemd-shutdownd.8 \
man/systemd-shutdownd.socket.8 \
@@ -311,6 +313,7 @@ man/systemd-kexec.service.8: man/systemd-halt.service.8
man/systemd-poweroff.service.8: man/systemd-halt.service.8
man/systemd-reboot.service.8: man/systemd-halt.service.8
man/systemd-remount-fs.8: man/systemd-remount-fs.service.8
+man/systemd-resume.8: man/systemd-***@.service.8
man/systemd-shutdown.8: man/systemd-halt.service.8
man/systemd-shutdownd.8: man/systemd-shutdownd.service.8
man/systemd-shutdownd.socket.8: man/systemd-shutdownd.service.8
@@ -592,6 +595,9 @@ man/systemd-reboot.service.html: man/systemd-halt.service.html
man/systemd-remount-fs.html: man/systemd-remount-fs.service.html
$(html-alias)

+man/systemd-resume.html: man/systemd-***@.service.html
+ $(html-alias)
+
man/systemd-shutdown.html: man/systemd-halt.service.html
$(html-alias)

@@ -1626,6 +1632,7 @@ EXTRA_DIST += \
man/systemd-readahead-replay.service.xml \
man/systemd-remount-fs.service.xml \
man/systemd-resolved.service.xml \
+ man/systemd-***@.service.xml \
man/systemd-***@.service.xml \
man/systemd-run.xml \
man/systemd-shutdownd.service.xml \
diff --git a/Makefile.am b/Makefile.am
index e238cde..c4327ca 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -371,7 +371,8 @@ rootlibexec_PROGRAMS = \
systemd-sleep \
systemd-bus-proxyd \
systemd-socket-proxyd \
- systemd-update-done
+ systemd-update-done \
+ systemd-resume

systemgenerator_PROGRAMS = \
systemd-getty-generator \
@@ -509,7 +510,8 @@ nodist_systemunit_DATA = \
units/initrd-udevadm-cleanup-db.service \
units/initrd-switch-root.service \
units/systemd-***@.service \
- units/systemd-update-done.service
+ units/systemd-update-done.service \
+ units/systemd-***@.service

dist_userunit_DATA = \
units/user/basic.target \
@@ -556,7 +558,8 @@ EXTRA_DIST += \
units/initrd-udevadm-cleanup-db.service.in \
units/initrd-switch-root.service.in \
units/systemd-***@.service.in \
- units/systemd-update-done.service.in
+ units/systemd-update-done.service.in \
+ units/systemd-***@.service.in

CLEANFILES += \
units/console-shell.service.m4 \
@@ -1930,6 +1933,14 @@ systemd_delta_LDADD = \
libsystemd-shared.la

# ------------------------------------------------------------------------------
+systemd_resume_SOURCES = \
+ src/resume/resume.c
+
+systemd_resume_LDADD = \
+ libsystemd-internal.la \
+ libsystemd-shared.la
+
+# ------------------------------------------------------------------------------
systemd_getty_generator_SOURCES = \
src/getty-generator/getty-generator.c

diff --git a/man/systemd-***@.service.xml b/man/systemd-***@.service.xml
new file mode 100644
index 0000000..b7d28fb
--- /dev/null
+++ b/man/systemd-***@.service.xml
@@ -0,0 +1,81 @@
+<?xml version="1.0"?>
+<!--*-nxml-*-->
+<!DOCTYPE refentry PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN" "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd">
+<!--
+ This file is part of systemd.
+
+ Copyright 2014 Ivan Shapovalov
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+-->
+<refentry id="systemd-***@.service">
+
+ <refentryinfo>
+ <title>systemd-***@.service</title>
+ <productname>systemd</productname>
+
+ <authorgroup>
+ <author>
+ <contrib>Developer</contrib>
+ <firstname>Ivan</firstname>
+ <surname>Shapovalov</surname>
+ <email>***@gmail.com</email>
+ </author>
+ </authorgroup>
+ </refentryinfo>
+
+ <refmeta>
+ <refentrytitle>systemd-***@.service</refentrytitle>
+ <manvolnum>8</manvolnum>
+ </refmeta>
+
+ <refnamediv>
+ <refname>systemd-***@.service</refname>
+ <refname>systemd-resume</refname>
+ <refpurpose>Resume from hibernation</refpurpose>
+ </refnamediv>
+
+ <refsynopsisdiv>
+ <para><filename>systemd-***@.service</filename></para>
+ <para><filename>/usr/lib/systemd/systemd-resume</filename></para>
+ </refsynopsisdiv>
+
+ <refsect1>
+ <title>Description</title>
+
+ <para><filename>systemd-***@.service</filename> is a
+ service that initiates hibernation resume from a device
+ containing the resume image. It is instantiated for each
+ device that is configured for resuming from.</para>
+
+ <para><filename>systemd-resume</filename> only supports
+ the in-kernel hibernation implementation, known as swsusp.
+ Internally, it works by writing the major:minor of specified
+ device node to <filename>/sys/power/resume</filename>.</para>
+
+ <para>Failing to initiate a resume is not an error condition.
+ It may mean that there was no resume image (e. g. if the
+ system has been simply powered off and not hibernated). In
+ such case, the boot is ordinarily continued.</para>
+ </refsect1>
+
+ <refsect1>
+ <title>See Also</title>
+ <para>
+ <citerefentry><refentrytitle>systemd</refentrytitle><manvolnum>1</manvolnum></citerefentry>,
+ <citerefentry><refentrytitle>systemd-resume-generator</refentrytitle><manvolnum>8</manvolnum></citerefentry>
+ </para>
+ </refsect1>
+
+</refentry>
diff --git a/src/resume/Makefile b/src/resume/Makefile
new file mode 120000
index 0000000..d0b0e8e
--- /dev/null
+++ b/src/resume/Makefile
@@ -0,0 +1 @@
+../Makefile
\ No newline at end of file
diff --git a/src/resume/resume.c b/src/resume/resume.c
new file mode 100644
index 0000000..49b0cc3
--- /dev/null
+++ b/src/resume/resume.c
@@ -0,0 +1,83 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+ This file is part of systemd.
+
+ Copyright 2014 Ivan Shapovalov
+
+ systemd is free software; you can redistribute it and/or modify it
+ under the terms of the GNU Lesser General Public License as published by
+ the Free Software Foundation; either version 2.1 of the License, or
+ (at your option) any later version.
+
+ systemd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stdio.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "log.h"
+#include "util.h"
+#include "fileio.h"
+
+int main(int argc, char *argv[]) {
+ struct stat st;
+ const char *device;
+ _cleanup_free_ char *major_minor = NULL;
+ int r;
+
+ if (argc != 2) {
+ log_error("This program expects one argument.");
+ return EXIT_FAILURE;
+ }
+
+ log_set_target(LOG_TARGET_AUTO);
+ log_parse_environment();
+ log_open();
+
+ umask(0022);
+
+ device = argv[1];
+
+ if (stat(device, &st) < 0) {
+ log_error("Failed to stat '%s': %m", device);
+ return EXIT_FAILURE;
+ }
+
+ if (!S_ISBLK(st.st_mode)) {
+ log_error("Resume device '%s' is not a block device.", device);
+ return EXIT_FAILURE;
+ }
+
+ if (asprintf(&major_minor, "%d:%d", major(st.st_rdev), minor(st.st_rdev)) < 0) {
+ log_oom();
+ return EXIT_FAILURE;
+ }
+
+ r = write_string_file("/sys/power/resume", major_minor);
+ if (r < 0) {
+ log_error("Failed to write '%s' to /sys/power/resume: %s",
+ major_minor, strerror(-r));
+ return EXIT_FAILURE;
+ }
+
+ /*
+ * The write above shall not return.
+ *
+ * However, failed resume is a normal condition (may mean that there is
+ * no hibernation image).
+ */
+
+ log_notice("Failed to resume from device '%s' (%s).",
+ device, major_minor);
+ return EXIT_SUCCESS;
+}
diff --git a/units/systemd-***@.service.in b/units/systemd-***@.service.in
new file mode 100644
index 0000000..c6aa0d2
--- /dev/null
+++ b/units/systemd-***@.service.in
@@ -0,0 +1,20 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=Resume from hibernation using device %f
+Documentation=man:systemd-***@.service(8)
+DefaultDependencies=no
+BindsTo=%i.device
+Wants=local-fs-pre.target
+After=%i.device systemd-udevd.service
+Before=local-fs-pre.target systemd-remount-fs.service systemd-fsck-root.service usr.mount
+ConditionPathExists=/etc/initrd-release
+
+[Service]
+Type=oneshot
+ExecStart=@rootlibexecdir@/systemd-resume %f
--
2.1.0
Lennart Poettering
2014-08-25 17:49:52 UTC
Permalink
Post by Ivan Shapovalov
+int main(int argc, char *argv[]) {
+ struct stat st;
+ const char *device;
+ _cleanup_free_ char *major_minor = NULL;
+ int r;
+
+ if (argc != 2) {
+ log_error("This program expects one argument.");
+ return EXIT_FAILURE;
+ }
+
+ log_set_target(LOG_TARGET_AUTO);
+ log_parse_environment();
+ log_open();
+
+ umask(0022);
+
+ device = argv[1];
+
+ if (stat(device, &st) < 0) {
+ log_error("Failed to stat '%s': %m", device);
+ return EXIT_FAILURE;
+ }
+
+ if (!S_ISBLK(st.st_mode)) {
+ log_error("Resume device '%s' is not a block device.", device);
+ return EXIT_FAILURE;
+ }
+
+ if (asprintf(&major_minor, "%d:%d", major(st.st_rdev), minor(st.st_rdev)) < 0) {
+ log_oom();
+ return EXIT_FAILURE;
+ }
+
+ r = write_string_file("/sys/power/resume", major_minor);
+ if (r < 0) {
+ log_error("Failed to write '%s' to /sys/power/resume: %s",
+ major_minor, strerror(-r));
+ return EXIT_FAILURE;
So, we explicitly do not break lines at 80ch... Nobody has such tiny
screens anymore. This doesn't really matter, but I 'd prefer if you
didn't break lines thaaaaat aggressively. I mean, don't overdo it with
super long lines either, but 140ch or so should be OK.

(this is just nitpicking, and not a necessity to fix before I merge it...)
Post by Ivan Shapovalov
+ }
+
+ /*
+ * The write above shall not return.
+ *
+ * However, failed resume is a normal condition (may mean that there is
+ * no hibernation image).
+ */
+
+ log_notice("Failed to resume from device '%s' (%s).",
+ device, major_minor);
same here..

Hmm, what's the logic here? Is this something where we should halt the
machine? How "fatal" shall we consider this error?
Post by Ivan Shapovalov
+ return EXIT_SUCCESS;
... especially, since you return "success" here...

I am just wondering whether log_notice() + EXIT_SUCCESS is the right
reaction here. Or maybe log_error() + freeze()? or maybe log_warning() +
EXIT_FAILURE?... Dunno. Just want to hear some rationale here...
Post by Ivan Shapovalov
+}
new file mode 100644
index 0000000..c6aa0d2
--- /dev/null
@@ -0,0 +1,20 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=Resume from hibernation using device %f
+DefaultDependencies=no
+BindsTo=%i.device
+Wants=local-fs-pre.target
+After=%i.device systemd-udevd.service
Ordering after systemd-udevd.service sounds unnecessary? I mean, no
devices will show up before udevd, but there's no need to do anything
about this.
Post by Ivan Shapovalov
+Before=local-fs-pre.target systemd-remount-fs.service
systemd-fsck-root.service usr.mount
The usr.mount should really not be listed.
Post by Ivan Shapovalov
+ConditionPathExists=/etc/initrd-release
+
+[Service]
+Type=oneshot
Looks great otherwise!

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-25 18:20:03 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
+int main(int argc, char *argv[]) {
+ struct stat st;
+ const char *device;
+ _cleanup_free_ char *major_minor = NULL;
+ int r;
+
+ if (argc != 2) {
+ log_error("This program expects one argument.");
+ return EXIT_FAILURE;
+ }
+
+ log_set_target(LOG_TARGET_AUTO);
+ log_parse_environment();
+ log_open();
+
+ umask(0022);
+
+ device = argv[1];
+
+ if (stat(device, &st) < 0) {
+ log_error("Failed to stat '%s': %m", device);
+ return EXIT_FAILURE;
+ }
+
+ if (!S_ISBLK(st.st_mode)) {
+ log_error("Resume device '%s' is not a block device.", device);
+ return EXIT_FAILURE;
+ }
+
+ if (asprintf(&major_minor, "%d:%d", major(st.st_rdev), minor(st.st_rdev)) < 0) {
+ log_oom();
+ return EXIT_FAILURE;
+ }
+
+ r = write_string_file("/sys/power/resume", major_minor);
+ if (r < 0) {
+ log_error("Failed to write '%s' to /sys/power/resume: %s",
+ major_minor, strerror(-r));
+ return EXIT_FAILURE;
So, we explicitly do not break lines at 80ch... Nobody has such tiny
screens anymore. This doesn't really matter, but I 'd prefer if you
didn't break lines thaaaaat aggressively. I mean, don't overdo it with
super long lines either, but 140ch or so should be OK.
(this is just nitpicking, and not a necessity to fix before I merge it...)
That's because I've seen 80-wrapped lines in couple of other places.
Fixed for v4.
Post by Lennart Poettering
Post by Ivan Shapovalov
+ }
+
+ /*
+ * The write above shall not return.
+ *
+ * However, failed resume is a normal condition (may mean that there is
+ * no hibernation image).
+ */
+
+ log_notice("Failed to resume from device '%s' (%s).",
+ device, major_minor);
same here..
Fixed as well.
Post by Lennart Poettering
Hmm, what's the logic here? Is this something where we should halt the
machine? How "fatal" shall we consider this error?
Post by Ivan Shapovalov
+ return EXIT_SUCCESS;
... especially, since you return "success" here...
I am just wondering whether log_notice() + EXIT_SUCCESS is the right
reaction here. Or maybe log_error() + freeze()? or maybe log_warning() +
EXIT_FAILURE?... Dunno. Just want to hear some rationale here...
This is absolutely not fatal, and arguably not an error condition at all.

If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.

We could return EXIT_FAILURE with something like "no image == failure to resume"
in mind, but this does not really worth attention as an error.

(Reaction to other errors handled in this file can be subject to discussion.

Errors like "wrong parameter, device not found" are essentially programmatic,
because the unit has correct dependencies in place.

Failing to write to /sys/power/resume can indicate that the resume has been
unsuccessful: corrupted image, filesystem mount timestamps are post hibernation,
and so on. I have not really triggered failure conditions so I don't know
whether a write error is really returned in this case. But if I were
kernel-space, I'd do it this way.)
Post by Lennart Poettering
Post by Ivan Shapovalov
+}
new file mode 100644
index 0000000..c6aa0d2
--- /dev/null
@@ -0,0 +1,20 @@
+# This file is part of systemd.
+#
+# systemd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation; either version 2.1 of the License, or
+# (at your option) any later version.
+
+[Unit]
+Description=Resume from hibernation using device %f
+DefaultDependencies=no
+BindsTo=%i.device
+Wants=local-fs-pre.target
+After=%i.device systemd-udevd.service
Ordering after systemd-udevd.service sounds unnecessary? I mean, no
devices will show up before udevd, but there's no need to do anything
about this.
Post by Ivan Shapovalov
+Before=local-fs-pre.target systemd-remount-fs.service
systemd-fsck-root.service usr.mount
The usr.mount should really not be listed.
Post by Ivan Shapovalov
+ConditionPathExists=/etc/initrd-release
+
+[Service]
+Type=oneshot
Looks great otherwise!
Lennart
Lennart Poettering
2014-08-26 00:59:25 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
Post by Ivan Shapovalov
+ log_notice("Failed to resume from device '%s' (%s).",
+ device, major_minor);
same here..
Fixed as well.
Post by Lennart Poettering
Hmm, what's the logic here? Is this something where we should halt the
machine? How "fatal" shall we consider this error?
Post by Ivan Shapovalov
+ return EXIT_SUCCESS;
... especially, since you return "success" here...
I am just wondering whether log_notice() + EXIT_SUCCESS is the right
reaction here. Or maybe log_error() + freeze()? or maybe log_warning() +
EXIT_FAILURE?... Dunno. Just want to hear some rationale here...
This is absolutely not fatal, and arguably not an error condition at all.
If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.
But then it should just be log_info(), not log_notice()?

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 11:45:10 UTC
Permalink
Post by Lennart Poettering
[...]
Post by Ivan Shapovalov
This is absolutely not fatal, and arguably not an error condition at all.
If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.
But then it should just be log_info(), not log_notice()?
syslog(3) says "normal, but significant, condition". I guess that this fits here,
but if you think log_info() is more appropriate, then let it be so.
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-26 17:45:11 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
[...]
Post by Ivan Shapovalov
This is absolutely not fatal, and arguably not an error condition at all.
If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.
But then it should just be log_info(), not log_notice()?
syslog(3) says "normal, but significant, condition". I guess that this fits here,
but if you think log_info() is more appropriate, then let it be so.
I'd vote for log_info(), but I figure it odesn't matter really, you choice...

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2014-08-26 17:45:11 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
[...]
Post by Ivan Shapovalov
This is absolutely not fatal, and arguably not an error condition at all.
If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.
But then it should just be log_info(), not log_notice()?
syslog(3) says "normal, but significant, condition". I guess that this fits here,
but if you think log_info() is more appropriate, then let it be so.
I'd vote for log_info(), but I figure it odesn't matter really, you choice...

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 11:45:10 UTC
Permalink
Post by Lennart Poettering
[...]
Post by Ivan Shapovalov
This is absolutely not fatal, and arguably not an error condition at all.
If a machine is simply rebooted, without hibernation image in place (but with
resume= kernel parameter), this code path will be hit.
But then it should just be log_info(), not log_notice()?
syslog(3) says "normal, but significant, condition". I guess that this fits here,
but if you think log_info() is more appropriate, then let it be so.
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-25 17:42:21 UTC
Permalink
Post by Ivan Shapovalov
This patchset allows systemd to parse resume= kernel command line parameter
and initiate resume from the specified device.
- a 'systemd-resume' tool which takes path to a device node and
writes its major:minor to /sys/power/state;
- a 'systemd-resume-generator' generator which parses the kernel command line
and instantiates the unit as necessary.
This functionality already exists in-kernel, but only for "/dev/sdXY"-style
pathes. Implementing it in userspace allows to use arbitrary udev-created
symlinks, e. g. persistent block device pathes ("/dev/disk/by-foo/bar").
Userspace parsing of resume= kernel command line parameter has been
traditionally done in initramfs via shell scripts (for Arch Linux, this is
"resume" mkinitcpio hook), so I feel that this feature has its place within
systemd.
Due to the nature of hibernation, the resume unit must be activated before
any modifications to filesystems take place. This can happen only in initramfs
before mounting anything.
So, first patch orders all non-root fsck after local-fs-pre.target, which in
turn allows to order the resume unit before those fsck instances.
Second and third patches add the tool, the unit and the generator.
There are some issues with this implementation.
- legacy usr.mount is not automatically ordered after local-fs-pre.target,
Not following here. We do not really support /usr split out unless it is
already mounted in the initrd. But in the initrd its called
"sysroot-usr.mount"... To me this doesn't look like something to do here?
Post by Ivan Shapovalov
- systemd-udevd.service, which is needed for creating persistent block device
symlinks, is transitively ordered after systemd-remount-fs.service via at
least systemd-udev-hwdb-update.service and systemd-sysusers.service.
Hence, if these units are present, an ordering cycle happens and resume is
impossible.
Hmm? What's the cycle precisely? Not following?

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-25 17:52:43 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
This patchset allows systemd to parse resume= kernel command line parameter
and initiate resume from the specified device.
- a 'systemd-resume' tool which takes path to a device node and
writes its major:minor to /sys/power/state;
- a 'systemd-resume-generator' generator which parses the kernel command line
and instantiates the unit as necessary.
This functionality already exists in-kernel, but only for "/dev/sdXY"-style
pathes. Implementing it in userspace allows to use arbitrary udev-created
symlinks, e. g. persistent block device pathes ("/dev/disk/by-foo/bar").
Userspace parsing of resume= kernel command line parameter has been
traditionally done in initramfs via shell scripts (for Arch Linux, this is
"resume" mkinitcpio hook), so I feel that this feature has its place within
systemd.
Due to the nature of hibernation, the resume unit must be activated before
any modifications to filesystems take place. This can happen only in initramfs
before mounting anything.
So, first patch orders all non-root fsck after local-fs-pre.target, which in
turn allows to order the resume unit before those fsck instances.
Second and third patches add the tool, the unit and the generator.
There are some issues with this implementation.
- legacy usr.mount is not automatically ordered after local-fs-pre.target,
Not following here. We do not really support /usr split out unless it is
already mounted in the initrd. But in the initrd its called
"sysroot-usr.mount"... To me this doesn't look like something to do here?
Theoretically it is possible to have initramfs's /usr split out.
I know that it sounds crazy, but if someone will do this, they will lose their
data if usr.mount not properly handled.
Post by Lennart Poettering
Post by Ivan Shapovalov
- systemd-udevd.service, which is needed for creating persistent block device
symlinks, is transitively ordered after systemd-remount-fs.service via at
least systemd-udev-hwdb-update.service and systemd-sysusers.service.
Hence, if these units are present, an ordering cycle happens and resume is
impossible.
Hmm? What's the cycle precisely? Not following?
systemd-remount.fs.service -> systemd-udev-hwdb-update.service -> systemd-udevd.service
systemd-sysusers.service

Here, the arrow represents an "After=" dependency.

If either "systemd-udev-hwdb-update.service" or "systemd-sysusers.service"
becomes part of the transaction (== becomes included in the initramfs),
it becomes impossible for "systemd-***@.service" to start after
"systemd-udevd.service". The outcome can vary:

- a 90 second wait for dev-disk-by\x2dfoo-bar.device and dependency failure
(if "After=systemd-udevd.service" has not been specified);
- an ordering cycle and removal of "systemd-***@.service" from transaction
(if "After=systemd-udevd.service" has been specified, just as it is now).

Both situations are very unlikely (who would add usr.mount to initramfs? who
would add systemd-sysusers.service to initramfs?), but nevertheless possible.
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-25 18:07:28 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
Post by Ivan Shapovalov
- legacy usr.mount is not automatically ordered after local-fs-pre.target,
Not following here. We do not really support /usr split out unless it is
already mounted in the initrd. But in the initrd its called
"sysroot-usr.mount"... To me this doesn't look like something to do here?
Theoretically it is possible to have initramfs's /usr split out.
I know that it sounds crazy, but if someone will do this, they will lose their
data if usr.mount not properly handled.
initrd cannot have their data split out. I am completely happy about
breaking this, should it exist (which I doubt).
Post by Ivan Shapovalov
If either "systemd-udev-hwdb-update.service" or "systemd-sysusers.service"
becomes part of the transaction (== becomes included in the initramfs),
- a 90 second wait for dev-disk-by\x2dfoo-bar.device and dependency failure
(if "After=systemd-udevd.service" has not been specified);
(if "After=systemd-udevd.service" has been specified, just as it is now).
Both situations are very unlikely (who would add usr.mount to initramfs? who
would add systemd-sysusers.service to initramfs?), but nevertheless possible.
Hmm, let me see, so you are basically saying that udev wants to run
after sysusers, and sysusers shall run after the file systems are
mounted, and that systemd-***@.service wants to run before that, but
needs to wait until the devices have popped up, which they won't until
udev is started?

So, I am pretty sure we don#t want an explicit After= order here between
dbus and systemd-***@.service...

Hmm, but yuck, I don't see a nice way to fix this for good. Grrr.

I'd probably just merge this as is, and let people who are crazy enough
to run sysuers or hwdb-update in the initrd, to figure this out. Let's
just wait until this pops up...

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-25 18:23:51 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
Post by Ivan Shapovalov
- legacy usr.mount is not automatically ordered after local-fs-pre.target,
Not following here. We do not really support /usr split out unless it is
already mounted in the initrd. But in the initrd its called
"sysroot-usr.mount"... To me this doesn't look like something to do here?
Theoretically it is possible to have initramfs's /usr split out.
I know that it sounds crazy, but if someone will do this, they will lose their
data if usr.mount not properly handled.
initrd cannot have their data split out. I am completely happy about
breaking this, should it exist (which I doubt).
OK. Removed "Before=usr.mount" for v4.
Post by Lennart Poettering
Post by Ivan Shapovalov
If either "systemd-udev-hwdb-update.service" or "systemd-sysusers.service"
becomes part of the transaction (== becomes included in the initramfs),
- a 90 second wait for dev-disk-by\x2dfoo-bar.device and dependency failure
(if "After=systemd-udevd.service" has not been specified);
(if "After=systemd-udevd.service" has been specified, just as it is now).
Both situations are very unlikely (who would add usr.mount to initramfs? who
would add systemd-sysusers.service to initramfs?), but nevertheless possible.
Hmm, let me see, so you are basically saying that udev wants to run
after sysusers, and sysusers shall run after the file systems are
needs to wait until the devices have popped up, which they won't until
udev is started?
Yes, I've meant exactly this.
Post by Lennart Poettering
So, I am pretty sure we don#t want an explicit After= order here between
Hmm, but yuck, I don't see a nice way to fix this for good. Grrr.
I'd probably just merge this as is, and let people who are crazy enough
to run sysuers or hwdb-update in the initrd, to figure this out. Let's
just wait until this pops up...
Lennart
So, do you want me to leave "After=systemd-udevd.service" or remove it?
(An ordering cycle or a waiting timeout?)
--
Ivan Shapovalov / intelfx /
Lennart Poettering
2014-08-26 01:00:30 UTC
Permalink
Post by Ivan Shapovalov
Post by Lennart Poettering
So, I am pretty sure we don#t want an explicit After= order here between
Hmm, but yuck, I don't see a nice way to fix this for good. Grrr.
I'd probably just merge this as is, and let people who are crazy enough
to run sysuers or hwdb-update in the initrd, to figure this out. Let's
just wait until this pops up...
Lennart
So, do you want me to leave "After=systemd-udevd.service" or remove it?
(An ordering cycle or a waiting timeout?)
Remove it. There's no improvement if you have it, and we don't do it in
other cases that are remotely similar to this: services depend on the
devices they manage, but never on the device manager itself.

Lennart
--
Lennart Poettering, Red Hat
Ivan Shapovalov
2014-08-26 11:49:32 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
So, I am pretty sure we don#t want an explicit After= order here between
Hmm, but yuck, I don't see a nice way to fix this for good. Grrr.
I'd probably just merge this as is, and let people who are crazy enough
to run sysuers or hwdb-update in the initrd, to figure this out. Let's
just wait until this pops up...
Lennart
So, do you want me to leave "After=systemd-udevd.service" or remove it?
(An ordering cycle or a waiting timeout?)
Remove it. There's no improvement if you have it, and we don't do it in
other cases that are remotely similar to this: services depend on the
devices they manage, but never on the device manager itself.
Lennart
OK. Fixed for v4.
--
Ivan Shapovalov / intelfx /
Ivan Shapovalov
2014-08-26 11:49:32 UTC
Permalink
Post by Lennart Poettering
Post by Ivan Shapovalov
Post by Lennart Poettering
So, I am pretty sure we don#t want an explicit After= order here between
Hmm, but yuck, I don't see a nice way to fix this for good. Grrr.
I'd probably just merge this as is, and let people who are crazy enough
to run sysuers or hwdb-update in the initrd, to figure this out. Let's
just wait until this pops up...
Lennart
So, do you want me to leave "After=systemd-udevd.service" or remove it?
(An ordering cycle or a waiting timeout?)
Remove it. There's no improvement if you have it, and we don't do it in
other cases that are remotely similar to this: services depend on the
devices they manage, but never on the device manager itself.
Lennart
OK. Fixed for v4.
--
Ivan Shapovalov / intelfx /
Loading...