You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2017/02/04 11:34:51 UTC
svn commit: r1781657 - in /subversion/trunk/subversion:
libsvn_fs_x/batch_fsync.c libsvn_fs_x/batch_fsync.h libsvn_fs_x/fs.c
tests/libsvn_fs_x/fs-x-pack-test.c
Author: stefan2
Date: Sat Feb 4 11:34:51 2017
New Revision: 1781657
URL: http://svn.apache.org/viewvc?rev=1781657&view=rev
Log:
Fix an initialization / life time issue with FSX's batch-fsync.
The svn_fs_x__init function may be called more than once and the FSX
module might later be unloaded and reloaded. So, make sure to create
the thread pool only once and to clean it up when the module unloads.
* subversion/libsvn_fs_x/batch_fsync.h
(svn_fs_x__batch_fsync_init): Add a owner pool parameter.
* subversion/libsvn_fs_x/batch_fsync.c
(thread_pool_initialized): Init flag to be used with svn_atomic__init_once.
(thread_pool_pre_cleanup): Cleanup only once but also make the batch-
fsync API re-initializable.
(create_thread_pool): Move the init code to here. Destroy the thread pool
whenever either the owning pool (i.e. during unload)
or the containing pool (apr_terminate) got cleaned up.
(svn_fs_x__batch_fsync_init): Run the actual initialization only once.
(svn_fs_x__batch_fsync_run): Error out on cases where the FS API init /
tear-down was not properly ordered with
the API usage.
* subversion/libsvn_fs_x/fs.c
(svn_fs_x__init): Update caller.
* subversion/tests/libsvn_fs_x/fs-x-pack-test.c
(test_batch_fsync): Update caller.
Modified:
subversion/trunk/subversion/libsvn_fs_x/batch_fsync.c
subversion/trunk/subversion/libsvn_fs_x/batch_fsync.h
subversion/trunk/subversion/libsvn_fs_x/fs.c
subversion/trunk/subversion/tests/libsvn_fs_x/fs-x-pack-test.c
Modified: subversion/trunk/subversion/libsvn_fs_x/batch_fsync.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/batch_fsync.c?rev=1781657&r1=1781656&r2=1781657&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/batch_fsync.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/batch_fsync.c Sat Feb 4 11:34:51 2017
@@ -29,6 +29,7 @@
#include "svn_dirent_uri.h"
#include "svn_private_config.h"
+#include "private/svn_atomic.h"
#include "private/svn_dep_compat.h"
#include "private/svn_mutex.h"
#include "private/svn_subr_private.h"
@@ -228,6 +229,9 @@ struct svn_fs_x__batch_fsync_t
/* Thread pool to execute the fsync tasks. */
static apr_thread_pool_t *thread_pool = NULL;
+/* Keep track on whether we already created the THREAD_POOL . */
+static svn_atomic_t thread_pool_initialized = FALSE;
+
#endif
/* We open non-directory files with these flags. */
@@ -236,22 +240,29 @@ static apr_thread_pool_t *thread_pool =
#if APR_HAS_THREADS
/* Destructor function that implicitly cleans up any running threads
- in the thread_pool given as DATA and releases their memory pools
- before they get destroyed themselves.
+ in the TRHEAD_POOL *once*.
Must be run as a pre-cleanup hook.
*/
static apr_status_t
thread_pool_pre_cleanup(void *data)
{
- apr_thread_pool_t *tp = data;
+ apr_thread_pool_t *tp = thread_pool;
+ if (!thread_pool)
+ return APR_SUCCESS;
+
+ thread_pool = NULL;
+ thread_pool_initialized = FALSE;
+
return apr_thread_pool_destroy(tp);
}
#endif
-svn_error_t *
-svn_fs_x__batch_fsync_init(void)
+/* Core implementation of svn_fs_x__batch_fsync_init. */
+static svn_error_t *
+create_thread_pool(void *baton,
+ apr_pool_t *owning_pool)
{
#if APR_HAS_THREADS
/* The thread-pool must be allocated from a thread-safe pool.
@@ -266,7 +277,8 @@ svn_fs_x__batch_fsync_init(void)
/* Work around an APR bug: The cleanup must happen in the pre-cleanup
hook instead of the normal cleanup hook. Otherwise, the sub-pools
containing the thread objects would already be invalid. */
- apr_pool_pre_cleanup_register(pool, thread_pool, thread_pool_pre_cleanup);
+ apr_pool_pre_cleanup_register(pool, NULL, thread_pool_pre_cleanup);
+ apr_pool_pre_cleanup_register(owning_pool, NULL, thread_pool_pre_cleanup);
/* let idle threads linger for a while in case more requests are
coming in */
@@ -280,6 +292,15 @@ svn_fs_x__batch_fsync_init(void)
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_fs_x__batch_fsync_init(apr_pool_t *owning_pool)
+{
+ /* Protect against multiple calls. */
+ return svn_error_trace(svn_atomic__init_once(&thread_pool_initialized,
+ create_thread_pool,
+ NULL, owning_pool));
+}
+
/* Destructor for svn_fs_x__batch_fsync_t. Releases all global pool memory
* and closes all open file handles. */
static apr_status_t
@@ -510,6 +531,10 @@ svn_fs_x__batch_fsync_run(svn_fs_x__batc
#if APR_HAS_THREADS
+ /* Forgot to call _init() or cleaned up the owning pool too early?
+ */
+ SVN_ERR_ASSERT(thread_pool);
+
/* If there are multiple fsyncs to perform, run them in parallel.
* Otherwise, skip the thread-pool and synchronization overhead. */
if (apr_hash_count(batch->files) > 1)
Modified: subversion/trunk/subversion/libsvn_fs_x/batch_fsync.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/batch_fsync.h?rev=1781657&r1=1781656&r2=1781657&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/batch_fsync.h (original)
+++ subversion/trunk/subversion/libsvn_fs_x/batch_fsync.h Sat Feb 4 11:34:51 2017
@@ -44,13 +44,14 @@
*/
typedef struct svn_fs_x__batch_fsync_t svn_fs_x__batch_fsync_t;
-/* Initialize the concurrent fsync infrastructure.
+/* Initialize the concurrent fsync infrastructure. Clean it up when
+ * OWNING_POOL gets cleared.
*
* This function must be called before using any of the other functions in
* in this module. It should only be called once.
*/
svn_error_t *
-svn_fs_x__batch_fsync_init(void);
+svn_fs_x__batch_fsync_init(apr_pool_t *owning_pool);
/* Set *RESULT_P to a new batch fsync structure, allocated in RESULT_POOL.
* If FLUSH_TO_DISK is not set, the resulting struct will not actually use
Modified: subversion/trunk/subversion/libsvn_fs_x/fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/fs.c?rev=1781657&r1=1781656&r2=1781657&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/fs.c Sat Feb 4 11:34:51 2017
@@ -665,7 +665,7 @@ svn_fs_x__init(const svn_version_t *load
loader_version->major);
SVN_ERR(svn_ver_check_list2(x_version(), checklist, svn_ver_equal));
- SVN_ERR(svn_fs_x__batch_fsync_init());
+ SVN_ERR(svn_fs_x__batch_fsync_init(common_pool));
*vtable = &library_vtable;
return SVN_NO_ERROR;
Modified: subversion/trunk/subversion/tests/libsvn_fs_x/fs-x-pack-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs_x/fs-x-pack-test.c?rev=1781657&r1=1781656&r2=1781657&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs_x/fs-x-pack-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs_x/fs-x-pack-test.c Sat Feb 4 11:34:51 2017
@@ -870,7 +870,7 @@ test_batch_fsync(const svn_test_opts_t *
/* Initialize infrastructure with a pool that lives as long as this
* application. */
- SVN_ERR(svn_fs_x__batch_fsync_init());
+ SVN_ERR(svn_fs_x__batch_fsync_init(pool));
/* We use and re-use the same batch object throughout this test. */
SVN_ERR(svn_fs_x__batch_fsync_create(&batch, TRUE, pool));