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

[Josselin Poiret]

Hi,



First and foremost, thank you for your review!



On 10/24/21 2:05 AM, Tobias Geerinckx-Rice wrote:

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



I'm investigating this currently, got a Hurd vm up and running ("guix system 
image -t hurd-raw" is very nice). We talked about this on IRC, and decided that 
it would be easier to simply used the Hurd's swapon binary (from the hurd 
package itself), rather than directly communicate with the default_pager 
server, since that'd involve writing a lot of Hurd RPC code. In the long run, 
this would be the way to go, just like for Linux (we use (guix build syscalls) 
instead of util-linux's swapon), but getting initial support in faster would be 
great.



>> 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.



Now that I'm looking at it, they really are not that complicated, I'll write 
something to describe them then.


>> +++ 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.’.



Noted! Maybe something along "Backing RAM with disk space." rather, as swap 
space isn't really memory?



>> +@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.



Will add.



>> +@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.


Welp, looks like you gave me the perfect excuse: from what I gather, Hurd does 
not have any flags for its swap space (see hurd/default_pager.defs, line 86). 
The other part of the reason is that I did not want to have even more 
"swap-partition-priority"/"swap-file-priority" duplicate accessors (more on 
swap-file/swap-partition below).



>> +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.



Abbreviation aside, the example is bad: even though theoretically presence of 
the swap file depends on the root filesystem being mounted, well, root-fs is 
always mounted and should be ignored (there's no filesystem-/ service either). 
A better example would be my personal configuration:



(swap-devices (list

 (swap-file

  (path "/btrfs/swapfile")

  (fs (car (filter (lambda (x)

                     (equal? (file-system-mount-point x)

                             "/btrfs"))

                   file-systems)))))) 



You can see here that /btrfs/swapfile wouldn't be accessible if the filesystem 
under fs wasn't mounted first.



>> +(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 mean, SWAP_FLAG_PRIO_SHIFT has been equal 0 since at least the initial linux 
git commit 17 years ago, so it might actually be sheer luck.



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


How I saw it: if SWAP_FLAG_PRIO_SHIFT ever changes from 0, and our code 
actually cared about its value, we'd still have to notice the change and change 
the value (it would be different if we were parsing linux headers instead). But 
I guess that's mostly laziness on my part, and since you wrote a nice 
replacement, I guess I'll add that.



> 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.


Will do!





On 10/24/21 1:58 PM, Tobias Geerinckx-Rice wrote:

> Oh no,

> 

> he's back.  With another annoying question: why don't we drop the whole 
> swap-partition/swap-file dichotomy?  The distinction is artificial insofar as 
> Linux doesn't make one.

> 

> Which end is supposed to explode if you

> 

>  (swap-partition (device "/home/nckx/swap"))

>  (swap-file (name "/dev/sda2"))

>

> ?

>

> What real-world drawback(s) do you see to

> 

>  (swap (space "/home/nckx/swap"))

>  (swap (space "/dev/sda2"))

>  (swap (space (uuid "ab-c-d-e-fgh")))

>  (swap (space (file-system-label "best-swaps")))

> 

> naming aside?



The motivating thing for me was that they have to be treated differently for 
hibernation purposes: you shouldn't make the dependencies of a swap file 
available (ie mount the filesystem it's on) but rather determine its offset 
inside the block device. For a swap partition, there's no such thing, you have 
to make the device itself available to the kernel. We could have a swapfile? 
flag instead though, but I'm still not convinced by both approaches. For the 
current patch though, nothing is going to explode there (although for a 
swap-file you do have to specify the filesystem it is on, but that's just my 
record definition forcing you to).



> Josselin Poiret via Guix-patches via 写道：

>> +(define (swap-partition->service-name spartition)

> 

> Nitpick: ->shepherd-service-name just for similarity to <file-system>s.

> 

> Aside, when I try to apply your third manual example, I get:

> 

>  guix system: error: service 'swap-/dev/sda2' requires

>  'file-system-/', which is not provided by any service



I forgot that the root filesystem is treated differently from others, so this 
example is borked. I'll add something akin to my config instead. On a 
tangential note, there's nothing stopping us from renaming root-file-system to 
file-system-/, so that these at least don't give an error, right (they're 
logically not wrong dependencies, although a bit useless)?



Thanks again for your review!

A revised patchset (yes, this time with multiple commits) will be coming soon, 
once I figure out the proper way to work with block devices on Hurd.



Best,

Josselin Poiret