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 2012/10/31 13:27:29 UTC
svn commit: r1404112 - in /subversion/trunk/subversion:
include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c
libsvn_subr/named_atomic.c tests/cmdline/svnadmin_tests.py
tests/libsvn_subr/named_atomic-test.c
Author: stefan2
Date: Wed Oct 31 12:27:29 2012
New Revision: 1404112
URL: http://svn.apache.org/viewvc?rev=1404112&view=rev
Log:
Change the way we implement shared memory setup for our named atomics.
Instead of using APR's SHM API, we create a persistent file and simply
mmap it.
The only downside to this is that the SHM file does not get cleaned up
automatically anymore. Therefore, we need to update tests and hotcopy
code.
The underlying issue has been analyzed in this thread:
http://svn.haxx.se/dev/archive-2012-10/0423.shtml
* subversion/include/private/svn_named_atomic.h
(svn_atomic_namespace__cleanup): declare new API
* subversion/libsvn_subr/named_atomic.c
(): update global docstring
(svn_atomic_namespace__create): create a persistent file; mmap it
(svn_atomic_namespace__cleanup): implement new API
* subversion/libsvn_fs_fs/fs_fs.c
(cleanup_revprop_namespace): new utility function
(hotcopy_body): fix comment; remove temp atomics files
* subversion/tests/cmdline/svnadmin_tests.py
(check_hotcopy_fsfs): don't compare temp files related to atomics
* subversion/tests/libsvn_subr/named_atomic-test.c
(cleanup_test_shm): new cleanup function
test_basics,
test_bignums,
test_multiple_atomics,
test_namespaces,
test_multithreaded,
test_multiprocess): call the new function
Modified:
subversion/trunk/subversion/include/private/svn_named_atomic.h
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_subr/named_atomic.c
subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c
Modified: subversion/trunk/subversion/include/private/svn_named_atomic.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_named_atomic.h?rev=1404112&r1=1404111&r2=1404112&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_named_atomic.h (original)
+++ subversion/trunk/subversion/include/private/svn_named_atomic.h Wed Oct 31 12:27:29 2012
@@ -83,6 +83,17 @@ svn_atomic_namespace__create(svn_atomic_
const char *name,
apr_pool_t *result_pool);
+/** Removes persistent data structures (files in particular) that got
+ * created for the namespace given by @a name. Use @a pool for temporary
+ * allocations.
+ *
+ * @note You must not call this while the respective namespace is still
+ * in use. Calling this multiple times for the same namespace is safe.
+ */
+svn_error_t *
+svn_atomic_namespace__cleanup(const char *name,
+ apr_pool_t *pool);
+
/** Find the atomic with the specified @a name in namespace @a ns and
* return it in @a *atomic. If no object with that name can be found, the
* behavior depends on @a auto_create. If it is @c FALSE, @a *atomic will
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1404112&r1=1404111&r2=1404112&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 31 12:27:29 2012
@@ -3160,6 +3160,16 @@ ensure_revprop_namespace(svn_fs_t *fs)
: SVN_NO_ERROR;
}
+/* Make sure the revprop_namespace member in FS is set. */
+static svn_error_t *
+cleanup_revprop_namespace(svn_fs_t *fs)
+{
+ const char* name = svn_dirent_join(fs->path,
+ ATOMIC_REVPROP_NAMESPACE,
+ fs->pool);
+ return svn_error_trace(svn_atomic_namespace__cleanup(name, fs->pool));
+}
+
/* Make sure the revprop_generation member in FS is set and, if necessary,
* initialized with the latest value stored on disk.
*/
@@ -11035,14 +11045,16 @@ hotcopy_body(void *baton, apr_pool_t *po
PATH_TXN_CURRENT, pool));
/* If a revprop generation file exists in the source filesystem,
- * force a fresh revprop caching namespace for the destination by
- * setting the generation to zero. We have no idea if the revprops
- * we copied above really belong to the currently cached generation. */
+ * reset it to zero (since this is on a different path, it will not
+ * overlap with data already in cache). Also, clean up stale files
+ * used for the named atomics implementation. */
SVN_ERR(svn_io_check_path(path_revprop_generation(src_fs, pool),
&kind, pool));
if (kind == svn_node_file)
SVN_ERR(write_revprop_generation_file(dst_fs, 0, pool));
+ SVN_ERR(cleanup_revprop_namespace(dst_fs));
+
/* Hotcopied FS is complete. Stamp it with a format file. */
SVN_ERR(write_format(svn_dirent_join(dst_fs->path, PATH_FORMAT, pool),
dst_ffd->format, max_files_per_dir, TRUE, pool));
Modified: subversion/trunk/subversion/libsvn_subr/named_atomic.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/named_atomic.c?rev=1404112&r1=1404111&r2=1404112&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/named_atomic.c (original)
+++ subversion/trunk/subversion/libsvn_subr/named_atomic.c Wed Oct 31 12:27:29 2012
@@ -24,7 +24,7 @@
#include "private/svn_named_atomic.h"
#include <apr_global_mutex.h>
-#include <apr_shm.h>
+#include <apr_mmap.h>
#include "svn_private_config.h"
#include "private/svn_atomic.h"
@@ -35,13 +35,16 @@
/* Implementation aspects.
*
- * We use a single shared memory block that will be created by the first
- * user and merely mapped by all subsequent ones. The memory block contains
- * an short header followed by a fixed-capacity array of named atomics. The
- * number of entries currently in use is stored in the header part.
+ * We use a single shared memory block (memory mapped file) that will be
+ * created by the first user and merely mapped by all subsequent ones.
+ * The memory block contains an short header followed by a fixed-capacity
+ * array of named atomics. The number of entries currently in use is stored
+ * in the header part.
*
- * Finding / creating the SHM object as well as adding new array entries
- * is being guarded by an APR global mutex.
+ * Finding / creating the MMAP object as well as adding new array entries
+ * is being guarded by an APR global mutex. Since releasing the MMAP
+ * structure and closing the underlying does not affect other users of the
+ * same, cleanup will not be synchronized.
*
* The array is append-only. Once a process mapped the block into its
* address space, it may freely access any of the used entries. However,
@@ -382,9 +385,13 @@ svn_atomic_namespace__create(svn_atomic_
apr_pool_t *result_pool)
{
apr_status_t apr_err;
+ svn_error_t *err;
+ apr_file_t *file;
+ apr_mmap_t *mmap;
const char *shm_name, *lock_name;
- apr_shm_t *shared_mem;
- int i;
+ svn_node_kind_t kind;
+
+ apr_pool_t *sub_pool = svn_pool_create(result_pool);
/* allocate the namespace data structure
*/
@@ -392,8 +399,8 @@ svn_atomic_namespace__create(svn_atomic_
/* construct the names of the system objects that we need
*/
- shm_name = apr_pstrcat(result_pool, name, SHM_NAME_SUFFIX, NULL);
- lock_name = apr_pstrcat(result_pool, name, MUTEX_NAME_SUFFIX, NULL);
+ shm_name = apr_pstrcat(sub_pool, name, SHM_NAME_SUFFIX, NULL);
+ lock_name = apr_pstrcat(sub_pool, name, MUTEX_NAME_SUFFIX, NULL);
/* initialize the lock objects
*/
@@ -416,45 +423,42 @@ svn_atomic_namespace__create(svn_atomic_
*/
SVN_ERR(lock(&new_ns->mutex));
- /* Attach to, or create, the shared memory segment. Loop to limit
- * the effect of races with other processes. I've seen the first
- * race happen in testing and I can imagine the second race
- * happening. It could race forever with a whole series of other
- * processes but the timing would have to be just right. Looping a
- * few times should work in most cases.
+ /* First, make sure that the underlying file exists. If it doesn't
+ * exist, create one and initialize its content.
*/
- for (i = 0; i < 10; ++i)
+ err = svn_io_check_path(shm_name, &kind, sub_pool);
+ if (!err && kind != svn_node_file)
{
- apr_err = apr_shm_attach(&shared_mem, shm_name, result_pool);
- if (!apr_err)
- {
- new_ns->data = apr_shm_baseaddr_get(shared_mem);
- break;
- }
-
- /* First race: failed to attach but another process could create. */
-
- apr_err = apr_shm_create(&shared_mem,
- sizeof(*new_ns->data),
- shm_name,
- result_pool);
- if (!apr_err)
+ err = svn_io_file_open(&file, shm_name,
+ APR_READ | APR_WRITE | APR_CREATE,
+ APR_OS_DEFAULT,
+ result_pool);
+ if (!err)
{
- new_ns->data = apr_shm_baseaddr_get(shared_mem);
-
- /* Zero all counters, values and names. */
- memset(new_ns->data, 0, sizeof(*new_ns->data));
- break;
+ /* Zero all counters, values and names.
+ */
+ struct shared_data_t initial_data;
+ memset(&initial_data, 0, sizeof(initial_data));
+ err = svn_io_file_write_full(file, &initial_data,
+ sizeof(initial_data), NULL,
+ sub_pool);
}
-
- /* Second race: failed to create but another process could delete. */
+ }
+ else
+ {
+ err = svn_io_file_open(&file, shm_name,
+ APR_READ | APR_WRITE, APR_OS_DEFAULT,
+ result_pool);
}
- if (apr_err)
- return unlock(&new_ns->mutex,
- svn_error_wrap_apr(apr_err,
- _("Can't get shared memory for named atomics")));
+ /* Now, map it into memory.
+ */
+ apr_err = apr_mmap_create(&mmap, file, 0, sizeof(*new_ns->data),
+ APR_MMAP_READ | APR_MMAP_WRITE , result_pool);
+ if (!apr_err)
+ new_ns->data = mmap->mm;
+ svn_pool_destroy(sub_pool);
/* Cache the number of existing, complete entries. There can't be
* incomplete ones from other processes because we hold the mutex.
@@ -471,6 +475,23 @@ svn_atomic_namespace__create(svn_atomic_
}
svn_error_t *
+svn_atomic_namespace__cleanup(const char *name,
+ apr_pool_t *pool)
+{
+ const char *shm_name, *lock_name;
+
+ /* file names used for the specified namespace */
+ shm_name = apr_pstrcat(pool, name, SHM_NAME_SUFFIX, NULL);
+ lock_name = apr_pstrcat(pool, name, MUTEX_NAME_SUFFIX, NULL);
+
+ /* remove these files if they exist */
+ SVN_ERR(svn_io_remove_file2(shm_name, TRUE, pool));
+ SVN_ERR(svn_io_remove_file2(lock_name, TRUE, pool));
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
svn_named_atomic__get(svn_named_atomic__t **atomic,
svn_atomic_namespace__t *ns,
const char *name,
Modified: subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py?rev=1404112&r1=1404111&r2=1404112&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py Wed Oct 31 12:27:29 2012
@@ -78,6 +78,12 @@ def check_hotcopy_fsfs(src, dst):
"source" % src_dirent)
# Compare all files in this directory
for src_file in src_files:
+ # Exclude temporary files
+ if src_file == 'rev-prop-atomicsShm':
+ continue
+ if src_file == 'rev-prop-atomicsMutex':
+ continue
+
src_path = os.path.join(src_dirpath, src_file)
dst_path = os.path.join(dst_dirpath, src_file)
if not os.path.isfile(dst_path):
Modified: subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c?rev=1404112&r1=1404111&r2=1404112&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c Wed Oct 31 12:27:29 2012
@@ -176,6 +176,20 @@ init_test_shm(apr_pool_t *pool)
return SVN_NO_ERROR;
}
+/* Remove temporary files from disk.
+ */
+static svn_error_t *
+cleanup_test_shm(apr_pool_t *pool)
+{
+ SVN_ERR(svn_atomic_namespace__cleanup(name_namespace, pool));
+ SVN_ERR(svn_atomic_namespace__cleanup(name_namespace1, pool));
+ SVN_ERR(svn_atomic_namespace__cleanup(name_namespace2, pool));
+
+ /* done */
+
+ return SVN_NO_ERROR;
+}
+
/* Prepare the shared memory for a run with COUNT workers.
*/
static svn_error_t *
@@ -519,6 +533,8 @@ test_basics(apr_pool_t *pool)
SVN_ERR(svn_named_atomic__read(&value, atomic));
SVN_TEST_ASSERT(value == 42);
+ SVN_ERR(cleanup_test_shm(pool));
+
return SVN_NO_ERROR;
}
@@ -567,6 +583,8 @@ test_bignums(apr_pool_t *pool)
SVN_ERR(svn_named_atomic__read(&value, atomic));
SVN_TEST_ASSERT(value == 98 * HUGE_VALUE);
+ SVN_ERR(cleanup_test_shm(pool));
+
return SVN_NO_ERROR;
}
@@ -629,6 +647,8 @@ test_multiple_atomics(apr_pool_t *pool)
SVN_TEST_ASSERT(value1 == 4 * HUGE_VALUE);
SVN_TEST_ASSERT(value2 == 98 * HUGE_VALUE);
+ SVN_ERR(cleanup_test_shm(pool));
+
return SVN_NO_ERROR;
}
@@ -678,6 +698,8 @@ test_namespaces(apr_pool_t *pool)
SVN_ERR(svn_named_atomic__read(&value, atomic2_alias));
SVN_TEST_ASSERT(value == 42 * HUGE_VALUE);
+ SVN_ERR(cleanup_test_shm(pool));
+
return SVN_NO_ERROR;
}
@@ -692,6 +714,8 @@ test_multithreaded(apr_pool_t *pool)
SVN_ERR(init_concurrency_test_shm(pool, hw_thread_count));
SVN_ERR(run_threads(pool, hw_thread_count, suggested_iterations, test_pipeline));
+ SVN_ERR(cleanup_test_shm(pool));
+
return SVN_NO_ERROR;
#else
return svn_error_create(SVN_ERR_TEST_SKIPPED, NULL, NULL);
@@ -712,6 +736,7 @@ test_multiprocess(apr_pool_t *pool)
SVN_ERR(init_concurrency_test_shm(pool, hw_thread_count));
SVN_ERR(run_procs(pool, TEST_PROC, hw_thread_count, suggested_iterations));
+ SVN_ERR(cleanup_test_shm(pool));
return SVN_NO_ERROR;
}
Re: svn commit: r1404112 - in /subversion/trunk/subversion:
include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c libsvn_subr/named_atomic.c
tests/cmdline/svnadmin_tests.py tests/libsvn_subr/named_atomic-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 31, 2012 at 2:15 PM, Philip Martin
<ph...@wandisco.com>wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
> > stefan2@apache.org writes:
> >
> >> Author: stefan2
> >> Date: Wed Oct 31 12:27:29 2012
> >> New Revision: 1404112
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1404112&view=rev
> >> Log:
> >> Change the way we implement shared memory setup for our named atomics.
> >> Instead of using APR's SHM API, we create a persistent file and simply
> >> mmap it.
> >
> >> +/* Make sure the revprop_namespace member in FS is set. */
> >> +static svn_error_t *
> >> +cleanup_revprop_namespace(svn_fs_t *fs)
> >> +{
> >> + const char* name = svn_dirent_join(fs->path,
> >
> > const char *name
> >
> >> + err = svn_io_check_path(shm_name, &kind, sub_pool);
> >> + if (!err && kind != svn_node_file)
> >> {
> >> - apr_err = apr_shm_attach(&shared_mem, shm_name, result_pool);
> >> - if (!apr_err)
> >> - {
> >> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
> >> - break;
> >> - }
> >> -
> >> - /* First race: failed to attach but another process could
> create. */
> >> -
> >> - apr_err = apr_shm_create(&shared_mem,
> >> - sizeof(*new_ns->data),
> >> - shm_name,
> >> - result_pool);
> >> - if (!apr_err)
> >> + err = svn_io_file_open(&file, shm_name,
> >> + APR_READ | APR_WRITE | APR_CREATE,
> >> + APR_OS_DEFAULT,
> >> + result_pool);
> >> + if (!err)
> >> {
> >> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
> >> -
> >> - /* Zero all counters, values and names. */
> >> - memset(new_ns->data, 0, sizeof(*new_ns->data));
> >> - break;
> >> + /* Zero all counters, values and names.
> >> + */
> >> + struct shared_data_t initial_data;
> >> + memset(&initial_data, 0, sizeof(initial_data));
> >> + err = svn_io_file_write_full(file, &initial_data,
> >> + sizeof(initial_data), NULL,
> >> + sub_pool);
> >> }
> >> -
> >> - /* Second race: failed to create but another process could
> delete. */
> >> + }
> >> + else
> >> + {
> >> + err = svn_io_file_open(&file, shm_name,
> >> + APR_READ | APR_WRITE, APR_OS_DEFAULT,
> >> + result_pool);
>
> Suppose the process that first created the file got interrupted between
> the svn_io_file_open and svn_io_file_write_full calls. The file is
> incomplete but this process is going to assume it is correct and use it.
> Will that work? Perhaps we should stat() the file to check the size?
>
Thanks for the review(s)!
As of r1404138, all issues should have been addressed.
Let's see what the build bots have to say about that ;)
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: svn commit: r1404112 - in /subversion/trunk/subversion: include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c libsvn_subr/named_atomic.c tests/cmdline/svnadmin_tests.py tests/libsvn_subr/named_atomic-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> stefan2@apache.org writes:
>
>> Author: stefan2
>> Date: Wed Oct 31 12:27:29 2012
>> New Revision: 1404112
>>
>> URL: http://svn.apache.org/viewvc?rev=1404112&view=rev
>> Log:
>> Change the way we implement shared memory setup for our named atomics.
>> Instead of using APR's SHM API, we create a persistent file and simply
>> mmap it.
>
>> +/* Make sure the revprop_namespace member in FS is set. */
>> +static svn_error_t *
>> +cleanup_revprop_namespace(svn_fs_t *fs)
>> +{
>> + const char* name = svn_dirent_join(fs->path,
>
> const char *name
>
>> + err = svn_io_check_path(shm_name, &kind, sub_pool);
>> + if (!err && kind != svn_node_file)
>> {
>> - apr_err = apr_shm_attach(&shared_mem, shm_name, result_pool);
>> - if (!apr_err)
>> - {
>> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
>> - break;
>> - }
>> -
>> - /* First race: failed to attach but another process could create. */
>> -
>> - apr_err = apr_shm_create(&shared_mem,
>> - sizeof(*new_ns->data),
>> - shm_name,
>> - result_pool);
>> - if (!apr_err)
>> + err = svn_io_file_open(&file, shm_name,
>> + APR_READ | APR_WRITE | APR_CREATE,
>> + APR_OS_DEFAULT,
>> + result_pool);
>> + if (!err)
>> {
>> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
>> -
>> - /* Zero all counters, values and names. */
>> - memset(new_ns->data, 0, sizeof(*new_ns->data));
>> - break;
>> + /* Zero all counters, values and names.
>> + */
>> + struct shared_data_t initial_data;
>> + memset(&initial_data, 0, sizeof(initial_data));
>> + err = svn_io_file_write_full(file, &initial_data,
>> + sizeof(initial_data), NULL,
>> + sub_pool);
>> }
>> -
>> - /* Second race: failed to create but another process could delete. */
>> + }
>> + else
>> + {
>> + err = svn_io_file_open(&file, shm_name,
>> + APR_READ | APR_WRITE, APR_OS_DEFAULT,
>> + result_pool);
Suppose the process that first created the file got interrupted between
the svn_io_file_open and svn_io_file_write_full calls. The file is
incomplete but this process is going to assume it is correct and use it.
Will that work? Perhaps we should stat() the file to check the size?
>> }
>>
>> - if (apr_err)
>> - return unlock(&new_ns->mutex,
>> - svn_error_wrap_apr(apr_err,
>> - _("Can't get shared memory for named atomics")));
>> + /* Now, map it into memory.
>> + */
>
> Need to do something with err, at present it leaks. The mmap call
> probably doesn't make sense if err is set.
>
>> + apr_err = apr_mmap_create(&mmap, file, 0, sizeof(*new_ns->data),
>> + APR_MMAP_READ | APR_MMAP_WRITE , result_pool);
>> + if (!apr_err)
>> + new_ns->data = mmap->mm;
>>
>> + svn_pool_destroy(sub_pool);
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download
>
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1404112 - in /subversion/trunk/subversion: include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c libsvn_subr/named_atomic.c tests/cmdline/svnadmin_tests.py tests/libsvn_subr/named_atomic-test.c
Posted by Philip Martin <ph...@wandisco.com>.
stefan2@apache.org writes:
> Author: stefan2
> Date: Wed Oct 31 12:27:29 2012
> New Revision: 1404112
>
> URL: http://svn.apache.org/viewvc?rev=1404112&view=rev
> Log:
> Change the way we implement shared memory setup for our named atomics.
> Instead of using APR's SHM API, we create a persistent file and simply
> mmap it.
> +/* Make sure the revprop_namespace member in FS is set. */
> +static svn_error_t *
> +cleanup_revprop_namespace(svn_fs_t *fs)
> +{
> + const char* name = svn_dirent_join(fs->path,
const char *name
> + err = svn_io_check_path(shm_name, &kind, sub_pool);
> + if (!err && kind != svn_node_file)
> {
> - apr_err = apr_shm_attach(&shared_mem, shm_name, result_pool);
> - if (!apr_err)
> - {
> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
> - break;
> - }
> -
> - /* First race: failed to attach but another process could create. */
> -
> - apr_err = apr_shm_create(&shared_mem,
> - sizeof(*new_ns->data),
> - shm_name,
> - result_pool);
> - if (!apr_err)
> + err = svn_io_file_open(&file, shm_name,
> + APR_READ | APR_WRITE | APR_CREATE,
> + APR_OS_DEFAULT,
> + result_pool);
> + if (!err)
> {
> - new_ns->data = apr_shm_baseaddr_get(shared_mem);
> -
> - /* Zero all counters, values and names. */
> - memset(new_ns->data, 0, sizeof(*new_ns->data));
> - break;
> + /* Zero all counters, values and names.
> + */
> + struct shared_data_t initial_data;
> + memset(&initial_data, 0, sizeof(initial_data));
> + err = svn_io_file_write_full(file, &initial_data,
> + sizeof(initial_data), NULL,
> + sub_pool);
> }
> -
> - /* Second race: failed to create but another process could delete. */
> + }
> + else
> + {
> + err = svn_io_file_open(&file, shm_name,
> + APR_READ | APR_WRITE, APR_OS_DEFAULT,
> + result_pool);
> }
>
> - if (apr_err)
> - return unlock(&new_ns->mutex,
> - svn_error_wrap_apr(apr_err,
> - _("Can't get shared memory for named atomics")));
> + /* Now, map it into memory.
> + */
Need to do something with err, at present it leaks. The mmap call
probably doesn't make sense if err is set.
> + apr_err = apr_mmap_create(&mmap, file, 0, sizeof(*new_ns->data),
> + APR_MMAP_READ | APR_MMAP_WRITE , result_pool);
> + if (!apr_err)
> + new_ns->data = mmap->mm;
>
> + svn_pool_destroy(sub_pool);
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1404112 - in /subversion/trunk/subversion: include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c libsvn_subr/named_atomic.c tests/cmdline/svnadmin_tests.py tests/libsvn_subr/named_atomic-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: woensdag 31 oktober 2012 13:27
> To: commits@subversion.apache.org
> Subject: svn commit: r1404112 - in /subversion/trunk/subversion:
> include/private/svn_named_atomic.h libsvn_fs_fs/fs_fs.c
> libsvn_subr/named_atomic.c tests/cmdline/svnadmin_tests.py
> tests/libsvn_subr/named_atomic-test.c
>
> Author: stefan2
> Date: Wed Oct 31 12:27:29 2012
> New Revision: 1404112
>
> URL: http://svn.apache.org/viewvc?rev=1404112&view=rev
> Log:
> Change the way we implement shared memory setup for our named
> atomics.
> Instead of using APR's SHM API, we create a persistent file and simply
> mmap it.
>
> The only downside to this is that the SHM file does not get cleaned up
> automatically anymore. Therefore, we need to update tests and hotcopy
> code.
>
> The underlying issue has been analyzed in this thread:
> http://svn.haxx.se/dev/archive-2012-10/0423.shtml
>
> * subversion/include/private/svn_named_atomic.h
> (svn_atomic_namespace__cleanup): declare new API
>
> * subversion/libsvn_subr/named_atomic.c
> (): update global docstring
> (svn_atomic_namespace__create): create a persistent file; mmap it
> (svn_atomic_namespace__cleanup): implement new API
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (cleanup_revprop_namespace): new utility function
> (hotcopy_body): fix comment; remove temp atomics files
>
> * subversion/tests/cmdline/svnadmin_tests.py
> (check_hotcopy_fsfs): don't compare temp files related to atomics
>
> * subversion/tests/libsvn_subr/named_atomic-test.c
> (cleanup_test_shm): new cleanup function
> test_basics,
> test_bignums,
> test_multiple_atomics,
> test_namespaces,
> test_multithreaded,
> test_multiprocess): call the new function
>
> Modified:
> subversion/trunk/subversion/include/private/svn_named_atomic.h
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> subversion/trunk/subversion/libsvn_subr/named_atomic.c
> subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
> subversion/trunk/subversion/tests/libsvn_subr/named_atomic-test.c
>
> Modified:
> subversion/trunk/subversion/include/private/svn_named_atomic.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private
> /svn_named_atomic.h?rev=1404112&r1=1404111&r2=1404112&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/include/private/svn_named_atomic.h
> (original)
> +++ subversion/trunk/subversion/include/private/svn_named_atomic.h
> Wed Oct 31 12:27:29 2012
> @@ -83,6 +83,17 @@ svn_atomic_namespace__create(svn_atomic_
> const char *name,
> apr_pool_t *result_pool);
>
> +/** Removes persistent data structures (files in particular) that got
> + * created for the namespace given by @a name. Use @a pool for
> temporary
> + * allocations.
> + *
> + * @note You must not call this while the respective namespace is still
> + * in use. Calling this multiple times for the same namespace is safe.
> + */
> +svn_error_t *
> +svn_atomic_namespace__cleanup(const char *name,
> + apr_pool_t *pool);
> +
> /** Find the atomic with the specified @a name in namespace @a ns and
> * return it in @a *atomic. If no object with that name can be found, the
> * behavior depends on @a auto_create. If it is @c FALSE, @a *atomic will
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs
> _fs.c?rev=1404112&r1=1404111&r2=1404112&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Oct 31 12:27:29
> 2012
> @@ -3160,6 +3160,16 @@ ensure_revprop_namespace(svn_fs_t *fs)
> : SVN_NO_ERROR;
> }
>
> +/* Make sure the revprop_namespace member in FS is set. */
> +static svn_error_t *
> +cleanup_revprop_namespace(svn_fs_t *fs)
> +{
> + const char* name = svn_dirent_join(fs->path,
> + ATOMIC_REVPROP_NAMESPACE,
> + fs->pool);
> + return svn_error_trace(svn_atomic_namespace__cleanup(name, fs-
> >pool));
> +}
> +
> /* Make sure the revprop_generation member in FS is set and, if necessary,
> * initialized with the latest value stored on disk.
> */
> @@ -11035,14 +11045,16 @@ hotcopy_body(void *baton, apr_pool_t *po
> PATH_TXN_CURRENT, pool));
>
> /* If a revprop generation file exists in the source filesystem,
> - * force a fresh revprop caching namespace for the destination by
> - * setting the generation to zero. We have no idea if the revprops
> - * we copied above really belong to the currently cached generation. */
> + * reset it to zero (since this is on a different path, it will not
> + * overlap with data already in cache). Also, clean up stale files
> + * used for the named atomics implementation. */
> SVN_ERR(svn_io_check_path(path_revprop_generation(src_fs, pool),
> &kind, pool));
> if (kind == svn_node_file)
> SVN_ERR(write_revprop_generation_file(dst_fs, 0, pool));
>
> + SVN_ERR(cleanup_revprop_namespace(dst_fs));
> +
> /* Hotcopied FS is complete. Stamp it with a format file. */
> SVN_ERR(write_format(svn_dirent_join(dst_fs->path, PATH_FORMAT,
> pool),
> dst_ffd->format, max_files_per_dir, TRUE, pool));
>
> Modified: subversion/trunk/subversion/libsvn_subr/named_atomic.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/na
> med_atomic.c?rev=1404112&r1=1404111&r2=1404112&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_subr/named_atomic.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/named_atomic.c Wed Oct 31
> 12:27:29 2012
> @@ -24,7 +24,7 @@
> #include "private/svn_named_atomic.h"
>
> #include <apr_global_mutex.h>
> -#include <apr_shm.h>
> +#include <apr_mmap.h>
>
> #include "svn_private_config.h"
> #include "private/svn_atomic.h"
> @@ -35,13 +35,16 @@
>
> /* Implementation aspects.
> *
> - * We use a single shared memory block that will be created by the first
> - * user and merely mapped by all subsequent ones. The memory block
> contains
> - * an short header followed by a fixed-capacity array of named atomics. The
> - * number of entries currently in use is stored in the header part.
> + * We use a single shared memory block (memory mapped file) that will be
> + * created by the first user and merely mapped by all subsequent ones.
> + * The memory block contains an short header followed by a fixed-capacity
> + * array of named atomics. The number of entries currently in use is stored
> + * in the header part.
> *
> - * Finding / creating the SHM object as well as adding new array entries
> - * is being guarded by an APR global mutex.
> + * Finding / creating the MMAP object as well as adding new array entries
> + * is being guarded by an APR global mutex. Since releasing the MMAP
> + * structure and closing the underlying does not affect other users of the
> + * same, cleanup will not be synchronized.
> *
> * The array is append-only. Once a process mapped the block into its
> * address space, it may freely access any of the used entries. However,
> @@ -382,9 +385,13 @@ svn_atomic_namespace__create(svn_atomic_
> apr_pool_t *result_pool)
> {
> apr_status_t apr_err;
> + svn_error_t *err;
> + apr_file_t *file;
> + apr_mmap_t *mmap;
> const char *shm_name, *lock_name;
> - apr_shm_t *shared_mem;
> - int i;
> + svn_node_kind_t kind;
> +
> + apr_pool_t *sub_pool = svn_pool_create(result_pool);
Why do you create a subpool if you can just make the caller pass a scratch_pool?
(Usualle we use 'subpool' and 'iterpool' for the variable name, not '(sub|iter)_pool')
Bert