----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/235/#review771 -----------------------------------------------------------
usr/src/uts/common/fs/zfs/dsl_dir.c (line 185) <https://reviews.csiden.org/r/235/#comment582> Isn't there a missing call to dsl_prop_fini in the error paths here? Particularly in the case of taking an errout. While it's the case at the moment that all we're doing is a list initialization and therefore nothing will leak, this leaves future us subject to a leak if the structure for tracking this ever changes. usr/src/uts/common/fs/zfs/dsl_prop.c (line 473) <https://reviews.csiden.org/r/235/#comment581> 'Unlike operations on to update' probably wants to be 'Unlike operations to update' - Robert Mustacchi On Aug. 29, 2015, 2:33 a.m., Justin Gibbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/235/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2015, 2:33 a.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Bugs: 6171 > https://www.illumos.org/issues/6171 > > > Repository: illumos-gate > > > Description > ------- > > A change to a property on a dataset must be propagated to its descendants > in case that property is inherited. For datasets whose information is > not currently loaded into memory (e.g. a snapshot that isn't currently > mounted), there is nothing to do; the property change will take effect > the next time that dataset is loaded. To handle updates to datasets that > are in-core, ZFS registers a callback entry for each property of each > loaded dataset with the dsl directory that holds that dataset. There > is a dsl directory associated with each live dataset that references > both the live dataset and any snapshots of the live dataset. A property > change is effected by doing a traversal of the tree of dsl directories > for a pool, starting at the directory sourcing the change, and invoking > these callbacks. > > The current implementation both registers and de-registers properties > individually for each loaded dataset. While registration for a property is > O(1) (insert into a list), de-registration is O(n) (search list and then > remove). The 'n' for de-registration, however, is not limited to the size > (number of snapshots + 1) of the dsl directory. The eviction portion > of the life cycle for the in core state of datasets is asynchronous, > which allows multiple copies of the dataset information to be in-core > at once. Only one of these copies is active at any time with the rest > going through tear down processing, but all copies contribute to the > cost of performing a dsl_prop_unregister(). > > One way to create multiple, in-flight copies of dataset information > is by performing "zfs list" operations from multiple threads > concurrently. In-core dataset information is loaded on demand and then > evicted when reference counts drops to zero. For datasets that are not > mounted, there is no persistent reference count to keep them resident. > So, a list operation will load them, compute the information required to > do the list operation, and then evict them. When performing this operation > from multiple threads it is possible that some of the in-core dataset > information will be reused, but also possible to lose the race and load > the dataset again, even while the same information is being torn down. > > Compounding the performance issue further is a change made for illumos > issue 5056 which made dataset eviction single threaded. In environments > using automation to manage ZFS datasets, it is now possible to create > enough of a backlog of dataset evictions to consume excessive amounts > of kernel memory and to bog down the system. > > The fix employed here is to make property de-registration O(1). With this > change in place, it is hoped that a single thread is more than sufficient > to handle eviction processing. If it isn't, the problem can be solved > by increasing the number of threads devoted to the eviction taskq. > > uts/common/fs/zfs/dsl_dataset.c > uts/common/fs/zfs/dsl_dir.c: > uts/common/fs/zfs/dsl_prop.c: > uts/common/fs/zfs/sys/dsl_dataset.h: > uts/common/fs/zfs/sys/dsl_dir.h: > uts/common/fs/zfs/sys/dsl_prop.h: > Associate dsl property callback records with both the > dsl directory and the dsl dataset that is registering the > callback. Both connections are protected by the dsl directory's > "dd_lock". > > When linking callbacks into a dsl directory, group them by > the property type. This helps reduce the space penalty for the > double association (the property name pointer is stored once > per dsl_dir instead of in each record) and reduces the number of > strcmp() calls required to do callback processing when updating > a single property. Property types are stored in a linked list > since currently ZFS registers a maximum of 10 property types > for each dataset. > > Note that the property buckets/records associated with a dsl > directory are created on demand, but only freed when the dsl > directory is freed. Given the static nature of property types > and their small number, there is no benefit to freeing the few > bytes of memory used to represent the property record earlier. > When a property record becomes empty, the dsl directory is either > going to become unreferenced a little later in this thread of > execution, or there is a high chance that another dataset is > going to be loaded that would recreate the bucket anyway. > > Replace dsl_prop_unregister() with dsl_prop_unregister_all(). > All callers of dsl_prop_unregister() are trying to remove > all property registrations for a given dsl dataset anyway. By > changing the API, we can avoid doing any lookups of callbacks > by property type and just traverse the list of all callbacks > for the dataset and free each one. > > uts/common/fs/zfs/dmu_objset.c: > uts/common/fs/zfs/zfs_vfsops.c: > Replace use of dsl_prop_unregister() with the new > dsl_prop_unregister_all() API. > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/sys/dsl_prop.h > 5fe18d6a7c550b621e9df0ff8458b703c6f1bcf3 > usr/src/uts/common/fs/zfs/sys/dsl_dir.h > f50014d95a8109e6d715371eee0af641e92730ae > usr/src/uts/common/fs/zfs/sys/dsl_dataset.h > 001bff566a888bd9ab1d9c0bc7a5243ae8ca4cb6 > usr/src/uts/common/fs/zfs/zfs_vfsops.c > 3559b9b7b32db88326f06faa42c83907ab1344e6 > usr/src/uts/common/fs/zfs/dsl_prop.c > 09eec21604f45aacecd6f87397954d50492a36e0 > usr/src/uts/common/fs/zfs/dsl_dir.c > 6a20ab3ca2adabddc81c6199374ed99e7ec920fb > usr/src/uts/common/fs/zfs/dsl_dataset.c > 7f63cd97bcde4ba4a7708c1311c89ed93c8a8cfa > usr/src/uts/common/fs/zfs/dmu_objset.c > 9ad18f0d11fbaa1edb14ac5f2adfd215cb4716b3 > > Diff: https://reviews.csiden.org/r/235/diff/ > > > Testing > ------- > > Ztest on illumos. > > > Thanks, > > Justin Gibbs > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
