From 100790a8dd7c7cbbcf35871d7361429e2ae09435 Mon Sep 17 00:00:00 2001 From: Christopher Voltz Date: Thu, 16 Feb 2017 15:41:48 -0600 Subject: [PATCH] Retry setting LED If the LED is being accessed by another process when we try to update it, the update will be lost. Add a retry loop which will read the state of the LED and update it until the LED is in the correct state. The number of times this will occur is limited to ensure that the ZEDlet won't hang ZED. Refactor to remove duplication so setting of the LED occurs in only one place. Cleanup a couple of the warnings generated by shellcheck which weren't the result of specific choices by the author. Several notes and warnings are still present but removing them would make the code less clear or require adding lines to tell shellcheck to ignore the warning. Remove ",i" from the documentation at the top of the file which appears to be a typographic error. Reviewed-by: Brian Behlendorf Reviewed-by: Tony Hutter Signed-off-by: Christopher Voltz Closes #5795 --- cmd/zed/zed.d/statechange-led.sh | 88 ++++++++++++-------------------- 1 file changed, 33 insertions(+), 55 deletions(-) diff --git a/cmd/zed/zed.d/statechange-led.sh b/cmd/zed/zed.d/statechange-led.sh index e2dc19b72a3d..7a8c1fde97b7 100755 --- a/cmd/zed/zed.d/statechange-led.sh +++ b/cmd/zed/zed.d/statechange-led.sh @@ -11,7 +11,7 @@ # only set the LED for that particular VDEV. This is the case for statechange # events and some vdev_* events. # -# 2. If those vars are not set, then check the state of all VDEVs in the pool,i +# 2. If those vars are not set, then check the state of all VDEVs in the pool # and set the LEDs accordingly. This is the case for pool_import events. # # Note that this script requires that your enclosure be supported by the @@ -41,31 +41,6 @@ zed_check_cmd "$ZPOOL" || exit 4 # Global used in set_led debug print vdev="" -# set_led (file) -# -# Write an enclosure sysfs file -# -# Arguments -# file: sysfs file to set (like /sys/class/enclosure/0:0:1:0/SLOT 10/fault) -# val: value to set it to -# -# Globals -# vdev: VDEV name for debug printing -# -# Return -# nothing -# -function set_led { - file="$1" - val="$2" - - # Set the value twice. I've seen enclosures that were - # flakey about setting it the first time. - echo "$val" > "$file" - echo "$val" > "$file" - zed_log_msg "vdev $vdev set '$file' LED to $val" -} - # check_and_set_led (file, val) # # Read an enclosure sysfs file, and write it if it's not already set to 'val' @@ -81,25 +56,34 @@ function check_and_set_led { file="$1" val="$2" - if [ ! -e "$ZEVENT_VDEV_ENC_SYSFS_PATH/fault" ] ; then + + if [ ! -e "$file" ] ; then return 3 fi - # We want to check the current state first, since writing to the - # 'fault' entry always always causes a SES command, even if the - # current state is already what you want. - current=$(cat "${file}") + # If another process is accessing the LED when we attempt to update it, + # the update will be lost so retry until the LED actually changes or we + # timeout. + for _ in $(seq 1 5); do + # We want to check the current state first, since writing to the + # 'fault' entry always always causes a SES command, even if the + # current state is already what you want. + current=$(cat "${file}") - # On some enclosures if you write 1 to fault, and read it back, - # it will return 2. Treat all non-zero values as 1 for - # simplicity. - if [ "$current" != "0" ] ; then - current=1 - fi + # On some enclosures if you write 1 to fault, and read it back, + # it will return 2. Treat all non-zero values as 1 for + # simplicity. + if [ "$current" != "0" ] ; then + current=1 + fi - if [ "$current" != "$val" ] ; then - set_led "$file" "$val" - fi + if [ "$current" != "$val" ] ; then + echo "$val" > "$file" + zed_log_msg "vdev $vdev set '$file' LED to $val" + else + break + fi + done } function state_to_val { @@ -130,7 +114,7 @@ function process_pool # Lookup all the current LED values and paths in parallel cmd='echo led_token=$(cat "$VDEV_ENC_SYSFS_PATH/fault"),"$VDEV_ENC_SYSFS_PATH",' - out=$($ZPOOL status -vc "$cmd" $pool | grep 'led_token=') + out=$($ZPOOL status -vc "$cmd" "$pool" | grep 'led_token=') while read -r vdev state read write chksum therest ; do # Read out current LED value and path @@ -142,29 +126,24 @@ function process_pool continue fi - # On some enclosures if you write 1 to fault, and read it back, - # it will return 2. Treat all non-zero values as 1 for - # simplicity. - if [ "$current_val" != "0" ] ; then - current_val=1 + if [ ! -e "$vdev_enc_sysfs_path/fault" ] ; then + rc=1 + zed_log_msg "vdev $vdev '$file/fault' doesn't exist" + continue; fi val=$(state_to_val "$state") if [ "$current_val" == "$val" ] ; then # LED is already set correctly - continue - fi - - if [ ! -e "$vdev_enc_sysfs_path/fault" ] ; then - rc=1 - zed_log_msg "vdev $vdev '$file/fault' doesn't exist" continue; fi - set_led "$vdev_enc_sysfs_path/fault" $val + check_and_set_led "$vdev_enc_sysfs_path/fault" "$val" + (( rc |= $? )) done <<< "$out" + if [ "$rc" == "0" ] ; then return 0 else @@ -176,10 +155,9 @@ function process_pool if [ ! -z "$ZEVENT_VDEV_ENC_SYSFS_PATH" ] && [ ! -z "$ZEVENT_VDEV_STATE_STR" ] ; then # Got a statechange for an individual VDEV val=$(state_to_val "$ZEVENT_VDEV_STATE_STR") - vdev="$(basename $ZEVENT_VDEV_PATH)" check_and_set_led "$ZEVENT_VDEV_ENC_SYSFS_PATH/fault" "$val" else # Process the entire pool - process_pool $(zed_guid_to_pool $ZEVENT_POOL_GUID) + process_pool "$(zed_guid_to_pool $ZEVENT_POOL_GUID)" fi