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

Re: [Issue 3596] 'hotcopy' of packed fsfs repos may corrupt target revprops.db

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Wed, 14 Apr 2010 23:52:19 +0300 (Jerusalem Daylight Time)

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
Received on 2010-04-14 22:52:07 CEST

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