On Mon, Jun 13, 2016 at 10:48:14 -0400, George Wilson wrote:
> 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.
Amusingly enough, the reason I ended up looking at this is exactly because
we have that kind of lookup in Nexenta's gate. It turns out that using the
vd looked up with aux == B_TRUE leads to a use-after-free. :) Yeah, I could
try to stash things elsewhere (e.g., make a copy of the vd), but IMO it's
not really worth it.
Jeff.
>
> 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
> >
--
Bad pun of the week: The formula 1 control computer suffered from a race
condition
-------------------------------------------
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
