[systemd-devel] [PATCH RFC] swap: use aliases to group swap units for same device

Zbigniew Jędrzejewski-Szmek zbyszek at in.waw.pl
Thu Oct 18 15:56:36 PDT 2012


A series of .swap units "following" one another are replaced with a
single unit with multiple names.

The idea is to simplify things for the user: only one swap unit per
swap area. It shouldn't matter whether the swap area was activated by
systemd or by direct swapon invocation. The kernel name (from
/proc/swaps) is preferred, but if swap is configured through a unit
file and not active, that name will be used instead.

The case where a swap unit refers (What=) to a symlink should behave
better than before.

Note: this patch is goes on top of some cleanup patches that are
pretty boring and thus I'm not posting them, so it might not apply
cleanly.

---
Hi,
another RFC. Comments appreciated.

I think that this change will simplify swap unit handling. I've been
running with it for some time, and things seem to work. Systemclt
output is definitely simpler: just one line.


 src/core/manager.h |   1 -
 src/core/swap.c    | 231 +++++++++++++++++++++--------------------------------
 src/core/swap.h    |   6 --
 3 files changed, 92 insertions(+), 146 deletions(-)

diff --git a/src/core/manager.h b/src/core/manager.h
index 2214502..3b2143c 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -162,7 +162,6 @@ struct Manager {
 
         /* Data specific to the swap filesystem */
         FILE *proc_swaps;
-        Hashmap *swaps_by_proc_swaps;
         bool request_reload;
         Watch swap_watch;
 
diff --git a/src/core/swap.c b/src/core/swap.c
index 00c7868..130d1ee 100644
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -53,33 +53,6 @@ static const UnitActiveState state_translation_table[_SWAP_STATE_MAX] = {
         [SWAP_FAILED] = UNIT_FAILED
 };
 
-static void swap_unset_proc_swaps(Swap *s) {
-        Swap *first;
-        Hashmap *swaps;
-
-        assert(s);
-
-        if (!s->parameters_proc_swaps.what)
-                return;
-
-        /* Remove this unit from the chain of swaps which share the
-         * same kernel swap device. */
-        swaps = UNIT(s)->manager->swaps_by_proc_swaps;
-        first = hashmap_get(swaps, s->parameters_proc_swaps.what);
-        LIST_REMOVE(Swap, same_proc_swaps, first, s);
-
-        if (first)
-                hashmap_remove_and_replace(swaps,
-                                           s->parameters_proc_swaps.what,
-                                           first->parameters_proc_swaps.what,
-                                           first);
-        else
-                hashmap_remove(swaps, s->parameters_proc_swaps.what);
-
-        free(s->parameters_proc_swaps.what);
-        s->parameters_proc_swaps.what = NULL;
-}
-
 static void swap_init(Unit *u) {
         Swap *s = SWAP(u);
 
@@ -117,8 +90,6 @@ static void swap_done(Unit *u) {
 
         assert(s);
 
-        swap_unset_proc_swaps(s);
-
         free(s->what);
         s->what = NULL;
 
@@ -195,6 +166,7 @@ static int swap_add_device_links(Swap *s) {
                 return 0;
 
         if (is_device_path(s->what))
+                // XXX: should p->noauto matter here?
                 return unit_add_node_link(UNIT(s), s->what,
                                           !p->noauto && p->nofail &&
                                           UNIT(s)->manager->running_as == SYSTEMD_SYSTEM);
@@ -316,9 +288,16 @@ static int swap_load(Unit *u) {
         return swap_verify(s);
 }
 
+/**
+ * Register one swap device.
+ * @param names are unit name, udev device names, and symlinks,
+ * @param what_proc_swaps is the name appearing in /proc/swaps,
+ * @param priority is the value appearing in /proc/swaps,
+ * @param noauto, nofail are from .swap file or /etc/fstab.
+ */
 static int swap_add_one(
                 Manager *m,
-                const char *what,
+                Set *names,
                 const char *what_proc_swaps,
                 int priority,
                 bool noauto,
@@ -327,22 +306,36 @@ static int swap_add_one(
 
         Unit *u = NULL;
         char _cleanup_free_ *e = NULL;
-        char *wp = NULL;
+        char *pe;
         bool delete = false;
         int r;
         SwapParameters *p;
-        Swap *first;
+        Iterator i;
 
         assert(m);
-        assert(what);
+        assert(names);
         assert(what_proc_swaps);
 
-        e = unit_name_from_path(what, ".swap");
+        SET_FOREACH(pe, names, i)
+                log_debug("swap name %s", pe);
+
+        e = unit_name_from_path(what_proc_swaps, ".swap");
         if (!e)
                 return log_oom();
 
+        /* Look for /proc/swaps name first. */
         u = manager_get_unit(m, e);
 
+        if (!u)
+                SET_FOREACH(pe, names, i) {
+                        char _cleanup_free_ *e2 = unit_name_from_path(pe, ".swap");
+                        if (!e2)
+                                return log_oom();
+                        u = manager_get_unit(m, e2);
+                        if (u)
+                                break;
+                }
+
         if (u &&
             SWAP(u)->from_proc_swaps &&
             !path_equal(SWAP(u)->parameters_proc_swaps.what, what_proc_swaps))
@@ -351,6 +344,8 @@ static int swap_add_one(
         if (!u) {
                 delete = true;
 
+                log_debug("Creating new swap unit %s for %s", e, what_proc_swaps);
+
                 u = unit_new(m, sizeof(Swap));
                 if (!u)
                         return log_oom();
@@ -359,7 +354,7 @@ static int swap_add_one(
                 if (r < 0)
                         goto fail;
 
-                SWAP(u)->what = strdup(what);
+                SWAP(u)->what = strdup(what_proc_swaps);
                 if (!SWAP(u)->what) {
                         r = log_oom();
                         goto fail;
@@ -369,37 +364,31 @@ static int swap_add_one(
         } else
                 delete = false;
 
+        log_debug("Adding swap %s (kernel name %s) prio=%d",
+                  u->id, what_proc_swaps, priority);
+
         p = &SWAP(u)->parameters_proc_swaps;
 
         if (!p->what) {
-                wp = strdup(what_proc_swaps);
-                if (!wp) {
+                p->what = strdup(what_proc_swaps);
+                if (!p->what) {
                         r = log_oom();
                         goto fail;
                 }
+        }
 
-                if (!m->swaps_by_proc_swaps) {
-                        m->swaps_by_proc_swaps = hashmap_new(string_hash_func, string_compare_func);
-                        if (!m->swaps_by_proc_swaps) {
-                                r = log_oom();
-                                goto fail;
-                        }
-                }
-
-                free(p->what);
-                p->what = wp;
-
-                first = hashmap_get(m->swaps_by_proc_swaps, wp);
-                LIST_PREPEND(Swap, same_proc_swaps, first, SWAP(u));
-
-                r = hashmap_replace(m->swaps_by_proc_swaps, wp, first);
+        SET_FOREACH(pe, names, i) {
+                char _cleanup_free_ *e2 = unit_name_from_path(pe, ".swap");
+                if (!e2)
+                        return log_oom();
+                r = unit_add_name(u, e2);
                 if (r < 0)
-                        goto fail;
+                        log_notice("Failed to add name %s to %s", e2, u->id);
         }
 
         if (set_flags) {
                 SWAP(u)->is_active = true;
-                SWAP(u)->just_activated = !SWAP(u)->from_proc_swaps;
+                SWAP(u)->just_activated |= !SWAP(u)->from_proc_swaps;
         }
 
         SWAP(u)->from_proc_swaps = true;
@@ -408,6 +397,9 @@ static int swap_add_one(
         p->noauto = noauto;
         p->nofail = nofail;
 
+        log_debug("Adding swap unit %s to dbus queue (is_active=%d, just_activated=%d, from_proc_swaps=%d)",
+                  u->id, SWAP(u)->is_active, SWAP(u)->just_activated, SWAP(u)->from_proc_swaps);
+
         unit_add_to_dbus_queue(u);
 
         return 0;
@@ -415,39 +407,60 @@ static int swap_add_one(
 fail:
         log_warning("Failed to load swap unit: %s", strerror(-r));
 
-        free(wp);
-
         if (delete && u)
                 unit_free(u);
 
         return r;
 }
 
+/**
+ * Process one line from /proc/swaps. Registers a swap device under
+ * - udev device name,
+ * - all symlinks,
+ * - given device name.
+ */
 static int swap_process_new_swap(Manager *m, const char *device, int prio, bool set_flags) {
         struct stat st;
         int r = 0, k;
+        Set* names;
 
         assert(m);
 
+        names = set_new(string_hash_func, string_compare_func);
+
         if (stat(device, &st) >= 0 && S_ISBLK(st.st_mode)) {
-                struct udev_device *d;
+                struct udev_device *d = NULL;
                 const char *dn;
                 struct udev_list_entry *item = NULL, *first = NULL;
 
-                /* So this is a proper swap device. Create swap units
+                /* So this is a proper swap device. Create unit names
                  * for all names this swap device is known under */
 
-                if (!(d = udev_device_new_from_devnum(m->udev, 'b', st.st_rdev)))
-                        return -ENOMEM;
+                d = udev_device_new_from_devnum(m->udev, 'b', st.st_rdev);
+                if (!d)
+                        return log_oom();
 
                 dn = udev_device_get_devnode(d);
-                if (dn)
-                        r = swap_add_one(m, dn, device, prio, false, false, set_flags);
+                if (dn) {
+                        char* name = strdup(dn);
+                        if (!name) {
+                                udev_device_unref(d);
+                                return log_oom();
+                        }
+
+                        r = set_put(names, name);
+                        if (r < 0) {
+                                udev_device_unref(d);
+                                free(name);
+                                return r;
+                        }
+                }
 
-                /* Add additional units for all symlinks */
+                /* Add additional names for all symlinks */
                 first = udev_device_get_devlinks_list_entry(d);
                 udev_list_entry_foreach(item, first) {
                         const char *p;
+                        char *name;
 
                         /* Don't bother with the /dev/block links */
                         p = udev_list_entry_get_name(item);
@@ -459,7 +472,13 @@ static int swap_process_new_swap(Manager *m, const char *device, int prio, bool
                                 if ((!S_ISBLK(st.st_mode)) || st.st_rdev != udev_device_get_devnum(d))
                                         continue;
 
-                        k = swap_add_one(m, p, device, prio, false, false, set_flags);
+                        name = strdup(p);
+                        if (!name) {
+                                log_oom();
+                                continue;
+                        }
+
+                        k = set_put(names, name);
                         if (k < 0)
                                 r = k;
                 }
@@ -467,7 +486,7 @@ static int swap_process_new_swap(Manager *m, const char *device, int prio, bool
                 udev_device_unref(d);
         }
 
-        k = swap_add_one(m, device, device, prio, false, false, set_flags);
+        k = swap_add_one(m, names, device, prio, false, false, set_flags);
         if (k < 0)
                 r = k;
 
@@ -1068,7 +1087,7 @@ static int swap_load_proc_swaps(Manager *m, bool set_flags) {
         (void) fscanf(m->proc_swaps, "%*s %*s %*s %*s %*s\n");
 
         for (i = 1;; i++) {
-                char *dev = NULL, *d;
+                char _cleanup_free_ *dev = NULL, *d = NULL;
                 int prio = 0, k;
 
                 k = fscanf(m->proc_swaps,
@@ -1083,19 +1102,14 @@ static int swap_load_proc_swaps(Manager *m, bool set_flags) {
                                 break;
 
                         log_warning("Failed to parse /proc/swaps: %u", i);
-                        free(dev);
                         continue;
                 }
 
                 d = cunescape(dev);
-                free(dev);
-
                 if (!d)
-                        return -ENOMEM;
+                        return log_oom();
 
                 k = swap_process_new_swap(m, d, prio, set_flags);
-                free(d);
-
                 if (k < 0)
                         r = k;
         }
@@ -1121,6 +1135,8 @@ int swap_fd_event(Manager *m, int events) {
         assert(m);
         assert(events & EPOLLPRI);
 
+        log_debug("/proc/swaps changed");
+
         r = swap_load_proc_swaps(m, true);
         if (r < 0) {
                 log_error("Failed to reread /proc/swaps: %s", strerror(-r));
@@ -1140,11 +1156,13 @@ int swap_fd_event(Manager *m, int events) {
         LIST_FOREACH(units_by_type, u, m->units_by_type[UNIT_SWAP]) {
                 Swap *swap = SWAP(u);
 
+                log_debug("Processing swap unit %s (is_active=%d, just_activated=%d from_proc_swaps=%d)",
+                  u->id, swap->is_active, swap->just_activated, swap->from_proc_swaps);
+
                 if (!swap->is_active) {
                         /* This has just been deactivated */
 
                         swap->from_proc_swaps = false;
-                        swap_unset_proc_swaps(swap);
 
                         switch (swap->state) {
 
@@ -1185,65 +1203,6 @@ int swap_fd_event(Manager *m, int events) {
         return 1;
 }
 
-static Unit *swap_following(Unit *u) {
-        Swap *s = SWAP(u);
-        Swap *other, *first = NULL;
-
-        assert(s);
-
-        if (streq_ptr(s->what, s->parameters_proc_swaps.what))
-                return NULL;
-
-        /* Make everybody follow the unit that's named after the swap
-         * device in the kernel */
-
-        LIST_FOREACH_AFTER(same_proc_swaps, other, s)
-                if (streq_ptr(other->what, other->parameters_proc_swaps.what))
-                        return UNIT(other);
-
-        LIST_FOREACH_BEFORE(same_proc_swaps, other, s) {
-                if (streq_ptr(other->what, other->parameters_proc_swaps.what))
-                        return UNIT(other);
-
-                first = other;
-        }
-
-        return UNIT(first);
-}
-
-static int swap_following_set(Unit *u, Set **_set) {
-        Swap *s = SWAP(u);
-        Swap *other;
-        Set *set;
-        int r;
-
-        assert(s);
-        assert(_set);
-
-        if (LIST_JUST_US(same_proc_swaps, s)) {
-                *_set = NULL;
-                return 0;
-        }
-
-        if (!(set = set_new(NULL, NULL)))
-                return -ENOMEM;
-
-        LIST_FOREACH_AFTER(same_proc_swaps, other, s)
-                if ((r = set_put(set, other)) < 0)
-                        goto fail;
-
-        LIST_FOREACH_BEFORE(same_proc_swaps, other, s)
-                if ((r = set_put(set, other)) < 0)
-                        goto fail;
-
-        *_set = set;
-        return 1;
-
-fail:
-        set_free(set);
-        return r;
-}
-
 static void swap_shutdown(Manager *m) {
         assert(m);
 
@@ -1251,9 +1210,6 @@ static void swap_shutdown(Manager *m) {
                 fclose(m->proc_swaps);
                 m->proc_swaps = NULL;
         }
-
-        hashmap_free(m->swaps_by_proc_swaps);
-        m->swaps_by_proc_swaps = NULL;
 }
 
 static int swap_enumerate(Manager *m) {
@@ -1386,7 +1342,7 @@ const UnitVTable swap_vtable = {
                 "Swap\0"
                 "Install\0",
 
-        .no_alias = true,
+        .no_alias = false,
         .no_instances = true,
 
         .init = swap_init,
@@ -1419,9 +1375,6 @@ const UnitVTable swap_vtable = {
         .bus_message_handler = bus_swap_message_handler,
         .bus_invalidating_properties =  bus_swap_invalidating_properties,
 
-        .following = swap_following,
-        .following_set = swap_following_set,
-
         .enumerate = swap_enumerate,
         .shutdown = swap_shutdown,
 
diff --git a/src/core/swap.h b/src/core/swap.h
index 35d47fd..23eca79 100644
--- a/src/core/swap.h
+++ b/src/core/swap.h
@@ -96,12 +96,6 @@ struct Swap {
         pid_t control_pid;
 
         Watch timer_watch;
-
-        /* In order to be able to distinguish dependencies on
-        different device nodes we might end up creating multiple
-        devices for the same swap. We chain them up here. */
-
-        LIST_FIELDS(struct Swap, same_proc_swaps);
 };
 
 extern const UnitVTable swap_vtable;
-- 
1.7.12.rc1.173.g9b27acc



More information about the systemd-devel mailing list