On Mon, Apr 21, 2014 at 9:53 AM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> 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.
I'm aware of that. It is one of the reasons to have
a "lock all" function. We already use nested locks
in txns and recently during pack. Both instances,
however, can't easily use a central function to get
the ordering right.
> 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.
>
Documented in r1589368.
> 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.
>
We lock the hotcopy *target* repo and will sync
txn-current as one of the last steps. Hence, we
cannot allow new txns to be started in the dest
repo until hotcopy completes.
It should be safe to keep allowing new txns and
even new commits in the source repo, though.
-- Stefan^2.
Received on 2014-04-23 13:04:21 CEST