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?
> 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)' ?
> + 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)
> /* 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
