You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Chia-liang Kao <cl...@clkao.org> on 2007/02/12 12:40:34 UTC
change_rev_prop atomicity
While trying to see how to have fs->change_rev_prop_if_value_is() implemented
for better atomicity, I saw something interesting in the related code.
In libsvn_fs_fs/revs-txns.c: svn_fs_fs__change_rev_prop, the __set_rev_prop
called is loading the revision proplist hash, changing the value for what is
requested, and writing back the whole hash as the new proplist. The code is
vulnerable for race condition when two clients are setting different revision
properties on the same revision at the same time.
Cheers,
CLK
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by David Glasser <gl...@mit.edu>.
On 2/19/07, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Sun, Feb 18, 2007 at 02:59:41PM -0500, David Glasser wrote:
> > >revs-txns.c currently just contains svn_fs_fs__revision_prop,
> > >svn_fs_fs__change_rev_prop, svn_fs_fs__get_txn_ids,
> > >svn_fs_fs__txn_prop, and svn_fs_fs__begin_txn. The second should move
> > >into fs_fs.c, for locking reasons. Is there any good reason not to
> > >just move them all into fs_fs.c and eliminate revs-txns.[ch] while I'm
> > >at it? It is much smaller than libsvn_fs_base's revs-txns.c.
> >
> > ... because if that's a reasonable decision, then I have a patch that
> > does that and a second patch that adds a lock around
> > svn_fs_fs__change_rev_prop that are ready to push.
> >
>
> I think that's a reasonable thing to do - and we will probably find that
> there's some additional refactoring we can do after we've done that.
> For example, svn_fs_fs__get_txn() can become a static function.
OK, I killed revs-txns in r23439 and (hopefully) fixed the race
condition in r23440.
I didn't do the refactoring you just mentioned.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Sun, Feb 18, 2007 at 02:59:41PM -0500, David Glasser wrote:
> >revs-txns.c currently just contains svn_fs_fs__revision_prop,
> >svn_fs_fs__change_rev_prop, svn_fs_fs__get_txn_ids,
> >svn_fs_fs__txn_prop, and svn_fs_fs__begin_txn. The second should move
> >into fs_fs.c, for locking reasons. Is there any good reason not to
> >just move them all into fs_fs.c and eliminate revs-txns.[ch] while I'm
> >at it? It is much smaller than libsvn_fs_base's revs-txns.c.
>
> ... because if that's a reasonable decision, then I have a patch that
> does that and a second patch that adds a lock around
> svn_fs_fs__change_rev_prop that are ready to push.
>
I think that's a reasonable thing to do - and we will probably find that
there's some additional refactoring we can do after we've done that.
For example, svn_fs_fs__get_txn() can become a static function.
Regards,
Malcolm
Re: change_rev_prop atomicity
Posted by David Glasser <gl...@mit.edu>.
On 2/18/07, David Glasser <gl...@mit.edu> wrote:
> Malcolm,
>
> revs-txns.c currently just contains svn_fs_fs__revision_prop,
> svn_fs_fs__change_rev_prop, svn_fs_fs__get_txn_ids,
> svn_fs_fs__txn_prop, and svn_fs_fs__begin_txn. The second should move
> into fs_fs.c, for locking reasons. Is there any good reason not to
> just move them all into fs_fs.c and eliminate revs-txns.[ch] while I'm
> at it? It is much smaller than libsvn_fs_base's revs-txns.c.
... because if that's a reasonable decision, then I have a patch that
does that and a second patch that adds a lock around
svn_fs_fs__change_rev_prop that are ready to push.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by David Glasser <gl...@mit.edu>.
On 2/14/07, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Mon, Feb 12, 2007 at 10:50:55AM -0600, Ben Collins-Sussman wrote:
> > Yeah, we should be using the fsfs writelock for that work, shouldn't we?
> >
>
> Yes. The functionality to change the value of a revprop should probably
> move into fs_fs.c, rather than exporting the write-lock code out.
Malcolm,
revs-txns.c currently just contains svn_fs_fs__revision_prop,
svn_fs_fs__change_rev_prop, svn_fs_fs__get_txn_ids,
svn_fs_fs__txn_prop, and svn_fs_fs__begin_txn. The second should move
into fs_fs.c, for locking reasons. Is there any good reason not to
just move them all into fs_fs.c and eliminate revs-txns.[ch] while I'm
at it? It is much smaller than libsvn_fs_base's revs-txns.c.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Feb 12, 2007 at 10:50:55AM -0600, Ben Collins-Sussman wrote:
> Yeah, we should be using the fsfs writelock for that work, shouldn't we?
>
Yes. The functionality to change the value of a revprop should probably
move into fs_fs.c, rather than exporting the write-lock code out.
Regards,
Malcolm
Re: change_rev_prop atomicity
Posted by Peter Lundblad <pl...@google.com>.
Ben Collins-Sussman writes:
> Yeah, we should be using the fsfs writelock for that work, shouldn't we?
>
+1.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by Ben Collins-Sussman <su...@red-bean.com>.
Yeah, we should be using the fsfs writelock for that work, shouldn't we?
On 2/12/07, David Glasser <gl...@mit.edu> wrote:
> On 2/12/07, Chia-liang Kao <cl...@clkao.org> wrote:
> > While trying to see how to have fs->change_rev_prop_if_value_is() implemented
> > for better atomicity, I saw something interesting in the related code.
> >
> > In libsvn_fs_fs/revs-txns.c: svn_fs_fs__change_rev_prop, the __set_rev_prop
> > called is loading the revision proplist hash, changing the value for what is
> > requested, and writing back the whole hash as the new proplist. The code is
> > vulnerable for race condition when two clients are setting different revision
> > properties on the same revision at the same time.
>
> Taking a look at the same code, I agree with CL that this is a race
> condition. While of course revprops are unversioned and we shouldn't
> be too worried about two clients changing the same revprop at the same
> time, if two clients try to change *different* revprops on the same
> revision at the same time it looks like you could lose one change.
>
> --dave
>
> --
> David Glasser | glasser@mit.edu | http://www.davidglasser.net/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: change_rev_prop atomicity
Posted by David Glasser <gl...@mit.edu>.
On 2/12/07, Chia-liang Kao <cl...@clkao.org> wrote:
> While trying to see how to have fs->change_rev_prop_if_value_is() implemented
> for better atomicity, I saw something interesting in the related code.
>
> In libsvn_fs_fs/revs-txns.c: svn_fs_fs__change_rev_prop, the __set_rev_prop
> called is loading the revision proplist hash, changing the value for what is
> requested, and writing back the whole hash as the new proplist. The code is
> vulnerable for race condition when two clients are setting different revision
> properties on the same revision at the same time.
Taking a look at the same code, I agree with CL that this is a race
condition. While of course revprops are unversioned and we shouldn't
be too worried about two clients changing the same revprop at the same
time, if two clients try to change *different* revprops on the same
revision at the same time it looks like you could lose one change.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org