From: Lennart Poettering Date: Fri, 25 Sep 2020 13:22:48 +0000 (+0200) Subject: loop-util: LOOP_CLR_FD is async, don't retry to reuse a device right after issuing it X-Git-Tag: v247-rc1~13^2~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=95c5009248086f8a6fd86808654072adb1395afa;p=thirdparty%2Fsystemd.git loop-util: LOOP_CLR_FD is async, don't retry to reuse a device right after issuing it When we fall back to classic LOOP_SET_FD logic in case LOOP_CONFIGURE didn't work we issue LOOP_CLR_FD first. But that call turns out to be potentially async in the kernel: if something else (let's say udev/blkid) is accessing the device the ioctl just sets the autoclear flag and exits. Hence quite often the LOOP_SET_FD will subsequently fail. Let's avoid the trouble, and immediately exit with EBUSY if LOOP_CONFIGURE fails, and but remember that LOOP_CONFIGURE is not available so that on the next iteration we go directly for LOOP_SET_FD instead. --- diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c index 156a41dd3a6..e4b44ab89b7 100644 --- a/src/shared/loop-util.c +++ b/src/shared/loop-util.c @@ -33,60 +33,78 @@ static void cleanup_clear_loop_close(int *fd) { (void) safe_close(*fd); } -static int loop_configure(int fd, const struct loop_config *c) { +static int loop_configure( + int fd, + const struct loop_config *c, + bool *try_loop_configure) { + _cleanup_close_ int lock_fd = -1; int r; assert(fd >= 0); assert(c); + assert(try_loop_configure); + + if (*try_loop_configure) { + if (ioctl(fd, LOOP_CONFIGURE, c) < 0) { + /* Do fallback only if LOOP_CONFIGURE is not supported, propagate all other + * errors. Note that the kernel is weird: non-existing ioctls currently return EINVAL + * rather than ENOTTY on loopback block devices. They should fix that in the kernel, + * but in the meantime we accept both here. */ + if (!ERRNO_IS_NOT_SUPPORTED(errno) && errno != EINVAL) + return -errno; - if (ioctl(fd, LOOP_CONFIGURE, c) < 0) { - /* Do fallback only if LOOP_CONFIGURE is not supported, propagate all other errors. Note that - * the kernel is weird: non-existing ioctls currently return EINVAL rather than ENOTTY on - * loopback block devices. They should fix that in the kernel, but in the meantime we accept - * both here. */ - if (!ERRNO_IS_NOT_SUPPORTED(errno) && errno != EINVAL) - return -errno; - } else { - bool good = true; + *try_loop_configure = false; + } else { + bool good = true; - if (c->info.lo_sizelimit != 0) { - /* Kernel 5.8 vanilla doesn't properly propagate the size limit into the block - * device. If it's used, let's immediately check if it had the desired effect - * hence. And if not use classic LOOP_SET_STATUS64. */ - uint64_t z; + if (c->info.lo_sizelimit != 0) { + /* Kernel 5.8 vanilla doesn't properly propagate the size limit into the + * block device. If it's used, let's immediately check if it had the desired + * effect hence. And if not use classic LOOP_SET_STATUS64. */ + uint64_t z; - if (ioctl(fd, BLKGETSIZE64, &z) < 0) { - r = -errno; - goto fail; - } + if (ioctl(fd, BLKGETSIZE64, &z) < 0) { + r = -errno; + goto fail; + } - if (z != c->info.lo_sizelimit) { - log_debug("LOOP_CONFIGURE is broken, doesn't honour .lo_sizelimit. Falling back to LOOP_SET_STATUS64."); - good = false; + if (z != c->info.lo_sizelimit) { + log_debug("LOOP_CONFIGURE is broken, doesn't honour .lo_sizelimit. Falling back to LOOP_SET_STATUS64."); + good = false; + } } - } - if (FLAGS_SET(c->info.lo_flags, LO_FLAGS_PARTSCAN)) { - /* Kernel 5.8 vanilla doesn't properly propagate the partition scanning flag into the - * block device. Let's hence verify if things work correctly here before - * returning. */ + if (FLAGS_SET(c->info.lo_flags, LO_FLAGS_PARTSCAN)) { + /* Kernel 5.8 vanilla doesn't properly propagate the partition scanning flag + * into the block device. Let's hence verify if things work correctly here + * before returning. */ + + r = blockdev_partscan_enabled(fd); + if (r < 0) + goto fail; + if (r == 0) { + log_debug("LOOP_CONFIGURE is broken, doesn't honour LO_FLAGS_PARTSCAN. Falling back to LOOP_SET_STATUS64."); + good = false; + } + } - r = blockdev_partscan_enabled(fd); - if (r < 0) + if (!good) { + /* LOOP_CONFIGURE doesn't work. Remember that. */ + *try_loop_configure = false; + + /* We return EBUSY here instead of retrying immediately with LOOP_SET_FD, + * because LOOP_CLR_FD is async: if the operation cannot be executed right + * away it just sets the autoclear flag on the device. This means there's a + * good chance we cannot actually reuse the loopback device right-away. Hence + * let's assume it's busy, avoid the trouble and let the calling loop call us + * again with a new, likely unused device. */ + r = -EBUSY; goto fail; - if (r == 0) { - log_debug("LOOP_CONFIGURE is broken, doesn't honour LO_FLAGS_PARTSCAN. Falling back to LOOP_SET_STATUS64."); - good = false; } - } - if (good) return 0; - - /* Otherwise, undo the attachment and use the old APIs */ - if (ioctl(fd, LOOP_CLR_FD) < 0) - return -errno; + } } /* Since kernel commit 5db470e229e22b7eda6e23b5566e532c96fb5bc3 (kernel v5.0) the LOOP_SET_STATUS64 @@ -143,6 +161,7 @@ int loop_device_make( LoopDevice **ret) { _cleanup_free_ char *loopdev = NULL; + bool try_loop_configure = true; struct loop_config config; LoopDevice *d = NULL; struct stat st; @@ -233,7 +252,7 @@ int loop_device_make( if (!IN_SET(errno, ENOENT, ENXIO)) return -errno; } else { - r = loop_configure(loop, &config); + r = loop_configure(loop, &config, &try_loop_configure); if (r >= 0) { loop_with_fd = TAKE_FD(loop); break;