[bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags

[Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 6846 bytes --]

Josselin,

Josselin Poiret via Guix-patches via 写道：
> This patchset adds new record types swap-partition and 
> swap-file, to be used in the swap-devices field of 
> operating-system.

Thank you so much for this.

Do you happen to know anything about how the Hurd handles swap?

> in the manual I refer to 'man 2 swapon' for the description of 
> these flags.

I think we should document the basics ourselves.  We can still 
refer to the man page if you think it's needed.  WDYT?

Pity that there's no (libc) Info node to which we can link.

> This works well on my laptop, whereas my swap file used to never 
> be swapon on boot because it wasn't available yet (on BTRFS on 
> LUKS). I don't have a swap partition lying around though so 
> testers welcome!

Also boots fine with my plain swap partition:

  (swap-devices (list (swap-partition
                       (device hibernation-device))))

Not having to explicitly manage HIBERNATION-DEVICE, as you suggest 
below, sounds nice too :-)

> I hope this can make it in time for the core-updates-frozen 
> merge.

As noted on IRC, I don't see a reason to involve core-updates at 
all.

We should take the time to define solid interfaces but, once done, 
this can go straight to master.

> I also plan to add swap file hibernation support eventually, 
> where the file offsets are automatically determined by guix (or 
> we could even write our own suspend/resume script in guile, see 
> https://www.kernel.org/doc/html/latest/power/userland-swsusp.html).

Okay.  As also implied on IRC… I have a very low opinion of 
uswsusp.  It's brittle, gimmicky, and introduces many ways for 
bugs to hide and boots to break.  We'll have to carefully track 
incompatible format changes and kernel/initrd generations.

If it is added, we shouldn't involve early userspace in cases 
where it's not strictly needed.

But that for later :-)

> +++ b/doc/guix.texi
> +* Swap Space::                  Adding swap space.

You're following existing precedent here, but I just read the same 
thing twice.

I suggest ‘Swap Space:: Adding virtual memory to free up precious 
RAM.’.

> +@cindex swap devices
> +A list of @code{<swap-partition>} or @code{<swap-file>} objects
> +(@pxref{Swap Space}), to be used for ``swap space'' 
> (@pxref{Memory
> +Concepts,,, libc, The GNU C Library Reference Manual}).

At the risk of leaving this very stubby, I think the (libc) ref 
should be moved to the Swap Space node, which readers might visit 
directly without reading the above.

> +@node Swap Space
> +@section Swap Space
> +@cindex swap space

…so, here.

I'm missing a short intro sentence that mentions what swap is for, 
and that it comes in 2 common forms.

The libc explanation is quite technical, doesn't actually define 
‘swap space’ except by implication, and immediately rambles on 
about zeroes that don't even exist.  As a new user, I think I'd 
feel lost.

> +@deftp {Data Type} swap-partition
> +Objects of this type represent swap partitions. They contain 
> the following
> +members:

(What are ‘swap partitions’?  Maybe explain the pros/cons of both 
in each @deftp intro.  Mostly a reminder to myself, but if you 
want to write more docs: be my guest.)

Always double-space after full stops in prose.

> +@item @code{flags} (default: @code{'()})
> +A list of flags. The supported flags are @code{'delayed} and
> +@code{('priority n)}, see @command{man 2 swapon} in the kernel 
> man pages
> +(@code{man-pages} guix package) for more information.

'delayed?  To?  When?

I'm unenthusiastic about this interface.

On the one hand, exposing this tiny and ossified list of 2.5 
‘flags’ (what even is that priority… thing…) this way feels like 
exposing users to an ugly C implementation detail for no benefit: 
why not

  (swap-partition
    (priority 5) ; or #f distinct from 0
    (discard? #t)
    …)

instead?

On the other hand: perhaps other kernels expose different flags 
and this model might make sense.  I'm not convinced, but I'm 
willing to be.

> +A string, specifying the file path of the swap file to use.

s/file path/name/

> +@item @code{fs}

s/fs/file-system/

As a rule, avoid such pointless abbreviation.  GNU's not unix, 
thankfully.

That said, why does this field exist at all?  The example given 
here:

> +@item (swap-file (path "/swapfile") (fs root-fs))
> +Use the file @file{/swapfile} as swap space, which is present 
> on the
> +@var{root-fs} filesystem.

…rather side-steps the question of how this is supposed to work, 
or in which situation it makes sense.  I feel like it's papering 
over a bug.

> +(define (swap-flags->bit-mask flags)

So I made the mistake of looking at how util-linux does this.

Firstly, it silently clamps (> priority max) to MAX.  I think it 
makes sense to follow that behaviour, but print a warning. 
Ignoring (< priority 0), with a warning, is fine.

Secondly, and this is just weird, ‘man 2 swapon’ explicitly 
documents:

  (prio << SWAP_FLAG_PRIO_SHIFT) & SWAP_FLAG_PRIO_MASK

so naturally util-linux's swapon.c explicitly does this:

  (prio & SWAP_FLAG_PRIO_MASK) << SWAP_FLAG_PRIO_SHIFT

What?  Surely this ancient code can't work just by sheer luck… 
I'll ask.

I see no advantage in ignoring SWAP_FLAG_PRIO_SHIFT, only risks. 
Let's not.

Here's how I'd write it:

--8<---------------cut here---------------start------------->8---
(define (swap-flags->bit-mask flags)
  "Return the number suitable for the 'flags' argument of 'mount' 
  that
corresponds to the symbols listed in FLAGS."
  (let loop ((flags flags))
    (match flags
      ((('priority p) rest ...)
       (if (< p 0)
           (begin (warning
                   (G_ "Ignoring swap priority ~a as it is less 
                   than 0.~%" p))
                  (loop rest))
           (let* ((max (ash SWAP_FLAG_PRIO_MASK (- 
           SWAP_FLAG_PRIO_SHIFT)))
                  (pri (if (> p max)
                           (begin (warning
                                   (G_ "Limiting swap priority ~a to ~a.~%"
                                       p max))
                                  max)
                           p)))
             (logior SWAP_FLAG_PREFER
                     (ash pri SWAP_FLAG_PRIO_SHIFT)
                     (loop rest)))))
      (('discard rest ...)
       (logior SWAP_FLAG_DISCARD (loop rest)))
      (()
       0))))
--8<---------------cut here---------------end--------------->8---

It should also handle invalid input by printing the offending 
symbol instead of a generic match error, but I'm about to board my 
train, and will call it a night here.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]