public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* Optimize debuginfod-client cache lookup/cleanup a little
@ 2022-05-09 22:57 Mark Wielaard
  2022-05-09 22:57 ` [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat Mark Wielaard
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-05-09 22:57 UTC (permalink / raw)
  To: elfutils-devel

Hi,

debuginfod-client would try to create the cache config files twice,
once through debuginfod_init_cache, which was always called before the
debuginfod_clean_cache check. Which called debuginfod_config_cache
which also tried to create the config files when they didn't exist
yet. debuginfod_config_cache however had a small bug that meant it
would not provide a valid struct stat if the config file didn't exist
yet. The first patch fixes that:

[PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat

Then the second patch removes debuginfod_init_cache which saves two
stat calls (but introduces a new mkdir call).

[PATCH 2/3] debuginfod: Remove debuginfod_init_cache

Finally as soon as debuginfod_clean_cache commits to clean the cache
dir we immediately update the mtime of the interval config file so
other threads will not try to simultaniously also try to clean up the
cache dir. Because that is just duplicate work.

[PATCH 3/3] debuginfod: update mtime of interval_path as early as possible

Cheers,

Mark

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat
  2022-05-09 22:57 Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
@ 2022-05-09 22:57 ` Mark Wielaard
  2022-05-09 22:57 ` [PATCH 2/3] debuginfod: Remove debuginfod_init_cache Mark Wielaard
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-05-09 22:57 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

If the condig file which value was requested from
debuginfod_config_cache didn't exist yet, stat would fail and no valid
struct stat would be returned even when the file was correctly
created. Fix this by always using O_CREAT to open the file, and reuse
that file descriptor to call fstat and for either writing the default
value or reading the config file value.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           |  6 ++++++
 debuginfod/debuginfod-client.c | 21 ++++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 257ac39f..46a6e22b 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2022-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_config_cache): Always open with
+	O_CREATE first, then use fstat, only write the cache_config_default_s
+	value if st_size == 0, otherwise read value from file.
+
 2022-05-09  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod.cxx (conninfo): Always provide servname to getnameinfo.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 89208216..3b2728f1 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -235,14 +235,19 @@ debuginfod_config_cache(char *config_path,
 			long cache_config_default_s,
 			struct stat *st)
 {
-  int fd;
-  /* if the config file doesn't exist, create one with DEFFILEMODE*/
-  if(stat(config_path, st) == -1)
+  int fd = open(config_path, O_CREAT | O_RDWR, DEFFILEMODE);
+  if (fd < 0)
+    return -errno;
+
+  if (fstat (fd, st) < 0)
     {
-      fd = open(config_path, O_CREAT | O_RDWR, DEFFILEMODE);
-      if (fd < 0)
-        return -errno;
+      int ret = -errno;
+      close (fd);
+      return ret;
+    }
 
+  if (st->st_size == 0)
+    {
       if (dprintf(fd, "%ld", cache_config_default_s) < 0)
 	{
 	  int ret = -errno;
@@ -251,10 +256,11 @@ debuginfod_config_cache(char *config_path,
 	}
 
       close (fd);
+      return cache_config_default_s;
     }
 
   long cache_config;
-  FILE *config_file = fopen(config_path, "r");
+  FILE *config_file = fdopen(fd, "r");
   if (config_file)
     {
       if (fscanf(config_file, "%ld", &cache_config) != 1)
@@ -264,6 +270,7 @@ debuginfod_config_cache(char *config_path,
   else
     cache_config = cache_config_default_s;
 
+  close (fd);
   return cache_config;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] debuginfod: Remove debuginfod_init_cache
  2022-05-09 22:57 Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
  2022-05-09 22:57 ` [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat Mark Wielaard
@ 2022-05-09 22:57 ` Mark Wielaard
  2022-05-09 22:57 ` [PATCH 3/3] debuginfod: update mtime of interval_path as early as possible Mark Wielaard
  2022-05-14 22:36 ` Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-05-09 22:57 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

debuginfod_init_cache would create all config files if they didn't
exist yet. It always made two stat calls. Then debuginfod_clean_cache
would call debuginfod_config_cache which did the same checks and
created any missing config files. Just make sure the cache_path
directory exists and remove debuginfod_init_cache before calling
debuginfod_clean_cache.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           |  6 ++++
 debuginfod/debuginfod-client.c | 61 +++++-----------------------------
 2 files changed, 15 insertions(+), 52 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 46a6e22b..c9aa4fcf 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@
+2022-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_init_cache): Remove.
+	(debuginfod_query_server): Don't call debuginfod_init_cache, call
+	mkdir then debuginfod_clean_cache.
+
 2022-05-09  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_config_cache): Always open with
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 3b2728f1..6bdf1908 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -274,55 +274,6 @@ debuginfod_config_cache(char *config_path,
   return cache_config;
 }
 
-/* Create the cache and interval file if they do not already exist.
-   Return 0 if cache and config file are initialized, otherwise return
-   the appropriate error code.  */
-static int
-debuginfod_init_cache (char *cache_path, char *interval_path, char *maxage_path)
-{
-  struct stat st;
-
-  /* If the cache and config file already exist then we are done.  */
-  if (stat(cache_path, &st) == 0 && stat(interval_path, &st) == 0)
-    return 0;
-
-  /* Create the cache and config files as necessary.  */
-  if (stat(cache_path, &st) != 0 && mkdir(cache_path, ACCESSPERMS) < 0)
-    return -errno;
-
-  int fd = -1;
-
-  /* init cleaning interval config file.  */
-  fd = open(interval_path, O_CREAT | O_RDWR, DEFFILEMODE);
-  if (fd < 0)
-    return -errno;
-
-  if (dprintf(fd, "%ld", cache_clean_default_interval_s) < 0)
-    {
-      int ret = -errno;
-      close (fd);
-      return ret;
-    }
-
-  close (fd);
-
-  /* init max age config file.  */
-  if (stat(maxage_path, &st) != 0
-      && (fd = open(maxage_path, O_CREAT | O_RDWR, DEFFILEMODE)) < 0)
-    return -errno;
-
-  if (dprintf(fd, "%ld", cache_default_max_unused_age_s) < 0)
-    {
-      int ret = -errno;
-      close (fd);
-      return ret;
-    }
-
-  close (fd);
-  return 0;
-}
-
-
 /* Delete any files that have been unmodied for a period
    longer than $DEBUGINFOD_CACHE_CLEAN_INTERVAL_S.  */
 static int
@@ -795,9 +746,15 @@ debuginfod_query_server (debuginfod_client *c,
   if (vfd >= 0)
     dprintf (vfd, "checking cache dir %s\n", cache_path);
 
-  rc = debuginfod_init_cache(cache_path, interval_path, maxage_path);
-  if (rc != 0)
-    goto out;
+  /* Make sure cache dir exists. debuginfo_clean_cache will then make
+     sure the interval, cache_miss and maxage files exist.  */
+  if (mkdir (cache_path, ACCESSPERMS) != 0
+      && errno != EEXIST)
+    {
+      rc = -errno;
+      goto out;
+    }
+
   rc = debuginfod_clean_cache(c, cache_path, interval_path, maxage_path);
   if (rc != 0)
     goto out;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/3] debuginfod: update mtime of interval_path as early as possible
  2022-05-09 22:57 Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
  2022-05-09 22:57 ` [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat Mark Wielaard
  2022-05-09 22:57 ` [PATCH 2/3] debuginfod: Remove debuginfod_init_cache Mark Wielaard
@ 2022-05-09 22:57 ` Mark Wielaard
  2022-05-14 22:36 ` Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-05-09 22:57 UTC (permalink / raw)
  To: elfutils-devel; +Cc: Mark Wielaard

Call utime on interval_path file as soon as the thread is committed to
cleanup the cache files. This will prevent other threads trying to
also commit to cleaning the cache files. Having multiple threads try
to clean the cache simultaniously doesn't improve cleanup speed
because the threads will try to delete the files in the same order.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 debuginfod/ChangeLog           | 5 +++++
 debuginfod/debuginfod-client.c | 7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index c9aa4fcf..209a22a7 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-05-09  Mark Wielaard  <mark@klomp.org>
+
+	* debuginfod-client.c (debuginfod_clean_cache): Move utime call to
+	before fts traversal.
+
 2022-05-09  Mark Wielaard  <mark@klomp.org>
 
 	* debuginfod-client.c (debuginfod_init_cache): Remove.
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 6bdf1908..b7b65aff 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -297,6 +297,11 @@ debuginfod_clean_cache(debuginfod_client *c,
     /* Interval has not passed, skip cleaning.  */
     return 0;
 
+  /* Update timestamp representing when the cache was last cleaned.
+     Do it at the start to reduce the number of threads trying to do a
+     cleanup simultaniously.  */
+  utime (interval_path, NULL);
+
   /* Read max unused age value from config file.  */
   rc = debuginfod_config_cache(max_unused_path,
 			       cache_default_max_unused_age_s, &st);
@@ -351,8 +356,6 @@ debuginfod_clean_cache(debuginfod_client *c,
   fts_close (fts);
   regfree (&re);
 
-  /* Update timestamp representing when the cache was last cleaned.  */
-  utime (interval_path, NULL);
   return 0;
 }
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Optimize debuginfod-client cache lookup/cleanup a little
  2022-05-09 22:57 Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
                   ` (2 preceding siblings ...)
  2022-05-09 22:57 ` [PATCH 3/3] debuginfod: update mtime of interval_path as early as possible Mark Wielaard
@ 2022-05-14 22:36 ` Mark Wielaard
  3 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-05-14 22:36 UTC (permalink / raw)
  To: elfutils-devel

Hi,

On Tue, May 10, 2022 at 12:57:20AM +0200, Mark Wielaard wrote:
> debuginfod-client would try to create the cache config files twice,
> once through debuginfod_init_cache, which was always called before the
> debuginfod_clean_cache check. Which called debuginfod_config_cache
> which also tried to create the config files when they didn't exist
> yet. debuginfod_config_cache however had a small bug that meant it
> would not provide a valid struct stat if the config file didn't exist
> yet. The first patch fixes that:
> 
> [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat
> 
> Then the second patch removes debuginfod_init_cache which saves two
> stat calls (but introduces a new mkdir call).
> 
> [PATCH 2/3] debuginfod: Remove debuginfod_init_cache
> 
> Finally as soon as debuginfod_clean_cache commits to clean the cache
> dir we immediately update the mtime of the interval config file so
> other threads will not try to simultaniously also try to clean up the
> cache dir. Because that is just duplicate work.
> 
> [PATCH 3/3] debuginfod: update mtime of interval_path as early as possible

Frank said off-list that he didn't see anything wrong with these
patches. So I pushed thse 3 patches.

12 points!

Mark


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-14 22:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 22:57 Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard
2022-05-09 22:57 ` [PATCH 1/3] debuginfod: Make sure debuginfod_config_cache always returns valid stat Mark Wielaard
2022-05-09 22:57 ` [PATCH 2/3] debuginfod: Remove debuginfod_init_cache Mark Wielaard
2022-05-09 22:57 ` [PATCH 3/3] debuginfod: update mtime of interval_path as early as possible Mark Wielaard
2022-05-14 22:36 ` Optimize debuginfod-client cache lookup/cleanup a little Mark Wielaard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).