diff mbox series

[bug#40927] Subject=[bug#40927] [PATCH] Allow resume from swap device during boot

Message ID 87o8qz798a.fsf@m4x.org
State Accepted
Headers show
Series [bug#40927] Subject=[bug#40927] [PATCH] Allow resume from swap device during boot | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job

Commit Message

Jean-Baptiste Note May 7, 2020, 8:58 p.m. UTC
Dear Tobias,

I hopefully found a way to reply to your message directly. I've checked
resume with your patch -- actually a slightly reworked version of it; a
few remarks:

- I've amended it as per Danny's suggestions regarding documentation location

- I've adjusted it to take the guixy --resume and --noresume options
  rather than the kernel norm resume and noresume. In particular this
  allows me to test the 'kernel' resume path and the 'initrd' resume
  path with the same os definition quickly, only adjusting the GRUB
  cmdline within GRUB.

- I find the added copyright years on top strange -- shouldn't it be
  2020 ? 

The results of my tests are attached as a separate file. In a nutshell:

- I can confirm that on my kernel (default guix linux-libre-5.4) the
  resume= option by itself does not resume at all. This seems to have been
  reported already:
  https://lists.gnu.org/archive/html/bug-guix/2016-11/msg00060.html

- resume-by-uuid from userspace does not work
- resume-by-label from userspace does not work either
(as much as I'd like them to).

(please note that, on top of this, I found no way to specify the swap
devices in the os definition by UUID or LABEL either -- so the
limitation does not bother me in the current state).

Each time these loop for 20 seconds, waiting for the partition to appear
(the partition is there, from dmesg, but somehow is not in the database
looked for in (canonicalize-device-spec spec) -- weird).

Regarding the interface, I'm not sure we should feel bound by the
argument naming convention of the kernel for resume handling from
userspace -- we could leave its own namespace for the kernel to handle,
for those kernels that can, and separate the resume handling as done by
initrd in a separate, more guixy namespace.

Actually I don't really understand why we would want overlapping of
names for two different codepaths; I think separating the names brings
more flexibility (you can control from the GRUB commandline which resume
path you want to take) and less confusion (it is clearer from the
commandline where the resuming will/should be handled).

To summarize:

* I still think that this feature is greatly needed: we can hibernate
  with the current software stack, but the default kernel won't resume,
  leaving the system in a bad state (swap is not activated by shepherd,
  see logs), and we need manual swapon to leave this bad state;

* This version of the patch looks like it can handle UUID and LABEL,
  while it can't -- for reasons that i've not dug;

* Meanwhile I find resume-by-uuid or resume-by-label currently
  pointless, given the limited ways we have to specify swap devices in
  the OS definition file.

I'd be in favor of doing the following:

* Dropping support for UUID and LABEL in the code *OR* signalling
  clearly in some comment that the path is currently non-functional;

* Including the patch ASAP to avoid the really bad state we're in
  currently after a suspend.

* Using the guixy --resume and --noresume options rather than the kernel
  ones

Please let me know how to proceed, and let me know how to handle the
copyright notice.

Kind regards,
Jean-Baptiste
# by device node -- does work
jb@guixrules ~$ sudo loginctl hibernate
jb@guixrules ~$ cat /proc/cmdline 
BOOT_IMAGE=/gnu/store/bkpkbms3mp8s45kpir4f2cnvxvgdvssp-linux-libre-5.4.39/bzImage --root=ed73cc09-aff3-41e4-90b6-51fb41a7d225 --system=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system --load=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system/boot modprobe.blacklist=pcspkr,snd_pcsp --resume=/dev/sda3 quiet

# by UUID -- does not work
jb@guixrules ~$ sudo swaplabel /dev/sdb3
swaplabel: /dev/sdb3: not a valid swap partition
jb@guixrules ~$ sudo swapon /dev/sdb3
swapon: /dev/sdb3: software suspend data detected. Rewriting the swap signature.
jb@guixrules ~$ sudo swaplabel /dev/sdb3
LABEL: resume_device
UUID:  446c81d4-efcf-4508-a9ab-bb38f74ff77d
jb@guixrules ~$ cat /proc/cmdline 
BOOT_IMAGE=/gnu/store/bkpkbms3mp8s45kpir4f2cnvxvgdvssp-linux-libre-5.4.39/bzImage --root=ed73cc09-aff3-41e4-90b6-51fb41a7d225 --system=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system --load=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system/boot modprobe.blacklist=pcspkr,snd_pcsp --resume=446c81d4-efcf-4508-a9ab-bb38f74ff77d quiet
jb@guixrules ~$ diff <(echo 446c81d4-efcf-4508-a9ab-bb38f74ff77d) <(echo 446c81d4-efcf-4508-a9ab-bb38f74ff77d)
jb@guixrules ~$ echo $?
0

# by LABEL -- does not work
jb@guixrules ~$ sudo swaplabel /dev/sdb3
swaplabel: /dev/sdb3: not a valid swap partition
jb@guixrules ~$ sudo swapon /dev/sdb3
swapon: /dev/sdb3: software suspend data detected. Rewriting the swap signature.
jb@guixrules ~$ sudo swaplabel 
swaplabel: no device specified
Try 'swaplabel --help' for more information.
jb@guixrules ~$ sudo swaplabel /dev/sdb3
LABEL: resume_device
UUID:  446c81d4-efcf-4508-a9ab-bb38f74ff77d
jb@guixrules ~$ cat /proc/cmdline 
BOOT_IMAGE=/gnu/store/bkpkbms3mp8s45kpir4f2cnvxvgdvssp-linux-libre-5.4.39/bzImage --root=ed73cc09-aff3-41e4-90b6-51fb41a7d225 --system=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system --load=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system/boot modprobe.blacklist=pcspkr,snd_pcsp --resume=resume_device quiet

# leaving it to the kernel -- does not work
jb@guixrules ~$ sudo swaplabel /dev/sdb3
swaplabel: /dev/sdb3: not a valid swap partition
jb@guixrules ~$ sudo swapon /dev/sdb3
swapon: /dev/sdb3: software suspend data detected. Rewriting the swap signature.
jb@guixrules ~$ cat /proc/cmdline 
BOOT_IMAGE=/gnu/store/bkpkbms3mp8s45kpir4f2cnvxvgdvssp-linux-libre-5.4.39/bzImage --root=ed73cc09-aff3-41e4-90b6-51fb41a7d225 --system=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system --load=/gnu/store/xcdn9naivwxhr4x0cq669zzn9f02x706-system/boot modprobe.blacklist=pcspkr,snd_pcsp resume=/dev/sdb3 quiet
diff mbox series

Patch

From 889fc647a84289747ae840778f0313193eca1316 Mon Sep 17 00:00:00 2001
From: Jean-Baptiste Note <jean-baptiste.note@m4x.org>
Date: Wed, 6 May 2020 20:38:21 +0000
Subject: [PATCH] linux-boot: Resume from hibernation.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* gnu/build/linux-boot.scm (resume-if-hibernated): New procedure.
(boot-system): Call it unless ‘noresume’ was specified.
---
 gnu/build/linux-boot.scm | 53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/gnu/build/linux-boot.scm b/gnu/build/linux-boot.scm
index 05e833c0c6..be1cda0181 100644
--- a/gnu/build/linux-boot.scm
+++ b/gnu/build/linux-boot.scm
@@ -1,5 +1,6 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2016, 2017, 2019, 2020 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2019 Guillaume Le Vaillant <glv@posteo.net>
 ;;;
@@ -110,6 +111,52 @@  OPTION doesn't appear in ARGUMENTS."
                        (substring arg (+ 1 (string-index arg #\=)))))
                 arguments)))
 
+(define (resume-if-hibernated device)
+  "Resume from hibernation if possible.  This is safe ONLY if no on-disk file
+systems have been mounted; calling it later risks severe file system
+corruption!  See <Documentation/power/swsusp.txt> in the kernel source
+directory.  This is the caller's responsibility, as is catching exceptions if
+resumption was supposed to happen but didn't.
+
+Resume only from DEVICE if it's a string.  If it's #f, use the kernel's default
+hibernation device (CONFIG_PM_STD_PARTITION).  Never return if resumption
+succeeds.  Return nothing otherwise.  The kernel logs any details to dmesg."
+
+  (define (string->major:minor string)
+    "Return a string with MAJOR:MINOR numbers of the device specified by STRING."
+
+    ;; The "resume=" kernel command-line option always provides a string, which
+    ;; can represent a device, a UUID, or a label.  Check for all three.
+    (let* ((spec (cond ((string-prefix? "/" string) string)
+                       ((uuid string) => identity)
+                       (else (file-system-label string))))
+           ;; XXX kernel's swsusp_resume_can_resume() waits for the resume
+           ;; device to appear if ‘resumewait’ is found on the command line;
+           ;; our canonicalize-device-spec gives up after 20 seconds.  We
+           ;; could loop (ew!) if someone relies on it…
+           (device (canonicalize-device-spec spec))
+           (rdev (stat:rdev (stat device)))
+           (minor (modulo rdev 256))
+           (major (/ (- rdev minor) 256)))
+      (format #f "~a:~a" major minor)))
+
+  ;; Write the MAJOR:MINOR numbers of the specified or default resume DEVICE to
+  ;; this magic file.  The kernel will immediately try to resume from it.
+  (let ((resume "/sys/power/resume"))
+    (when (file-exists? resume)         ; this kernel supports hibernation
+      ;; Honour the kernel's default device (only) if none other was given.
+      (let ((major:minor (if device
+                             (string->major:minor device)
+                             (let ((default (call-with-input-file resume
+                                              read-line)))
+                               ;; Don't waste time echoing ‘nothing’ to /sys.
+                               (if (string=? "0:0" default)
+                                   #f
+                                   default)))))
+        (when major:minor
+          (call-with-output-file resume ; may throw an ‘Invalid argument’
+            (cut display major:minor <>))))))) ; may never return
+
 (define* (make-disk-device-nodes base major #:optional (minor 0))
   "Make the block device nodes around BASE (something like \"/root/dev/sda\")
 with the given MAJOR number, starting with MINOR."
@@ -504,6 +551,12 @@  upon error."
         (load-linux-modules-from-directory linux-modules
                                            linux-module-directory)
 
+        (unless (member "--noresume" args)
+          ;; Try to resume immediately after loading (storage) modules
+          ;; but before any on-disk file systems have been mounted.
+          (false-if-exception           ; failure is not fatal
+           (resume-if-hibernated (find-long-option "--resume" args))))
+
         (when keymap-file
           (let ((status (system* "loadkeys" keymap-file)))
             (unless (zero? status)
-- 
2.26.2