From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id kMnPNQa9dmGZGwAAgWs5BA (envelope-from ) for ; Mon, 25 Oct 2021 16:19:50 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id 2NR7MQa9dmFlKgAAB5/wlQ (envelope-from ) for ; Mon, 25 Oct 2021 14:19:50 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 0EF4F1102D for ; Mon, 25 Oct 2021 16:19:50 +0200 (CEST) Received: from localhost ([::1]:55918 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mf0pJ-00075H-5A for larch@yhetil.org; Mon, 25 Oct 2021 10:19:49 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:56026) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mf0na-0003XO-MU for guix-patches@gnu.org; Mon, 25 Oct 2021 10:18:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:60593) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mf0na-0005Xw-C9 for guix-patches@gnu.org; Mon, 25 Oct 2021 10:18:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mf0na-00025n-7D for guix-patches@gnu.org; Mon, 25 Oct 2021 10:18:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#51346] [PATCH 0/1 core-updates-frozen] Rework swap device to add dependencies and flags Resent-From: Josselin Poiret Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 25 Oct 2021 14:18:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 51346 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Tobias Geerinckx-Rice Cc: 51346@debbugs.gnu.org X-Debbugs-Original-Cc: 51346@debbugs.gnu.org, guix-patches@gnu.org Received: via spool by 51346-submit@debbugs.gnu.org id=B51346.16351714597987 (code B ref 51346); Mon, 25 Oct 2021 14:18:02 +0000 Received: (at 51346) by debbugs.gnu.org; 25 Oct 2021 14:17:39 +0000 Received: from localhost ([127.0.0.1]:43901 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mf0nD-00024l-AT for submit@debbugs.gnu.org; Mon, 25 Oct 2021 10:17:39 -0400 Received: from jpoiret.xyz ([206.189.101.64]:40882) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mf0nB-00024c-0R for 51346@debbugs.gnu.org; Mon, 25 Oct 2021 10:17:37 -0400 Received: from authenticated-user (jpoiret.xyz [206.189.101.64]) by jpoiret.xyz (Postfix) with ESMTPA id 2F5D2184F41; Mon, 25 Oct 2021 14:17:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jpoiret.xyz; s=dkim; t=1635171455; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=M3f2fVS+TODdAoIvnA+CB2uvAfRbEJh3P7zaZ405LMY=; b=NDfhkBSruiK05cKzHg7CIO26WFdE15g0KfdRAyhulAQD4diS3BWjSPUjQLRP4yxKq/h9Ax URrttONcmt1HIrlDLIzIZ3rVgrzZX4bb/RpAoZG91VW5+LonK6oWesKhsopa3ZaKzGeu40 ZX1QqYID0yvnlO5wSjeuPeutI+3L48pM+BBUzJX1SlblYKjKbNeT9nWuxZkNE8I8Kz+P9g 1NpG37/0QLK0BxGTkBK0aC3UEGLcsfqT1DkZhkx1KjiOUK1ILC2087ZdS22UuHdJyN717Q IAJBrPOdpRpXSvOdShgQlcQZSGY/ATt0L4py5LkxOnJRvUHRn1TacnRUkjWk5g== References: <87fsssdqg2.fsf@jpoiret.xyz> <8735orjcvb.fsf@nckx> Message-ID: <9a5e71f6-91d0-a226-ff51-609598aa3625@jpoiret.xyz> Date: Mon, 25 Oct 2021 14:17:16 +0000 MIME-Version: 1.0 In-Reply-To: <8735orjcvb.fsf@nckx> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spamd-Bar: / X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" Reply-to: Josselin Poiret X-ACL-Warn: , Josselin Poiret via Guix-patches From: Josselin Poiret via Guix-patches via X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1635171590; h=from:from:sender:sender:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:resent-cc: resent-from:resent-sender:resent-message-id:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=M3f2fVS+TODdAoIvnA+CB2uvAfRbEJh3P7zaZ405LMY=; b=LpzMmN6qFbXv7QMra9gCu550YxljT8wSma3pO73zohLw06PD6sB0T+hvld7hgtxaKDyK0+ LTf/2qZKGX7fLjBur4lMlJ8VuUuYuEuVw1XUu1ikeDhyr69Vn+wRHJRKCivODDsKhb9lzI XzVVGcD0q0KMVN9tUg9UGYmkKHyr04c0r07EWs8FXZyO2Ww1e8tMHldygxgT5bpUM8/KLR OYpc4OV5QlnGffRDbixitL1kNYKPeMaQWbXShnSe6M6qeCPq80pr/4ZcYW5ECl2Y3yhy7v gCezA7wswSTGqi0f56CImhBz8GXF5Em1SRpvX6h7oirBBJguO+e9f5GeX3b45g== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1635171590; a=rsa-sha256; cv=none; b=Gc2MQMPzSf8AUI55B+BPYFNcYcbrn5ZoNQdJvR249vPm35G3Pjsfstnipx3RNIj33ykZk0 Iy/L1z4yhJwALL6O9+kcIjGw8qy51+kF9vVH2vLnah3b+emL6wQ8iNP+kPh5lEWBXPxb0M FEDOrf9G9QjUpOYIt/VM2qSQt7YZ/d82jzpIjYED4t5uf+0a5F8DsUx6mrc+7mOhifOgNr fdO1TAnDPLqWejKOcxRzBX1Q8V6EMcny00B5MunBV2gGln/5VsHYS+p555WFaPbXaZCakz vIZucYJKYcPAgkujvlMycopXwQaue85uQU8aDosAIjLNPuXwRUt28C+GbtDM6Q== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=jpoiret.xyz header.s=dkim header.b=NDfhkBSr; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Spam-Score: -3.33 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=jpoiret.xyz header.s=dkim header.b=NDfhkBSr; dmarc=pass (policy=none) header.from=gnu.org; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: 0EF4F1102D X-Spam-Score: -3.33 X-Migadu-Scanner: scn1.migadu.com X-TUID: 637gOaKPKmxd 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{} or @code{} 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 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