You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stephan Kulow <co...@kde.org> on 2005/02/23 14:33:10 UTC
ugly problem found while trying to test KDE SVN
Hi!
I did a little test import of KDE CVS to SVN (thanks to #cvs2svn :),
but found a severe problem when trying to some things at the limit.
You can see the directory I'm commiting to at
https://svn.kde.org/viewcvs/trunk/kde-i18n/templates/
Mainly I did two test commits. One where I was the only one commiting
and that worked. But when I asked someone else to commit a little change
(somewhere in the repository) while I was commiting, I had this output of
svn:
Sending others/ksetiwatch.pot
Sending others/kssh.pot
Sending others/kwebget.pot
Sending www.kde.org/media.pot
Transmitting file data ..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................svn: Commit failed (details follow):
svn: Connection closed unexpectedly
svn: Your commit message was left in a temporary file:
Doing some strace on the server it became obvious, that the server was
simply running out of memory. Having had some bad experience with cvs,
we set a ulimit on our server to disallow (unintended) DoS attacks. But 128MB
are fine to do a commit, no?
Now before the server was aborted due the ulimit, it was acting like this in the
strace:
open("/home/kde/db/revs/330743", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 8452, [8452], SEEK_SET) = 0
read(4, "id: 1.0.r330743/8452\ntype: dir\np"..., 4096) = 627
close(4) = 0
open("/home/kde/db/revs/330742", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 7997, [7997], SEEK_SET) = 0
read(4, "id: 1.0.r330742/7997\ntype: dir\np"..., 4096) = 543
close(4) = 0
open("/home/kde/db/revs/330741", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 15693, [15693], SEEK_SET) = 0
read(4, "id: 1.0.r330741/15693\ntype: dir\n"..., 4096) = 636
close(4) = 0
open("/home/kde/db/revs/330740", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 19708, [19708], SEEK_SET) = 0
read(4, "id: 1.0.r330740/19708\ntype: dir\n"..., 4096) = 542
close(4) = 0
open("/home/kde/db/revs/330739", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 7760, [7760], SEEK_SET) = 0
read(4, "id: 1.0.r330739/7760\ntype: dir\np"..., 4096) = 546
close(4) = 0
open("/home/kde/db/revs/330738", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 10026, [10026], SEEK_SET) = 0
read(4, "id: 1.0.r330738/10026\ntype: dir\n"..., 4096) = 638
close(4) = 0
open("/home/kde/db/revs/330737", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 15292, [15292], SEEK_SET) = 0
read(4, "id: 1.0.r330737/15292\ntype: dir\n"..., 4096) = 633
close(4) = 0
open("/home/kde/db/revs/330736", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 80588, [80588], SEEK_SET) = 0
read(4, "id: 1.0.r330736/80588\ntype: dir\n"..., 4096) = 616
close(4) = 0
open("/home/kde/db/revs/330735", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 16008, [16008], SEEK_SET) = 0
read(4, "id: 1.0.r330735/16008\ntype: dir\n"..., 4096) = 696
close(4) = 0
open("/home/kde/db/revs/330734", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 11384, [11384], SEEK_SET) = 0
read(4, "id: 1.0.r330734/11384\ntype: dir\n"..., 4096) = 539
close(4) = 0
open("/home/kde/db/revs/330733", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 13222, [13222], SEEK_SET) = 0
read(4, "id: 1.0.r330733/13222\ntype: dir\n"..., 4096) = 551
close(4) = 0
open("/home/kde/db/revs/330732", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 13480, [13480], SEEK_SET) = 0
read(4, "id: 1.0.r330732/13480\ntype: dir\n"..., 4096) = 551
close(4) = 0
open("/home/kde/db/revs/330731", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 16106, [16106], SEEK_SET) = 0
read(4, "id: 1.0.r330731/16106\ntype: dir\n"..., 4096) = 536
close(4) = 0
open("/home/kde/db/revs/330730", O_RDONLY|O_LARGEFILE) = 4
_llseek(4, 26851, [26851], SEEK_SET) = 0
read(4, "id: 1.0.r330730/26851\ntype: dir\n"..., 4096) = 799
close(4) = 0
open("/home/kde/db/revs/330729", O_RDONLY|O_LARGEFILE) = 4
brk(0) = 0xe805000
brk(0xe827000) = 0xe827000
Basically it opened all revisions downwards starting from HEAD and seeked
something in there and (I asumme) puts it in some list eating up memory.
As you can find out on the viewcvs, it's not the case that every of these revisions
saw a change to /trunk/kde-i18n/templates, so from what I understood it shouldn't
open them, e.g. https://svn.kde.org/viewcvs?rev=330730&view=rev is pretty
unrelated.
Why is svnserve (version 1.1.3 btw) doing that? If this is happening whenever
two people commit at the same time, then I'm afraid this would make KDE moving
to subversion impossible ;(
Greetings, Stephan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Michael Matz wrote:
> Hi,
>
> On Wed, 23 Feb 2005, Stephan Kulow wrote:
>
>
>>Doing some strace on the server it became obvious, that the server was
>>simply running out of memory. Having had some bad experience with cvs,
>>we set a ulimit on our server to disallow (unintended) DoS attacks. But
>>128MB are fine to do a commit, no?
>>
>>Now before the server was aborted due the ulimit, it was acting like
>>this in the strace:
>>
>>open("/home/kde/db/revs/330743", O_RDONLY|O_LARGEFILE) = 4
>>_llseek(4, 8452, [8452], SEEK_SET) = 0
>>read(4, "id: 1.0.r330743/8452\ntype: dir\np"..., 4096) = 627
>>close(4) = 0
>
>
>>[... opening _all_ revs in reverse, even those not containing files
>> touched by the commit ...]
>
>
>>Why is svnserve (version 1.1.3 btw) doing that? If this is happening
>>whenever two people commit at the same time, then I'm afraid this would
>>make KDE moving to subversion impossible ;(
>
>
> It's 100% reproducable, so any idea anyone on this list? Anything we can
> try to help solving this problem (short of using bdb, and does it really
> not have this problem of opening all revs, even if it couldn't be
> noticed by a simple strace)? This is a showstopper for using svn as
> KDEs version management system, as concurrent commits to separate subtrees
> need to be possible.
>
> AFAIU the above shouldn't happen due to skip lists (or however you call
> them ;-)) and anyway should touch only revs affected by the commit, not
> all of them, but the strace shows that this is not the case.
Skip lists mean you don't need to apply all the deltas from each
revision, you still have to open the rev file in order to find the
dirents for directories in previous revisions, it has nothing to do with
the skip deltas used for the actual file contents.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by Michael Matz <ma...@suse.de>.
Hi,
On Wed, 23 Feb 2005, Stephan Kulow wrote:
> Doing some strace on the server it became obvious, that the server was
> simply running out of memory. Having had some bad experience with cvs,
> we set a ulimit on our server to disallow (unintended) DoS attacks. But
> 128MB are fine to do a commit, no?
>
> Now before the server was aborted due the ulimit, it was acting like
> this in the strace:
>
> open("/home/kde/db/revs/330743", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 8452, [8452], SEEK_SET) = 0
> read(4, "id: 1.0.r330743/8452\ntype: dir\np"..., 4096) = 627
> close(4) = 0
> [... opening _all_ revs in reverse, even those not containing files
> touched by the commit ...]
> Why is svnserve (version 1.1.3 btw) doing that? If this is happening
> whenever two people commit at the same time, then I'm afraid this would
> make KDE moving to subversion impossible ;(
It's 100% reproducable, so any idea anyone on this list? Anything we can
try to help solving this problem (short of using bdb, and does it really
not have this problem of opening all revs, even if it couldn't be
noticed by a simple strace)? This is a showstopper for using svn as
KDEs version management system, as concurrent commits to separate subtrees
need to be possible.
AFAIU the above shouldn't happen due to skip lists (or however you call
them ;-)) and anyway should touch only revs affected by the commit, not
all of them, but the strace shows that this is not the case.
Ciao,
Michael.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> It's one of those things I've come to just accept about my world, like
> perpetual war in the middle east.
So, you see the violence inherent in the system now?
> No objection. This problem has dogged the BDB back end for a long time.
>
> If we want to get even more radical, we could do away with the concept
> of trails completely, instead passing the FS, pool, and db_txn
> parameters explicitly to each function. The trail's "undo" field could
> be replaced with pool cleanups.
Indeed. We didn't realize it at the time, but pools already provided
what we invented trails to provide. Oh well...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2005-02-28 at 14:55, C. Michael Pilato wrote:
> Is it just me, or does the fact that so many of the libsvn_fs_base
> functions take trails and not pools (and just use trail->pool) kinda
> sickening?
It's one of those things I've come to just accept about my world, like
perpetual war in the middle east.
> Anybody opposed to my (or someone else's) gradually working to add
> pool parameter to everything that currently only takes a trail in that
> module?
No objection. This problem has dogged the BDB back end for a long time.
If we want to get even more radical, we could do away with the concept
of trails completely, instead passing the FS, pool, and db_txn
parameters explicitly to each function. The trail's "undo" field could
be replaced with pool cleanups.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> * The DAG code for performing the predecessor walk does not use
> subpools.
Is it just me, or does the fact that so many of the libsvn_fs_base
functions take trails and not pools (and just use trail->pool) kinda
sickening?
Anybody opposed to my (or someone else's) gradually working to add
pool parameter to everything that currently only takes a trail in that
module?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: ugly problem found while trying to test KDE SVN
Posted by Philip Martin <ph...@codematters.co.uk>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> @@ -238,29 +239,39 @@
> svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
> dag_node_t *this_node;
> svn_boolean_t done = FALSE;
> + apr_pool_t *last_iterpool, *new_iterpool;
>
> + last_iterpool = svn_pool_create (pool);
> this_node = node;
> while ((! done) && this_node)
> {
> node_revision_t *noderev;
>
> + new_iterpool = svn_pool_create (pool);
> +
> /* Get the node revision for THIS_NODE so we can examine its
> predecessor id. */
> - SVN_ERR (get_node_revision (&noderev, this_node, pool));
> + SVN_ERR (get_node_revision (&noderev, this_node, new_iterpool));
>
> + apr_pool_destroy (last_iterpool);
> +
> /* If THIS_NODE has a predecessor, replace THIS_NODE with the
> precessor, else set it to NULL. */
> if (noderev->predecessor_id)
> SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
> - noderev->predecessor_id, pool));
> + noderev->predecessor_id,
> + new_iterpool));
> else
> this_node = NULL;
>
> /* Now call the user-supplied callback with our predecessor
> node. */
> if (callback)
> - SVN_ERR (callback (baton, this_node, &done, pool));
> + SVN_ERR (callback (baton, this_node, &done, new_iterpool));
> +
> + last_iterpool = new_iterpool;
> }
> + apr_pool_destroy (last_iterpool);
>
> return SVN_NO_ERROR;
> }
Assumming _clear is "better" than _create/_destroy, how about:
last_iterpool = svn_pool_create (...);
iterpool = svn_pool_create (...);
while (...)
{
apr_pool_t *tmp_iterpool = last_iterpool;
last_iterpool = iterpool;
iterpool = tmp_iterpool;
svn_pool_clear (iterpool);
/* Allocations from iterpool will remain valid on the
next iteration. */
...
}
svn_pool_destroy (iterpool);
svn_pool_destroy (last_iterpool);
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: ugly problem found while trying to test KDE SVN
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Julian Foad <ju...@btopenworld.com> writes:
> Here, this_node is allocated in iterpool. On the next
> iteration of the loop, iterpool is cleared and then this_node
> is used (not just in the non-NULL test but in the call to
> get_node_revision). That looks wrong.
Good catch. I guess this survives my testing through luck; who
knows. I took inspiration from a construct i noted earlier in
fs_fs.c:get_combined_window, cycling through two pools for the
loop to fix this. Survives my testing, test suite is running
now. I'll commit when it's finished.
> That line appears to be redundant, as the pool is cleared a few
> lines further on, inside the next loop. Maybe you wanted to be
> sure it is cleared here as soon as the first loop has finished
> with it
Nope, it was just a mistake. Thanks.
Fix runaway memory usage when auto-merging a large tree. Before,
a simple cp of a tree of 70,000 files would die when running into
a 512 MB resource limit; now it uses only 8 MB.
* subversion/libsvn_fs_fs/dag.c
(svn_fs_fs__dag_walk_predecessors): Use an iterpool for the loop.
* subversion/libsvn_fs_fs/tree.c
(merge): Use an iterpool for the two loops.
=== subversion/libsvn_fs_fs/tree.c
==================================================================
--- subversion/libsvn_fs_fs/tree.c (/trunk) (revision 1924)
+++ subversion/libsvn_fs_fs/tree.c (/local/pool-cleanup) (revision 1924)
@@ -1300,6 +1300,7 @@
apr_hash_t *s_entries, *t_entries, *a_entries;
apr_hash_index_t *hi;
svn_fs_t *fs;
+ apr_pool_t *iterpool;
/* Make sure everyone comes from the same filesystem. */
fs = svn_fs_fs__dag_get_fs (ancestor);
@@ -1488,6 +1489,7 @@
a_entries = svn_fs_fs__copy_dir_entries (a_entries, pool);
/* for each entry E in a_entries... */
+ iterpool = svn_pool_create (pool);
for (hi = apr_hash_first (pool, a_entries);
hi;
hi = apr_hash_next (hi))
@@ -1498,6 +1500,8 @@
void *val;
apr_ssize_t klen;
+ svn_pool_clear (iterpool);
+
/* KEY will be the entry name in ancestor, VAL the dirent */
apr_hash_this (hi, &key, &klen, &val);
a_entry = val;
@@ -1532,13 +1536,13 @@
else
{
SVN_ERR (id_check_ancestor (&a_ancestorof_t, fs, a_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
if (a_ancestorof_t)
{
/* this is an &&, so we need both ancestor checks. */
SVN_ERR (id_check_ancestor (&t_ancestorof_s, fs,
t_entry->id, s_entry->id,
- pool));
+ iterpool));
if (t_ancestorof_s)
{
/* This is Case 1. */
@@ -1552,7 +1556,7 @@
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs,
s_entry->id, t_entry->id,
- pool));
+ iterpool));
if (! s_ancestorof_t)
{
/* This is Case 2. */
@@ -1575,7 +1579,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, t_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* or if target entry is different from both and
unrelated to source, and all three entries are
@@ -1588,11 +1592,11 @@
svn_node_kind_t s_kind, t_kind, a_kind;
SVN_ERR (svn_fs_fs__dag_get_node (&s_ent_node, fs,
- s_entry->id, pool));
+ s_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&t_ent_node, fs,
- t_entry->id, pool));
+ t_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&a_ent_node, fs,
- a_entry->id, pool));
+ a_entry->id, iterpool));
s_kind = svn_fs_fs__dag_node_kind (s_ent_node);
t_kind = svn_fs_fs__dag_node_kind (t_ent_node);
@@ -1605,19 +1609,19 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* ... just recurse. */
new_tpath = svn_path_join (target_path, t_entry->name,
- pool);
+ iterpool);
SVN_ERR (merge (conflict_p, new_tpath,
t_ent_node, s_ent_node, a_ent_node,
- txn_id, pool));
+ txn_id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_predecessor_count (&pred_count,
s_ent_node,
- pool));
+ iterpool));
/* If target is an immediate descendant of ancestor,
and source is also a descendant of ancestor, we
@@ -1625,7 +1629,7 @@
source. */
SVN_ERR (update_ancestry (fs, s_entry->id,
t_entry->id, txn_id,
- new_tpath, pred_count, pool));
+ new_tpath, pred_count, iterpool));
}
/* Else target entry has changed since ancestor entry,
but it changed either to source entry or to a
@@ -1643,7 +1647,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* Else if E did not change between ancestor and source,
@@ -1666,7 +1670,7 @@
_("Unexpected immutable node at '%s'"), target_path);
SVN_ERR (svn_fs_fs__dag_delete (target, t_entry->name,
- txn_id, pool));
+ txn_id, iterpool));
/* Seems cleanest to remove it from the target entries
hash now, even though no code would break if we
@@ -1688,7 +1692,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
}
else
{
@@ -1698,8 +1702,8 @@
this change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
t_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
}
}
/* E exists in neither target nor source */
@@ -1710,8 +1714,8 @@
for that change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
a_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
/* ### kff todo: what about the rename case? */
}
@@ -1734,6 +1738,8 @@
apr_ssize_t klen;
svn_boolean_t s_ancestorof_t = FALSE;
+ svn_pool_clear (iterpool);
+
apr_hash_this (hi, &key, &klen, &val);
s_entry = val;
t_entry = apr_hash_get (t_entries, key, klen);
@@ -1747,7 +1753,7 @@
if (t_entry)
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs, s_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
}
/* E does not exist in target */
@@ -1761,7 +1767,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, s_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* E exists in target but is different from E in source */
else if (! s_ancestorof_t)
@@ -1769,7 +1775,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
/* The remaining case would be: E exists in target and is
* same as in source. This implies a twin add, so target
@@ -1777,7 +1783,8 @@
*/
}
}
-
+ apr_pool_destroy (iterpool);
+
/* All entries in ancestor and source have been accounted for.
*
* Any entry E in target that does not exist in ancestor or source
=== subversion/libsvn_fs_fs/dag.c
==================================================================
--- subversion/libsvn_fs_fs/dag.c (/trunk) (revision 1924)
+++ subversion/libsvn_fs_fs/dag.c (/local/pool-cleanup) (revision 1924)
@@ -23,6 +23,7 @@
#include "svn_error.h"
#include "svn_fs.h"
#include "svn_props.h"
+#include "svn_pools.h"
#include "dag.h"
#include "err.h"
@@ -238,29 +239,39 @@
svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
dag_node_t *this_node;
svn_boolean_t done = FALSE;
+ apr_pool_t *last_iterpool, *new_iterpool;
+ last_iterpool = svn_pool_create (pool);
this_node = node;
while ((! done) && this_node)
{
node_revision_t *noderev;
+ new_iterpool = svn_pool_create (pool);
+
/* Get the node revision for THIS_NODE so we can examine its
predecessor id. */
- SVN_ERR (get_node_revision (&noderev, this_node, pool));
+ SVN_ERR (get_node_revision (&noderev, this_node, new_iterpool));
+ apr_pool_destroy (last_iterpool);
+
/* If THIS_NODE has a predecessor, replace THIS_NODE with the
precessor, else set it to NULL. */
if (noderev->predecessor_id)
SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
- noderev->predecessor_id, pool));
+ noderev->predecessor_id,
+ new_iterpool));
else
this_node = NULL;
/* Now call the user-supplied callback with our predecessor
node. */
if (callback)
- SVN_ERR (callback (baton, this_node, &done, pool));
+ SVN_ERR (callback (baton, this_node, &done, new_iterpool));
+
+ last_iterpool = new_iterpool;
}
+ apr_pool_destroy (last_iterpool);
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: ugly problem found while trying to test KDE SVN
Posted by Julian Foad <ju...@btopenworld.com>.
Eric Gillespie wrote:
> I've tested fairly extensively, and the test suite passes.
Excellent.
> If there are no objections, i'll commit it.
I'm no expert, but I have a concern...
> === subversion/libsvn_fs_fs/dag.c
> ==================================================================
> --- subversion/libsvn_fs_fs/dag.c (revision 1910)
> +++ subversion/libsvn_fs_fs/dag.c (local)
> @@ -23,6 +23,7 @@
> #include "svn_error.h"
> #include "svn_fs.h"
> #include "svn_props.h"
> +#include "svn_pools.h"
>
> #include "dag.h"
> #include "err.h"
> @@ -236,29 +237,34 @@
> svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
> dag_node_t *this_node;
> svn_boolean_t done = FALSE;
> + apr_pool_t *iterpool;
>
> + iterpool = svn_pool_create (pool);
> this_node = node;
> while ((! done) && this_node)
> {
> node_revision_t *noderev;
>
> + svn_pool_clear (iterpool);
> +
> /* Get the node revision for THIS_NODE so we can examine its
> predecessor id. */
> - SVN_ERR (get_node_revision (&noderev, this_node, pool));
> + SVN_ERR (get_node_revision (&noderev, this_node, iterpool));
>
> /* If THIS_NODE has a predecessor, replace THIS_NODE with the
> precessor, else set it to NULL. */
> if (noderev->predecessor_id)
> SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
> - noderev->predecessor_id, pool));
> + noderev->predecessor_id, iterpool));
Here, this_node is allocated in iterpool. On the next iteration of the loop,
iterpool is cleared and then this_node is used (not just in the non-NULL test
but in the call to get_node_revision). That looks wrong.
> else
> this_node = NULL;
>
> /* Now call the user-supplied callback with our predecessor
> node. */
> if (callback)
> - SVN_ERR (callback (baton, this_node, &done, pool));
> + SVN_ERR (callback (baton, this_node, &done, iterpool));
> }
> + apr_pool_destroy (iterpool);
>
> return SVN_NO_ERROR;
> }
> === subversion/libsvn_fs_fs/tree.c
> ==================================================================
> --- subversion/libsvn_fs_fs/tree.c (revision 1910)
> +++ subversion/libsvn_fs_fs/tree.c (local)
[...]
> @@ -1724,6 +1728,7 @@
> }
>
> /* For each entry E in source but not in ancestor */
> + svn_pool_clear (iterpool);
That line appears to be redundant, as the pool is cleared a few lines further
on, inside the next loop. Maybe you wanted to be sure it is cleared here as
soon as the first loop has finished with it, as an idiom to try to catch
accidental use of iterpool outside the loop. I'm not sure that that is
worthwhile, but if you think it is, then that's OK with me.
> for (hi = apr_hash_first (pool, s_entries);
> hi;
> hi = apr_hash_next (hi))
> @@ -1734,6 +1739,8 @@
> apr_ssize_t klen;
> svn_boolean_t s_ancestorof_t = FALSE;
>
> + svn_pool_clear (iterpool);
> +
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: ugly problem found while trying to test KDE SVN
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Greg Hudson <gh...@MIT.EDU> writes:
> I have looked into this problem after getting some more information
> from Stephan over IRC, and here are my findings:
Thanks for the excellent findings, Greg. Upon reading this
message, i realized this problem is probably the strange out of
memory problem our release engineers over here see when doing
weekly release branches.
This problem is what sent me on my last memory usage quest, and
the two resulting changes helped for a while, but the problem
eventually came back. Unfortunately, we were still dying in
purge_txn, which made no sense to me. I figured we must have
been coming *very* close to the resource limit somewhere else,
and then actually hitting the limit in purge_txn. I could never
figure out where, though.
With your findings, i finally knew how to reproduce the problem:
run two cp operations for our tree at the same time. Sure
enough, i can now reproduce it reliably. So, i iterpool-ified
tree.c:merge and dag.c:svn_fs_fs__dag_walk_predecessors, and the
problem is solved.
I've tested fairly extensively, and the test suite passes. If
there are no objections, i'll commit it.
Fix runaway memory usage when auto-merging a large tree.
* subversion/libsvn_fs_fs/dag.c
(svn_fs_fs__dag_walk_predecessors): Use an iterpool for the loop.
* subversion/libsvn_fs_fs/tree.c
(merge): Use an iterpool for the two loops.
=== subversion/libsvn_fs_fs/dag.c
==================================================================
--- subversion/libsvn_fs_fs/dag.c (revision 1910)
+++ subversion/libsvn_fs_fs/dag.c (local)
@@ -23,6 +23,7 @@
#include "svn_error.h"
#include "svn_fs.h"
#include "svn_props.h"
+#include "svn_pools.h"
#include "dag.h"
#include "err.h"
@@ -236,29 +237,34 @@
svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
dag_node_t *this_node;
svn_boolean_t done = FALSE;
+ apr_pool_t *iterpool;
+ iterpool = svn_pool_create (pool);
this_node = node;
while ((! done) && this_node)
{
node_revision_t *noderev;
+ svn_pool_clear (iterpool);
+
/* Get the node revision for THIS_NODE so we can examine its
predecessor id. */
- SVN_ERR (get_node_revision (&noderev, this_node, pool));
+ SVN_ERR (get_node_revision (&noderev, this_node, iterpool));
/* If THIS_NODE has a predecessor, replace THIS_NODE with the
precessor, else set it to NULL. */
if (noderev->predecessor_id)
SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
- noderev->predecessor_id, pool));
+ noderev->predecessor_id, iterpool));
else
this_node = NULL;
/* Now call the user-supplied callback with our predecessor
node. */
if (callback)
- SVN_ERR (callback (baton, this_node, &done, pool));
+ SVN_ERR (callback (baton, this_node, &done, iterpool));
}
+ apr_pool_destroy (iterpool);
return SVN_NO_ERROR;
}
=== subversion/libsvn_fs_fs/tree.c
==================================================================
--- subversion/libsvn_fs_fs/tree.c (revision 1910)
+++ subversion/libsvn_fs_fs/tree.c (local)
@@ -1300,6 +1300,7 @@
apr_hash_t *s_entries, *t_entries, *a_entries;
apr_hash_index_t *hi;
svn_fs_t *fs;
+ apr_pool_t *iterpool;
/* Make sure everyone comes from the same filesystem. */
fs = svn_fs_fs__dag_get_fs (ancestor);
@@ -1488,6 +1489,7 @@
a_entries = svn_fs_fs__copy_dir_entries (a_entries, pool);
/* for each entry E in a_entries... */
+ iterpool = svn_pool_create (pool);
for (hi = apr_hash_first (pool, a_entries);
hi;
hi = apr_hash_next (hi))
@@ -1498,6 +1500,8 @@
void *val;
apr_ssize_t klen;
+ svn_pool_clear (iterpool);
+
/* KEY will be the entry name in ancestor, VAL the dirent */
apr_hash_this (hi, &key, &klen, &val);
a_entry = val;
@@ -1532,13 +1536,13 @@
else
{
SVN_ERR (id_check_ancestor (&a_ancestorof_t, fs, a_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
if (a_ancestorof_t)
{
/* this is an &&, so we need both ancestor checks. */
SVN_ERR (id_check_ancestor (&t_ancestorof_s, fs,
t_entry->id, s_entry->id,
- pool));
+ iterpool));
if (t_ancestorof_s)
{
/* This is Case 1. */
@@ -1552,7 +1556,7 @@
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs,
s_entry->id, t_entry->id,
- pool));
+ iterpool));
if (! s_ancestorof_t)
{
/* This is Case 2. */
@@ -1575,7 +1579,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, t_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* or if target entry is different from both and
unrelated to source, and all three entries are
@@ -1588,11 +1592,11 @@
svn_node_kind_t s_kind, t_kind, a_kind;
SVN_ERR (svn_fs_fs__dag_get_node (&s_ent_node, fs,
- s_entry->id, pool));
+ s_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&t_ent_node, fs,
- t_entry->id, pool));
+ t_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&a_ent_node, fs,
- a_entry->id, pool));
+ a_entry->id, iterpool));
s_kind = svn_fs_fs__dag_node_kind (s_ent_node);
t_kind = svn_fs_fs__dag_node_kind (t_ent_node);
@@ -1605,19 +1609,19 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* ... just recurse. */
new_tpath = svn_path_join (target_path, t_entry->name,
- pool);
+ iterpool);
SVN_ERR (merge (conflict_p, new_tpath,
t_ent_node, s_ent_node, a_ent_node,
- txn_id, pool));
+ txn_id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_predecessor_count (&pred_count,
s_ent_node,
- pool));
+ iterpool));
/* If target is an immediate descendant of ancestor,
and source is also a descendant of ancestor, we
@@ -1625,7 +1629,7 @@
source. */
SVN_ERR (update_ancestry (fs, s_entry->id,
t_entry->id, txn_id,
- new_tpath, pred_count, pool));
+ new_tpath, pred_count, iterpool));
}
/* Else target entry has changed since ancestor entry,
but it changed either to source entry or to a
@@ -1643,7 +1647,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* Else if E did not change between ancestor and source,
@@ -1666,7 +1670,7 @@
"Unexpected immutable node at '%s'", target_path);
SVN_ERR (svn_fs_fs__dag_delete (target, t_entry->name,
- txn_id, pool));
+ txn_id, iterpool));
/* Seems cleanest to remove it from the target entries
hash now, even though no code would break if we
@@ -1688,7 +1692,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
}
else
{
@@ -1698,8 +1702,8 @@
this change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
t_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
}
}
/* E exists in neither target nor source */
@@ -1710,8 +1714,8 @@
for that change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
a_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
/* ### kff todo: what about the rename case? */
}
@@ -1724,6 +1728,7 @@
}
/* For each entry E in source but not in ancestor */
+ svn_pool_clear (iterpool);
for (hi = apr_hash_first (pool, s_entries);
hi;
hi = apr_hash_next (hi))
@@ -1734,6 +1739,8 @@
apr_ssize_t klen;
svn_boolean_t s_ancestorof_t = FALSE;
+ svn_pool_clear (iterpool);
+
apr_hash_this (hi, &key, &klen, &val);
s_entry = val;
t_entry = apr_hash_get (t_entries, key, klen);
@@ -1747,7 +1754,7 @@
if (t_entry)
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs, s_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
}
/* E does not exist in target */
@@ -1761,7 +1768,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, s_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* E exists in target but is different from E in source */
else if (! s_ancestorof_t)
@@ -1769,7 +1776,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
/* The remaining case would be: E exists in target and is
* same as in source. This implies a twin add, so target
@@ -1777,7 +1784,8 @@
*/
}
}
-
+ apr_pool_destroy (iterpool);
+
/* All entries in ancestor and source have been accounted for.
*
* Any entry E in target that does not exist in ancestor or source
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by Greg Hudson <gh...@MIT.EDU>.
> Doing some strace on the server it became obvious, that the server
> was simply running out of memory. Having had some bad experience
> with cvs, we set a ulimit on our server to disallow (unintended) DoS
> attacks. But 128MB are fine to do a commit, no?
I have looked into this problem after getting some more information
from Stephan over IRC, and here are my findings:
* The auto-merge code does not use subpools.
* The auto-merge code asks questions using id_check_ancestor(),
which once upon a time was a very cheap function, but now involves
a predecessor walk.
* In this case, it sounds like we're asking "is A an ancestor of B",
where the answer is "no", but B is the root directory or at least
a directory with many many past revs, so the ancestor walk is
extremely expensive. (I don't yet know exactly what leads to this
expensive ancestor walk, so I don't know how often it happens, but
clearly it's happening in this KDE test case.)
* The DAG code for performing the predecessor walk does not use
subpools.
* In summary, the auto-merge code was written a long time ago, and
no one has wanted to touch it since, and apparently it has scaled
very poorly ever since we switched to triplet-form IDs.
These findings apply to both back ends, not just FSFS, since this is
code FSFS stole wholesale from BDB and didn't have to touch. It's
possible that the problem is worse in FSFS, in that FSFS might take
longer to do a predecessor walk or might allocate more memory per
predecessor, or both. Or it might be worse in BDB. Hard to say.
Fixing this problem is well beyond the threshold of effort I'm
prepared to put in. I'll note that:
* Fixing the memory leaks should be pretty easy, but I don't think
we have many (or any?) test cases which cover this code, so
testing the fixes might be painful.
* Fixing the fundamental performance issue will require deep
analysis of an algorithm we've spent years trying very hard not to
look at.
* A possible workaround is to provide a repository or FS option to
disable auto-merges. That means large commits would be prone to
failure, unless we introduced a second option to write-lock the
repository starting from the beginning of a commit. (Easier than
Unfortunately, that would in turn lead to repositories getting
stuck because someone started a commit and then fell off the
network or otherwise failed to gracefully terminate it.
For reference, I'll include the stack trace from Stephan which led me
to the above conclusions. There's no debugging information, but it's
not really needed. All you need to know is that this is what the code
is doing while it's iterating over tens of thousands of rev files and
gobbling up memory.
#4 0x404dd193 in __read_nocancel () from /lib/tls/libpthread.so.0
#5 0x402330c3 in apr_file_read () from /usr/lib/libapr-0.so.0
#6 0x40233372 in apr_file_getc () from /usr/lib/libapr-0.so.0
#7 0x400fa0df in svn_io_file_getc () from /usr/lib/libsvn_subr-1.so.0
#8 0x400fa149 in svn_io_read_length_line () from /usr/lib/libsvn_subr-1.so.0
#9 0x400953b9 in read_header_block () from /usr/lib/libsvn_fs_fs-1.so.0
#10 0x40099940 in svn_fs_fs__get_node_revision () from /usr/lib/libsvn_fs_fs-1.so.0
#11 0x40091961 in get_node_revision () from /usr/lib/libsvn_fs_fs-1.so.0
#12 0x40091ca8 in svn_fs_fs__dag_get_node () from /usr/lib/libsvn_fs_fs-1.so.0
#13 0x40091da2 in svn_fs_fs__dag_walk_predecessors () from /usr/lib/libsvn_fs_fs-1.so.0
#14 0x40091eee in svn_fs_fs__dag_is_ancestor () from /usr/lib/libsvn_fs_fs-1.so.0
#15 0x4009d3b5 in id_check_ancestor () from /usr/lib/libsvn_fs_fs-1.so.0
#16 0x4009dbb6 in merge () from /usr/lib/libsvn_fs_fs-1.so.0
#17 0x4009dfb5 in merge_changes () from /usr/lib/libsvn_fs_fs-1.so.0
#18 0x4009e7a5 in svn_fs_fs__commit_txn () from /usr/lib/libsvn_fs_fs-1.so.0
#19 0x40089cb6 in svn_fs_commit_txn () from /usr/lib/libsvn_fs-1.so.0
#20 0x4007a7f8 in svn_repos_fs_commit_txn () from /usr/lib/libsvn_repos-1.so.0
#21 0x40075fcb in close_edit () from /usr/lib/libsvn_repos-1.so.0
#22 0x40026b49 in svn_client__do_commit () from /usr/lib/libsvn_client-1.so.0
#23 0x4002501f in svn_client_commit () from /usr/lib/libsvn_client-1.so.0
#24 0x0804cb1d in svn_cl__commit ()
#25 0x08050a4b in main ()
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by Stephan Kulow <co...@kde.org>.
Am Wednesday 23 February 2005 23:36 schrieb C. Michael Pilato:
> Could this be svnserve doing post-commit deltification or something?
> I'm just guessing.
post-commit would mean the client already got a success and starts to fetch
data, no? This was not the case.
Greetings, Stephan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: ugly problem found while trying to test KDE SVN
Posted by "C. Michael Pilato" <cm...@collab.net>.
Could this be svnserve doing post-commit deltification or something?
I'm just guessing.
Stephan Kulow <co...@kde.org> writes:
> Hi!
>
> I did a little test import of KDE CVS to SVN (thanks to #cvs2svn :),
> but found a severe problem when trying to some things at the limit.
>
> You can see the directory I'm commiting to at
> https://svn.kde.org/viewcvs/trunk/kde-i18n/templates/
>
> Mainly I did two test commits. One where I was the only one commiting
> and that worked. But when I asked someone else to commit a little change
> (somewhere in the repository) while I was commiting, I had this output of
> svn:
>
> Sending others/ksetiwatch.pot
> Sending others/kssh.pot
> Sending others/kwebget.pot
> Sending www.kde.org/media.pot
> Transmitting file data .....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................svn: Commit failed (details follow):
> svn: Connection closed unexpectedly
> svn: Your commit message was left in a temporary file:
>
> Doing some strace on the server it became obvious, that the server was
> simply running out of memory. Having had some bad experience with cvs,
> we set a ulimit on our server to disallow (unintended) DoS attacks. But 128MB
> are fine to do a commit, no?
>
> Now before the server was aborted due the ulimit, it was acting like this in the
> strace:
>
> open("/home/kde/db/revs/330743", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 8452, [8452], SEEK_SET) = 0
> read(4, "id: 1.0.r330743/8452\ntype: dir\np"..., 4096) = 627
> close(4) = 0
> open("/home/kde/db/revs/330742", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 7997, [7997], SEEK_SET) = 0
> read(4, "id: 1.0.r330742/7997\ntype: dir\np"..., 4096) = 543
> close(4) = 0
> open("/home/kde/db/revs/330741", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 15693, [15693], SEEK_SET) = 0
> read(4, "id: 1.0.r330741/15693\ntype: dir\n"..., 4096) = 636
> close(4) = 0
> open("/home/kde/db/revs/330740", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 19708, [19708], SEEK_SET) = 0
> read(4, "id: 1.0.r330740/19708\ntype: dir\n"..., 4096) = 542
> close(4) = 0
> open("/home/kde/db/revs/330739", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 7760, [7760], SEEK_SET) = 0
> read(4, "id: 1.0.r330739/7760\ntype: dir\np"..., 4096) = 546
> close(4) = 0
> open("/home/kde/db/revs/330738", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 10026, [10026], SEEK_SET) = 0
> read(4, "id: 1.0.r330738/10026\ntype: dir\n"..., 4096) = 638
> close(4) = 0
> open("/home/kde/db/revs/330737", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 15292, [15292], SEEK_SET) = 0
> read(4, "id: 1.0.r330737/15292\ntype: dir\n"..., 4096) = 633
> close(4) = 0
> open("/home/kde/db/revs/330736", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 80588, [80588], SEEK_SET) = 0
> read(4, "id: 1.0.r330736/80588\ntype: dir\n"..., 4096) = 616
> close(4) = 0
> open("/home/kde/db/revs/330735", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 16008, [16008], SEEK_SET) = 0
> read(4, "id: 1.0.r330735/16008\ntype: dir\n"..., 4096) = 696
> close(4) = 0
> open("/home/kde/db/revs/330734", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 11384, [11384], SEEK_SET) = 0
> read(4, "id: 1.0.r330734/11384\ntype: dir\n"..., 4096) = 539
> close(4) = 0
> open("/home/kde/db/revs/330733", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 13222, [13222], SEEK_SET) = 0
> read(4, "id: 1.0.r330733/13222\ntype: dir\n"..., 4096) = 551
> close(4) = 0
> open("/home/kde/db/revs/330732", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 13480, [13480], SEEK_SET) = 0
> read(4, "id: 1.0.r330732/13480\ntype: dir\n"..., 4096) = 551
> close(4) = 0
> open("/home/kde/db/revs/330731", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 16106, [16106], SEEK_SET) = 0
> read(4, "id: 1.0.r330731/16106\ntype: dir\n"..., 4096) = 536
> close(4) = 0
> open("/home/kde/db/revs/330730", O_RDONLY|O_LARGEFILE) = 4
> _llseek(4, 26851, [26851], SEEK_SET) = 0
> read(4, "id: 1.0.r330730/26851\ntype: dir\n"..., 4096) = 799
> close(4) = 0
> open("/home/kde/db/revs/330729", O_RDONLY|O_LARGEFILE) = 4
> brk(0) = 0xe805000
> brk(0xe827000) = 0xe827000
>
> Basically it opened all revisions downwards starting from HEAD and seeked
> something in there and (I asumme) puts it in some list eating up memory.
> As you can find out on the viewcvs, it's not the case that every of these revisions
> saw a change to /trunk/kde-i18n/templates, so from what I understood it shouldn't
> open them, e.g. https://svn.kde.org/viewcvs?rev=330730&view=rev is pretty
> unrelated.
>
> Why is svnserve (version 1.1.3 btw) doing that? If this is happening whenever
> two people commit at the same time, then I'm afraid this would make KDE moving
> to subversion impossible ;(
>
> Greetings, Stephan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org