You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/09/19 12:31:30 UTC
Re: svn commit: r25869 - in trunk/subversion: include/private
libsvn_fs_fs tests/libsvn_fs
Hi Blair,
With respect to the FSFS transaction-current code you commited a while
ago...
On Fri, Jul 27, 2007 at 07:22:48PM -0700, blair@tigris.org wrote:
> (get_and_increment_txn_key_body):
> New function used as a callback for svn_fs_fs__with_write_lock().
> This gets the current base 36 value in transaction-current and
> increments it. It returns the original value by the baton.
I'm curious at to whether there was any reason that you chose to use the
fs-wide write-lock to update the file rather than locking the file
directly? If not, is there any reason we shouldn't change it to do so?
Regards,
Malcolm
Re: svn commit: r25869 - in trunk/subversion: include/private libsvn_fs_fs tests/libsvn_fs
Posted by David Glasser <gl...@davidglasser.net>.
On 9/19/07, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Wed, Sep 19, 2007 at 09:26:02AM -0700, David Glasser wrote:
> > On 9/19/07, Blair Zajac <bl...@orcaware.com> wrote:
> > > Didn't think of locking transaction-current. So we could definitely
> > > do that.
> >
> > Yes, if it is feasible to make the transaction-current and commit
> > locks separate (and I don't see why it wouldn't be...) I would
> > definitely be +1.
> >
>
> We can (and probably should: if we don't fix it for 1.5, we won't be
> able to fix it easily thereafter).
>
> Unfortunately, I forgot that we also need to create and manage a mutex
> for cross-thread synchronisation, so we'd need to duplicate a fair bit
> of the current write-lock code. Anyone see an easy way to abstract that
> away so that we don't need to copy and paste everything? (bonus
> question: anyone see a way to abstract it into a file that's _not_
> fs_fs.c, which is ~180KB and growing?)
The following patch locks transaction-current itself instead of using
the write-lock. (I assume it's OK to lock the file even if we're
changing it by overwriting, right? that is, there's no need to
introduce transaction-current.lock?) epg also suggested locking
revprop files individually instead of using write-lock there; maybe
I'll do that later. It doesn't abstract out of fs_fs.c, but it
doesn't add too many lines of code either.
What do you think?
[[[
To get and increment the FSFS transaction-current file, just lock
'transaction-current' itself instead of 'write-lock', so that new
transactions and commits don't block each other. In order to do so,
abstract out the locking code a little more.
* subversion/libsvn_fs_fs/fs.h
(fs_fs_shared_data_t): Add a txn_current_lock mutex field to protect
the transaction-current file from concurrent access by multiple
threads of one process.
* subversion/libsvn_fs_fs/fs.c
(fs_serialized_init): Create the txn_current_lock mutex.
* subversion/libsvn_fs_fs/fs_fs.c
(path_txn_current): New helper function for getting the path of the
transaction-current file.
(get_lock_on_filesystem): Rename from get_write_lock, and generalize
to lock a given file instead of 'write-lock'. (Also move earlier
in the file.)
(with_some_lock): Extract most of svn_fs_fs__with_write_lock into
this function, which takes as argument a lock filename and (if
threading) a mutex, instead of a svn_fs_t.
(svn_fs_fs__with_write_lock): Now just a wrapper around
with_some_lock (and moved earlier in the file).
(with_txn_current_lock): New function which runs its body with the
transaction-current file (and mutex) locked.
(get_and_increment_txn_key_body): Use new path_txn_current helper
function.
(create_txn_dir): Call get_and_increment_txn_key_body using
with_txn_current_lock instead of svn_fs_fs__with_write_lock (this
change is the whole point of the revision).
* subversion/libsvn_fs_fs/structure
(Layout of the FS directory): Document that transaction-current is
modified under its own lock.
]]]
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
Re: svn commit: r25869 - in trunk/subversion: include/private
libsvn_fs_fs tests/libsvn_fs
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Wed, Sep 19, 2007 at 09:26:02AM -0700, David Glasser wrote:
> On 9/19/07, Blair Zajac <bl...@orcaware.com> wrote:
> > Didn't think of locking transaction-current. So we could definitely
> > do that.
>
> Yes, if it is feasible to make the transaction-current and commit
> locks separate (and I don't see why it wouldn't be...) I would
> definitely be +1.
>
We can (and probably should: if we don't fix it for 1.5, we won't be
able to fix it easily thereafter).
Unfortunately, I forgot that we also need to create and manage a mutex
for cross-thread synchronisation, so we'd need to duplicate a fair bit
of the current write-lock code. Anyone see an easy way to abstract that
away so that we don't need to copy and paste everything? (bonus
question: anyone see a way to abstract it into a file that's _not_
fs_fs.c, which is ~180KB and growing?)
Regards,
Malcolm
Re: svn commit: r25869 - in trunk/subversion: include/private libsvn_fs_fs tests/libsvn_fs
Posted by David Glasser <gl...@davidglasser.net>.
On 9/19/07, Blair Zajac <bl...@orcaware.com> wrote:
>
> On Sep 19, 2007, at 5:31 AM, Malcolm Rowe wrote:
> > I'm curious at to whether there was any reason that you chose to
> > use the
> > fs-wide write-lock to update the file rather than locking the file
> > directly? If not, is there any reason we shouldn't change it to do
> > so?
>
> No reason. Originally, I was going to have a transaction-
> current.lock file and lock that, but after discussion on the list and
> a test by Peter, IIRC, that showed you can get 70,000 locks and
> unlocks on a single file, we decided to use the fs-wide lock.
>
> Didn't think of locking transaction-current. So we could definitely
> do that.
Yes, if it is feasible to make the transaction-current and commit
locks separate (and I don't see why it wouldn't be...) I would
definitely be +1.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25869 - in trunk/subversion: include/private libsvn_fs_fs tests/libsvn_fs
Posted by Blair Zajac <bl...@orcaware.com>.
On Sep 19, 2007, at 5:31 AM, Malcolm Rowe wrote:
> Hi Blair,
>
> With respect to the FSFS transaction-current code you commited a while
> ago...
>
> On Fri, Jul 27, 2007 at 07:22:48PM -0700, blair@tigris.org wrote:
>> (get_and_increment_txn_key_body):
>> New function used as a callback for svn_fs_fs__with_write_lock().
>> This gets the current base 36 value in transaction-current and
>> increments it. It returns the original value by the baton.
>
> I'm curious at to whether there was any reason that you chose to
> use the
> fs-wide write-lock to update the file rather than locking the file
> directly? If not, is there any reason we shouldn't change it to do
> so?
No reason. Originally, I was going to have a transaction-
current.lock file and lock that, but after discussion on the list and
a test by Peter, IIRC, that showed you can get 70,000 locks and
unlocks on a single file, we decided to use the fs-wide lock.
Didn't think of locking transaction-current. So we could definitely
do that.
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org