[PATCH 1/3] debuginfod: Replace futimes with futimens

[PATCH 1/3] debuginfod: Replace futimes with futimens

[Jan Alexander Steffens (heftig)]

Similar to what 8c4aa0ef998191ed828a37190dc179b91649938a did for ar and
strip, replace the non-standard futimes with the POSIX futimens.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 debuginfod/debuginfod-client.c |  6 +++---
 debuginfod/debuginfod.cxx      | 13 ++++++-------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index b33408eb..460afd5c 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1721,10 +1721,10 @@ debuginfod_query_server (debuginfod_client *c,
   if (curl_res != CURLE_OK)
     mtime = time(NULL); /* fall back to current time */
 
-  struct timeval tvs[2];
+  struct timespec tvs[2];
   tvs[0].tv_sec = tvs[1].tv_sec = mtime;
-  tvs[0].tv_usec = tvs[1].tv_usec = 0;
-  (void) futimes (fd, tvs);  /* best effort */
+  tvs[0].tv_nsec = tvs[1].tv_nsec = 0;
+  (void) futimens (fd, tvs);  /* best effort */
 
   /* PR27571: make cache files casually unwriteable; dirs are already 0700 */
   (void) fchmod(fd, 0400);
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 99b1f2b9..b39c0591 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1635,13 +1635,12 @@ extract_section (int elf_fd, int64_t parent_mtime,
 		throw libc_exception (errno, "cannot write to temporary file");
 
 	      /* Set mtime to be the same as the parent file's mtime.  */
-	      struct timeval tvs[2];
+	      struct timespec tvs[2];
 	      if (fstat (elf_fd, &fs) != 0)
 		throw libc_exception (errno, "cannot fstat file");
 
-	      tvs[0].tv_sec = tvs[1].tv_sec = fs.st_mtime;
-	      tvs[0].tv_usec = tvs[1].tv_usec = 0;
-	      (void) futimes (fd, tvs);
+	      tvs[0] = tvs[1] = fs.st_mtim;
+	      (void) futimens (fd, tvs);
 
 	      /* Add to fdcache.  */
 	      fdcache.intern (b_source, section, tmppath, data->d_size, true);
@@ -1951,10 +1950,10 @@ handle_buildid_r_match (bool internal_req_p,
 
       // Set the mtime so the fdcache file mtimes, even prefetched ones,
       // propagate to future webapi clients.
-      struct timeval tvs[2];
+      struct timespec tvs[2];
       tvs[0].tv_sec = tvs[1].tv_sec = archive_entry_mtime(e);
-      tvs[0].tv_usec = tvs[1].tv_usec = 0;
-      (void) futimes (fd, tvs);  /* best effort */
+      tvs[0].tv_nsec = tvs[1].tv_nsec = archive_entry_mtime_nsec(e);
+      (void) futimens (fd, tvs);  /* best effort */
 
       if (r != 0) // stage 3
         {
-- 
2.40.0

[PATCH 2/3] debuginfod: Don't touch access time of new files

[Jan Alexander Steffens (heftig)]

Instead of copying the mtime, which might be far in the past, don't
touch the access time. This will prevent cache cleaning from considering
the file as old immediately.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 debuginfod/debuginfod-client.c | 16 +++++++++-------
 debuginfod/debuginfod.cxx      | 10 +++++++---
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 460afd5c..1a2d7573 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1718,13 +1718,15 @@ debuginfod_query_server (debuginfod_client *c,
 #else
   CURLcode curl_res = curl_easy_getinfo(verified_handle, CURLINFO_FILETIME, (void*) &mtime);
 #endif
-  if (curl_res != CURLE_OK)
-    mtime = time(NULL); /* fall back to current time */
-
-  struct timespec tvs[2];
-  tvs[0].tv_sec = tvs[1].tv_sec = mtime;
-  tvs[0].tv_nsec = tvs[1].tv_nsec = 0;
-  (void) futimens (fd, tvs);  /* best effort */
+  if (curl_res == CURLE_OK)
+    {
+      struct timespec tvs[2];
+      tvs[0].tv_sec = 0;
+      tvs[0].tv_nsec = UTIME_OMIT;
+      tvs[1].tv_sec = mtime;
+      tvs[1].tv_nsec = 0;
+      (void) futimens (fd, tvs);  /* best effort */
+    }
 
   /* PR27571: make cache files casually unwriteable; dirs are already 0700 */
   (void) fchmod(fd, 0400);
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index b39c0591..5ef6cc32 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1639,7 +1639,9 @@ extract_section (int elf_fd, int64_t parent_mtime,
 	      if (fstat (elf_fd, &fs) != 0)
 		throw libc_exception (errno, "cannot fstat file");
 
-	      tvs[0] = tvs[1] = fs.st_mtim;
+	      tvs[0].tv_sec = 0;
+	      tvs[0].tv_nsec = UTIME_OMIT;
+	      tvs[1] = fs.st_mtim;
 	      (void) futimens (fd, tvs);
 
 	      /* Add to fdcache.  */
@@ -1951,8 +1953,10 @@ handle_buildid_r_match (bool internal_req_p,
       // Set the mtime so the fdcache file mtimes, even prefetched ones,
       // propagate to future webapi clients.
       struct timespec tvs[2];
-      tvs[0].tv_sec = tvs[1].tv_sec = archive_entry_mtime(e);
-      tvs[0].tv_nsec = tvs[1].tv_nsec = archive_entry_mtime_nsec(e);
+      tvs[0].tv_sec = 0;
+      tvs[0].tv_nsec = UTIME_OMIT;
+      tvs[1].tv_sec = archive_entry_mtime(e);
+      tvs[1].tv_nsec = archive_entry_mtime_nsec(e);
       (void) futimens (fd, tvs);  /* best effort */
 
       if (r != 0) // stage 3
-- 
2.40.0

[PATCH 3/3] debuginfod: When retrieving files from cache, update atime manually

[Jan Alexander Steffens (heftig)]

The cache cleaning logic requires atime to be correct (strictatime) but
most users on Linux only have relatime or even noatime.

Attempt to update the atime manually so that the cache works properly.

Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
---
 debuginfod/debuginfod-client.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 1a2d7573..484dc7b3 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -618,6 +618,19 @@ path_escape (const char *src, char *dest)
   dest[q] = '\0';
 }
 
+/* Attempt to update the atime */
+static void
+update_atime (int fd)
+{
+  struct timespec tvs[2];
+
+  tvs[0].tv_sec = tvs[1].tv_sec = 0;
+  tvs[0].tv_nsec = UTIME_NOW;
+  tvs[1].tv_nsec = UTIME_OMIT;
+
+  (void) futimens (fd, tvs);  /* best effort */
+}
+
 /* Attempt to read an ELF/DWARF section with name SECTION from FD and write
    it to a separate file in the debuginfod cache.  If successful the absolute
    path of the separate file containing SECTION will be stored in USR_PATH.
@@ -761,6 +774,7 @@ extract_section (int fd, const char *section, char *fd_path, char **usr_path)
 	    *usr_path = sec_path;
 	  else
 	    free (sec_path);
+	  update_atime(fd);
 	  rc = sec_fd;
 	  goto out2;
 	}
@@ -1098,6 +1112,7 @@ debuginfod_query_server (debuginfod_client *c,
                 }
             }
           /* Success!!!! */
+          update_atime(fd);
           rc = fd;
           goto out;
         }
-- 
2.40.0

Re: [PATCH 1/3] debuginfod: Replace futimes with futimens

[Frank Ch. Eigler]

Hi -

> Similar to what 8c4aa0ef998191ed828a37190dc179b91649938a did for ar and
> strip, replace the non-standard futimes with the POSIX futimens.

Thanks, merged (with a little ChangeLog thing added ... please free us, mjw!).

- FChE

Re: [PATCH 3/3] debuginfod: When retrieving files from cache, update atime manually

[Frank Ch. Eigler]

Hi -

> The cache cleaning logic requires atime to be correct (strictatime) but
> most users on Linux only have relatime or even noatime.

This is not really correct: relatime is the kernel default and works
fine with the cache.  atime values updated once a day are still plenty
for caches with a multi-day preservation default.

> Attempt to update the atime manually so that the cache works properly.

Has this been observed to work on noatime-mounted filesystems?  It's
at worst harmless so we could merge it, but it's kind of weird.  Do
you know such precedents in other software that consumes atime?

- FChE

Re: [PATCH 3/3] debuginfod: When retrieving files from cache, update atime manually

[Jan Alexander Steffens (heftig)]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

On Wed, Mar 29, 2023 at 10:54 PM Frank Ch. Eigler <fche@redhat.com> wrote:

> Hi -
>
> > The cache cleaning logic requires atime to be correct (strictatime) but
> > most users on Linux only have relatime or even noatime.
>
> This is not really correct: relatime is the kernel default and works
> fine with the cache.  atime values updated once a day are still plenty
> for caches with a multi-day preservation default.
>

Ah, I didn't know about the one-day threshold. So that means only
noatime is actually affected.


>
> > Attempt to update the atime manually so that the cache works properly.
>
> Has this been observed to work on noatime-mounted filesystems?  It's
> at worst harmless so we could merge it, but it's kind of weird.  Do
> you know such precedents in other software that consumes atime?
>

Yes, I have a noatime-mounted btrfs (because with snapshots, relatime
can cause entire directory trees to get copied-on-write) and with this
patch series the cache seems to work properly. Without it it is pretty
broken, immediately evicting files that were just downloaded.

The only software I know of that uses atime is mutt, although I have
never used it myself. Looking through the source of mutt and neomutt
on GitHub shows that they use the same approach:

https://github.com/muttmua/mutt/blob/1066be975f284ce6fdbb00a4e41b1738d52887d0/muttlib.c#L2204-L2212
(mutt muttlib.c:2204)

https://github.com/neomutt/neomutt/blob/d13dd3bb912d93f73dd4dd2d476e1c37d31a8422/mutt/file.c#L1062-L1075
(neomutt mutt/file.c:1062)