Jeff,
Seems reasonable to me. If we wanted to provide more information about the
vdev being removed then you could make a call to spa_lookup_by_guid(spa,
guid, B_TRUE) just prior to the call to spa_vdev_remove_aux(). You would
only do this is vd == NULL but that could be another way to enhance this
logic.
Thanks,
George
On Fri, Jun 10, 2016 at 5:14 PM, Alan Somers <asom...@freebsd.org> wrote:
> On Fri, Jun 10, 2016 at 2:53 PM, Josef 'Jeff' Sipek
> <jef...@josefsipek.net> wrote:
> > 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
>
> From inspection your proposal looks good to me, but I won't have time
> to test it until next week.
>
> -Alan
>
-------------------------------------------
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
