On 14.05.2011 03:54, Daniel Shahaf wrote:
> stefan2_at_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.
Received on 2011-05-15 18:18:22 CEST