Blair Zajac wrote:
> The multi-threaded RPC svn server we've been working on is getting core dumps
> using the BDB backend. I think our server is different than any of the other
> svn servers (mod_dav_svn, svnserve) in that we cache opened svn_fs_t,
> svn_fs_txn_t and other objects between RPC calls to save IO. The server can
> serve a second RPC request with a different thread that created the svn_fs_t and
> svn_fs_txn_t's, which is leading to the problem
>
> Using svn 1.5.1 with apr 1.2.2 we can get the following stack trace:
Here's the patch, which didn't come through.
[[[
In a multi-threaded environment using BDB, if one thread opens a repository,
saves the svn_fs_t, completes its work, and a second thread starts working with
the svn_fs_t, then the underlying bdb_env_baton_t's error_info points to the
first thread's bdb_error_info_t's. Additionally, if the second thread then
closes the BDB with svn_fs_bdb__close(), it'll free the error_info field that
the first thread is expecting to be valid.
Remove the error_info field from the bdb_env_baton_t and introduce a new
function for accessing it to ensure that the thread calling using the
bdb_error_info_t always works with its own copy.
* subversion/libsvn_fs_base/bdb/env.h
(bdb_env_baton_t):
Remove the error_info field.
(svn_fs_bdb__get_error_info):
New function to get a bdb_error_info_t from a bdb_env_baton_t.
* subversion/libsvn_fs_base/bdb/env.c
(svn_fs_bdb__get_error_info):
New function that wraps get_error_info() but takes a bdb_env_baton_t
instead of a bdb_env_t.
(convert_bdb_error):
No longer set the error_info field in the bdb_env_baton_t.
(svn_fs_bdb__close):
Use get_error_info() to get the bdb_error_info_t instead of
getting it directly from the structure.
* subversion/libsvn_fs_base/bdb/bdb-err.h
(SVN_BDB_ERR):
Use svn_fs_bdb__get_error_info() to get the bdb_error_info_t instead of
getting it directly from the structure.
* subversion/libsvn_fs_base/bdb/bdb-err.c
(svn_fs_bdb__dberr),
(svn_fs_bdb__dberrf),
(svn_fs_bdb__wrap_db):
Use svn_fs_bdb__get_error_info() to get the bdb_error_info_t instead of
getting it directly from the structure.
(create_env):
No longer set the error_info field in the bdb_error_info_t.
* subversion/libsvn_fs_base/fs.c
(base_bdb_set_errcall):
Use svn_fs_bdb__get_error_info() to get the bdb_error_info_t instead of
getting it directly from the structure.
Patch by: Michael Zhang <mzhang_at_imageworks.com>
Reviewed by: blair
]]]
Index: subversion/libsvn_fs_base/bdb/bdb-err.c
===================================================================
--- subversion/libsvn_fs_base/bdb/bdb-err.c (revision 34540)
+++ subversion/libsvn_fs_base/bdb/bdb-err.c (working copy)
@@ -51,10 +51,11 @@
svn_error_t *
svn_fs_bdb__dberr(bdb_env_baton_t *bdb_baton, int db_err)
{
+ bdb_error_info_t *error_info = svn_fs_bdb__get_error_info(bdb_baton);
svn_error_t *child_errors;
- child_errors = bdb_baton->error_info->pending_errors;
- bdb_baton->error_info->pending_errors = NULL;
+ child_errors = error_info->pending_errors;
+ error_info->pending_errors = NULL;
return svn_error_create(bdb_err_to_apr_err(db_err), child_errors,
db_strerror(db_err));
@@ -69,9 +70,10 @@
char *msg;
svn_error_t *err;
svn_error_t *child_errors;
+ bdb_error_info_t *error_info = svn_fs_bdb__get_error_info(bdb_baton);
- child_errors = bdb_baton->error_info->pending_errors;
- bdb_baton->error_info->pending_errors = NULL;
+ child_errors = error_info->pending_errors;
+ error_info->pending_errors = NULL;
err = svn_error_create(bdb_err_to_apr_err(db_err), child_errors, NULL);
@@ -90,8 +92,9 @@
if (! db_err)
{
- svn_error_clear(bfd->bdb->error_info->pending_errors);
- bfd->bdb->error_info->pending_errors = NULL;
+ bdb_error_info_t *error_info = svn_fs_bdb__get_error_info(bfd->bdb);
+ svn_error_clear(error_info->pending_errors);
+ error_info->pending_errors = NULL;
return SVN_NO_ERROR;
}
Index: subversion/libsvn_fs_base/bdb/bdb-err.h
===================================================================
--- subversion/libsvn_fs_base/bdb/bdb-err.h (revision 34540)
+++ subversion/libsvn_fs_base/bdb/bdb-err.h (working copy)
@@ -81,11 +81,13 @@
and call other functions that return a Berkeley DB error code. */
#define SVN_BDB_ERR(bdb, expr) \
do { \
+ bdb_error_info_t *error_info; \
int db_err__temp = (expr); \
if (db_err__temp) \
return svn_fs_bdb__dberr((bdb), db_err__temp); \
- svn_error_clear((bdb)->error_info->pending_errors); \
- (bdb)->error_info->pending_errors = NULL; \
+ error_info = svn_fs_bdb__get_error_info(bdb); \
+ svn_error_clear(error_info->pending_errors); \
+ error_info->pending_errors = NULL; \
} while (0)
Index: subversion/libsvn_fs_base/bdb/env.c
===================================================================
--- subversion/libsvn_fs_base/bdb/env.c (revision 34540)
+++ subversion/libsvn_fs_base/bdb/env.c (working copy)
@@ -180,6 +180,11 @@
#define get_error_info(bdb) (&(bdb)->error_info)
#endif /* APR_HAS_THREADS */
+bdb_error_info_t *
+svn_fs_bdb__get_error_info(bdb_env_baton_t *bdb_baton)
+{
+ return get_error_info(bdb_baton->bdb);
+}
/* Convert a BDB error to a Subversion error. */
static svn_error_t *
@@ -190,7 +195,6 @@
bdb_env_baton_t bdb_baton;
bdb_baton.env = bdb->env;
bdb_baton.bdb = bdb;
- bdb_baton.error_info = get_error_info(bdb);
SVN_BDB_ERR(&bdb_baton, db_err);
}
return SVN_NO_ERROR;
@@ -507,6 +511,7 @@
{
svn_error_t *err = SVN_NO_ERROR;
bdb_env_t *bdb = bdb_baton->bdb;
+ bdb_error_info_t *error_info = get_error_info(bdb);
SVN_ERR_ASSERT(bdb_baton->env == bdb_baton->bdb->env);
@@ -518,11 +523,11 @@
guard against potential races between this and the cleanup_env cleanup
callback. It's not clear if that can actually happen, but better safe
than sorry. */
- if (0 == --bdb_baton->error_info->refcount && bdb->pool)
+ if (0 == --error_info->refcount && bdb->pool)
{
- svn_error_clear(bdb_baton->error_info->pending_errors);
+ svn_error_clear(error_info->pending_errors);
#if APR_HAS_THREADS
- free(bdb_baton->error_info);
+ free(error_info);
apr_threadkey_private_set(NULL, bdb->error_info);
#endif
}
@@ -674,11 +679,13 @@
if (!err)
{
+ bdb_error_info_t *error_info;
+
*bdb_batonp = apr_palloc(pool, sizeof **bdb_batonp);
(*bdb_batonp)->env = bdb->env;
(*bdb_batonp)->bdb = bdb;
- (*bdb_batonp)->error_info = get_error_info(bdb);
- ++(*bdb_batonp)->error_info->refcount;
+ error_info = get_error_info(bdb);
+ ++error_info->refcount;
apr_pool_cleanup_register(pool, *bdb_batonp, cleanup_env_baton,
apr_pool_cleanup_null);
}
Index: subversion/libsvn_fs_base/bdb/env.h
===================================================================
--- subversion/libsvn_fs_base/bdb/env.h (revision 34540)
+++ subversion/libsvn_fs_base/bdb/env.h (working copy)
@@ -78,9 +78,6 @@
/* The (opaque) cached environment descriptor. */
bdb_env_t *bdb;
-
- /* The error info related to this baton. */
- bdb_error_info_t *error_info;
} bdb_env_baton_t;
@@ -140,6 +137,9 @@
/* Set the panic flag on the open BDB environment. */
void svn_fs_bdb__set_panic(bdb_env_baton_t *bdb_baton);
+/* Get the bdb_error_info_t for the open BDB environment specific for
+ the thread that is calling it. */
+bdb_error_info_t *svn_fs_bdb__get_error_info(bdb_env_baton_t *bdb_baton);
/* Remove the Berkeley DB environment at PATH.
*
Index: subversion/libsvn_fs_base/fs.c
===================================================================
--- subversion/libsvn_fs_base/fs.c (revision 34540)
+++ subversion/libsvn_fs_base/fs.c (working copy)
@@ -303,9 +303,10 @@
void (*db_errcall_fcn)(const char *errpfx, char *msg))
{
base_fs_data_t *bfd = fs->fsap_data;
+ bdb_error_info_t *error_info = svn_fs_bdb__get_error_info(bfd->bdb);
SVN_ERR(svn_fs__check_fs(fs, TRUE));
- bfd->bdb->error_info->user_callback = db_errcall_fcn;
+ error_info->user_callback = db_errcall_fcn;
return SVN_NO_ERROR;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=978945
Received on 2008-12-03 17:25:45 CET