> On Jan. 27, 2015, 12:28 a.m., Robert Mustacchi wrote: > > 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.
I agree that this is a problem -- if we lose the race in dmu_buf_set_user_ie(), we will free the handle with a non-NULL sa_spill_tab. But that is unrelated to the changes Justin has made (see https://reviews.csiden.org/r/131/diff/#). In this file he's just cleaned up some code and added the sa_dbu field. Otherwise this is unchanged since it was integrated in 2010. - Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/154/#review448 ----------------------------------------------------------- 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
