From f0af1f20acc04b744f4ce1ed4b028e84576541d7 Mon Sep 17 00:00:00 2001

From: Lennart Poettering <lennart@poettering.net>

Date: Thu, 26 Sep 2013 20:14:24 +0200

Subject: [PATCH] core: rework how we match mount units against each other

Previously to automatically create dependencies between mount units we

matched every mount unit agains all others resulting in O(n^2)

complexity. On setups with large amounts of mount units this might make

things slow.

This change replaces the matching code to use a hashtable that is keyed

by a path prefix, and points to a set of units that require that path to

be around. When a new mount unit is installed it is hence sufficient to

simply look up this set of units via its own file system paths to know

which units to order after itself.

This patch also changes all unit types to only create automatic mount

dependencies via the RequiresMountsFor= logic, and this is exposed to

the outside to make things more transparent.

With this change we still have some O(n) complexities in place when

handling mounts, but that's currently unavoidable due to kernel APIs,

and still substantially better than O(n^2) as before.

https://bugs.freedesktop.org/show_bug.cgi?id=69740

---

 src/core/automount.c                  |  33 +----

 src/core/automount.h                  |   2 -

 src/core/load-fragment-gperf.gperf.m4 |   2 +-

 src/core/load-fragment.c              |  47 ++++---

 src/core/manager.c                    |  44 +++++-

 src/core/manager.h                    |  12 +-

 src/core/mount.c                      | 255 +++++++++-------------------------

 src/core/path.c                       |  32 +----

 src/core/path.h                       |   4 -

 src/core/socket.c                     |  49 ++-----

 src/core/socket.h                     |   4 -

 src/core/swap.c                       |  40 +-----

 src/core/swap.h                       |   2 -

 src/core/unit.c                       | 162 +++++++++++++++++----

 src/core/unit.h                       |   6 +-

 src/shared/socket-util.c              |   8 +-

 src/shared/socket-util.h              |   2 +-

 17 files changed, 309 insertions(+), 395 deletions(-)

diff --git a/src/core/automount.c b/src/core/automount.c

index 6762392..d1379e0 100644

--- a/src/core/automount.c

+++ b/src/core/automount.c

@@ -117,42 +117,17 @@ static void automount_done(Unit *u) {

         a->tokens = NULL;

 }

-int automount_add_one_mount_link(Automount *a, Mount *m) {

+static int automount_add_mount_links(Automount *a) {

+        _cleanup_free_ char *parent = NULL;

         int r;

         assert(a);

-        assert(m);

-

-        if (UNIT(a)->load_state != UNIT_LOADED ||

-            UNIT(m)->load_state != UNIT_LOADED)

-                return 0;

-

-        if (!path_startswith(a->where, m->where))

-                return 0;

-        if (path_equal(a->where, m->where))

-                return 0;

-

-        r = unit_add_two_dependencies(UNIT(a), UNIT_AFTER, UNIT_REQUIRES, UNIT(m), true);

+        r = path_get_parent(a->where, &parent);

         if (r < 0)

                 return r;

-        return 0;

-}

-

-static int automount_add_mount_links(Automount *a) {

-        Unit *other;

-        int r;

-

-        assert(a);

-

-        LIST_FOREACH(units_by_type, other, UNIT(a)->manager->units_by_type[UNIT_MOUNT]) {

-                r = automount_add_one_mount_link(a, MOUNT(other));

-                if (r < 0)

-                        return r;

-        }

-

-        return 0;

+        return unit_require_mounts_for(UNIT(a), parent);

 }

 static int automount_add_default_dependencies(Automount *a) {

diff --git a/src/core/automount.h b/src/core/automount.h

index 0c6b8a7..a7a25d3 100644

--- a/src/core/automount.h

+++ b/src/core/automount.h

@@ -62,8 +62,6 @@ extern const UnitVTable automount_vtable;

 int automount_send_ready(Automount *a, int status);

-int automount_add_one_mount_link(Automount *a, Mount *m);

-

 const char* automount_state_to_string(AutomountState i) _const_;

 AutomountState automount_state_from_string(const char *s) _pure_;

diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4

index 33c6880..3f064a9 100644

--- a/src/core/load-fragment-gperf.gperf.m4

+++ b/src/core/load-fragment-gperf.gperf.m4

@@ -117,7 +117,7 @@ Unit.PropagateReloadTo,          config_parse_unit_deps,             UNIT_PROPAG

 Unit.ReloadPropagatedFrom,       config_parse_unit_deps,             UNIT_RELOAD_PROPAGATED_FROM,   0

 Unit.PropagateReloadFrom,        config_parse_unit_deps,             UNIT_RELOAD_PROPAGATED_FROM,   0

 Unit.PartOf,                     config_parse_unit_deps,             UNIT_PART_OF,                  0

-Unit.RequiresMountsFor,          config_parse_unit_requires_mounts_for, 0,                          offsetof(Unit, requires_mounts_for)

+Unit.RequiresMountsFor,          config_parse_unit_requires_mounts_for, 0,                          0

 Unit.StopWhenUnneeded,           config_parse_bool,                  0,                             offsetof(Unit, stop_when_unneeded)

 Unit.RefuseManualStart,          config_parse_bool,                  0,                             offsetof(Unit, refuse_manual_start)

 Unit.RefuseManualStop,           config_parse_bool,                  0,                             offsetof(Unit, refuse_manual_stop)

diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c

index f4a268c..ed8602f 100644

--- a/src/core/load-fragment.c

+++ b/src/core/load-fragment.c

@@ -1742,33 +1742,48 @@ int config_parse_unit_condition_null(const char *unit,

 DEFINE_CONFIG_PARSE_ENUM(config_parse_notify_access, notify_access, NotifyAccess, "Failed to parse notify access specifier");

 DEFINE_CONFIG_PARSE_ENUM(config_parse_start_limit_action, start_limit_action, StartLimitAction, "Failed to parse start limit action specifier");

-int config_parse_unit_requires_mounts_for(const char *unit,

-                                          const char *filename,

-                                          unsigned line,

-                                          const char *section,

-                                          const char *lvalue,

-                                          int ltype,

-                                          const char *rvalue,

-                                          void *data,

-                                          void *userdata) {

+int config_parse_unit_requires_mounts_for(

+                const char *unit,

+                const char *filename,

+                unsigned line,

+                const char *section,

+                const char *lvalue,

+                int ltype,

+                const char *rvalue,

+                void *data,

+                void *userdata) {

         Unit *u = userdata;

+        char *state;

+        size_t l;

+        char *w;

         int r;

-        bool empty_before;

         assert(filename);

         assert(lvalue);

         assert(rvalue);

         assert(data);

-        empty_before = !u->requires_mounts_for;

+        FOREACH_WORD_QUOTED(w, l, rvalue, state) {

+                _cleanup_free_ char *n;

+

+                n = strndup(w, l);

+                if (!n)

+                        return log_oom();

-        r = config_parse_path_strv(unit, filename, line, section, lvalue, ltype,

-                                   rvalue, data, userdata);

+                if (!utf8_is_valid(n)) {

+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,

+                                   "Path is not UTF-8 clean, ignoring assignment: %s", rvalue);

+                        continue;

+                }

-        /* Make it easy to find units with requires_mounts set */

-        if (empty_before && u->requires_mounts_for)

-                LIST_PREPEND(Unit, has_requires_mounts_for, u->manager->has_requires_mounts_for, u);

+                r = unit_require_mounts_for(u, n);

+                if (r < 0) {

+                        log_syntax(unit, LOG_ERR, filename, line, r,

+                                   "Failed to add required mount for, ignoring: %s", rvalue);

+                        continue;

+                }

+        }

         return r;

 }

diff --git a/src/core/manager.c b/src/core/manager.c

index 669af15..dadbedd 100644

--- a/src/core/manager.c

+++ b/src/core/manager.c

@@ -771,6 +771,9 @@ void manager_free(Manager *m) {

         for (i = 0; i < RLIMIT_NLIMITS; i++)

                 free(m->rlimit[i]);

+        assert(hashmap_isempty(m->units_requiring_mounts_for));

+        hashmap_free(m->units_requiring_mounts_for);

+

         free(m);

 }

@@ -783,9 +786,11 @@ int manager_enumerate(Manager *m) {

         /* Let's ask every type to load all units from disk/kernel

          * that it might know */

         for (c = 0; c < _UNIT_TYPE_MAX; c++)

-                if (unit_vtable[c]->enumerate)

-                        if ((q = unit_vtable[c]->enumerate(m)) < 0)

+                if (unit_vtable[c]->enumerate) {

+                        q = unit_vtable[c]->enumerate(m);

+                        if (q < 0)

                                 r = q;

+                }

         manager_dispatch_load_queue(m);

         return r;

@@ -2765,6 +2770,41 @@ void manager_status_printf(Manager *m, bool ephemeral, const char *status, const

         va_end(ap);

 }

+int manager_get_unit_by_path(Manager *m, const char *path, const char *suffix, Unit **_found) {

+        _cleanup_free_ char *p = NULL;

+        Unit *found;

+

+        assert(m);

+        assert(path);

+        assert(suffix);

+        assert(_found);

+

+        p = unit_name_from_path(path, suffix);

+        if (!p)

+                return -ENOMEM;

+

+        found = manager_get_unit(m, p);

+        if (!found) {

+                *_found = NULL;

+                return 0;

+        }

+

+        *_found = found;

+        return 1;

+}

+

+Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path) {

+        char p[strlen(path)+1];

+

+        assert(m);

+        assert(path);

+

+        strcpy(p, path);

+        path_kill_slashes(p);

+

+        return hashmap_get(m->units_requiring_mounts_for, streq(p, "/") ? "" : p);

+}

+

 void watch_init(Watch *w) {

         assert(w);

diff --git a/src/core/manager.h b/src/core/manager.h

index 3969553..a3049b5 100644

--- a/src/core/manager.h

+++ b/src/core/manager.h

@@ -103,9 +103,6 @@ struct Manager {

          * type we maintain a per type linked list */

         LIST_HEAD(Unit, units_by_type[_UNIT_TYPE_MAX]);

-        /* To optimize iteration of units that have requires_mounts_for set */

-        LIST_HEAD(Unit, has_requires_mounts_for);

-

         /* Units that need to be loaded */

         LIST_HEAD(Unit, load_queue); /* this is actually more a stack than a queue, but uh. */

@@ -251,6 +248,11 @@ struct Manager {

         char *switch_root;

         char *switch_root_init;

+

+        /* This maps all possible path prefixes to the units needing

+         * them. It's a hashmap with a path string as key and a Set as

+         * value where Unit objects are contained. */

+        Hashmap *units_requiring_mounts_for;

 };

 int manager_new(SystemdRunningAs running_as, bool reexecuting, Manager **m);

@@ -263,6 +265,8 @@ int manager_startup(Manager *m, FILE *serialization, FDSet *fds);

 Job *manager_get_job(Manager *m, uint32_t id);

 Unit *manager_get_unit(Manager *m, const char *name);

+int manager_get_unit_by_path(Manager *m, const char *path, const char *suffix, Unit **_found);

+

 int manager_get_job_from_dbus_path(Manager *m, const char *s, Job **_j);

 int manager_load_unit_prepare(Manager *m, const char *name, const char *path, DBusError *e, Unit **_ret);

@@ -316,4 +320,6 @@ void manager_recheck_journal(Manager *m);

 void manager_set_show_status(Manager *m, bool b);

 void manager_status_printf(Manager *m, bool ephemeral, const char *status, const char *format, ...) _printf_attr_(4,5);

+Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path);

+

 void watch_init(Watch *w);

diff --git a/src/core/mount.c b/src/core/mount.c

index 78c5c1e..3d46557 100644

--- a/src/core/mount.c

+++ b/src/core/mount.c

@@ -157,138 +157,58 @@ _pure_ static MountParameters* get_mount_parameters(Mount *m) {

 }

 static int mount_add_mount_links(Mount *m) {

-        Unit *other;

-        int r;

+        _cleanup_free_ char *parent = NULL;

         MountParameters *pm;

-

-        assert(m);

-

-        pm = get_mount_parameters_fragment(m);

-

-        /* Adds in links to other mount points that might lie below or

-         * above us in the hierarchy */

-

-        LIST_FOREACH(units_by_type, other, UNIT(m)->manager->units_by_type[UNIT_MOUNT]) {

-                Mount *n = MOUNT(other);

-                MountParameters *pn;

-

-                if (n == m)

-                        continue;

-

-                if (UNIT(n)->load_state != UNIT_LOADED)

-                        continue;

-

-                pn = get_mount_parameters_fragment(n);

-

-                if (path_startswith(m->where, n->where)) {

-

-                        if ((r = unit_add_dependency(UNIT(m), UNIT_AFTER, UNIT(n), true)) < 0)

-                                return r;

-

-                        if (pn)

-                                if ((r = unit_add_dependency(UNIT(m), UNIT_REQUIRES, UNIT(n), true)) < 0)

-                                        return r;

-

-                } else if (path_startswith(n->where, m->where)) {

-

-                        if ((r = unit_add_dependency(UNIT(n), UNIT_AFTER, UNIT(m), true)) < 0)

-                                return r;

-

-                        if (pm)

-                                if ((r = unit_add_dependency(UNIT(n), UNIT_REQUIRES, UNIT(m), true)) < 0)

-                                        return r;

-

-                } else if (pm && pm->what && path_startswith(pm->what, n->where)) {

-

-                        if ((r = unit_add_dependency(UNIT(m), UNIT_AFTER, UNIT(n), true)) < 0)

-                                return r;

-

-                        if ((r = unit_add_dependency(UNIT(m), UNIT_REQUIRES, UNIT(n), true)) < 0)

-                                return r;

-

-                } else if (pn && pn->what && path_startswith(pn->what, m->where)) {

-

-                        if ((r = unit_add_dependency(UNIT(n), UNIT_AFTER, UNIT(m), true)) < 0)

-                                return r;

-

-                        if ((r = unit_add_dependency(UNIT(n), UNIT_REQUIRES, UNIT(m), true)) < 0)

-                                return r;

-                }

-        }

-

-        return 0;

-}

-

-static int mount_add_swap_links(Mount *m) {

         Unit *other;

+        Iterator i;

+        Set *s;

         int r;

         assert(m);

-        LIST_FOREACH(units_by_type, other, UNIT(m)->manager->units_by_type[UNIT_SWAP]) {

-                r = swap_add_one_mount_link(SWAP(other), m);

+        if (!path_equal(m->where, "/")) {

+                /* Adds in links to other mount points that might lie further

+                 * up in the hierarchy */

+                r = path_get_parent(m->where, &parent);

                 if (r < 0)

                         return r;

-        }

-        return 0;

-}

-

-static int mount_add_path_links(Mount *m) {

-        Unit *other;

-        int r;

-

-        assert(m);

-

-        LIST_FOREACH(units_by_type, other, UNIT(m)->manager->units_by_type[UNIT_PATH]) {

-                r = path_add_one_mount_link(PATH(other), m);

+                r = unit_require_mounts_for(UNIT(m), parent);

                 if (r < 0)

                         return r;

         }

-        return 0;

-}

-

-static int mount_add_automount_links(Mount *m) {

-        Unit *other;

-        int r;

-

-        assert(m);

-

-        LIST_FOREACH(units_by_type, other, UNIT(m)->manager->units_by_type[UNIT_AUTOMOUNT]) {

-                r = automount_add_one_mount_link(AUTOMOUNT(other), m);

+        /* Adds in links to other mount points that might be needed

+         * for the source path (if this is a bind mount) to be

+         * available. */

+        pm = get_mount_parameters_fragment(m);

+        if (pm && path_is_absolute(pm->what)) {

+                r = unit_require_mounts_for(UNIT(m), pm->what);

                 if (r < 0)

                         return r;

         }

-        return 0;

-}

+        /* Adds in links to other units that use this path or paths

+         * further down in the hierarchy */

+        s = manager_get_units_requiring_mounts_for(UNIT(m)->manager, m->where);

+        SET_FOREACH(other, s, i) {

-static int mount_add_socket_links(Mount *m) {

-        Unit *other;

-        int r;

+                if (other->load_state != UNIT_LOADED)

+                        continue;

-        assert(m);

+                if (other == UNIT(m))

+                        continue;

-        LIST_FOREACH(units_by_type, other, UNIT(m)->manager->units_by_type[UNIT_SOCKET]) {

-                r = socket_add_one_mount_link(SOCKET(other), m);

+                r = unit_add_dependency(other, UNIT_AFTER, UNIT(m), true);

                 if (r < 0)

                         return r;

-        }

-        return 0;

-}

-

-static int mount_add_requires_mounts_links(Mount *m) {

-        Unit *other;

-        int r;

-

-        assert(m);

-

-        LIST_FOREACH(has_requires_mounts_for, other, UNIT(m)->manager->has_requires_mounts_for) {

-                r = unit_add_one_mount_link(other, m);

-                if (r < 0)

-                        return r;

+                if (UNIT(m)->fragment_path) {

+                        /* If we have fragment configuration, then make this dependency required */

+                        r = unit_add_dependency(other, UNIT_REQUIRES, UNIT(m), true);

+                        if (r < 0)

+                                return r;

+                }

         }

         return 0;

@@ -567,8 +487,9 @@ static int mount_fix_timeouts(Mount *m) {

 }

 static int mount_verify(Mount *m) {

+        _cleanup_free_ char *e = NULL;

         bool b;

-        char *e;

+

         assert(m);

         if (UNIT(m)->load_state != UNIT_LOADED)

@@ -577,12 +498,11 @@ static int mount_verify(Mount *m) {

         if (!m->from_fragment && !m->from_proc_self_mountinfo)

                 return -ENOENT;

-        if (!(e = unit_name_from_path(m->where, ".mount")))

+        e = unit_name_from_path(m->where, ".mount");

+        if (!e)

                 return -ENOMEM;

         b = unit_has_name(UNIT(m), e);

-        free(e);

-

         if (!b) {

                 log_error_unit(UNIT(m)->id,

                                "%s's Where setting doesn't match unit name. Refusing.",

@@ -646,26 +566,6 @@ static int mount_add_extras(Mount *m) {

         if (r < 0)

                 return r;

-        r = mount_add_socket_links(m);

-        if (r < 0)

-                return r;

-

-        r = mount_add_swap_links(m);

-        if (r < 0)

-                return r;

-

-        r = mount_add_path_links(m);

-        if (r < 0)

-                return r;

-

-        r = mount_add_requires_mounts_links(m);

-        if (r < 0)

-                return r;

-

-        r = mount_add_automount_links(m);

-        if (r < 0)

-                return r;

-

         r = mount_add_quota_links(m);

         if (r < 0)

                 return r;

@@ -1650,79 +1550,56 @@ fail:

 static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {

         int r = 0;

         unsigned i;

-        char *device, *path, *options, *options2, *fstype, *d, *p, *o;

         assert(m);

         rewind(m->proc_self_mountinfo);

         for (i = 1;; i++) {

+                _cleanup_free_ char *device = NULL, *path = NULL, *options = NULL, *options2 = NULL, *fstype = NULL, *d = NULL, *p = NULL, *o = NULL;

                 int k;

-                device = path = options = options2 = fstype = d = p = o = NULL;

-

-                if ((k = fscanf(m->proc_self_mountinfo,

-                                "%*s "       /* (1) mount id */

-                                "%*s "       /* (2) parent id */

-                                "%*s "       /* (3) major:minor */

-                                "%*s "       /* (4) root */

-                                "%ms "       /* (5) mount point */

-                                "%ms"        /* (6) mount options */

-                                "%*[^-]"     /* (7) optional fields */

-                                "- "         /* (8) separator */

-                                "%ms "       /* (9) file system type */

-                                "%ms"        /* (10) mount source */

-                                "%ms"        /* (11) mount options 2 */

-                                "%*[^\n]",   /* some rubbish at the end */

-                                &path,

-                                &options,

-                                &fstype,

-                                &device,

-                                &options2)) != 5) {

-

-                        if (k == EOF)

-                                break;

-

+                k = fscanf(m->proc_self_mountinfo,

+                           "%*s "       /* (1) mount id */

+                           "%*s "       /* (2) parent id */

+                           "%*s "       /* (3) major:minor */

+                           "%*s "       /* (4) root */

+                           "%ms "       /* (5) mount point */

+                           "%ms"        /* (6) mount options */

+                           "%*[^-]"     /* (7) optional fields */

+                           "- "         /* (8) separator */

+                           "%ms "       /* (9) file system type */

+                           "%ms"        /* (10) mount source */

+                           "%ms"        /* (11) mount options 2 */

+                           "%*[^\n]",   /* some rubbish at the end */

+                           &path,

+                           &options,

+                           &fstype,

+                           &device,

+                           &options2);

+

+                if (k == EOF)

+                        break;

+

+                if (k != 5) {

                         log_warning("Failed to parse /proc/self/mountinfo:%u.", i);

-                        goto clean_up;

+                        continue;

                 }

                 o = strjoin(options, ",", options2, NULL);

-                if (!o) {

-                        r = -ENOMEM;

-                        goto finish;

-                }

+                if (!o)

+                        return log_oom();

-                if (!(d = cunescape(device)) ||

-                    !(p = cunescape(path))) {

-                        r = -ENOMEM;

-                        goto finish;

-                }

+                d = cunescape(device);

+                p = cunescape(path);

+                if (!d || !p)

+                        return log_oom();

-                if ((k = mount_add_one(m, d, p, o, fstype, 0, set_flags)) < 0)

+                k = mount_add_one(m, d, p, o, fstype, 0, set_flags);

+                if (k < 0)

                         r = k;

-

-clean_up:

-                free(device);

-                free(path);

-                free(options);

-                free(options2);

-                free(fstype);

-                free(d);

-                free(p);

-                free(o);

         }

-finish:

-        free(device);

-        free(path);

-        free(options);

-        free(options2);

-        free(fstype);

-        free(d);

-        free(p);

-        free(o);

-

         return r;

 }

diff --git a/src/core/path.c b/src/core/path.c

index 8a09deb..99e2fed 100644

--- a/src/core/path.c

+++ b/src/core/path.c

@@ -241,10 +241,6 @@ static bool path_spec_check_good(PathSpec *s, bool initial) {

         return good;

 }

-static bool path_spec_startswith(PathSpec *s, const char *what) {

-        return path_startswith(s->path, what);

-}

-

 static void path_spec_mkdir(PathSpec *s, mode_t mode) {

         int r;

@@ -301,38 +297,14 @@ static void path_done(Unit *u) {

         path_free_specs(p);

 }

-int path_add_one_mount_link(Path *p, Mount *m) {

+static int path_add_mount_links(Path *p) {

         PathSpec *s;

         int r;

         assert(p);

-        assert(m);

-

-        if (UNIT(p)->load_state != UNIT_LOADED ||

-            UNIT(m)->load_state != UNIT_LOADED)

-                return 0;

         LIST_FOREACH(spec, s, p->specs) {

-                if (!path_spec_startswith(s, m->where))

-                        continue;

-

-                r = unit_add_two_dependencies(UNIT(p), UNIT_AFTER, UNIT_REQUIRES,

-                                              UNIT(m), true);

-                if (r < 0)

-                        return r;

-        }

-

-        return 0;

-}

-

-static int path_add_mount_links(Path *p) {

-        Unit *other;

-        int r;

-

-        assert(p);

-

-        LIST_FOREACH(units_by_type, other, UNIT(p)->manager->units_by_type[UNIT_MOUNT]) {

-                r = path_add_one_mount_link(p, MOUNT(other));

+                r = unit_require_mounts_for(UNIT(p), s->path);

                 if (r < 0)

                         return r;

         }

diff --git a/src/core/path.h b/src/core/path.h

index 6adab58..dec3df7 100644

--- a/src/core/path.h

+++ b/src/core/path.h

@@ -90,10 +90,6 @@ struct Path {

         PathResult result;

 };

-/* Called from the mount code figure out if a mount is a dependency of

- * any of the paths of this path object */

-int path_add_one_mount_link(Path *p, Mount *m);

-

 void path_free_specs(Path *p);

 extern const UnitVTable path_vtable;

diff --git a/src/core/socket.c b/src/core/socket.c

index 25c2a51..778f8c8 100644

--- a/src/core/socket.c

+++ b/src/core/socket.c

@@ -258,53 +258,24 @@ static int socket_verify(Socket *s) {

         return 0;