You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2014/04/21 10:21:06 UTC
Re: svn commit: r1588815 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h hotcopy.c recovery.c
On 20 April 2014 22:35, <st...@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
Re: svn commit: r1588815 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h hotcopy.c recovery.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Apr 21, 2014 at 11:16 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 April 2014 12:21, Ivan Zhakov <iv...@visualsvn.com> wrote:
> > On 20 April 2014 22:35, <st...@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.
> >>
> > [...]
> >
> Also this commit seems to break two svnadmin tests at least on Windows:
> [[[
> At least one test FAILED, checking C:\Ivan\SVN\test\tests.log
> FAIL: svnadmin_tests.py 8: 'svnadmin hotcopy PATH .'
> FAIL: svnadmin_tests.py 27: 'svnadmin hotcopy --incremental PATH .'
> ]]]
>
Sorry! Forgot to include the test suite fix.
Done in r1588897.
However, our hotcopy implementation is quite
broken from a API contract POV. Working on it.
-- Stefan^2.
RE: svn commit: r1588815 - in /subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c fs_fs.h hotcopy.c recovery.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 21 april 2014 11:16
> To: Subversion Development; Stefan Fuhrman
> Subject: Re: svn commit: r1588815 - in
> /subversion/trunk/subversion/libsvn_fs_fs: fs_fs.c fs_fs.h hotcopy.c
> recovery.c
>
> On 21 April 2014 12:21, Ivan Zhakov <iv...@visualsvn.com> wrote:
> > On 20 April 2014 22:35, <st...@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.
> >>
> > [...]
> >
> Also this commit seems to break two svnadmin tests at least on Windows:
> [[[
> At least one test FAILED, checking C:\Ivan\SVN\test\tests.log
> FAIL: svnadmin_tests.py 8: 'svnadmin hotcopy PATH .'
> FAIL: svnadmin_tests.py 27: 'svnadmin hotcopy --incremental PATH .'
> ]]]
I haven't verified the cause yet, but I see/confirm these test failures on Windows.
(They didn't fail yesterday)
Bert
Re: svn commit: r1588815 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h hotcopy.c recovery.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 21 April 2014 12:21, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 20 April 2014 22:35, <st...@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.
>>
> [...]
>
Also this commit seems to break two svnadmin tests at least on Windows:
[[[
At least one test FAILED, checking C:\Ivan\SVN\test\tests.log
FAIL: svnadmin_tests.py 8: 'svnadmin hotcopy PATH .'
FAIL: svnadmin_tests.py 27: 'svnadmin hotcopy --incremental PATH .'
]]]
Re: svn commit: r1588815 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs_fs.c fs_fs.h hotcopy.c recovery.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Apr 21, 2014 at 10:21 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 20 April 2014 22:35, <st...@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.