Philip Martin wrote on Wed, 14 Apr 2010 at 12:17 +0100:
> -svn_error_t *
> -svn_fs_fs__open_rep_cache(svn_fs_t *fs,
> - apr_pool_t *pool)
> +static svn_error_t*
> +open_rep_cache(fs_fs_data_t *ffd,
> + const char *fs_path,
> + apr_pool_t *fs_pool,
> + apr_pool_t *scratch_pool)
> {
> - fs_fs_data_t *ffd = fs->fsap_data;
> const char *db_path;
> int version;
>
> - /* Be idempotent. */
> + /* Be idempotent. ### This is not thread safe. */
> if (ffd->rep_cache_db)
> return SVN_NO_ERROR;
>
I suppose we could fix the thread-unsafety while we're here. Something like
this?
[[[
In FSFS, open the FS object's rep-cache atomically. Extend fs_fs_data_t
and svn_atomic__init_once() to support this.
(Yes, as it stands, this patch actually rev's a private API. This is
for presentational purposes; I'll do the "rev all callers" dance when
it's committed.)
Index: subversion/include/private/svn_atomic.h
===================================================================
--- subversion/include/private/svn_atomic.h (revision 934086)
+++ subversion/include/private/svn_atomic.h (working copy)
@@ -106,8 +106,20 @@ extern "C" {
* @a global_status must be a pointer to a global, zero-initialized
* #svn_atomic_t. @a init_func is a pointer to the function that performs
* the actual initialization, and @a pool is passed on to the init_func
- * for its use.
+ * for its use. @a baton will be passed to @a init_func. (In most uses,
+ * @a baton will be @c NULL.)
*
+ * @since New in 1.7.
+ */
+svn_error_t *
+svn_atomic__init_once2(volatile svn_atomic_t *global_status,
+ svn_error_t *(*init_func)(void*,apr_pool_t*),
+ void *baton,
+ apr_pool_t* pool);
+
+/**
+ * Similar to svn_atomic__init_once2(), but without a @a baton.
+ *
* @since New in 1.5.
*/
svn_error_t *
Index: subversion/libsvn_fs_fs/fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs.h (revision 934086)
+++ subversion/libsvn_fs_fs/fs.h (working copy)
@@ -30,6 +30,7 @@
#include "svn_fs.h"
#include "svn_config.h"
+#include "private/svn_atomic.h"
#include "private/svn_cache.h"
#include "private/svn_fs_private.h"
#include "private/svn_sqlite.h"
@@ -245,6 +246,9 @@ typedef struct
/* The sqlite database used for rep caching. */
svn_sqlite__db_t *rep_cache_db;
+ /* Thread-safe boolean */
+ svn_atomic_t rep_cache_db_opened;
+
/* The sqlite database used for revprops. */
svn_sqlite__db_t *revprop_db;
Index: subversion/libsvn_fs_fs/rep-cache.c
===================================================================
--- subversion/libsvn_fs_fs/rep-cache.c (revision 934086)
+++ subversion/libsvn_fs_fs/rep-cache.c (working copy)
@@ -38,18 +38,15 @@
REP_CACHE_DB_SQL_DECLARE_STATEMENTS(statements);
-svn_error_t *
-svn_fs_fs__open_rep_cache(svn_fs_t *fs,
- apr_pool_t *pool)
+static svn_error_t *
+open_rep_cache(void *baton,
+ apr_pool_t *pool)
{
+ svn_fs_t *fs = baton;
fs_fs_data_t *ffd = fs->fsap_data;
const char *db_path;
int version;
- /* Be idempotent. */
- if (ffd->rep_cache_db)
- return SVN_NO_ERROR;
-
/* Open (or create) the sqlite database. It will be automatically
closed when fs->pool is destoyed. */
db_path = svn_dirent_join(fs->path, REP_CACHE_DB_NAME, pool);
@@ -71,6 +68,15 @@ REP_CACHE_DB_SQL_DECLARE_STATEMENTS(statements);
}
svn_error_t *
+svn_fs_fs__open_rep_cache(svn_fs_t *fs,
+ apr_pool_t *pool)
+{
+ fs_fs_data_t *ffd = fs->fsap_data;
+ return svn_error_return(svn_atomic__init_once2(&ffd->rep_cache_db_opened,
+ open_rep_cache, fs, pool));
+}
+
+svn_error_t *
svn_fs_fs__get_rep_reference(representation_t **rep,
svn_fs_t *fs,
svn_checksum_t *checksum,
Index: subversion/libsvn_subr/atomic.c
===================================================================
--- subversion/libsvn_subr/atomic.c (revision 934086)
+++ subversion/libsvn_subr/atomic.c (working copy)
@@ -29,9 +29,11 @@
#define SVN_ATOMIC_INIT_FAILED 2
#define SVN_ATOMIC_INITIALIZED 3
-svn_error_t*
-svn_atomic__init_once(volatile svn_atomic_t *global_status,
- svn_error_t *(*init_func)(apr_pool_t*), apr_pool_t* pool)
+svn_error_t *
+svn_atomic__init_once2(volatile svn_atomic_t *global_status,
+ svn_error_t *(*init_func)(void*,apr_pool_t*),
+ void *baton,
+ apr_pool_t* pool)
{
/* !! Don't use localizable strings in this function, because these
!! might cause deadlocks. This function can be used to initialize
@@ -46,7 +48,7 @@
if (status == SVN_ATOMIC_UNINITIALIZED)
{
- svn_error_t *err = init_func(pool);
+ svn_error_t *err = init_func(baton, pool);
if (err)
{
#if APR_HAS_THREADS
@@ -81,3 +83,20 @@
return SVN_NO_ERROR;
}
+
+static svn_error_t *
+wrap_init_func(void *baton, apr_pool_t *pool)
+{
+ svn_error_t *(*wrapped_init_func)(apr_pool_t*) = baton;
+ return svn_error_return(wrapped_init_func(pool));
+}
+
+svn_error_t *
+svn_atomic__init_once(volatile svn_atomic_t *global_status,
+ svn_error_t *(*init_func)(apr_pool_t*), apr_pool_t* pool)
+{
+ return svn_error_return(
+ svn_atomic__init_once2(global_status,
+ wrap_init_func, init_func,
+ pool));
+}
]]]
Daniel
> /* Open (or create) the sqlite database. It will be automatically
> closed when fs->pool is destoyed. */
> - db_path = svn_dirent_join(fs->path, REP_CACHE_DB_NAME, pool);
> + db_path = svn_dirent_join(fs_path, REP_CACHE_DB_NAME, scratch_pool);
> SVN_ERR(svn_sqlite__open(&ffd->rep_cache_db, db_path,
> svn_sqlite__mode_rwcreate, statements,
> 0, NULL,
> - fs->pool, pool));
> + fs_pool, scratch_pool));
>
> - SVN_ERR(svn_sqlite__read_schema_version(&version, ffd->rep_cache_db,
> pool));
> + SVN_ERR(svn_sqlite__read_schema_version(&version, ffd->rep_cache_db,
> + scratch_pool));
> + /* ### This is not thread safe. */
> if (version < REP_CACHE_SCHEMA_FORMAT)
> {
> /* Must be 0 -- an uninitialized (no schema) database. Create
