From 09373c1a50297079e6b0447ea97af4e9a60f77fa Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Oct 2024 14:43:45 +0900 Subject: [PATCH 1/4] udev-node: skip stack directory creation for diskseq The disk/by-diskseq symlink should not be shared with multiple block devices. Hence, it is not necessary to create stack directory for the symlink that manages which device owns the symlink. This is not just a optimization. If a service unit tries to mount a disk image but the service fails, then the diskseq of the loop device for the image may be continuously increased during restart, and inodes in /run may increase rapidly, as the stack directories are cleaned up only when udev queue is empty. Fixes #34637. --- src/udev/udev-node.c | 83 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index a4e73025a0..0b9ce5ed6a 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -423,6 +423,85 @@ static int node_get_current(const char *slink, int dirfd, char **ret_id, int *re return 0; } +static int link_update_diskseq(sd_device *dev, const char *slink, bool add) { + _cleanup_free_ char *buf = NULL; + const char *fname, *diskseq; + int r; + + assert(dev); + assert(slink); + + if (!device_in_subsystem(dev, "block")) + return 0; + + fname = path_startswith(slink, "/dev/disk/by-diskseq"); + if (isempty(fname)) + return 0; + + if (device_is_devtype(dev, "partition")) { + _cleanup_free_ char *suffix = NULL; + const char *partn, *p; + + /* Check if the symlink has an expected suffix "-part%n". See 60-persistent-storage.rules. */ + + r = sd_device_get_sysnum(dev, &partn); + if (r < 0) { + /* Cannot verify the symlink is owned by this device. Let's create the stack directory for the symlink. */ + log_device_debug_errno(dev, r, "Failed to get sysnum, but symlink '%s' is requested, ignoring: %m", slink); + return 0; + } + + suffix = strjoin("-part", partn); + if (!suffix) + return -ENOMEM; + + p = endswith(fname, suffix); + if (!p) { + log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested, proceeding anyway.", slink); + return 0; + } + + buf = strndup(fname, p - fname); + if (!buf) + return -ENOMEM; + + fname = buf; + } + + /* Check if the diskseq part of the symlink is in digits. */ + if (!in_charset(fname, DIGITS)) { + log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested, proceeding anyway.", slink); + return 0; /* unexpected by-diskseq symlink */ + } + + /* On removal, we cannot verify the diskseq. Skipping further check below. */ + if (!add) { + if (unlink(slink) < 0 && errno != ENOENT) + return log_device_debug_errno(dev, errno, "Failed to remove '%s': %m", slink); + + (void) rmdir_parents(slink, "/dev"); + return 1; /* done */ + } + + /* Check if the diskseq matches with the DISKSEQ property. */ + r = sd_device_get_property_value(dev, "DISKSEQ", &diskseq); + if (r < 0) { + log_device_debug_errno(dev, r, "Failed to get DISKSEQ property, but symlink '%s' is requested, ignoring: %m", slink); + return 0; + } + + if (!streq(fname, diskseq)) { + log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested (DISKSEQ=%s), proceeding anyway.", slink, diskseq); + return 0; + } + + r = node_symlink(dev, /* devnode = */ NULL, slink); + if (r < 0) + return r; + + return 1; /* done */ +} + static int link_update(sd_device *dev, const char *slink, bool add) { _cleanup_free_ char *current_id = NULL, *devnode = NULL; _cleanup_close_ int dirfd = -EBADF, lockfd = -EBADF; @@ -431,6 +510,10 @@ static int link_update(sd_device *dev, const char *slink, bool add) { assert(dev); assert(slink); + r = link_update_diskseq(dev, slink, add); + if (r != 0) + return r; + r = stack_directory_open(dev, slink, &dirfd, &lockfd); if (r < 0) return r; From 88d186e4829bc7ef4a4253fe2bf4857903bca830 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Oct 2024 15:39:36 +0900 Subject: [PATCH 2/4] test: add test case for issue #34637 --- test/units/TEST-17-UDEV.diskseq.sh | 57 ++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100755 test/units/TEST-17-UDEV.diskseq.sh diff --git a/test/units/TEST-17-UDEV.diskseq.sh b/test/units/TEST-17-UDEV.diskseq.sh new file mode 100755 index 0000000000..0cb717fbd0 --- /dev/null +++ b/test/units/TEST-17-UDEV.diskseq.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +# shellcheck disable=SC2010 +# shellcheck disable=SC2317 +set -ex +set -o pipefail + +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + +# This is a test case for issue #34637. + +at_exit() ( + set +e + + systemctl stop test-diskseq.service || : + rm -f /run/systemd/system/test-diskseq.service + systemctl daemon-reload + + [[ -d "$TMPDIR" ]] && rm -rf "$TMPDIR" + + udevadm control --log-level=info +) + +trap at_exit EXIT + +udevadm control --log-level=debug + +TMPDIR="$(mktemp -d)" +truncate -s 16M "$TMPDIR"/foo.raw +mkfs.ext4 -L foo "$TMPDIR"/foo.raw + +mkdir -p /run/systemd/system/ +cat >/run/systemd/system/test-diskseq.service < Date: Sun, 6 Oct 2024 14:39:46 +0900 Subject: [PATCH 3/4] udev-node: rename node_symlink() -> node_create_symlink() Also adds short comment for the NULL arguments. No functional change, just refactoring. --- src/udev/udev-node.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index 0b9ce5ed6a..f22b4e8e11 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -70,7 +70,7 @@ int udev_node_cleanup(void) { return 0; } -static int node_symlink(sd_device *dev, const char *devnode, const char *slink) { +static int node_create_symlink(sd_device *dev, const char *devnode, const char *slink) { struct stat st; int r; @@ -495,7 +495,7 @@ static int link_update_diskseq(sd_device *dev, const char *slink, bool add) { return 0; } - r = node_symlink(dev, /* devnode = */ NULL, slink); + r = node_create_symlink(dev, /* devnode = */ NULL, slink); if (r < 0) return r; @@ -557,7 +557,7 @@ static int link_update(sd_device *dev, const char *slink, bool add) { /* This device has the equal or a higher priority than the current. Let's * create the devlink to our device node. */ - return node_symlink(dev, NULL, slink); + return node_create_symlink(dev, /* devnode = */ NULL, slink); } } else { @@ -580,7 +580,7 @@ static int link_update(sd_device *dev, const char *slink, bool add) { if (r < 0) return log_device_debug_errno(dev, r, "Failed to determine device node with the highest priority for '%s': %m", slink); if (r > 0) - return node_symlink(dev, devnode, slink); + return node_create_symlink(dev, devnode, slink); log_device_debug(dev, "No reference left for '%s', removing", slink); @@ -644,7 +644,7 @@ int udev_node_update(sd_device *dev, sd_device *dev_old) { return log_device_debug_errno(dev, r, "Failed to get device path: %m"); /* always add /dev/{block,char}/$major:$minor */ - r = node_symlink(dev, NULL, filename); + r = node_create_symlink(dev, /* devnode = */ NULL, filename); if (r < 0) return log_device_warning_errno(dev, r, "Failed to create device symlink '%s': %m", filename); From 96067f7669393dccab1af827c987d0d429a3e384 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Oct 2024 14:41:23 +0900 Subject: [PATCH 4/4] udev-node: introduce node_remove_symlink() Also - make the failure propagated, - remove /dev/block or /dev/char if empty. Hopefully, no effective functionality changed. Just refactoring. --- src/udev/udev-node.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index f22b4e8e11..a0bf0a9619 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -70,6 +70,17 @@ int udev_node_cleanup(void) { return 0; } +static int node_remove_symlink(sd_device *dev, const char *slink) { + assert(dev); + assert(slink); + + if (unlink(slink) < 0 && errno != ENOENT) + return log_device_debug_errno(dev, errno, "Failed to remove '%s': %m", slink); + + (void) rmdir_parents(slink, "/dev"); + return 0; +} + static int node_create_symlink(sd_device *dev, const char *devnode, const char *slink) { struct stat st; int r; @@ -476,10 +487,10 @@ static int link_update_diskseq(sd_device *dev, const char *slink, bool add) { /* On removal, we cannot verify the diskseq. Skipping further check below. */ if (!add) { - if (unlink(slink) < 0 && errno != ENOENT) - return log_device_debug_errno(dev, errno, "Failed to remove '%s': %m", slink); + r = node_remove_symlink(dev, slink); + if (r < 0) + return r; - (void) rmdir_parents(slink, "/dev"); return 1; /* done */ } @@ -583,13 +594,7 @@ static int link_update(sd_device *dev, const char *slink, bool add) { return node_create_symlink(dev, devnode, slink); log_device_debug(dev, "No reference left for '%s', removing", slink); - - if (unlink(slink) < 0 && errno != ENOENT) - log_device_debug_errno(dev, errno, "Failed to remove '%s', ignoring: %m", slink); - - (void) rmdir_parents(slink, "/dev"); - - return 0; + return node_remove_symlink(dev, slink); } static int device_get_devpath_by_devnum(sd_device *dev, char **ret) { @@ -671,10 +676,7 @@ int udev_node_remove(sd_device *dev) { return log_device_debug_errno(dev, r, "Failed to get device path: %m"); /* remove /dev/{block,char}/$major:$minor */ - if (unlink(filename) < 0 && errno != ENOENT) - return log_device_debug_errno(dev, errno, "Failed to remove '%s': %m", filename); - - return 0; + return node_remove_symlink(dev, filename); } static int udev_node_apply_permissions_impl(