systemd-sleep: always attempt hibernation if configured #14270

zachsmith · 2019-12-07T01:06:07Z

When calculation of swap file offset is unsupported, rely on the /sys/power/resume & /sys/power/resume_offset values rather than requiring a matching swap entry to be identified.

Refactor to use dev_t for comparison of resume= device instead of string.

Fixes #14249

diabonas

On Btrfs the comparison between the device ID from stat

systemd/src/shared/sleep-config.c

Line 207 in 43a2005

swap_dev = streq(swap->type, "partition") ? sb.st_rdev : sb.st_dev;

and the device ID from /sys/power/resume

systemd/src/shared/sleep-config.c

Line 256 in 43a2005

r = read_one_line_file("/sys/power/resume", &resume);

does not work, see #14249 (comment). Therefore this PR does not allow for hibernation on my system (it might be further complicated by the fact that my Btrfs file system is encrypted using LUKS, I will be able to test a non-encrypted file system tomorrow at the earliest).

I think the easiest fix would be to record in the /proc/swaps parsing loop whether a swap file on a Btrfs file system has been found (i.e. whether resume_offset is 0 for one of the entries) and change

systemd/src/shared/sleep-config.c

Lines 396 to 398 in 43a2005

    
           if (!streq(sys_resume, "0:0") && !location_is_resume_device(hibernate_location, sys_resume, sys_offset)) 
        
                   return log_warning_errno(SYNTHETIC_ERRNO(ENOSYS), "/sys/power/resume and /sys/power/resume_offset has no matching entry in /proc/swaps; Hibernation will fail: resume=%s, resume_offset=%" PRIu64, 
        
                                            sys_resume, sys_offset);

to produce only a warning in this case, but to continue with the selected swap entry anyway (like systemd did before #13682).

lgtm-com · 2019-12-08T00:24:04Z

This pull request introduces 1 alert when merging fc51f44 into debda5a - view on LGTM.com

new alerts:

1 for Comparison result is always the same

lgtm-com · 2019-12-08T00:59:50Z

This pull request introduces 1 alert when merging 814e467 into debda5a - view on LGTM.com

new alerts:

1 for Comparison result is always the same

lgtm-com · 2019-12-08T01:33:54Z

This pull request introduces 1 alert when merging eb4c81f into debda5a - view on LGTM.com

new alerts:

1 for Comparison result is always the same

diabonas · 2019-12-08T19:43:52Z

src/shared/sleep-config.c

                                break;
                }
        }

        if (!hibernate_location)
                return log_debug_errno(SYNTHETIC_ERRNO(ENOSYS), "No swap partitions or files were found");

-        if (!streq(sys_resume, "0:0") && !location_is_resume_device(hibernate_location, sys_resume, sys_offset))
+        int resume_match = location_is_resume_device(hibernate_location, sys_resume, sys_offset);


This will only check whether the selected swap entry (i.e. the "highest priority swap with most remaining space") is on a Btrfs file system. This fails in case

there are multiple swap entries and

the hibernation location is a Btrfs swap file (hence cannot be found by location_is_resume_device in the parsing loop), but

the selected swap entry is not on a Btrfs file system.

I think that would only happen if you had a swap file on a non-Btfs fs configured with a higher priority than the swap entry for resume= and resume_offset=.

I wonder if what we should do instead is to bypass the location detection if SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK is set because if the user has that set it seems to imply that they have their swaps configured correctly. In this case, find_hibernate_location would just return the values for resume= and resume_offset= or return an error if they are not set.

@diabonas, I pushed up a new change that will allow SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK to bypass the location detection if the user has correctly configured /sys/power/resume (and by extension, /sys/power/resume_offset when using a swapfile).

src/shared/sleep-config.c

poettering · 2019-12-09T12:31:51Z

src/shared/sleep-config.c

        assert(location);
        assert(location->resume);
        assert(sys_resume);

-        return streq(sys_resume, location->resume) && sys_offset == location->resume_offset;
+        bool resume_is_set = !streq(sys_resume, "0:0");


Hmm, can't we change thing so that instead of string comparing sys_resume with "0:0" everywhere we just say that sys_resume set to NULL means what "0:0" currently means? i.e. in read_resume_files() already filter our "0:0" and set *ret_resume to NULL when we see it.

Propagating this magic "I am not set" string through all layers is just weird.

Actually thinking about this, read_resume_files() should probably change ret_resume to be of type dev_t, and use parse_dev() to parse it to dev_t directly? i.e. we should parse where we read the data, and then prpoagate parsed data, instead of literal strings

poettering · 2019-12-09T12:33:24Z

src/shared/sleep-config.c

+         */
+        if (getenv_bool("SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK") > 0) {
+                if (streq(sys_resume, "0:0"))
+                    return log_warning_errno(SYNTHETIC_ERRNO(ENOSYS),


indentation off

poettering · 2019-12-09T12:34:13Z

src/shared/sleep-config.c

+        if (resume_match >= 0) {
+                log_debug("Hibernation will attempt to use swap entry with path: %s, device: %s, offset: %" PRIu64 ", priority: %i",
+                          hibernate_location->swap->device, hibernate_location->resume, hibernate_location->resume_offset, hibernate_location->swap->priority);
+        } else {


no {} around single-line if blocks according to our coding style

diabonas

The latest version of this PR works for my LUKS-encrypted Btrfs swap file when enabling SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK=1.

I'm not sure about the purpose of this environment variable though: to me it would make more sense if we would record if location_is_resume_device returned -1/maybe for any of the entries in /proc/swaps and use the swap defined by sys_resume/sys_offset in this case (probably with a warning message that it wasn't possible to check the free space). After all, this is just an additional safety check that is impossible to fulfil with the current implementation, why make users jump through additional hoops to enable the use of their explicitly configured swap device?

diabonas · 2019-12-09T16:40:23Z

src/shared/sleep-config.c

@@ -421,6 +496,13 @@ static bool enough_swap_for_hibernation(void) {
        if (r < 0)
                return false;

+        /* in cases where we were not able to identify available swap information, log a message with override instructions and fail */
+        if (r > 0 && hibernate_location->swap == NULL) {
+                log_debug("Unable to determine available swap space for resume=%s, resume_offset=%" PRIu64 ";set SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK=1 to override this check.",


This should be log_warning, otherwise most people will never see this information.

Agreed. It would be great to make unable to check the space as a warning while confirmed insufficient space as error. After all if hibernation failed, the system should continue to function without corruption.

src/shared/sleep-config.c

src/sleep/sleep.c

poettering · 2019-12-12T18:50:38Z

looks pretty ok, but the %lu thing looks suspicious?

poettering · 2019-12-12T18:51:17Z

and thanks for the enum and dev_t rework, this looks a lot better now!

poettering · 2019-12-13T12:40:47Z

ci didn't like it

zachsmith · 2019-12-15T01:06:13Z

Works fine on my LUKS-encrypted system with a swap file on a Btrfs partition when setting SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK=1 in /etc/systemd/system/systemd-logind.service.d/override.conf, thanks :) +1

Awesome! Glad to hear it worked with your LUKS setup. 👍

keszybz

in the end, I think the best course of action is to give Btrfs users the ability to override find_hibernate_location - if they have /sys/power/resume and /sys/power/resume_offset configured correctly, hibernation should work (unless there isn't enough space).

Yes! I started working on a review yesterday, and wrote something to the same effect. Please squash/rebase the commits. The comments below might be outdated already.

keszybz · 2019-12-14T14:59:19Z

src/shared/sleep-config.c

+typedef enum {
+        UNMATCHED = 0,
+        MATCHED = 1,
+        UNKNOWN = 2


RESUME_UNMATCHED, RESUME_MATCHED, RESUME_UNKOWN.
And put a comma after the last definition too, no need to make it special.

keszybz · 2019-12-14T15:00:43Z

src/shared/sleep-config.c

-                    return log_warning_errno(SYNTHETIC_ERRNO(ENOSYS),
-                                             "SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK is set but /sys/power/resume is not configured; hibernation will fail");
+                if (sys_resume == 0)
+                        return log_warning_errno(SYNTHETIC_ERRNO(ENOSYS),


This chunk should go the first commit.

keszybz · 2019-12-14T15:13:23Z

src/shared/sleep-config.c

@@ -310,6 +328,19 @@ int find_hibernate_location(HibernateLocation **ret_hibernate_location) {
        if (r < 0)
                return r;

+        /* Users of swapfiles on Btrfs should override swap discovery because
+         * systemd will be unable to correctly calculate offsets.
+         */


I don't think this recommendation is good. If we cannot determine if the offset if specified correctly because of unsupported fs type, we should just assume it is. There is absolutely no point in asking the users to set a variable, since the result will be exactly the same (we will accept the value), except that the user has to do a manual step.

keszybz · 2019-12-15T09:18:32Z

Oh, I read your comment in the issue, and I see that I misunderstood what you are saying. If the user uses brtrfs swapfile, we should just assume that the configuration is correct in the part that we cannot verify, and check the parts that we can verify. No point in asking the user to set a variable.

zachsmith · 2019-12-16T16:12:12Z

Oh, I read your comment in the issue, and I see that I misunderstood what you are saying. If the user uses brtrfs swapfile, we should just assume that the configuration is correct in the part that we cannot verify, and check the parts that we can verify. No point in asking the user to set a variable.

Thanks, @keszybz. I think this should be ready to go unless there is more feedback that needs to be incorporated.

poettering · 2019-12-16T17:36:04Z

src/sleep/sleep.c

@@ -39,18 +39,19 @@ STATIC_DESTRUCTOR_REGISTER(arg_verb, freep);

 static int write_hibernate_location_info(const HibernateLocation *hibernate_location) {
        char offset_str[DECIMAL_STR_MAX(uint64_t)];
+        char resume_str[DECIMAL_STR_MAX(dev_t)];


hmm, i'd size this DECIMAL_STR_MAX(unsigned) * 2 + STRLEN(":") or so

src/sleep/sleep.c

poettering · 2019-12-17T09:39:18Z

@poettering migrating the resume device comparison from string to dev_t re-exposed the fact that Btrfs will not return the correct dev_t from stat.

Note that we have a helper to deal with that: get_block_device() and btrfs_get_block_device_fd() which have specil knowledge of btrfs ioctls to retrieve the backing block device.

zachsmith · 2019-12-17T15:48:15Z

Note that we have a helper to deal with that: get_block_device() and btrfs_get_block_device_fd() which have specil knowledge of btrfs ioctls to retrieve the backing block device.

Thanks @poettering - changed swap_device_to_major_minor to use get_block_device in the case of a swap file.

keszybz

No, sorry, I think the code is still trying to do the wrong thing. If we cannot figure out if a device matches (because it is on btrfs or another fs where our implementation is deficient), we should emit a debug level warning and assume that the device matches.

Partially quoting @diabonas:

in what kind of situation would resume and resume_offset be configured, but SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK should be unset [to avoid false positive because the code cannot deal with some fs types]?

I think that the answer is "never". I.e. the variable makes sense to skip size-of-memory-check, but we should not require it for skipping unsupported filesystems.

keszybz · 2019-12-17T16:53:41Z

src/shared/sleep-config.c

@@ -373,13 +403,18 @@ int find_hibernate_location(HibernateLocation **ret_hibernate_location) {
                        if (r < 0)
                                return r;

+                        dev_t swap_device;
+                        r = parse_dev(swap_device_id, &swap_device);


First a conversion from a number to a string, and then back from a string to a number. swap_device_to_major_minor() should just return dev_t.

Totally agree with @keszybz. There is no point to ask the user to set an environment variable.
When the user setup kernel parameters it is already explicit enough.

There are a few important points to point out of why this environment variable is inconvenient.

Present: It will break existing setup. The user must figure out the new way and begs the question why the old way just work. It also means that the PR that introduces this bug is incomplete.

Future: Eventually Systemd is going to fix this, where the memory check is done properly for swap file on btrfs. This benefit will not propagate to the user since the environment variable is set. Once this environment variable is configured it will stay there. I am not going to monitor the changelog for when I can remove the variable to get the benefit of memory checks.

First a conversion from a number to a string, and then back from a string to a number. swap_device_to_major_minor() should just return dev_t.

Thanks for this suggestion. This was a good catch.

zachsmith · 2019-12-21T01:55:21Z

No, sorry, I think the code is still trying to do the wrong thing. If we cannot figure out if a device matches (because it is on btrfs or another fs where our implementation is deficient), we should emit a debug level warning and assume that the device matches.

Partially quoting @diabonas:

in what kind of situation would resume and resume_offset be configured, but SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK should be unset [to avoid false positive because the code cannot deal with some fs types]?

I think that the answer is "never". I.e. the variable makes sense to skip size-of-memory-check, but we should not require it for skipping unsupported filesystems.

Updated to remove requirement to use SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK for Btrfs swap files.

zachsmith · 2019-12-21T02:30:02Z

Thanks for all the constructive feedback @diabonas, @lkjell, @helloworld1, @keszybz, and @poettering.

I've pushed changes to remove the requirement to set SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK when using a swap file on Btrfs - you all made good points on this.

For those of you using Btrfs swapfile, any testing with your setups would be appreciated.

diabonas · 2019-12-21T16:43:40Z

Great, thank you for all your work! The updated PR works out of the box (i.e. without having to set SYSTEMD_BYPASS_HIBERNATION_MEMORY_CHECK) on my system with a swap file on a LUKS-encrypted Btrfs partition.

src/shared/sleep-config.c

poettering · 2020-01-03T14:28:35Z

src/shared/sleep-config.c

        if (r < 0)
                return log_debug_errno(r, "Error reading /sys/power/resume: %m");

+        r  = parse_dev(resume_str, &resume);


spurious space before "="

poettering · 2020-01-03T14:29:35Z

src/shared/sleep-config.c

+/*
+ * Determine if the HibernateLocation matches the resume= (device) and resume_offset= (file).
+ */
+static bool location_is_resume_device(const HibernateLocation *location, const dev_t sys_resume, const uint64_t sys_offset) {


please drop the const from non-pointer parameters, that has no effect... (i.e. leave the one for location, drop the one from sys_resume and sys_offset)

poettering · 2020-01-03T14:30:28Z

src/shared/sleep-config.c

+        bool resume_match = location_is_resume_device(hibernate_location, sys_resume, sys_offset);
+
+        /* resume= is set, but a matching /proc/swaps entry was not found */
+        if (!resume_match && sys_resume > 0) {


0? do we know dev_t is not signed? should be != 0, no?

src/shared/sleep-config.h

When calculation of swap file offset is unsupported, rely on the /sys/power/resume & /sys/power/resume_offset values if configured rather than requiring a matching swap entry to be identified. Refactor to use dev_t for comparison of resume= device instead of string.

keszybz · 2020-01-07T13:35:52Z

src/shared/sleep-config.c

-        if (asprintf(&major_minor, "%u:%u", major(swap_dev), minor(swap_dev)) < 0)
-                return log_oom();
+        if (streq(swap->type, "partition")) {
+                if(!S_ISBLK(sb.st_mode))


Missing space after if.

keszybz · 2020-01-07T15:03:57Z

src/shared/sleep-config.c

+
+        /* resume= is set, but a matching /proc/swaps entry was not found */
+        if (!resume_match && sys_resume != 0) {
+                log_debug("/sys/power/resume appears to be configured but a matching swap in /proc/swaps could not be identified; hibernation may fail");


This is still not OK. We can distinguish the cases where:

there is no swap in /proc/swaps

there is a device in /proc/swaps but a different device is configured in /sys/power/resume

/sys/power/resume is populated and there's a matching device in /proc/swaps but we cannot verify the offset.

With your patch, after swapoff -a I get:

/sys/power/resume appears to be configured but a matching swap in /proc/swaps could not be identified; hibernation may fail Unable to determine remaining swap space; hibernation may fail Hibernation configured and possible: yes

But hibernation will surely fail, so saying that it "might fail" is wrong.

See #14511. I think it DTRT.

Thanks @keszybz - sorry you had to go make some final changes :-(

@zachsmith thank you for working on this. It's not an easy piece of code, as all the back-and-forth shows.

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from ffe5cee to 0f015c0 Compare December 7, 2019 01:09

zachsmith mentioned this pull request Dec 7, 2019

Systemd 244 Hibernation failed due to sys/power/resume and /sys/power/resume_offset has no matching entry in /proc/swaps #14249

Closed

zachsmith changed the title ~~systemd-sleep: fix hibernation regression on btrfs~~ systemd-sleep: fix hibernation regression on Btrfs Dec 7, 2019

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch 2 times, most recently from 37349d5 to d9774e7 Compare December 7, 2019 01:37

diabonas suggested changes Dec 7, 2019

View reviewed changes

yuwata added the sleep label Dec 7, 2019

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch 3 times, most recently from f048cce to fc51f44 Compare December 7, 2019 23:57

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch 2 times, most recently from ab919a1 to 814e467 Compare December 8, 2019 00:34

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 814e467 to eb4c81f Compare December 8, 2019 01:10

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch 2 times, most recently from ebedc31 to 1826278 Compare December 8, 2019 03:47

diabonas suggested changes Dec 8, 2019

View reviewed changes

poettering reviewed Dec 9, 2019

View reviewed changes

src/shared/sleep-config.c Show resolved Hide resolved

poettering reviewed Dec 9, 2019

View reviewed changes

diabonas reviewed Dec 9, 2019

View reviewed changes

poettering reviewed Dec 12, 2019

View reviewed changes

src/shared/sleep-config.c Outdated Show resolved Hide resolved

poettering reviewed Dec 12, 2019

View reviewed changes

src/sleep/sleep.c Outdated Show resolved Hide resolved

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 5b5cde5 to 56a82b2 Compare December 13, 2019 15:57

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 8b359b0 to 989fbb1 Compare December 15, 2019 01:27

keszybz reviewed Dec 15, 2019

View reviewed changes

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 989fbb1 to 928541d Compare December 15, 2019 16:12

zachsmith changed the title ~~systemd-sleep: fix hibernation regression on Btrfs~~ systemd-sleep: remove matching swap requirement for resume= Dec 15, 2019

poettering reviewed Dec 16, 2019

View reviewed changes

src/sleep/sleep.c Show resolved Hide resolved

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 928541d to df70f92 Compare December 17, 2019 15:46

keszybz requested changes Dec 17, 2019

View reviewed changes

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from df70f92 to 9a5e8cc Compare December 21, 2019 01:54

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 9a5e8cc to e9b6fbd Compare December 21, 2019 02:24

zachsmith requested a review from keszybz December 21, 2019 02:30

zachsmith changed the title ~~systemd-sleep: remove matching swap requirement for resume=~~ systemd-sleep: always attempt hibernation if configured Dec 21, 2019

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from e9b6fbd to 8c8502f Compare December 21, 2019 17:12

poettering requested changes Jan 3, 2020

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jan 3, 2020

zachsmith force-pushed the systemd-sleep-fix-btrs-hibernate-bug branch from 8c8502f to 5213327 Compare January 6, 2020 04:15

zachsmith requested a review from poettering January 6, 2020 04:19

keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jan 7, 2020

keszybz requested changes Jan 7, 2020

View reviewed changes

keszybz mentioned this pull request Jan 7, 2020

Sleep check rework #14511

Merged

poettering merged commit 5213327 into systemd:master Jan 8, 2020

	if (!streq(sys_resume, "0:0") && !location_is_resume_device(hibernate_location, sys_resume, sys_offset))
	return log_warning_errno(SYNTHETIC_ERRNO(ENOSYS), "/sys/power/resume and /sys/power/resume_offset has no matching entry in /proc/swaps; Hibernation will fail: resume=%s, resume_offset=%" PRIu64,
	sys_resume, sys_offset);

systemd-sleep: always attempt hibernation if configured #14270

systemd-sleep: always attempt hibernation if configured #14270

Conversation

zachsmith commented Dec 7, 2019 • edited

diabonas left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 8, 2019

lgtm-com bot commented Dec 8, 2019

lgtm-com bot commented Dec 8, 2019

Choose a reason for hiding this comment

zachsmith Dec 9, 2019 • edited

Choose a reason for hiding this comment

zachsmith Dec 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diabonas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Dec 12, 2019

poettering commented Dec 12, 2019

poettering commented Dec 13, 2019

zachsmith commented Dec 15, 2019

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented Dec 15, 2019

zachsmith commented Dec 16, 2019

Choose a reason for hiding this comment

poettering commented Dec 17, 2019

zachsmith commented Dec 17, 2019

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachsmith commented Dec 21, 2019

zachsmith commented Dec 21, 2019 • edited

diabonas commented Dec 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachsmith commented Dec 7, 2019 •

edited

zachsmith Dec 9, 2019 •

edited

zachsmith Dec 9, 2019 •

edited

zachsmith commented Dec 21, 2019 •

edited