Stefan Fuhrmann <stef...@apache.org> writes: > In FSFS, use sub-pools for temporaries in those API function implementations > that call open_path(). > > All these functions are complex enough to make the overhead of creating a > temporary sub-pool be outweighed by the benefits of tighter memory usage and > better isolation from pool usage scheme. > > * subversion/libsvn_fs_fs/tree.c > (fs_change_node_prop, > fs_make_dir, > fs_delete_node, > fs_make_file): These functions don't return allocated memory, so use > the new sub-pool for all allocations. > (apply_textdelta): Allocate the node to be stored in the baton in the > baton's pool. That way, all remaining allocations > from POOL are temporaries. > (fs_apply_textdelta): Pass a temporary sub-pool to the above function. > (apply_text, > fs_apply_text): Same as for apply_textdelta and fs_apply_textdelta. > (fs_closest_copy): Use a temporary sub-pool for all queries that don't > produce the objects to return.
I have several concerns about this change (including the following-up r1648272, r1648243 and other related changes): 1) It violates the APR pool usage conventions, as per Subversion Community Guide [1]: [[[ Functions should not create/destroy pools for their operation; they should use a pool provided by the caller. Again, the caller knows more about how the function will be used, how often, how many times, etc. thus, it should be in charge of the function's memory usage. ]]] Functions within this patch, like fs_change_node_prop(), allocate a constant amount of memory and do not require a subpool. The real reason why we added these subpools is the DAG node cache happening to bind its lifetime to the passed-in pools, so this is a workaround, and it does not follow our set of established rules. It is the cache locking concept that *is broken*, not that we need subpools everywhere. If we choose to "fix" the problem this way, we will quickly find ourselves in a very dangerous situation. Miss one subpool — you're dead. Pass a potentially long-living DAV resource pool as a scratch pool for a one-time FSFS DAG operation — you're dead again. How should anyone know that all the calls to open_path() or get_dag() that happen to lock the DAG node cache now require a subpool? Should we constantly review all these places? 2) I am now witnessing random Apache HTTPD Server segfaults (Subversion 1.9.0-dev built from r1648272), and I suspect them to be a consequence of these recent pool-related changes. Here is a sample backtrace of the crash: libsvn_fs-1.dll!get_node_revision_body() libsvn_fs-1.dll!svn_fs_fs__dag_get_node() libsvn_fs-1.dll!open_path() libsvn_fs-1.dll!svn_fs_fs__node_id() libsvn_fs-1.dll!svn_fs_fs__check_path() mod_dav_svn.so!prep_regular() mod_dav_svn.so!get_resource() mod_dav.so!dav_get_resource() mod_dav.so!dav_method_get() ... Given the above, I am -1 on doing this. Please revert this change and other related changes that were supposed to fix the problem. [1] http://subversion.apache.org/docs/community-guide/conventions#apr-pools Regards, Evgeny Kotkov
