[systemd-devel] [PATCH, REVIEW, v2] find_symlinks: adds a cache of enabled unit symbolic link state

2014-10-17 Thread Ken Sedgwick 
The current find_symlinks_fd code traverses the config directories
duplicatively. This is a performance problem if 1000s of units are
being controlled. This patch adds a hashmap cache of symbolic link
state which is filled in one traversal and then consulted as needed to
prevent re-traversal.

The enabled_context cache lives in the manager struct.  Initially the
cache is empty and is filled on first use.  A pointer to the cache is
passed to all routines which can call find_symlinks_fd.  If a NULL is
passed to find_symlinks_fd a temporary cache is created and used for
the single call.

The cache has two levels, the first is keyed by config_path and the
second by the names and paths of the units.

The cache contains both forward and reverse mappings; from symbolic
link name to target and from target to symbolic link name.

The cache contains entries for both the basename of the unit and the
full path of the link name and target.

The test-enabled patch (previously submitted) checks that the results
of unit_file_get_state and unit_file_get_list do not change as a
result of this cache. This patch presumes the test-enabled patch has
already been applied.

The test-manyunits patch (previously submitted) checks that the performance
of unit_file_get_list is acceptable in the face of thousands of units.
This patch presumes the test-manyunits patch has already been applied.
---
 src/core/dbus-manager.c   |   6 +-
 src/core/manager.c|   6 +
 src/core/manager.h|   2 +
 src/core/unit.c   |   2 +-
 src/shared/install.c  | 273 +-
 src/shared/install.h  |  18 ++-
 src/systemctl/systemctl.c |   6 +-
 src/test/test-enabled.c   |  24 ++--
 src/test/test-install.c   |  87 ---
 src/test/test-manyunits.c |  11 +-
 src/test/test-unit-file.c |   2 +-
 11 files changed, 325 insertions(+), 112 deletions(-)

diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index 57db1c9..0ce41b0 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -1403,7 +1403,7 @@ static int method_list_unit_files(sd_bus *bus, 
sd_bus_message *message, void *us
 if (!h)
 return -ENOMEM;
 
-r = unit_file_get_list(m-running_as == SYSTEMD_SYSTEM ? 
UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h);
+r = unit_file_get_list(m-running_as == SYSTEMD_SYSTEM ? 
UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h, m-enabled);
 if (r  0)
 goto fail;
 
@@ -1454,7 +1454,7 @@ static int method_get_unit_file_state(sd_bus *bus, 
sd_bus_message *message, void
 
 scope = m-running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : 
UNIT_FILE_USER;
 
-state = unit_file_get_state(scope, NULL, name);
+state = unit_file_get_state(scope, NULL, name, m-enabled);
 if (state  0)
 return state;
 
@@ -1843,7 +1843,7 @@ static int method_add_dependency_unit_files(sd_bus *bus, 
sd_bus_message *message
 
 scope = m-running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : 
UNIT_FILE_USER;
 
-r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, 
force, changes, n_changes);
+r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, 
force, NULL, changes, n_changes);
 if (r  0)
 return r;
 
diff --git a/src/core/manager.c b/src/core/manager.c
index 726977f..f8f41fb 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -465,6 +465,10 @@ int manager_new(SystemdRunningAs running_as, bool 
test_run, Manager **_m) {
 if (r  0)
 goto fail;
 
+m-enabled = enabled_context_new();
+if (!m-enabled)
+goto fail;
+
 r = set_ensure_allocated(m-startup_units, NULL);
 if (r  0)
 goto fail;
@@ -822,6 +826,8 @@ void manager_free(Manager *m) {
 hashmap_free(m-watch_pids2);
 hashmap_free(m-watch_bus);
 
+enabled_context_free(m-enabled);
+
 set_free(m-startup_units);
 set_free(m-failed_units);
 
diff --git a/src/core/manager.h b/src/core/manager.h
index 8e3c146..3f54fe0 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -72,6 +72,7 @@ typedef enum ManagerExitCode {
 #include unit-name.h
 #include exit-status.h
 #include show-status.h
+#include install.h
 #include failure-action.h
 
 struct Manager {
@@ -82,6 +83,7 @@ struct Manager {
 /* Active jobs and units */
 Hashmap *units;  /* name string = Unit object n:1 */
 Hashmap *jobs;   /* job id = Job object 1:1 */
+EnabledContext *enabled; /* name string = is enabled */
 
 /* To make it easy to iterate through the units of a specific
  * type we maintain a per type linked list */
diff --git a/src/core/unit.c b/src/core/unit.c
index 0389e6e..3e29944 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2941,7 +2941,7 @@ UnitFileState unit_get_unit_file_state(Unit *u) {
 if (u-unit_file_state  0

Re: [systemd-devel] [PATCH, REVIEW, v2] find_symlinks: adds a cache of enabled unit symbolic link state

2014-10-17 Thread Ronny Chevalier 
Hi,

2014-10-18 1:34 GMT+02:00 Ken Sedgwick ksedg...@bonsai.com:
 The current find_symlinks_fd code traverses the config directories
 duplicatively. This is a performance problem if 1000s of units are
 being controlled. This patch adds a hashmap cache of symbolic link
 state which is filled in one traversal and then consulted as needed to
 prevent re-traversal.

 The enabled_context cache lives in the manager struct.  Initially the
 cache is empty and is filled on first use.  A pointer to the cache is
 passed to all routines which can call find_symlinks_fd.  If a NULL is
 passed to find_symlinks_fd a temporary cache is created and used for
 the single call.

 The cache has two levels, the first is keyed by config_path and the
 second by the names and paths of the units.

 The cache contains both forward and reverse mappings; from symbolic
 link name to target and from target to symbolic link name.

 The cache contains entries for both the basename of the unit and the
 full path of the link name and target.

 The test-enabled patch (previously submitted) checks that the results
 of unit_file_get_state and unit_file_get_list do not change as a
 result of this cache. This patch presumes the test-enabled patch has
 already been applied.

 The test-manyunits patch (previously submitted) checks that the performance
 of unit_file_get_list is acceptable in the face of thousands of units.
 This patch presumes the test-manyunits patch has already been applied.
If there is dependencies between your patches, you should use
something like git format-patch -3 to generate 3 patchs from the 3
last commits. It will add [PATCH 1/3] [PATCH 2/3]... on the subjects
of the mails. It helps knowing if a patch needs to be applied before
applying another.

 ---
  src/core/dbus-manager.c   |   6 +-
  src/core/manager.c|   6 +
  src/core/manager.h|   2 +
  src/core/unit.c   |   2 +-
  src/shared/install.c  | 273 
 +-
  src/shared/install.h  |  18 ++-
  src/systemctl/systemctl.c |   6 +-
  src/test/test-enabled.c   |  24 ++--
  src/test/test-install.c   |  87 ---
  src/test/test-manyunits.c |  11 +-
  src/test/test-unit-file.c |   2 +-
  11 files changed, 325 insertions(+), 112 deletions(-)

 diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
 index 57db1c9..0ce41b0 100644
 --- a/src/core/dbus-manager.c
 +++ b/src/core/dbus-manager.c
 @@ -1403,7 +1403,7 @@ static int method_list_unit_files(sd_bus *bus, 
 sd_bus_message *message, void *us
  if (!h)
  return -ENOMEM;

 -r = unit_file_get_list(m-running_as == SYSTEMD_SYSTEM ? 
 UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h);
 +r = unit_file_get_list(m-running_as == SYSTEMD_SYSTEM ? 
 UNIT_FILE_SYSTEM : UNIT_FILE_USER, NULL, h, m-enabled);
  if (r  0)
  goto fail;

 @@ -1454,7 +1454,7 @@ static int method_get_unit_file_state(sd_bus *bus, 
 sd_bus_message *message, void

  scope = m-running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : 
 UNIT_FILE_USER;

 -state = unit_file_get_state(scope, NULL, name);
 +state = unit_file_get_state(scope, NULL, name, m-enabled);
  if (state  0)
  return state;

 @@ -1843,7 +1843,7 @@ static int method_add_dependency_unit_files(sd_bus 
 *bus, sd_bus_message *message

  scope = m-running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : 
 UNIT_FILE_USER;

 -r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, 
 force, changes, n_changes);
 +r = unit_file_add_dependency(scope, runtime, NULL, l, target, dep, 
 force, NULL, changes, n_changes);
  if (r  0)
  return r;

 diff --git a/src/core/manager.c b/src/core/manager.c
 index 726977f..f8f41fb 100644
 --- a/src/core/manager.c
 +++ b/src/core/manager.c
 @@ -465,6 +465,10 @@ int manager_new(SystemdRunningAs running_as, bool 
 test_run, Manager **_m) {
  if (r  0)
  goto fail;

 +m-enabled = enabled_context_new();
 +if (!m-enabled)
 +goto fail;
 +
  r = set_ensure_allocated(m-startup_units, NULL);
  if (r  0)
  goto fail;
 @@ -822,6 +826,8 @@ void manager_free(Manager *m) {
  hashmap_free(m-watch_pids2);
  hashmap_free(m-watch_bus);

 +enabled_context_free(m-enabled);
 +
  set_free(m-startup_units);
  set_free(m-failed_units);

 diff --git a/src/core/manager.h b/src/core/manager.h
 index 8e3c146..3f54fe0 100644
 --- a/src/core/manager.h
 +++ b/src/core/manager.h
 @@ -72,6 +72,7 @@ typedef enum ManagerExitCode {
  #include unit-name.h
  #include exit-status.h
  #include show-status.h
 +#include install.h
  #include failure-action.h

  struct Manager {
 @@ -82,6 +83,7 @@ struct Manager {
  /* Active jobs and units */
  Hashmap *units;  /* name string = Unit object n:1 */
  Hashmap *jobs;   /* job id = Job