[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Mon, 29 Dec 2014 15:46:02 +0300

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

This is an archived mail posted to the Subversion Dev mailing list.