On 20 April 2014 22:42, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
> On Sat, Apr 19, 2014 at 12:05 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>
>> On 17 March 2014 03:09, <stefan2_at_apache.org> wrote:
>> > Author: stefan2
>> > Date: Sun Mar 16 23:09:45 2014
>> > New Revision: 1578176
>> >
>> > URL: http://svn.apache.org/r1578176
>> > Log:
>> > Model the FSFS pack lock similarly to our other file based locks in
>> > FSFS.
>> > This gives us proper mutex functionality should multiple threads try to
>> > pack the same repo at the same time.
>> >
>> > * subversion/libsvn_fs_fs/fs.h
>> > (fs_fs_shared_data_t): Add a thread mutex alongside the file lock.
>> >
>> > * subversion/libsvn_fs_fs/fs.c
>> > (fs_serialized_init): Initialize the new mutex.
>> >
>> > * subversion/libsvn_fs_fs/fs_fs.h
>> > (svn_fs_fs__get_lock_on_filesystem): Privatize again.
>> > (svn_fs_fs__with_pack_lock): New lock handling function similar
>> > to what we do for the other locks.
>> >
>> > * subversion/libsvn_fs_fs/fs_fs.c
>> > (path_pack_lock): New utility function.
>> > (svn_fs_fs__get_lock_on_filesystem): Rename back to ...
>> > (get_lock_on_filesystem): ... this again.
>> > (with_some_lock_file): Update caller.
>> > (svn_fs_fs__with_pack_lock): Implement the new lock function.
>> >
>> > * subversion/libsvn_fs_fs/pack.c
>> > (svn_fs_fs__pack): Use the new pack lock handling function.
>> >
>> > Suggested by: ivan
>> >
>> Hi Stefan,
>>
>> Thanks for reworking this.
>>
>> But with new code is more clear that separate pack lock break
>> concurrent svnadmin hotcopy: hotcopy operation only takes write-lock,
>> not taking pack-lock. Thus we Subversion hotcopy operation may copy
>> partially packed repository.
>
>
> Good catch!
>
> Turns out that there are more operations that
> should take out the pack lock as well as even
> the "create new txn" lock.
>
> r1588815 does all of that and makes adding
> more locks or changing lock policies easier
> in the future. As a side-effect, it fixes a lock
> leak case of e.g. failing youngest rev updates.
>
Using nested locks is extremely slippery way: it's very easy end up
with deadlock if locks will be obtained in different order. It's not a
case now, but this could be easily done in future code changes. So
r1588815 missing important comment that we must maintain the same
documented order of obtaining locks.
Also it's not clear why we need txn-lock for hotcopy operation:
txn-current updated by atomic move file and txns folder is not copied.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-04-21 09:54:32 CEST