On 14.05.2011 03:54, Daniel Shahaf wrote:
stef...@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
Author: stefan2
Date: Sun Apr 17 14:48:33 2011
New Revision: 1094150
URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
Log:
Support concurrent transactions on FSFS: reset txn-local caches upon
closing the txn (i.e. don't wait until pool cleanup) and support concurrency
by simply disabling these caches permanently for the respective session
once a concurrency has been detected.
* subversion/libsvn_fs_fs/fs.h
(fs_fs_data_t): add concurrent_transactions member
(svn_fs_fs__reset_txn_caches): declare new private API function
* subversion/libsvn_fs_fs/caching.c
(txn_cleanup_baton_t): new utility struct
(remove_txn_cache): reset txn-local only if they match the ones to clean up
(init_txn_callbacks): new destructor init utility
(svn_fs_fs__initialize_txn_caches): gracefully support multiple concurrent
txns
in the *same* FSFS session by permanently disabling txn-local caches
(svn_fs_fs__reset_txn_caches): implement new private API function
* subversion/libsvn_fs_fs/fs_fs.c
(purge_shared_txn_body): reset txn-local caches immediately at the end
of the given txn
* subversion/libsvn_fs_fs/tree.c
(svn_fs_fs__commit_txn): dito
Modified:
subversion/trunk/subversion/libsvn_fs_fs/caching.c
subversion/trunk/subversion/libsvn_fs_fs/fs.h
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
subversion/trunk/subversion/libsvn_fs_fs/tree.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Apr 17 14:48:33 2011
@@ -331,17 +331,51 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
return SVN_NO_ERROR;
}
+/* Baton to be used for the remove_txn_cache() pool cleanup function, */
+struct txn_cleanup_baton_t
+{
+ /* the cache to reset */
+ svn_cache__t *txn_cache;
+
+ /* the position where to reset it */
+ svn_cache__t **to_reset;
+};
+
/* APR pool cleanup handler that will reset the cache pointer given in
BATON_VOID. */
static apr_status_t
remove_txn_cache(void *baton_void)
{
- svn_cache__t **cache_p = baton_void;
- *cache_p = NULL;
+ struct txn_cleanup_baton_t *baton = baton_void;
+
+ /* be careful not to hurt performance by resetting newer txn's caches */
+ if (*baton->to_reset == baton->txn_cache)
+ *baton->to_reset = NULL;
If I understand correctly, this line actually has the same effect as
svn_fs_fs__reset_txn_caches() --- namely, it zeroes out the
FFD->txn_dir_cache member?
Perhaps you could add a comment to save future readers a bit of
mental double-pointer arithmetic?
r1103419 should make that a little more obvious.
return APR_SUCCESS;
}
+static svn_error_t *
+init_txn_callbacks(svn_cache__t **cache,
+ apr_pool_t *pool)
+{
+ if (cache != NULL)
+ {
This condition will always be true. Did you mean 'if (*cache)' ?
Oops. Fixed in r1103422.
+ struct txn_cleanup_baton_t *baton;
+
+ baton = apr_palloc(pool, sizeof(*baton));
+ baton->txn_cache = *cache;
+ baton->to_reset = cache;
+
+ apr_pool_cleanup_register(pool,
+ baton,
+ remove_txn_cache,
+ apr_pool_cleanup_null);
+ }
+
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
const char *txn_id,
@@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
":", svn_uuid_generate(pool), ":",
(char *)NULL);
- /* There must be no concurrent transactions in progress for the same
- FSFS access / session object. Maybe, you forgot to clean POOL. */
- SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
+ /* We don't support caching for concurrent transactions in the SAME
+ * FSFS session. Maybe, you forgot to clean POOL. */
+ if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
+ {
+ ffd->txn_dir_cache = NULL;
+ ffd->concurrent_transactions = TRUE;
+
+ return SVN_NO_ERROR;
+ }
Why is this safe?
Is it because svn_fs_fs__initialize_txn_caches() is called from all the
relevant codepaths? (presumably the relevant codepaths are those that
create txns)
Correct. The assumption that is being made here is that either
* transactions won't use txn-local caches without previously
calling svn_fs_fs__initialize_txn_caches, or
* no two transactions are being run concurrently within the
same FSFS session.
Otherwise, one txn might read from a different txn's caches.
I say caches here for generality. Currently, there is at most
one such cache.
/* create a txn-local directory cache */
if (svn_fs__get_global_membuffer_cache())
@@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
pool));
/* reset the transaction-specific cache if the pool gets cleaned up. */
- apr_pool_cleanup_register(pool,
-&(ffd->txn_dir_cache),
- remove_txn_cache,
- apr_pool_cleanup_null);
+ init_txn_callbacks(&(ffd->txn_dir_cache), pool);
return SVN_NO_ERROR;
}
+
+void
+svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
+{
+ /* we can always just reset the caches. This may degrade performance but
+ * can never cause in incorrect behavior. */
+
+ fs_fs_data_t *ffd = fs->fsap_data;
+ ffd->txn_dir_cache = NULL;
+}
\ No newline at end of file
Thanks for all the review!
-- Stefan^2.
