I've been looking at the code that 6922 introduced and to me it looks like
the change is too complicated/misleading and slightly buggy.
l2cache case
------------
At the beginning of the function, we try to convert a guid to a vdev_t
pointer by calling spa_lookup_by_guid with aux == B_FALSE. It is my
understanding that this will always result in vd == NULL if the guid is for
an L2ARC device. That means that when we make it into the sav_vdevs != NULL
else-if, the sole user of vd will always be NULL.
This brings up the next question. How useful are these events since they don't
include the removed vdev's guid and path?
spares case
-----------
This is similar to the l2cache case, but a bit more complicated. Unlike the
l2cache case, this case can fail with EBUSY. However, in the event of a
failure, the code still generates the event. This is easy to fix by moving
the event generation higher up.
Here, we certainly want to use vd, since vd can be either NULL or non-NULL.
It is NULL (and unspare is 0) in the case of 'zpool remove' of an unused
spare device, but non-NULL (and unspare is 1) in the case of 'zpool detach'
of a removed device that a spare took over from.
So, with all this said, unless I'm wrong about something, I'd like review
for the combined change:
diff --git a/usr/src/uts/common/fs/zfs/spa.c b/usr/src/uts/common/fs/zfs/spa.c
index 07abe37..0265887 100644
--- a/usr/src/uts/common/fs/zfs/spa.c
+++ b/usr/src/uts/common/fs/zfs/spa.c
@@ -5473,10 +5473,10 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t
unspare)
ZPOOL_CONFIG_SPARES, spares, nspares, nv);
spa_load_spares(spa);
spa->spa_spares.sav_sync = B_TRUE;
+ spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
} else {
error = SET_ERROR(EBUSY);
}
- spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
} else if (spa->spa_l2cache.sav_vdevs != NULL &&
nvlist_lookup_nvlist_array(spa->spa_l2cache.sav_config,
ZPOOL_CONFIG_L2CACHE, &l2cache, &nl2cache) == 0 &&
@@ -5488,7 +5488,7 @@ spa_vdev_remove(spa_t *spa, uint64_t guid, boolean_t
unspare)
ZPOOL_CONFIG_L2CACHE, l2cache, nl2cache, nv);
spa_load_l2cache(spa);
spa->spa_l2cache.sav_sync = B_TRUE;
- spa_event_notify(spa, vd, ESC_ZFS_VDEV_REMOVE_AUX);
+ spa_event_notify(spa, NULL, ESC_ZFS_VDEV_REMOVE_AUX);
} else if (vd != NULL && vd->vdev_islog) {
ASSERT(!locked);
ASSERT(vd == vd->vdev_top);
Thanks,
Jeff.
--
I already backed up the [server] once, I can do it again.
- a sysadmin threatening to do more frequent backups
-------------------------------------------
openzfs-developer
Archives: https://www.listbox.com/member/archive/274414/=now
RSS Feed: https://www.listbox.com/member/archive/rss/274414/28015062-cce53afa
Modify Your Subscription:
https://www.listbox.com/member/?member_id=28015062&id_secret=28015062-f966d51c
Powered by Listbox: http://www.listbox.com
