From a145f8c06c354328b4a6ed8a7621c4b21cbca05b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 16:58:32 +0200 Subject: [PATCH 01/10] loop-util: let's cut trailing whitespace, not trailing lines This doesn't really make any real difference, given the file should only contain a single line. But it's conceptually more correct to just remove the trailing newline/whitespace then the whole lines coming after that. i.e. if the file actually contains more lines than one, this should probably be considered an error. --- src/shared/loop-util.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 6356ff44d8..026adf070d 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -62,9 +62,7 @@ static int get_current_uevent_seqnum(uint64_t *ret) { if (r < 0) return log_debug_errno(r, "Failed to read current uevent sequence number: %m"); - truncate_nl(p); - - r = safe_atou64(p, ret); + r = safe_atou64(strstrip(p), ret); if (r < 0) return log_debug_errno(r, "Failed to parse current uevent sequence number: %s", p); From 7ffc7f3fcc86260417a7b09bd40f9c517da371df Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 17:00:38 +0200 Subject: [PATCH 02/10] loop-util: slightly rework device_has_block_children() Let's match by devtype, i.e. the official way to distinguish "whole" block devices from partitions. Also add debug logging for devices we thus ignore. --- src/shared/loop-util.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 026adf070d..e5c7acb470 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -71,7 +71,7 @@ static int get_current_uevent_seqnum(uint64_t *ret) { static int device_has_block_children(sd_device *d) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; - const char *main_sn, *main_ss; + const char *main_ss, *main_dt; sd_device *q; int r; @@ -80,10 +80,6 @@ static int device_has_block_children(sd_device *d) { /* Checks if the specified device currently has block device children (i.e. partition block * devices). */ - r = sd_device_get_sysname(d, &main_sn); - if (r < 0) - return r; - r = sd_device_get_subsystem(d, &main_ss); if (r < 0) return r; @@ -91,6 +87,13 @@ static int device_has_block_children(sd_device *d) { if (!streq(main_ss, "block")) return -EINVAL; + r = sd_device_get_devtype(d, &main_dt); + if (r < 0) + return r; + + if (!streq(main_dt, "disk")) /* Refuse invocation on partition block device, insist on "whole" device */ + return -EINVAL; + r = sd_device_enumerator_new(&e); if (r < 0) return r; @@ -104,26 +107,34 @@ static int device_has_block_children(sd_device *d) { return r; FOREACH_DEVICE(e, q) { - const char *ss, *sn; + const char *ss, *dt; r = sd_device_get_subsystem(q, &ss); - if (r < 0) + if (r < 0) { + log_device_debug_errno(q, r, "Failed to get subsystem of child, ignoring: %m"); continue; + } - if (!streq(ss, "block")) + if (!streq(ss, "block")) { + log_device_debug(q, "Skipping child that is not a block device (subsystem=%s).", ss); continue; + } - r = sd_device_get_sysname(q, &sn); - if (r < 0) + r = sd_device_get_devtype(q, &dt); + if (r < 0) { + log_device_debug_errno(q, r, "Failed to get devtype of child, ignoring: %m"); continue; + } - if (streq(sn, main_sn)) + if (!streq(dt, "partition")) { + log_device_debug(q, "Skipping non-partition child (devtype=%s).", dt); continue; + } - return 1; /* we have block device children */ + return true; /* we have block device children */ } - return 0; + return false; } static int loop_configure( From da9ae8ea6c3d1c05ec171e4472669c61b43b65d8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 18:09:10 +0200 Subject: [PATCH 03/10] test-loop-block: don't spawn threads in case N_THREADS is 1 Let's simplify things if we are only supposed to create a single thread. Makes it easier to debug things with gdb. --- src/test/test-loop-block.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/test/test-loop-block.c b/src/test/test-loop-block.c index 9c8c55bca2..54f5d28da7 100644 --- a/src/test/test-loop-block.c +++ b/src/test/test-loop-block.c @@ -228,20 +228,26 @@ int main(int argc, char *argv[]) { slow_tests_enabled() ? 5 * USEC_PER_SEC : 1 * USEC_PER_SEC); - for (unsigned i = 0; i < N_THREADS; i++) - assert_se(pthread_create(threads + i, NULL, thread_func, FD_TO_PTR(fd)) == 0); + assert_cc(N_THREADS > 0); + + if (N_THREADS > 1) + for (unsigned i = 0; i < N_THREADS; i++) + assert_se(pthread_create(threads + i, NULL, thread_func, FD_TO_PTR(fd)) == 0); log_notice("All threads started now."); - for (unsigned i = 0; i < N_THREADS; i++) { - log_notice("Joining thread #%u.", i); + if (N_THREADS == 1) + assert_se(thread_func(FD_TO_PTR(fd)) == NULL); + else + for (unsigned i = 0; i < N_THREADS; i++) { + log_notice("Joining thread #%u.", i); - void *k; - assert_se(pthread_join(threads[i], &k) == 0); - assert_se(k == NULL); + void *k; + assert_se(pthread_join(threads[i], &k) == 0); + assert_se(k == NULL); - log_notice("Joined thread #%u.", i); - } + log_notice("Joined thread #%u.", i); + } log_notice("Threads are all terminated now."); From cc53046620ac193a2d963761ff30a2bb4bf6ddb0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 18:56:47 +0200 Subject: [PATCH 04/10] loop-util: take a LOCK_EX BSD file lock on control device while we acquire a loopback device --- src/shared/loop-util.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index e5c7acb470..4f9c2de577 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -514,6 +514,17 @@ static int loop_device_make_internal( for (unsigned n_attempts = 0;;) { _cleanup_close_ int loop = -1; + /* Let's take a lock on the control device first. On a busy system, where many programs + * attempt to allocate a loopback device at the same time, we might otherwise keep looping + * around relatively heavy operations: asking for a free loopback device, then opening it, + * validating it, attaching something to it. Let's serialize this whole operation, to make + * unnecessary busywork less likely. Note that this is just something we do to optimize our + * own code (and whoever else decides to use LOCK_EX locks for this), taking this lock is not + * necessary, it just means it's less likely we have to iterate through this loop again and + * again if our own code races against our own code. */ + if (flock(control, LOCK_EX) < 0) + return -errno; + nr = ioctl(control, LOOP_CTL_GET_FREE); if (nr < 0) return -errno; @@ -542,6 +553,11 @@ static int loop_device_make_internal( return r; } + /* OK, this didn't work, let's try again a bit later, but first release the lock on the + * control device */ + if (flock(control, LOCK_UN) < 0) + return -errno; + if (++n_attempts >= 64) /* Give up eventually */ return -EBUSY; From 49043f811559a98232c0401371f85953cd0ae273 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 23:34:16 +0200 Subject: [PATCH 05/10] loop-util: use ERRNO_IS_DEVICE_ABSENT() macro where appropriate --- src/shared/loop-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 4f9c2de577..257af7340f 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -536,7 +536,7 @@ static int loop_device_make_internal( if (loop < 0) { /* Somebody might've gotten the same number from the kernel, used the device, * and called LOOP_CTL_REMOVE on it. Let's retry with a new number. */ - if (!IN_SET(errno, ENOENT, ENXIO)) + if (!ERRNO_IS_DEVICE_ABSENT(errno)) return -errno; } else { r = loop_configure(loop, nr, &config, &try_loop_configure, &seqnum, ×tamp); From 3e9210577dbe78b3c0cc9a8b0488c51f7339dd02 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 23:34:44 +0200 Subject: [PATCH 06/10] loop-util: explicitly close loopback block device before sleeping attach_empty() file takes a BSD file lock on the device, and we really should release that before going to sleep. hence explicitly close the block device before the sleep instead of relying on _cleanup_ to close it after the sleep. --- src/shared/loop-util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 257af7340f..073755fb9e 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -561,6 +561,9 @@ static int loop_device_make_internal( if (++n_attempts >= 64) /* Give up eventually */ return -EBUSY; + /* Now close the loop device explicitly. This will release any lock acquired by + * attach_empty_file() or similar, while we sleep below. */ + loop = safe_close(loop); loopdev = mfree(loopdev); /* Wait some random time, to make collision less likely. Let's pick a random time in the From 3b195f63fce57c012ce72eb942540dd1fb5f54b7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 5 Apr 2022 23:36:33 +0200 Subject: [PATCH 07/10] loop-util: add debug message with details about acquired loopback device --- src/shared/loop-util.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 073755fb9e..39a9a766c0 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -616,6 +616,12 @@ static int loop_device_make_internal( .timestamp_not_before = timestamp, }; + log_debug("Successfully acquired %s, devno=%u:%u, nr=%i, diskseq=%" PRIu64, + d->node, + major(d->devno), minor(d->devno), + d->nr, + d->diskseq); + *ret = d; return d->fd; } From d4dd3689fa7a791c7b3198debab25b290668da52 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 6 Apr 2022 11:49:08 +0200 Subject: [PATCH 08/10] test: allow specifying test threads/iterations/timeouts via cmdline That's useful when running tests manually. --- src/test/test-loop-block.c | 59 +++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/src/test/test-loop-block.c b/src/test/test-loop-block.c index 54f5d28da7..35a981bcce 100644 --- a/src/test/test-loop-block.c +++ b/src/test/test-loop-block.c @@ -10,10 +10,12 @@ #include "fileio.h" #include "fs-util.h" #include "gpt.h" +#include "main-func.h" #include "missing_loop.h" #include "mkfs-util.h" #include "mount-util.h" #include "namespace-util.h" +#include "parse-util.h" #include "string-util.h" #include "strv.h" #include "tests.h" @@ -21,8 +23,9 @@ #include "user-util.h" #include "virt.h" -#define N_THREADS 5 -#define N_ITERATIONS 3 +static unsigned arg_n_threads = 5; +static unsigned arg_n_iterations = 3; +static usec_t arg_timeout = 0; static usec_t end = 0; @@ -30,7 +33,7 @@ static void* thread_func(void *ptr) { int fd = PTR_TO_FD(ptr); int r; - for (unsigned i = 0; i < N_ITERATIONS; i++) { + for (unsigned i = 0; i < arg_n_iterations; i++) { _cleanup_(loop_device_unrefp) LoopDevice *loop = NULL; _cleanup_(umount_and_rmdir_and_freep) char *mounted = NULL; _cleanup_(dissected_image_unrefp) DissectedImage *dissected = NULL; @@ -106,20 +109,47 @@ static bool have_root_gpt_type(void) { #endif } -int main(int argc, char *argv[]) { +static int run(int argc, char *argv[]) { _cleanup_free_ char *p = NULL, *cmd = NULL; _cleanup_(pclosep) FILE *sfdisk = NULL; _cleanup_(loop_device_unrefp) LoopDevice *loop = NULL; _cleanup_close_ int fd = -1; _cleanup_(dissected_image_unrefp) DissectedImage *dissected = NULL; _cleanup_(umount_and_rmdir_and_freep) char *mounted = NULL; - pthread_t threads[N_THREADS]; sd_id128_t id; int r; test_setup_logging(LOG_DEBUG); log_show_tid(true); log_show_time(true); + log_show_color(true); + + if (argc >= 2) { + r = safe_atou(argv[1], &arg_n_threads); + if (r < 0) + return log_error_errno(r, "Failed to parse first argument (number of threads): %s", argv[1]); + if (arg_n_threads <= 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Number of threads must be at least 1, refusing."); + } + + if (argc >= 3) { + r = safe_atou(argv[2], &arg_n_iterations); + if (r < 0) + return log_error_errno(r, "Failed to parse second argument (number of iterations): %s", argv[2]); + if (arg_n_iterations <= 0) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Number of iterations must be at least 1, refusing."); + } + + if (argc >= 4) { + r = parse_sec(argv[3], &arg_timeout); + if (r < 0) + return log_error_errno(r, "Failed to parse third argument (timeout): %s", argv[3]); + } + + if (argc >= 5) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Too many arguments (expected 3 at max)."); + + pthread_t threads[arg_n_threads]; if (!have_root_gpt_type()) { log_tests_skipped("No root partition GPT defined for this architecture, exiting."); @@ -223,23 +253,22 @@ int main(int argc, char *argv[]) { log_notice("Threads are being started now"); - /* Let's make sure we run for 10s on slow systems at max */ - end = usec_add(now(CLOCK_MONOTONIC), - slow_tests_enabled() ? 5 * USEC_PER_SEC : - 1 * USEC_PER_SEC); + /* zero timeout means pick default: let's make sure we run for 10s on slow systems at max */ + if (arg_timeout == 0) + arg_timeout = slow_tests_enabled() ? 5 * USEC_PER_SEC : 1 * USEC_PER_SEC; - assert_cc(N_THREADS > 0); + end = usec_add(now(CLOCK_MONOTONIC), arg_timeout); - if (N_THREADS > 1) - for (unsigned i = 0; i < N_THREADS; i++) + if (arg_n_threads > 1) + for (unsigned i = 0; i < arg_n_threads; i++) assert_se(pthread_create(threads + i, NULL, thread_func, FD_TO_PTR(fd)) == 0); log_notice("All threads started now."); - if (N_THREADS == 1) + if (arg_n_threads == 1) assert_se(thread_func(FD_TO_PTR(fd)) == NULL); else - for (unsigned i = 0; i < N_THREADS; i++) { + for (unsigned i = 0; i < arg_n_threads; i++) { log_notice("Joining thread #%u.", i); void *k; @@ -253,3 +282,5 @@ int main(int argc, char *argv[]) { return 0; } + +DEFINE_MAIN_FUNCTION_WITH_POSITIVE_FAILURE(run); From e745ddb16f7910402f30e26cb1133a1e886c9742 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 6 Apr 2022 11:57:13 +0200 Subject: [PATCH 09/10] test-loop-block: conditionalize things on HAVE_BLKID correctly --- src/test/test-loop-block.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/test/test-loop-block.c b/src/test/test-loop-block.c index 35a981bcce..2f3c3745e2 100644 --- a/src/test/test-loop-block.c +++ b/src/test/test-loop-block.c @@ -27,6 +27,7 @@ static unsigned arg_n_threads = 5; static unsigned arg_n_iterations = 3; static usec_t arg_timeout = 0; +#if HAVE_BLKID static usec_t end = 0; static void* thread_func(void *ptr) { @@ -100,6 +101,7 @@ static void* thread_func(void *ptr) { return NULL; } +#endif static bool have_root_gpt_type(void) { #ifdef GPT_ROOT_NATIVE @@ -114,9 +116,6 @@ static int run(int argc, char *argv[]) { _cleanup_(pclosep) FILE *sfdisk = NULL; _cleanup_(loop_device_unrefp) LoopDevice *loop = NULL; _cleanup_close_ int fd = -1; - _cleanup_(dissected_image_unrefp) DissectedImage *dissected = NULL; - _cleanup_(umount_and_rmdir_and_freep) char *mounted = NULL; - sd_id128_t id; int r; test_setup_logging(LOG_DEBUG); @@ -149,8 +148,6 @@ static int run(int argc, char *argv[]) { if (argc >= 5) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Too many arguments (expected 3 at max)."); - pthread_t threads[arg_n_threads]; - if (!have_root_gpt_type()) { log_tests_skipped("No root partition GPT defined for this architecture, exiting."); return EXIT_TEST_SKIP; @@ -217,6 +214,13 @@ static int run(int argc, char *argv[]) { sfdisk = NULL; assert_se(loop_device_make(fd, O_RDWR, 0, UINT64_MAX, LO_FLAGS_PARTSCAN, &loop) >= 0); + +#if HAVE_BLKID + _cleanup_(dissected_image_unrefp) DissectedImage *dissected = NULL; + _cleanup_(umount_and_rmdir_and_freep) char *mounted = NULL; + pthread_t threads[arg_n_threads]; + sd_id128_t id; + assert_se(dissect_image(loop->fd, NULL, NULL, loop->diskseq, loop->uevent_seqnum_not_before, loop->timestamp_not_before, 0, &dissected) >= 0); assert_se(dissected->partitions[PARTITION_ESP].found); @@ -279,6 +283,9 @@ static int run(int argc, char *argv[]) { } log_notice("Threads are all terminated now."); +#else + log_notice("Cutting test short, since we do not have libblkid."); +#endif return 0; } From 1cefb9a6af1a4e87d7602886fc425aa9637020b4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Apr 2022 14:11:44 +0200 Subject: [PATCH 10/10] dissect: improve debug logging in probe_filesystem() --- src/shared/dissect-image.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 6e287ecac7..2cf031cdeb 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -92,21 +92,21 @@ int probe_filesystem(const char *node, char **ret_fstype) { errno = 0; r = blkid_do_safeprobe(b); - if (r == 1) { - log_debug("No type detected on partition %s", node); + if (r == 1) goto not_found; - } if (r == -2) return log_debug_errno(SYNTHETIC_ERRNO(EUCLEAN), "Results ambiguous for partition %s", node); if (r != 0) - return errno_or_else(EIO); + return log_debug_errno(errno_or_else(EIO), "Failed to probe partition %s: %m", node); (void) blkid_probe_lookup_value(b, "TYPE", &fstype, NULL); if (fstype) { char *t; + log_debug("Probed fstype '%s' on partition %s.", fstype, node); + t = strdup(fstype); if (!t) return -ENOMEM; @@ -116,6 +116,7 @@ int probe_filesystem(const char *node, char **ret_fstype) { } not_found: + log_debug("No type detected on partition %s", node); *ret_fstype = NULL; return 0; #else