all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org>
To: Josselin Poiret <dev@jpoiret.xyz>
Cc: 51346@debbugs.gnu.org
Subject: [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags
Date: Sun, 24 Oct 2021 04:05:30 +0200	[thread overview]
Message-ID: <8735orjcvb.fsf@nckx> (raw)
In-Reply-To: <87fsssdqg2.fsf@jpoiret.xyz>

[-- 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 --]

  parent reply	other threads:[~2021-10-24  3:55 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-23  9:46 [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Josselin Poiret via Guix-patches via
2021-10-23  8:55 ` [bug#51346] [PATCH 1/1] gnu: system: Add support for swap " Josselin Poiret via Guix-patches via
2021-10-24 13:58   ` Tobias Geerinckx-Rice via Guix-patches via
2021-10-27 15:09     ` [bug#51346] [PATCH v2 0/4] Rework swap, add flags and dependencies Josselin Poiret via Guix-patches via
2021-10-27 15:09       ` [bug#51346] [PATCH v2 1/4] gnu: system: Rework swap space support, add dependencies Josselin Poiret via Guix-patches via
2021-11-15 10:56         ` [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Ludovic Courtès
2021-11-15 11:04         ` Ludovic Courtès
2021-11-15 20:26           ` [bug#51346] [PATCH v3 0/5] " Josselin Poiret via Guix-patches via
2021-11-15 20:26             ` [bug#51346] [PATCH v3 1/5] gnu: system: Rework swap space support, add dependencies Josselin Poiret via Guix-patches via
2021-11-15 20:26             ` [bug#51346] [PATCH v3 2/5] gnu: system: Warn about swap-devices format change Josselin Poiret via Guix-patches via
2021-11-15 20:26             ` [bug#51346] [PATCH v3 3/5] gnu: system: Add swap flags Josselin Poiret via Guix-patches via
2021-11-15 20:26             ` [bug#51346] [PATCH v3 4/5] gnu: system: Filter out boot dependencies from swap-space Josselin Poiret via Guix-patches via
2021-11-15 20:26             ` [bug#51346] [PATCH v3 5/5] doc: Add new Swap Space section Josselin Poiret via Guix-patches via
2021-11-23  9:23             ` bug#51346: [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Ludovic Courtès
2021-10-27 15:09       ` [bug#51346] [PATCH v2 2/4] gnu: system: Add swap flags Josselin Poiret via Guix-patches via
2021-11-15 10:59         ` [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Ludovic Courtès
2021-10-27 15:09       ` [bug#51346] [PATCH v2 3/4] gnu: system: Filter out boot dependencies from swap-space Josselin Poiret via Guix-patches via
2021-10-27 15:09       ` [bug#51346] [PATCH v2 4/4] doc: Add new Swap Space section Josselin Poiret via Guix-patches via
2021-11-15 11:01         ` [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Ludovic Courtès
2021-10-24  2:05 ` Tobias Geerinckx-Rice via Guix-patches via [this message]
2021-10-25 14:17   ` Josselin Poiret via Guix-patches via

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8735orjcvb.fsf@nckx \
    --to=guix-patches@gnu.org \
    --cc=51346@debbugs.gnu.org \
    --cc=dev@jpoiret.xyz \
    --cc=me@tobias.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.