----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/154/#review448 -----------------------------------------------------------
In this case, I'm not sure why this is still a part of your kmem_cache's constructor. If you're using a kmem_cachem anything that a constructor sets, you have to reset when you call kmem_cache_free, otherwise you're not going to get the same object back at a subsequent allocation. The changes to the deconstructor make sense, because there's no reason to NULL out a field in a deconstructor, just to destroy things like the mutex. However, the contents of the constructor don't seem to be honored. The idea is that you should NULL out any necessary fields that your constructor sets before you call kmem_cache_free, not as part of the kmem_cache_alloc. This is especially important if anyone comes around this code later, as the constructor and deconstructor guarantee those invariants. Consider, for example, the sa_spill_tab member. In this case, you allocate the object in sa_handle_from_db(). It may have sa_spill_tab set in sa_build_index. However, neither of dmu_buf_init_user() or dmu_buf_set_user_ie() appear to touch the member. So you can end up doing a subsequent free just a few lines later in sa_handle_from_db() and violate your own constraints. It's a bit hard to tell if every member suffers from this problem or not. In this case, it seems like you're going to be better off if you assume that the constructor does nothing other than initialize the mutex and then always reset the fields to a known state on allocation. Alternatively, if you want to maintain the current constructor, then you really need to go to all of the call sites of kmem_cache_free to the sa_cache and make sure the invariants you declared in your constructor are honored. - Robert Mustacchi On Jan. 27, 2015, 12:01 a.m., Justin Gibbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/154/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2015, 12:01 a.m.) > > > Review request for OpenZFS Developer Mailing List and Rich Lowe. > > > Repository: illumos-gate > > > Description > ------- > > 5562 ZFS sa_handle's violate kmem invariants, debug kernels panic on boot > > uts/common/fs/zfs/sa.c: > In sa_cache_destructor(), remove unnecessary reset of > dbu_evict_func to NULL. > > In sa_handle_get_from_db(), reset dbu_evict_func to NULL > when the sa handle is allocated. > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/sa.c 2e3156c00a4de4304929ef9d578e4e7dbb9f5326 > > Diff: https://reviews.csiden.org/r/154/diff/ > > > Testing > ------- > > ztest via zloop on illumos > > > Thanks, > > Justin Gibbs > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
