On 20 April 2014 22:35, <stefan2_at_apache.org> wrote:
> Author: stefan2
> Date: Sun Apr 20 18:35:43 2014
> New Revision: 1588815
>
> URL: http://svn.apache.org/r1588815
> Log:
> Enable FSFS to take out more than file lock at once through a single call.
> Use that functionality to take out the new pack lock for upgrade, hotcopy
> and recovery. Also, disallow new TXNs during upgrade and recovery.
>
> The core is the introduction of a new data type describing a lock to take
> out, which can be nested / chained. Switch all existing lock function to
> using that infrastructure.
>
[...]
> + switch (lock_id)
> + {
> + case write_lock: baton->mutex = ffsd->fs_write_lock;
> + baton->lock_path = path_lock(baton->fs,
> + baton->lock_pool);
> + baton->is_global_lock = TRUE;
> + break;
Please use standard indentation.
> +
> + case txn_lock: baton->mutex = ffsd->txn_current_lock;
> + baton->lock_path = svn_fs_fs__path_txn_current_lock
> + (baton->fs, baton->lock_pool);
> + baton->is_global_lock = FALSE;
> + break;
> +
> + case pack_lock: baton->mutex = ffsd->fs_pack_lock;
> + baton->lock_path = path_pack_lock(baton->fs,
> + baton->lock_pool);
> + baton->is_global_lock = FALSE;
> + break;
> + }
> +}
> +
> +/* Return the baton for the innermost lock of a (potential) lock chain.
> + The baton shall take out LOCK_ID from FS and execute BODY with BATON
> + while the lock is being held. Allocate the result in a sub-pool of POOL.
> + */
> +static with_lock_baton_t *
> +create_lock_baton(svn_fs_t *fs,
> + lock_id_t lock_id,
> + svn_error_t *(*body)(void *baton,
> + apr_pool_t *pool),
> + void *baton,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *lock_pool = svn_pool_create(pool);
> + with_lock_baton_t *result = apr_pcalloc(lock_pool, sizeof(*result));
> +
> + result->fs = fs;
> + result->body = body;
> + result->baton = baton;
> + result->lock_pool = lock_pool;
> + result->is_inner_most_lock = TRUE;
> + result->is_outer_most_lock = TRUE;
> +
> + init_lock_baton(result, lock_id);
> +
> + return result;
> +}
> +
> +/* Return a baton that wraps NESTED and requests LOCK_ID as additional lock.
> + */
> +static with_lock_baton_t *
> +chain_lock_baton(lock_id_t lock_id,
> + with_lock_baton_t *nested)
> +{
> + apr_pool_t *lock_pool = nested->lock_pool;
> + with_lock_baton_t *result = apr_pcalloc(lock_pool, sizeof(*result));
> +
> + result->fs = nested->fs;
> + result->body = with_lock;
> + result->baton = nested;
> + result->lock_pool = lock_pool;
> + result->is_inner_most_lock = FALSE;
> + result->is_outer_most_lock = TRUE;
> + nested->is_outer_most_lock = FALSE;
> +
> + init_lock_baton(result, lock_id);
> +
> + return result;
> +}
> +
I don't see bugs in the code above, but it is very hard to read and
understand what is going on and what fields are initialized in
different cases.
> svn_error_t *
> svn_fs_fs__with_write_lock(svn_fs_t *fs,
> svn_error_t *(*body)(void *baton,
> @@ -203,16 +358,9 @@ svn_fs_fs__with_write_lock(svn_fs_t *fs,
> void *baton,
> apr_pool_t *pool)
> {
> - fs_fs_data_t *ffd = fs->fsap_data;
> - fs_fs_shared_data_t *ffsd = ffd->shared;
> -
> - SVN_MUTEX__WITH_LOCK(ffsd->fs_write_lock,
> - with_some_lock_file(fs, body, baton,
> - path_lock(fs, pool),
> - TRUE,
> - pool));
> -
> - return SVN_NO_ERROR;
> + return svn_error_trace(
> + with_lock(create_lock_baton(fs, write_lock, body, baton, pool),
> + pool));
> }
>
> svn_error_t *
> @@ -222,20 +370,11 @@ svn_fs_fs__with_pack_lock(svn_fs_t *fs,
> void *baton,
> apr_pool_t *pool)
> {
> - fs_fs_data_t *ffd = fs->fsap_data;
> - fs_fs_shared_data_t *ffsd = ffd->shared;
> -
> - SVN_MUTEX__WITH_LOCK(ffsd->fs_pack_lock,
> - with_some_lock_file(fs, body, baton,
> - path_pack_lock(fs, pool),
> - FALSE,
> - pool));
> -
> - return SVN_NO_ERROR;
> + return svn_error_trace(
> + with_lock(create_lock_baton(fs, pack_lock, body, baton, pool),
> + pool));
> }
>
> -/* Run BODY (with BATON and POOL) while the txn-current file
> - of FS is locked. */
> svn_error_t *
> svn_fs_fs__with_txn_current_lock(svn_fs_t *fs,
> svn_error_t *(*body)(void *baton,
> @@ -243,18 +382,34 @@ svn_fs_fs__with_txn_current_lock(svn_fs_
> void *baton,
> apr_pool_t *pool)
> {
> + return svn_error_trace(
> + with_lock(create_lock_baton(fs, txn_lock, body, baton, pool),
> + pool));
> +}
> +
> +svn_error_t *
> +svn_fs_fs__with_all_locks(svn_fs_t *fs,
> + svn_boolean_t allow_new_txns,
> + svn_error_t *(*body)(void *baton,
> + apr_pool_t *pool),
> + void *baton,
> + apr_pool_t *pool)
I think that the following function signature will be much easier to
use and *implement*.
svn_fs_fs__with_locks_many(svn_fs_t *fs,
svn_boolean_t write_lock,
svn_boolean_t txn_lock,
svn_booelan_t pack_lock,
...)
Seriously, could you tell me in what order locks are obtained without
debugger? May be I'm dumb but I cannot answer this question. Coding in
way that only author understand the code is bad thing especially for
open source project IMHO.
[..]
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-04-21 10:21:59 CEST