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

Re: [PATCH] BDB error_info threading issue causing core dumps

From: Blair Zajac <blair_at_orcaware.com>
Date: Wed, 03 Dec 2008 08:16:19 -0800

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

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