You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2014/12/29 13:46:02 UTC
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Stefan Fuhrmann <st...@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
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Dec 30, 2014 at 5:01 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 30.12.2014 16:35, Ivan Zhakov wrote:
> > Just to make things clear. The thorough review was already provided.
> > In short, subpools are not a solution for the root cause of the
> > problem being discussed.
>
The last bit is where we disagree. I see more then one
issue here:
1. There is an L1 DAG node cache.
Problem: Added complexity.
2. It uses (now: used) ref counting to allow for copy-less access.
Problems: Lock management adds complexity. Binding lock
lifetime to pools triggered the memory consumption problem.
3. Poor isolation of API users from implementation details.
This is what allowed problem (2) to manifest outside FSFS.
4. Having single-pool API functions.
Problems: No clear distinction between data that needs to be
preserved (results) and temporaries. Attempts to tighten pool
usage will likely introduce bugs or break implicit / undocumented
assumptions. It is also unclear - without reading the docstrings
carefully - whether the caller expects a reference to e.g. something
in svn_fs_t or an object with independent lifetime.
Complexity in (1) is mitigated by having a narrow interface (few
callers) and is justified by various advantages over the main
membuffer cache.
Problems from (2) should be solved as of r1648538. While DAG
nodes are still heavy objects, they have shrunk and flattened
since the 1.7 times when this code got written. Copying them
is no longer much of an overhead.
Issue (3) still stands. A possible solution has been reverted in
r1648538.
Number (4) is a temporary problem and possibly caused the
crash reported in the original post.
Tightening pool usage is worthwhile because there is almost
no FS API function thatis inherently trivial. They have to walk
path hierarchies, read files, retry on alternative code paths etc.
This ties back into issue (3) where traces of that work leak to
the API caller in the form of used pool memory. All the caching
makes it feel like constant memory consumption but it is probably
possible to allocate a few MB with a seemingly harmless sequence
of FS calls.
It strikes me that the root problem is amazingly similar to the issue we
> had with pool lifetimes and locking in the implementation of DB_REGISTER
> support in the BDB back-end (that one was my bug). At the time, I
> believe we found that no possible pool usage pattern could avoid bad
> interaction with reference-counted objects in pools, given the way pools
> currently work in APR.
>
> This is going to be a problem with any long-lived cache that depends on
> pools that are created outside our control, as is the case with
> HTTPd/mod_dav_svn, where pool lifetimes in general are completely
> unpredictable. Combined with unpredictable request patterns and even
> less predictable data patterns (since repository content is completely
> outside our control), this makes pools amazingly unsuitable for
> cross-request caches. Even the lifetime of global pools is unpredictable
> because we don't control global pool destruction order.
>
> Given that our API is what it is, I can see no alternative to fixing the
> way the caches work. Reference counting only works when you have
> complete control over the lifetime of both the cache and the referrer.
>
I'm not familiar with the DB_REGISTER issue but
I assume that it somehow bound to or extended the
effective lifetime of an svn_fs_t. The DAG node cache
is different in that references to it never need to survive
past the boundary of any FS API call. They are merely
there to allow the equivalent of
fs_api_func()
{
/* Careful: requesting one node must not invalidate the
respective other. */
do_func(get_node("A", lock-it), get_node("B", lock-it));
/* nobody cares about A and B any more at this point. */
}
Local sub-pools would safely limit the lock lifetime and
solve the problem of leaking implementation details.
-- Stefan^2.
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Branko Čibej <br...@wandisco.com>.
On 30.12.2014 16:35, Ivan Zhakov wrote:
> Just to make things clear. The thorough review was already provided.
> In short, subpools are not a solution for the root cause of the
> problem being discussed.
It strikes me that the root problem is amazingly similar to the issue we
had with pool lifetimes and locking in the implementation of DB_REGISTER
support in the BDB back-end (that one was my bug). At the time, I
believe we found that no possible pool usage pattern could avoid bad
interaction with reference-counted objects in pools, given the way pools
currently work in APR.
This is going to be a problem with any long-lived cache that depends on
pools that are created outside our control, as is the case with
HTTPd/mod_dav_svn, where pool lifetimes in general are completely
unpredictable. Combined with unpredictable request patterns and even
less predictable data patterns (since repository content is completely
outside our control), this makes pools amazingly unsuitable for
cross-request caches. Even the lifetime of global pools is unpredictable
because we don't control global pool destruction order.
Given that our API is what it is, I can see no alternative to fixing the
way the caches work. Reference counting only works when you have
complete control over the lifetime of both the cache and the referrer.
-- Brane
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 30 December 2014 at 17:29, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> n Tue, Dec 30, 2014 at 2:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 29 December 2014 at 17:39, Stefan Fuhrmann
>> <st...@wandisco.com> wrote:
>> > On Mon, Dec 29, 2014 at 1:46 PM, Evgeny Kotkov
>> > <ev...@visualsvn.com>
>> > wrote:
>> >>
>> >> Stefan Fuhrmann <st...@apache.org> writes:
>> >>
>> [...]
>> >>
>> >> 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.
>> >
>> >
>> > I will keep the added sub-pools in place for now. The problems
>> > they cause now will always occur when we move code to the
>> > two-pool paradigm. The DAG cache issue is simply the trigger
>> > to tighten our pool usage in FSFS.
>> >
>> Stefan,
>>
>> Do I understand correctly that you're basically going to ignore this veto?
>
>
> No.
Thanks for reverting these changes!
> I had simply hoped that my explanations would give
> you or Evgeny pause to actually review the changes and
> find the spot where the wrong pool was used - something
> like the one that r1648272 fixed. If that could not be found
> within a few days, I had of course reverted the changes.
>
> But apparently no review is going to happen.
> Back to old bad normal in r1648532.
Just to make things clear. The thorough review was already provided.
In short, subpools are not a solution for the root cause of the
problem being discussed.
--
Ivan Zhakov
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
n Tue, Dec 30, 2014 at 2:03 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 29 December 2014 at 17:39, Stefan Fuhrmann
> <st...@wandisco.com> wrote:
> > On Mon, Dec 29, 2014 at 1:46 PM, Evgeny Kotkov <
> evgeny.kotkov@visualsvn.com>
> > wrote:
> >>
> >> Stefan Fuhrmann <st...@apache.org> writes:
> >>
> [...]
> >>
> >> 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.
> >
> >
> > I will keep the added sub-pools in place for now. The problems
> > they cause now will always occur when we move code to the
> > two-pool paradigm. The DAG cache issue is simply the trigger
> > to tighten our pool usage in FSFS.
> >
> Stefan,
>
> Do I understand correctly that you're basically going to ignore this veto?
>
No. I had simply hoped that my explanations would give
you or Evgeny pause to actually review the changes and
find the spot where the wrong pool was used - something
like the one that r1648272 fixed. If that could not be found
within a few days, I had of course reverted the changes.
But apparently no review is going to happen.
Back to old bad normal in r1648532.
-- Stefan^2.
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 29 December 2014 at 17:39, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> On Mon, Dec 29, 2014 at 1:46 PM, Evgeny Kotkov <ev...@visualsvn.com>
> wrote:
>>
>> Stefan Fuhrmann <st...@apache.org> writes:
>>
[...]
>>
>> 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.
>
>
> I will keep the added sub-pools in place for now. The problems
> they cause now will always occur when we move code to the
> two-pool paradigm. The DAG cache issue is simply the trigger
> to tighten our pool usage in FSFS.
>
Stefan,
Do I understand correctly that you're basically going to ignore this veto?
--
Ivan Zhakov
Re: svn commit: r1648253 - /subversion/trunk/subversion/libsvn_fs_fs/tree.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Dec 29, 2014 at 1:46 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Stefan Fuhrmann <st...@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.
> ]]]
>
That rule cannot be violated as it states an optional requirement.
It says "should", not "shall" or "must" - and for a good reason.
As they stand, the rules a incomplete and have some unintended
consequences (think recursion without loops). I plan on writing
a post on that topic tomorrow.
> Functions within this patch, like fs_change_node_prop(), allocate a
> constant
> amount of memory and do not require a subpool.
How do you know? Is there some expressed guarantee about
this in the code.
For the code at hand, you are also wrong. get_dag() needs at
least O(path length) memory in the general case. How much
memory will be needed at each directory level to read the
respective noderev is everyone's guess.
> 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.
>
That is true. Although I would not call it work-around. Is is
necessary to isolate API users from implementation details.
> It is the cache locking concept that *is broken*, not that we need
> subpools
> everywhere.
Demonstrate that reference counting is a broken concept.
You may not agree with its use here. You may also argue
that *not* always locking the cache is the broken element
of the design.
You might also demonstrate that it is / has become now
basically ineffective even in the high-load scenarios that it
is meant to help with.
> 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.
>
That is why isolating the implementation from the API
users is important - with or without cache locking. The
queries we need to run may look trivial but often involve
various file buffers and other "heavy" structures. Because
the user may call sequences of those seemingly simple
FS functions using the same pool, they may pile up.
Bottom line: cleaning up at the FS API boundary is a
idea in any case.
> 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?
>
Yes, that is a potential problem. Possible solutions include
* Making the DAG cache work without locking (see below).
* Make the FS API function implementations use sub-pools.
* Switch the FS API to the two-pool paradigm.
Ideally, we would do all of them because each one provides
benefits in its own rights. As of r1648272, the second point
should be fulfilled either by using explicit sub-pools or using
ordinary iterpools. I probably missed a few, so please, review.
The whole point of the refcounting DAG cache is that DAG
nodes are relatively heavy objects (due to the noderev struct
in them) while we usually need only a single element, e.g.
the location of the next directory level. So, this is what I will
try and report back as soon as the results are in:
* Split open_path into one producing the full parent path.
This one copies all DAG nodes from the cache along the path.
Remove the get_dag-related shortcuts from it.
* The other contains all the current short paths and only returns
the final node. Also as a copy of the cached DAG node if
it came from cache.
* Remove the cache locking code.
> 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:
>
This problem is most likely caused by some returned path
not being allocated in the result pool. You may set BUCKET_COUNT
in tree.c to 1. That should make it fail more consistently.
> 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.
>
I will keep the added sub-pools in place for now. The problems
they cause now will always occur when we move code to the
two-pool paradigm. The DAG cache issue is simply the trigger
to tighten our pool usage in FSFS.
-- Stefan^2.