Stefan Fuhrmann <stefan2_at_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
Received on 2014-12-29 13:55:14 CET