Stefan Fuhrmann wrote on Sun, May 15, 2011 at 18:17:22 +0200:
> 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.
>>>
>>> + 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.
>
Okay. There is only one code path that initializes txn's, so I'm not
too worried about documenting this centrally...
But: the call in make_txn_root() to svn_fs_fs__initialize_txn_caches()
is missing an SVN_ERR() wrapper. Could you look into these please?
(I've fixed *all* instances in the code a while back using vim+gcc ---
I'd be happy not to see more instances added :-))
> 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!
>
Thanks for the fixes :)
> -- Stefan^2.
>
Received on 2011-05-16 11:38:16 CEST