[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