efivars: retry open and read operations

On my laptop (Lenovo X1carbo 4th) I very occasionally see test-boot-timestamps
fail with this tb:

262/494 test-boot-timestamps                    FAIL    0.7348453998565674 s (killed by signal 6 SIGABRT)

08:12:48 SYSTEMD_LANGUAGE_FALLBACK_MAP='/home/zbyszek/src/systemd/src/locale/language-fallback-map' SYSTEMD_KBD_MODEL_MAP='/home/zbyszek/src/systemd/src/locale/kbd-model-map' PATH='/home/zbyszek/src/systemd/build:/home/zbyszek/.local/bin:/usr/lib64/qt-3.3/bin:/usr/share/Modules/bin:/usr/condabin:/usr/lib64/ccache:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/home/zbyszek/bin:/var/lib/snapd/snap/bin' /home/zbyszek/src/systemd/build/test-boot-timestamps
--- stderr ---
Failed to read $container of PID 1, ignoring: Permission denied
Found container virtualization none.
Failed to get SystemdOptions EFI variable, ignoring: Interrupted system call
Failed to read ACPI FPDT: Permission denied
Failed to read LoaderTimeInitUSec: Interrupted system call
Failed to read EFI loader data: Interrupted system call
Assertion 'q >= 0' failed at src/test/test-boot-timestamps.c:84, function main(). Aborting.

Normally it takes ~0.02s, but here there's a slowdown to 0.73 and things fail with EINTR.
This happens only occasionally, and I haven't been able to capture a strace.

It would be to ignore that case in test-boot-timestamps or always translate
EINTR to -ENODATA. Nevertheless, I think it's better to retry, since this gives
as more resilient behaviour and avoids a transient failure.

See
https://github.com/torvalds/linux/blob/master/fs/efivarfs/file.c#L75
and
https://github.com/torvalds/linux/commit/bef3efbeb897b56867e271cdbc5f8adaacaeb9cd.

diff --git a/src/basic/efivars.c b/src/basic/efivars.c
index 502c3a0c4448aa54565674e7372bd660ec8c43f7..aa08ad2e278f7f4f2fae4c9f7be0e7c6b7f8f209 100644 (file)
--- a/src/basic/efivars.c
+++ b/src/basic/efivars.c
@@ -24,6 +24,10 @@
 
 #if ENABLE_EFI
 
+/* Reads from efivarfs sometimes fail with EINTR. Retry that many times. */
+#define EFI_N_RETRIES 5
+#define EFI_RETRY_DELAY (50 * USEC_PER_MSEC)
+
 char* efi_variable_path(sd_id128_t vendor, const char *name) {
         char *p;
 
@@ -56,8 +60,8 @@ int efi_get_variable(
                 return -ENOMEM;
 
         if (!ret_value && !ret_size && !ret_attribute) {
-                /* If caller is not interested in anything, just check if the variable exists and is readable
-                 * to us. */
+                /* If caller is not interested in anything, just check if the variable exists and is
+                 * readable. */
                 if (access(p, R_OK) < 0)
                         return -errno;
 
@@ -66,7 +70,7 @@ int efi_get_variable(
 
         fd = open(p, O_RDONLY|O_NOCTTY|O_CLOEXEC);
         if (fd < 0)
-                return -errno;
+                return log_debug_errno(errno, "open(\"%s\") failed: %m", p);
 
         if (fstat(fd, &st) < 0)
                 return -errno;
@@ -76,9 +80,26 @@ int efi_get_variable(
                 return -E2BIG;
 
         if (ret_value || ret_attribute) {
-                n = read(fd, &a, sizeof(a));
-                if (n < 0)
-                        return -errno;
+                /* The kernel ratelimits reads from the efivarfs because EFI is inefficient, and we'll
+                 * occasionally fail with EINTR here. A slowdown is better than a failure for us, so
+                 * retry a few times and eventually fail with -EBUSY.
+                 *
+                 * See https://github.com/torvalds/linux/blob/master/fs/efivarfs/file.c#L75
+                 * and
+                 * https://github.com/torvalds/linux/commit/bef3efbeb897b56867e271cdbc5f8adaacaeb9cd.
+                 */
+                for (unsigned try = 0;; try++) {
+                        n = read(fd, &a, sizeof(a));
+                        if (n >= 0)
+                                break;
+                        log_debug_errno(errno, "read from \"%s\" failed: %m", p);
+                        if (errno != EINTR)
+                                return -errno;
+                        if (try >= EFI_N_RETRIES)
+                                return -EBUSY;
+                        usleep(EFI_RETRY_DELAY);
+                }
+
                 if (n != sizeof(a))
                         return -EIO;
         }