From 49f80dcec83be59e14be76e15e69933cbca7f846 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 16:57:29 +0100 Subject: [PATCH 1/6] umount: line break comments again break them like we usually do, taking our intended line width into account. --- src/shutdown/umount.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 88dc03ed62..df339a994b 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -460,8 +460,8 @@ static int umount_with_timeout(MountPoint *m, int umount_log_level) { return r; } -/* This includes remounting readonly, which changes the kernel mount options. - * Therefore the list passed to this function is invalidated, and should not be reused. */ +/* This includes remounting readonly, which changes the kernel mount options. Therefore the list passed to + * this function is invalidated, and should not be reused. */ static int mount_points_list_umount(MountPoint **head, bool *changed, int umount_log_level) { MountPoint *m; int n_failed = 0; @@ -471,26 +471,18 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, int umount LIST_FOREACH(mount_point, m, *head) { if (m->try_remount_ro) { - /* We always try to remount directories - * read-only first, before we go on and umount + /* We always try to remount directories read-only first, before we go on and umount * them. * - * Mount points can be stacked. If a mount - * point is stacked below / or /usr, we - * cannot umount or remount it directly, - * since there is no way to refer to the - * underlying mount. There's nothing we can do - * about it for the general case, but we can - * do something about it if it is aliased - * somewhere else via a bind mount. If we - * explicitly remount the super block of that - * alias read-only we hence should be - * relatively safe regarding keeping a dirty fs - * we cannot otherwise see. + * Mount points can be stacked. If a mount point is stacked below / or /usr, we + * cannot umount or remount it directly, since there is no way to refer to the + * underlying mount. There's nothing we can do about it for the general case, but we + * can do something about it if it is aliased somewhere else via a bind mount. If we + * explicitly remount the super block of that alias read-only we hence should be + * relatively safe regarding keeping a dirty fs we cannot otherwise see. * - * Since the remount can hang in the instance of - * remote filesystems, we remount asynchronously - * and skip the subsequent umount if it fails. */ + * Since the remount can hang in the instance of remote filesystems, we remount + * asynchronously and skip the subsequent umount if it fails. */ if (remount_with_timeout(m, umount_log_level) < 0) { /* Remount failed, but try unmounting anyway, * unless this is a mount point we want to skip. */ @@ -501,9 +493,8 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, int umount } } - /* Skip / and /usr since we cannot unmount that - * anyway, since we are running from it. They have - * already been remounted ro. */ + /* Skip / and /usr since we cannot unmount that anyway, since we are running from it. They + * have already been remounted ro. */ if (nonunmountable_path(m->path)) continue; From 610f9a42c4a8c34d9b3a2b6c6289e8c45b57283f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 16:58:27 +0100 Subject: [PATCH 2/6] umount: remove unneeded variable --- src/shutdown/umount.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index df339a994b..0b8044dacb 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -325,7 +325,6 @@ static int dm_list_get(MountPoint **head) { static int delete_loopback(const char *device) { _cleanup_close_ int fd = -1; - int r; assert(device); @@ -333,8 +332,7 @@ static int delete_loopback(const char *device) { if (fd < 0) return errno == ENOENT ? 0 : -errno; - r = ioctl(fd, LOOP_CLR_FD, 0); - if (r >= 0) + if (ioctl(fd, LOOP_CLR_FD, 0) >= 0) return 1; /* ENXIO: not bound, so no error */ From 88287615e631d2023ff337a08b6ff45b1cfa58ee Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 16:59:42 +0100 Subject: [PATCH 3/6] umount: show correct error message We fucked up errno vs. r two times, let's correct that. While we are at it, let's handle the error first, like we usually do, and the clean case without indentation. --- src/shutdown/umount.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 0b8044dacb..51ed05cb6a 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -515,13 +515,14 @@ static int swap_points_list_off(MountPoint **head, bool *changed) { LIST_FOREACH_SAFE(mount_point, m, n, *head) { log_info("Deactivating swap %s.", m->path); - if (swapoff(m->path) == 0) { - *changed = true; - mount_point_free(head, m); - } else { + if (swapoff(m->path) < 0) { log_warning_errno(errno, "Could not deactivate swap %s: %m", m->path); n_failed++; + continue; } + + *changed = true; + mount_point_free(head, m); } return n_failed; @@ -551,15 +552,15 @@ static int loopback_points_list_detach(MountPoint **head, bool *changed, int umo log_info("Detaching loopback %s.", m->path); r = delete_loopback(m->path); - if (r >= 0) { - if (r > 0) - *changed = true; - - mount_point_free(head, m); - } else { - log_full_errno(umount_log_level, errno, "Could not detach loopback %s: %m", m->path); + if (r < 0) { + log_full_errno(umount_log_level, r, "Could not detach loopback %s: %m", m->path); n_failed++; + continue; } + if (r > 0) + *changed = true; + + mount_point_free(head, m); } return n_failed; @@ -584,23 +585,24 @@ static int dm_points_list_detach(MountPoint **head, bool *changed, int umount_lo continue; } - log_info("Detaching DM %u:%u.", major(m->devnum), minor(m->devnum)); + log_info("Detaching DM %s (%u:%u).", m->path, major(m->devnum), minor(m->devnum)); r = delete_dm(m->devnum); - if (r >= 0) { - *changed = true; - mount_point_free(head, m); - } else { - log_full_errno(umount_log_level, errno, "Could not detach DM %s: %m", m->path); + if (r < 0) { + log_full_errno(umount_log_level, r, "Could not detach DM %s: %m", m->path); n_failed++; + continue; } + + *changed = true; + mount_point_free(head, m); } return n_failed; } static int umount_all_once(bool *changed, int umount_log_level) { - int r; _cleanup_(mount_points_list_free) LIST_HEAD(MountPoint, mp_list_head); + int r; assert(changed); From 63135a2d8db0b89b226aad2838702a1813b761e3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 17:02:05 +0100 Subject: [PATCH 4/6] umount: detect root loopback device the same way as we detect root DM devices get_block_device() is just the nicer way to do it (since it also odes btrfs). Also, let's already collect the dev_t of the loopback device when we enumerate things, that allows us to do the checks simpler without constantly stat()ing things over and over again. --- src/shutdown/umount.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 51ed05cb6a..47d1a3d676 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -249,8 +249,10 @@ static int loopback_list_get(MountPoint **head) { _cleanup_free_ char *p = NULL; const char *dn; MountPoint *lb; + dev_t devnum; - if (sd_device_get_devname(d, &dn) < 0) + if (sd_device_get_devnum(d, &devnum) < 0 || + sd_device_get_devname(d, &dn) < 0) continue; p = strdup(dn); @@ -263,6 +265,7 @@ static int loopback_list_get(MountPoint **head) { *lb = (MountPoint) { .path = TAKE_PTR(p), + .devnum = devnum, }; LIST_PREPEND(mount_point, *head, lb); @@ -530,22 +533,16 @@ static int swap_points_list_off(MountPoint **head, bool *changed) { static int loopback_points_list_detach(MountPoint **head, bool *changed, int umount_log_level) { MountPoint *m, *n; - int n_failed = 0, k; - struct stat root_st; + int n_failed = 0, r; + dev_t rootdev = 0; assert(head); assert(changed); - k = lstat("/", &root_st); + (void) get_block_device("/", &rootdev); LIST_FOREACH_SAFE(mount_point, m, n, *head) { - int r; - struct stat loopback_st; - - if (k >= 0 && - major(root_st.st_dev) != 0 && - lstat(m->path, &loopback_st) >= 0 && - root_st.st_dev == loopback_st.st_rdev) { + if (major(rootdev) != 0 && rootdev == m->devnum) { n_failed++; continue; } @@ -569,17 +566,14 @@ static int loopback_points_list_detach(MountPoint **head, bool *changed, int umo static int dm_points_list_detach(MountPoint **head, bool *changed, int umount_log_level) { MountPoint *m, *n; int n_failed = 0, r; - dev_t rootdev; + dev_t rootdev = 0; assert(head); assert(changed); - r = get_block_device("/", &rootdev); - if (r <= 0) - rootdev = 0; + (void) get_block_device("/", &rootdev); LIST_FOREACH_SAFE(mount_point, m, n, *head) { - if (major(rootdev) != 0 && rootdev == m->devnum) { n_failed ++; continue; From b877c3b06f15a025748b9f09621ddf1bd00cacce Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 17:58:03 +0100 Subject: [PATCH 5/6] umount: check LO_FLAGS_AUTOCLEAR after LOOP_CLR_FD claimed success Fixes: #14410 Replaces: #14386 --- src/shutdown/umount.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 47d1a3d676..cc86f4180e 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -328,6 +328,7 @@ static int dm_list_get(MountPoint **head) { static int delete_loopback(const char *device) { _cleanup_close_ int fd = -1; + struct loop_info64 info; assert(device); @@ -335,14 +336,31 @@ static int delete_loopback(const char *device) { if (fd < 0) return errno == ENOENT ? 0 : -errno; - if (ioctl(fd, LOOP_CLR_FD, 0) >= 0) + if (ioctl(fd, LOOP_CLR_FD, 0) < 0) { + if (errno == ENXIO) /* Nothing bound, didn't do anything */ + return 0; + + return -errno; + } + + if (ioctl(fd, LOOP_GET_STATUS64, &info) < 0) { + /* If the LOOP_CLR_FD above succeeded we'll see ENXIO here. */ + if (errno == ENXIO) + log_debug("Successfully detached loopback device %s.", device); + else + log_debug_errno(errno, "Failed to invoke LOOP_GET_STATUS64 on loopback device %s, ignoring: %m", device); /* the LOOP_CLR_FD at least worked, let's hope for the best */ + return 1; + } - /* ENXIO: not bound, so no error */ - if (errno == ENXIO) - return 0; + /* Linux makes LOOP_CLR_FD succeed whenever LO_FLAGS_AUTOCLEAR is set without actually doing + * anything. Very confusing. Let's hence not claim we did anything in this case. */ + if (FLAGS_SET(info.lo_flags, LO_FLAGS_AUTOCLEAR)) + log_debug("Successfully called LOOP_CLR_FD on a loopback device %s with autoclear set, which is a NOP.", device); + else + log_debug("Weird, LOOP_CLR_FD succeeded but the device is still attached on %s.", device); - return -errno; + return -EBUSY; /* Nothing changed, the device is still attached, hence it apparently is still busy */ } static int delete_dm(dev_t devnum) { From 4ca8072fd6195c0d8e1c0da24db6b67a97070260 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 20 Dec 2019 18:37:24 +0100 Subject: [PATCH 6/6] umount: when we fail to detach a loopback device, set the auto-clear flag We might get lucky and this cleans up things later on automatically for us. --- src/shutdown/umount.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index cc86f4180e..8a5e80eeaa 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -340,7 +340,30 @@ static int delete_loopback(const char *device) { if (errno == ENXIO) /* Nothing bound, didn't do anything */ return 0; - return -errno; + if (errno != EBUSY) + return log_debug_errno(errno, "Failed to clear loopback device %s: %m", device); + + if (ioctl(fd, LOOP_GET_STATUS64, &info) < 0) { + if (errno == ENXIO) /* What? Suddenly detached after all? That's fine by us then. */ + return 1; + + log_debug_errno(errno, "Failed to invoke LOOP_GET_STATUS64 on loopback device %s, ignoring: %m", device); + return -EBUSY; /* propagate original error */ + } + + if (FLAGS_SET(info.lo_flags, LO_FLAGS_AUTOCLEAR)) /* someone else already set LO_FLAGS_AUTOCLEAR for us? fine by us */ + return -EBUSY; /* propagate original error */ + + info.lo_flags |= LO_FLAGS_AUTOCLEAR; + if (ioctl(fd, LOOP_SET_STATUS64, &info) < 0) { + if (errno == ENXIO) /* Suddenly detached after all? Fine by us */ + return 1; + + log_debug_errno(errno, "Failed to set LO_FLAGS_AUTOCLEAR flag for loop device %s, ignoring: %m", device); + } else + log_debug("Successfully set LO_FLAGS_AUTOCLEAR flag for loop device %s.", device); + + return -EBUSY; } if (ioctl(fd, LOOP_GET_STATUS64, &info) < 0) {