Merge pull request #20715 from yuwata/udev-node-follow-ups

udev-node: several follow-ups

src/udev/udev-node.c

@@ -272,14 +272,14 @@ static int update_timestamp(sd_device *dev, const char *path, struct stat *prev)
 
         /* Even if a symlink in the stack directory is created/removed, the mtime of the directory may
          * not be changed. Why? Let's consider the following situation. For simplicity, let's assume
-         * there exist three udev workers (A, B, and C) and all of them calls link_update() for the
+         * there exist two udev workers (A and B) and all of them calls link_update() for the same
-         * same devlink simultaneously.
+         * devlink simultaneously.
          *
-         * 1. B creates/removes a symlink in the stack directory.
+         * 1. A creates/removes a symlink in the stack directory.
          * 2. A calls the first stat() in the loop of link_update().
          * 3. A calls link_find_prioritized().
-         * 4. C creates/removes another symlink in the stack directory, so the result of the step 3 is outdated.
+         * 4. B creates/removes another symlink in the stack directory, so the result of the step 3 is outdated.
-         * 5. B and C finish link_update().
+         * 5. B finishes link_update().
          * 6. A creates/removes devlink according to the outdated result in the step 3.
          * 7. A calls the second stat() in the loop of link_update().
          *
@@ -334,25 +334,30 @@ static int update_stack_directory(sd_device *dev, const char *dirname, bool add)
                 return log_oom_debug();
 
         if (!add) {
-                bool unlink_failed = false;
+                int unlink_error = 0, stat_error = 0;
 
                 if (stat(dirname, &st) < 0) {
                         if (errno == ENOENT)
                                 return 0; /* The stack directory is already removed. That's OK. */
-                        log_device_debug_errno(dev, errno, "Failed to stat %s, ignoring: %m", dirname);
+                        stat_error = -errno;
                 }
 
-                if (unlink(filename) < 0) {
+                if (unlink(filename) < 0)
-                        unlink_failed = true;
+                        unlink_error = -errno;
-                        if (errno != ENOENT)
-                                log_device_debug_errno(dev, errno, "Failed to remove %s, ignoring: %m", filename);
-                }
 
                 if (rmdir(dirname) >= 0 || errno == ENOENT)
                         return 0;
 
-                if (unlink_failed)
+                if (unlink_error < 0) {
-                        return 0; /* If we failed to remove the symlink, there is almost nothing we can do. */
+                        if (unlink_error == -ENOENT)
+                                return 0;
+
+                        /* If we failed to remove the symlink, then there is almost nothing we can do. */
+                        return log_device_debug_errno(dev, unlink_error, "Failed to remove %s: %m", filename);
+                }
+
+                if (stat_error < 0)
+                        return log_device_debug_errno(dev, stat_error, "Failed to stat %s: %m", dirname);
 
                 /* The symlink was removed. Check if the timestamp of directory is changed. */
                 r = update_timestamp(dev, dirname, &st);

test/TEST-64-UDEV-STORAGE/test.sh

@@ -20,17 +20,19 @@ fi
 
 test_append_files() {
     (
-        instmods "=block"
+        instmods "=block" "=md" "=nvme" "=scsi"
-        instmods "=md"
-        instmods "=scsi"
-        instmods "=nvme"
         install_dmevent
         generate_module_dependencies
-        inst_binary lsblk
+        image_install lsblk wc
-        inst_binary wc
+
+        # Configure multipath
+        if command -v multipath && command -v multipathd; then
+            install_multipath
+        fi
 
         for i in {0..127}; do
             dd if=/dev/zero of="${TESTDIR:?}/disk$i.img" bs=1M count=1
+            echo "device$i" >"${TESTDIR:?}/disk$i.img"
         done
     )
 }
@@ -185,6 +187,65 @@ EOF
     QEMU_SMP=1 QEMU_TIMEOUT=60 test_run_one "${1:?}"
 }
 
+testcase_multipath_basic_failover() {
+    if ! command -v multipath || ! command -v multipathd; then
+        echo "Missing multipath tools, skipping the test..."
+        return 77
+    fi
+
+    local qemu_opts=("-device virtio-scsi-pci,id=scsi")
+    local partdisk="${TESTDIR:?}/multipathpartitioned.img"
+    local image lodev nback ndisk wwn
+
+    if [[ ! -e "$partdisk" ]]; then
+        dd if=/dev/zero of="$partdisk" bs=1M count=16
+        lodev="$(losetup --show -f -P "$partdisk")"
+        sfdisk "${lodev:?}" <<EOF
+label: gpt
+
+name="first_partition", size=5M
+uuid="deadbeef-dead-dead-beef-000000000000", name="failover_part", size=5M
+EOF
+        udevadm settle
+        mkfs.ext4 -U "deadbeef-dead-dead-beef-111111111111" -L "failover_vol" "${lodev}p2"
+        losetup -d "$lodev"
+    fi
+
+    # Add 64 multipath devices, each backed by 4 paths
+    for ndisk in {0..63}; do
+        wwn="0xDEADDEADBEEF$(printf "%.4d" "$ndisk")"
+        # Use a partitioned disk for the first device to test failover
+        [[ $ndisk -eq 0 ]] && image="$partdisk" || image="${TESTDIR:?}/disk$ndisk.img"
+
+        for nback in {0..3}; do
+            qemu_opts+=(
+                "-device scsi-hd,drive=drive${ndisk}x${nback},serial=MPIO$ndisk,wwn=$wwn"
+                "-drive format=raw,cache=unsafe,file=$image,file.locking=off,if=none,id=drive${ndisk}x${nback}"
+            )
+        done
+    done
+
+    KERNEL_APPEND="systemd.setenv=TEST_FUNCTION_NAME=${FUNCNAME[0]} ${USER_KERNEL_APPEND:-}"
+    QEMU_OPTIONS="${qemu_opts[*]} ${USER_QEMU_OPTIONS:-}"
+    test_run_one "${1:?}"
+}
+
+# Test case for issue https://github.com/systemd/systemd/issues/19946
+testcase_simultaneous_events() {
+    local qemu_opts=("-device virtio-scsi-pci,id=scsi")
+    local partdisk="${TESTDIR:?}/simultaneousevents.img"
+
+    dd if=/dev/zero of="$partdisk" bs=1M count=110
+    qemu_opts+=(
+        "-device scsi-hd,drive=drive1,serial=deadbeeftest"
+        "-drive format=raw,cache=unsafe,file=$partdisk,if=none,id=drive1"
+    )
+
+    KERNEL_APPEND="systemd.setenv=TEST_FUNCTION_NAME=${FUNCNAME[0]} ${USER_KERNEL_APPEND:-}"
+    QEMU_OPTIONS="${qemu_opts[*]} ${USER_QEMU_OPTIONS:-}"
+    test_run_one "${1:?}"
+}
+
 # Allow overriding which tests should be run from the "outside", useful for manual
 # testing (make -C test/... TESTCASES="testcase1 testcase2")
 if [[ -v "TESTCASES" && -n "$TESTCASES" ]]; then

test/test-functions

@@ -909,6 +909,32 @@ install_dmevent() {
     fi
 }
 
+install_multipath() {
+    instmods "=md" multipath
+    image_install kpartx /lib/udev/kpartx_id lsmod mpathpersist multipath multipathd partx
+    image_install "${ROOTLIBDIR:?}"/system/multipathd.{service,socket}
+    if get_bool "$LOOKS_LIKE_DEBIAN"; then
+        inst_rules 56-dm-parts.rules 56-dm-mpath.rules 60-multipath.rules 68-del-part-nodes.rules 95-kpartx.rules
+    else
+        inst_rules 11-dm-mpath.rules 11-dm-parts.rules 62-multipath.rules 66-kpartx.rules 68-del-part-nodes.rules
+    fi
+    mkdir -p "${initdir:?}/etc/multipath"
+
+    local file
+    while read -r file; do
+        # Install libraries required by the given library
+        inst_libs "$file"
+        # Install the library itself and create necessary symlinks
+        inst_library "$file"
+    done < <(find /lib*/multipath -type f)
+
+    if get_bool "$LOOKS_LIKE_ARCH"; then
+        # On Arch the multipath libraries are not linked against libgcc_s.so.1,
+        # but it's still required at runtime
+        inst_library "/lib64/libgcc_s.so.1"
+    fi
+}
+
 install_compiled_systemd() {
     dinfo "Install compiled systemd"
 

test/units/testsuite-64.sh

@@ -4,6 +4,28 @@
 set -eux
 set -o pipefail
 
+# Check if all symlinks under /dev/disk/ are valid
+helper_check_device_symlinks() {
+    local dev link target
+
+    while read -r link; do
+        target="$(readlink -f "$link")"
+        # Both checks should do virtually the same thing, but check both to be
+        # on the safe side
+        if [[ ! -e "$link" || ! -e "$target" ]]; then
+            echo >&2 "ERROR: symlink '$link' points to '$target' which doesn't exist"
+            return 1
+        fi
+
+        # Check if the symlink points to the correct device in /dev
+        dev="/dev/$(udevadm info -q name "$link")"
+        if [[ "$target" != "$dev" ]]; then
+            echo >&2 "ERROR: symlink '$link' points to '$target' but '$dev' was expected"
+            return 1
+        fi
+    done < <(find /dev/disk -type l)
+}
+
 testcase_megasas2_basic() {
     lsblk -S
     [[ "$(lsblk --scsi --noheadings | wc -l)" -ge 128 ]]
@@ -19,12 +41,150 @@ testcase_virtio_scsi_identically_named_partitions() {
     [[ "$(lsblk --noheadings -a -o NAME,PARTLABEL | grep -c "Hello world")" -eq $((16 * 8)) ]]
 }
 
+testcase_multipath_basic_failover() {
+    local dmpath i path wwid
+
+    # Configure multipath
+    cat >/etc/multipath.conf <<\EOF
+defaults {
+    # Use /dev/mapper/$WWN paths instead of /dev/mapper/mpathX
+    user_friendly_names no
+    find_multipaths yes
+    enable_foreign "^$"
+}
+
+blacklist_exceptions {
+    property "(SCSI_IDENT_|ID_WWN)"
+}
+
+blacklist {
+}
+EOF
+    modprobe -v dm_multipath
+    systemctl start multipathd.service
+    systemctl status multipathd.service
+    multipath -ll
+    ls -l /dev/disk/by-id/
+
+    for i in {0..63}; do
+        wwid="deaddeadbeef$(printf "%.4d" "$i")"
+        path="/dev/disk/by-id/wwn-0x$wwid"
+        dmpath="$(readlink -f "$path")"
+
+        lsblk "$path"
+        multipath -C "$dmpath"
+        # We should have 4 active paths for each multipath device
+        [[ "$(multipath -l "$path" | grep -c running)" -eq 4 ]]
+    done
+
+    # Test failover (with the first multipath device that has a partitioned disk)
+    echo "${FUNCNAME[0]}: test failover"
+    local device expected link mpoint part
+    local -a devices
+    mpoint="$(mktemp -d /mnt/mpathXXX)"
+    wwid="deaddeadbeef0000"
+    path="/dev/disk/by-id/wwn-0x$wwid"
+
+    # All following symlinks should exists and should be valid
+    local -a part_links=(
+        "/dev/disk/by-id/wwn-0x$wwid-part2"
+        "/dev/disk/by-partlabel/failover_part"
+        "/dev/disk/by-partuuid/deadbeef-dead-dead-beef-000000000000"
+        "/dev/disk/by-label/failover_vol"
+        "/dev/disk/by-uuid/deadbeef-dead-dead-beef-111111111111"
+    )
+    for link in "${part_links[@]}"; do
+        test -e "$link"
+    done
+
+    # Choose a random symlink to the failover data partition each time, for
+    # a better coverage
+    part="${part_links[$RANDOM % ${#part_links[@]}]}"
+
+    # Get all devices attached to a specific multipath device (in H:C:T:L format)
+    # and sort them in a random order, so we cut off different paths each time
+    mapfile -t devices < <(multipath -l "$path" | grep -Eo '[0-9]+:[0-9]+:[0-9]+:[0-9]+' | sort -R)
+    if [[ "${#devices[@]}" -ne 4 ]]; then
+        echo "Expected 4 devices attached to WWID=$wwid, got ${#devices[@]} instead"
+        return 1
+    fi
+    # Drop the last path from the array, since we want to leave at least one path active
+    unset "devices[3]"
+    # Mount the first multipath partition, write some data we can check later,
+    # and then disconnect the remaining paths one by one while checking if we
+    # can still read/write from the mount
+    mount -t ext4 "$part" "$mpoint"
+    expected=0
+    echo -n "$expected" >"$mpoint/test"
+    # Sanity check we actually wrote what we wanted
+    [[ "$(<"$mpoint/test")" == "$expected" ]]
+
+    for device in "${devices[@]}"; do
+        echo offline >"/sys/class/scsi_device/$device/device/state"
+        [[ "$(<"$mpoint/test")" == "$expected" ]]
+        expected="$((expected + 1))"
+        echo -n "$expected" >"$mpoint/test"
+
+        # Make sure all symlinks are still valid
+        for link in "${part_links[@]}"; do
+            test -e "$link"
+        done
+    done
+
+    multipath -l "$path"
+    # Three paths should be now marked as 'offline' and one as 'running'
+    [[ "$(multipath -l "$path" | grep -c offline)" -eq 3 ]]
+    [[ "$(multipath -l "$path" | grep -c running)" -eq 1 ]]
+
+    umount "$mpoint"
+    rm -fr "$mpoint"
+}
+
+testcase_simultaneous_events() {
+    local blockdev part partscript
+
+    blockdev="$(readlink -f /dev/disk/by-id/scsi-*_deadbeeftest)"
+    partscript="$(mktemp)"
+
+    if [[ ! -b "$blockdev" ]]; then
+        echo "ERROR: failed to find the test SCSI block device"
+        return 1
+    fi
+
+    cat >"$partscript" <<EOF
+$(printf 'name="test%d", size=2M\n' {1..50})
+EOF
+
+    # Initial partition table
+    sfdisk -q -X gpt "$blockdev" <"$partscript"
+
+    # Delete the partitions, immediatelly recreate them, wait for udev to settle
+    # down, and then check if we have any dangling symlinks in /dev/disk/. Rinse
+    # and repeat.
+    #
+    # On unpatched udev versions the delete-recreate cycle may trigger a race
+    # leading to dead symlinks in /dev/disk/
+    for i in {1..100}; do
+        sfdisk -q --delete "$blockdev"
+        sfdisk -q -X gpt "$blockdev" <"$partscript"
+
+        if ((i % 10 == 0)); then
+            udevadm settle
+            helper_check_device_symlinks
+        fi
+    done
+
+    rm -f "$partscript"
+}
+
 : >/failed
 
 udevadm settle
-
 lsblk -a
 
+echo "Check if all symlinks under /dev/disk/ are valid (pre-test)"
+helper_check_device_symlinks
+
 # TEST_FUNCTION_NAME is passed on the kernel command line via systemd.setenv=
 # in the respective test.sh file
 if ! command -v "${TEST_FUNCTION_NAME:?}"; then
@@ -35,6 +195,9 @@ fi
 echo "TEST_FUNCTION_NAME=$TEST_FUNCTION_NAME"
 "$TEST_FUNCTION_NAME"
 
+echo "Check if all symlinks under /dev/disk/ are valid (post-test)"
+helper_check_device_symlinks
+
 systemctl status systemd-udevd
 
 touch /testok