On Mon, Apr 21, 2014 at 10:21 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 20 April 2014 22:35, <stef...@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. > Done in r1589243. > > + > > +/* 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. > All fields are always being initialized. r1589373 explains all init. values now. > -/* 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*. > I dropped the ALLOW_NEW_TXNS parameter from the signature in r1589250. It is now as simple as the the specific lock functions. I honestly believe that neither of the following alternative would result in code that is easier to maintain: * Use 3 nested, conditional SVN_MUTEX__WITH_LOCK + with_some_lock_file calls. That would require at least 2 new body functions and 1 (maybe 2) new baton struct. Basically, we would then run the same sequence calling e.g. with_txn_current_lock and let the baton chain unroll. Less flexible, less obvious, less efficient. * Add a special function that conditionally takes out 3 mutexes (without using the convenient macro) and up to 3 file locks - always being anxious not to miss an error condition or to leak a lock. That's be some brittle code that's also hard to extend once we decide to add more locks. > 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, > ...) > Patches are welcome. > 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. > r1589376 explicitly documents the lock acquisition order, now. I don't think the code is harder to understand than any of our stream handling code because it uses the same pattern. As a bonus, all the relevant code is in a single location (as opposed to streams meandering through numerous source files - with good reason). -- Stefan^2.
