On Mon, Apr 21, 2014 at 10:21 AM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> 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.
>
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.
Received on 2014-04-23 13:49:51 CEST