> 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. > > Matthew Ahrens wrote: > 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.
I can't comment on the historical choice of using a constructor/destructor here. It predates my introdoction to ZFS. :-) Given that the majority of the structure must be touched after allocation anyway, it does seem better to just inialize the two remaining untouched fields then, rather than do all of them in the constructor. - Justin ----------------------------------------------------------- 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
