Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zed: Remove dead code from statechange-led.sh #11964

Closed
wants to merge 1 commit into from

Conversation

tonyhutter
Copy link
Contributor

Motivation and Context

Fixes: #11934

Description

statechange-led.sh calls process_pool() if either $ZEVENT_VDEV_ENC_SYSFS_PATH or $ZEVENT_VDEV_STATE_STR are not set by zed. That should never happen, and thus process_pool() will never get called. Furthermore, process_pool() is
totally broken (#11934). This patch removes process_pool().

How Has This Been Tested?

Ran zpool offline -f <tank> <vdev> and saw the fault LED go on. Ran zpool clear <pool> and saw fault LED go off.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

statechange-led.sh calls process_pool() if either $ZEVENT_VDEV_ENC_SYSFS_PATH
or $ZEVENT_VDEV_STATE_STR are not set by zed.  That should never happen,
and thus process_pool() will never get called.  Furthermore, process_pool() is
totally broken (openzfs#11934).  This patch removes process_pool().

Fixes: openzfs#11934
Signed-off-by: Tony Hutter <[email protected]>
@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Apr 28, 2021

Are you quite sure? This script is linked from pool_import-led.sh, vdev_attach-led.sh, and vdev_clear-led.sh, and these two

$ git grep ESC_ZFS_VDEV_ATTACH
include/sys/sysevent/eventdefs.h:#define        ESC_ZFS_VDEV_ATTACH             "vdev_attach"
module/zfs/spa.c:       spa_event_notify(spa, newvd, NULL, ESC_ZFS_VDEV_ATTACH);
$ git grep ESC_ZFS_VDEV_CLEAR
include/sys/fs/zfs.h: * ESC_ZFS_VDEV_CLEAR
include/sys/sysevent/eventdefs.h:#define        ESC_ZFS_VDEV_CLEAR              "vdev_clear"
module/zfs/vdev.c:              spa_event_notify(spa, vd, NULL, ESC_ZFS_VDEV_CLEAR);

pass the vdev, but

$ git grep ESC_ZFS_POOL_IMPORT
include/sys/sysevent/eventdefs.h:#define        ESC_ZFS_POOL_IMPORT             "pool_import"
module/zfs/spa.c:               spa_event_notify(spa, NULL, NULL, ESC_ZFS_POOL_IMPORT);
module/zfs/spa.c:       spa_event_notify(spa, NULL, NULL, ESC_ZFS_POOL_IMPORT);

does not.

By necessity, then, in zfs_event_create(), if (vd) {, and, hence,

VERIFY0(nvlist_add_uint64(resource,
    FM_EREPORT_PAYLOAD_ZFS_VDEV_STATE, vd->vdev_state));
// ...
if (vd->vdev_enc_sysfs_path != NULL)
	VERIFY0(nvlist_add_string(resource,
	    FM_EREPORT_PAYLOAD_ZFS_VDEV_ENC_SYSFS_PATH,
	    vd->vdev_enc_sysfs_path));

are only hit on those two events, as indicated in point 2. in the script header (I would comment there, but this proves impossible for GitHub).

So this would break LED updates on pool import (which I don't see a test for in the How Was This Tested sexion, oddly?). The proper testing procedure would be to export an OK pool, manually set the fault state on enclosures, then import it and validate the import event clears the faults.

@tonyhutter
Copy link
Contributor Author

@nabijaczleweli I forgot about pool_import-led.sh, thanks for pointing that out. Yea, you're probably right that it's not going to work. There is a zpool status -c fault_led that might help for those case. Let me look into that.

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Apr 28, 2021

That already does what the script in my patch in #11935 does, except without writing the path, which you'll find is rather inadequate.

@tonyhutter
Copy link
Contributor Author

Yep, closing in favor of: #11935

@tonyhutter tonyhutter closed this Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zed.d/pool_import-led.sh fallback path terminally broken
3 participants