You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2009/01/05 07:38:06 UTC
packing race condition
Problem:
Race condition in 'svnadmin pack' may cause corruption (data loss).
Details:
See pack_shard() in fs_fs.c. Note the two svn_io_remove_dir2() calls.
Suppose two 'svnadmin pack' run in parallel, and that one of them
reaches the first call (which removes the pack dir) just after the first
of them reaches the second call (which removes the non-packed shard dir).
Result:
The whole shard (that the two 'svnadmin pack' worked on) is physically
deleted from the (OS) filesystem. Effectively, it does:
% rm -rf db/revs/5/
% rm -rf db/revs/5.pack/
and then (one of the svnadmin's) throws an error (because it tries to
pack the shard that had just been removed).
Proof:
I can reproduce this (on a testing shard_size=4 repository) by placing
breakpoints in the function around the remove_dir2() calls.
(s/place breakpoints/call svn_cmdline_prompt_user2/)
Fix:
Hyrum, who identified the problem on IRC, also suggested the solution of
adding a lock file to 'svnadmin pack'. That looks doable to me, although
it should require passing an FS object (svn_fs_t) to svn_fs_fs__pack (so
there is a mutex to pass to with_some_lock()).
I intend to look into a patch along these lines, but posting here for
comments on the approach.
Daniel
(For example, another way might be to abort if '5.pack' already exists
(rather than remove it and proceed). But would that work?)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004476
Re: packing race condition
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K. Wright wrote on Tue, 6 Jan 2009 at 09:08 -0600:
> Daniel Shahaf wrote:
> > > > > Added the use of the write lock in r35031.
> > ...
> > > Do you need acquire_fs_mutex()?
> >
> > Ping? From reading fs-loader.c it seems to me that acquire_fs_mutex()
> > is only needed when common_pool is used. But svn_fs_pack() doesn't use
> > common_pool. Hence my question.
> >
> > (i.e., I'm not saying it's wrong the way it is; I'm only saying that I don't
> > see why *not* acquiring that mutex would be wrong.)
>
> I'm not sure. I simply emulated what I saw for upgrade. It sounds like you
> know more about this than I do. :)
>
I read the code (and the patch) and had a question. Don't call me an
expert just because of that. :-)
> -Hyrum
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1007750
Re: packing race condition
Posted by "C. Michael Pilato" <cm...@collab.net>.
Hyrum K. Wright wrote:
> Daniel Shahaf wrote:
>>>>> Added the use of the write lock in r35031.
>> ...
>>> Do you need acquire_fs_mutex()?
>> Ping? From reading fs-loader.c it seems to me that acquire_fs_mutex()
>> is only needed when common_pool is used. But svn_fs_pack() doesn't use
>> common_pool. Hence my question.
>>
>> (i.e., I'm not saying it's wrong the way it is; I'm only saying that I
>> don't see why *not* acquiring that mutex would be wrong.)
>
> I'm not sure. I simply emulated what I saw for upgrade. It sounds like you
> know more about this than I do. :)
Uhh... don't model after 'upgrade'. That was written by some FSFS-ignorant
dude out in Harrisburg, NC...
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1007706
Re: packing race condition
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Daniel Shahaf wrote:
>>>> Added the use of the write lock in r35031.
> ...
>> Do you need acquire_fs_mutex()?
>
> Ping? From reading fs-loader.c it seems to me that acquire_fs_mutex()
> is only needed when common_pool is used. But svn_fs_pack() doesn't use
> common_pool. Hence my question.
>
> (i.e., I'm not saying it's wrong the way it is; I'm only saying that I
> don't see why *not* acquiring that mutex would be wrong.)
I'm not sure. I simply emulated what I saw for upgrade. It sounds like you
know more about this than I do. :)
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1007696
Re: packing race condition
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > > Added the use of the write lock in r35031.
...
> Do you need acquire_fs_mutex()?
Ping? From reading fs-loader.c it seems to me that acquire_fs_mutex()
is only needed when common_pool is used. But svn_fs_pack() doesn't use
common_pool. Hence my question.
(i.e., I'm not saying it's wrong the way it is; I'm only saying that I
don't see why *not* acquiring that mutex would be wrong.)
Thanks,
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1007652
Re: packing race condition
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K. Wright wrote on Mon, 5 Jan 2009 at 13:00 -0600:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Daniel Shahaf wrote:
> > Hyrum K. Wright wrote on Mon, 5 Jan 2009 at 10:15 -0600:
> > Added the use of the write lock in r35031. Comments welcome.
> >
> >
> >> r35031 uses acquire_fs_mutex() and doesn't use svn_fs_fs__with_write_lock().
> >> Now, maybe it's a stupid question, but shouldn't it be the other way around?
>
> It uses both. Check out svn_fs_fs__pack() in fs_fs.c.
>
Sorry, didn't read carefully enough. Do you need acquire_fs_mutex(),
though?
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005887
Re: packing race condition
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Daniel Shahaf wrote:
> Hyrum K. Wright wrote on Mon, 5 Jan 2009 at 10:15 -0600:
> Added the use of the write lock in r35031. Comments welcome.
>
>
>> r35031 uses acquire_fs_mutex() and doesn't use svn_fs_fs__with_write_lock().
>> Now, maybe it's a stupid question, but shouldn't it be the other way around?
It uses both. Check out svn_fs_fs__pack() in fs_fs.c.
- -Hyrum
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
iEYEARECAAYFAkliWMgACgkQCwOubk4kUXwYagCfb48CBxWIDQam23OD8trzaPIx
+EAAoOg8UkdxmqA4gQnPXd26q/BVgtiV
=btD1
-----END PGP SIGNATURE-----
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005882
Re: packing race condition
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K. Wright wrote on Mon, 5 Jan 2009 at 10:15 -0600:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Added the use of the write lock in r35031. Comments welcome.
>
r35031 uses acquire_fs_mutex() and doesn't use svn_fs_fs__with_write_lock().
Now, maybe it's a stupid question, but shouldn't it be the other way around?
Daniel
> - -Hyrum
>
> Hyrum K. Wright wrote:
> > It doesn't use the write lock currently, but modifying it to do so would
> > probably solve the problem. When I originally wrote packing, I intended it to
> > be single-threaded, and if somebody attempted to run two instances in parallel,
> > well, Don't Do That. Using the write lock won't let 'svnadmin pack' run any
> > quicker, (i.e., the process isn't parallelized between two concurrent
> > instances), but it would prevent the race condition Daniel describes.
> >
> > When we were chatting on IRC the other day, I wasn't sure if upgrade used the
> > write lock, or a read-write lock (does FSFS even have such a thing?) and I
> > didn't have time to check. If we can get away with using the write-lock, which
> > I believe we can, it seems to be the most maintainable solution going forward.
> >
> > As an aside, I've been thinking that letting pack notify of progress would be
> > useful, especially for the big repositories on which it was intended to run.
> > Thoughts?
> >
> > -Hyrum
> >
> > David Glasser wrote:
> >> You mean svnadmin pack doesn't just work within the FSFS write lock? That
> >> would seem logical to me... fits with svnadmin upgrade, eg.
> >
> >> --dave
> >
> >> On Jan 5, 2009 12:22 AM, "Daniel Shahaf" <d....@daniel.shahaf.name> wrote:
> >
> >> Problem:
> >> Race condition in 'svnadmin pack' may cause corruption (data loss).
> >
> >> Details:
> >> See pack_shard() in fs_fs.c. Note the two svn_io_remove_dir2() calls.
> >> Suppose two 'svnadmin pack' run in parallel, and that one of them
> >> reaches the first call (which removes the pack dir) just after the first
> >> of them reaches the second call (which removes the non-packed shard dir).
> >
> >> Result:
> >> The whole shard (that the two 'svnadmin pack' worked on) is physically
> >> deleted from the (OS) filesystem. Effectively, it does:
> >
> >> % rm -rf db/revs/5/
> >> % rm -rf db/revs/5.pack/
> >
> >> and then (one of the svnadmin's) throws an error (because it tries to
> >> pack the shard that had just been removed).
> >
> >> Proof:
> >> I can reproduce this (on a testing shard_size=4 repository) by placing
> >> breakpoints in the function around the remove_dir2() calls.
> >
> >> (s/place breakpoints/call svn_cmdline_prompt_user2/)
> >
> >> Fix:
> >> Hyrum, who identified the problem on IRC, also suggested the solution of
> >> adding a lock file to 'svnadmin pack'. That looks doable to me, although
> >> it should require passing an FS object (svn_fs_t) to svn_fs_fs__pack (so
> >> there is a mutex to pass to with_some_lock()).
> >
> >> I intend to look into a patch along these lines, but posting here for
> >> comments on the approach.
> >
> >> Daniel
> >
> >> (For example, another way might be to abort if '5.pack' already exists
> >> (rather than remove it and proceed). But would that work?)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
>
> iEYEARECAAYFAkliMiEACgkQCwOubk4kUXzwxQCgrsZeZU9mo4ynGTlmnQs8G/Yk
> 0ekAnjJzkHomUbZU6XakJ2H2tELldmzz
> =szwS
> -----END PGP SIGNATURE-----
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005881
Re: packing race condition
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Added the use of the write lock in r35031. Comments welcome.
- -Hyrum
Hyrum K. Wright wrote:
> It doesn't use the write lock currently, but modifying it to do so would
> probably solve the problem. When I originally wrote packing, I intended it to
> be single-threaded, and if somebody attempted to run two instances in parallel,
> well, Don't Do That. Using the write lock won't let 'svnadmin pack' run any
> quicker, (i.e., the process isn't parallelized between two concurrent
> instances), but it would prevent the race condition Daniel describes.
>
> When we were chatting on IRC the other day, I wasn't sure if upgrade used the
> write lock, or a read-write lock (does FSFS even have such a thing?) and I
> didn't have time to check. If we can get away with using the write-lock, which
> I believe we can, it seems to be the most maintainable solution going forward.
>
> As an aside, I've been thinking that letting pack notify of progress would be
> useful, especially for the big repositories on which it was intended to run.
> Thoughts?
>
> -Hyrum
>
> David Glasser wrote:
>> You mean svnadmin pack doesn't just work within the FSFS write lock? That
>> would seem logical to me... fits with svnadmin upgrade, eg.
>
>> --dave
>
>> On Jan 5, 2009 12:22 AM, "Daniel Shahaf" <d....@daniel.shahaf.name> wrote:
>
>> Problem:
>> Race condition in 'svnadmin pack' may cause corruption (data loss).
>
>> Details:
>> See pack_shard() in fs_fs.c. Note the two svn_io_remove_dir2() calls.
>> Suppose two 'svnadmin pack' run in parallel, and that one of them
>> reaches the first call (which removes the pack dir) just after the first
>> of them reaches the second call (which removes the non-packed shard dir).
>
>> Result:
>> The whole shard (that the two 'svnadmin pack' worked on) is physically
>> deleted from the (OS) filesystem. Effectively, it does:
>
>> % rm -rf db/revs/5/
>> % rm -rf db/revs/5.pack/
>
>> and then (one of the svnadmin's) throws an error (because it tries to
>> pack the shard that had just been removed).
>
>> Proof:
>> I can reproduce this (on a testing shard_size=4 repository) by placing
>> breakpoints in the function around the remove_dir2() calls.
>
>> (s/place breakpoints/call svn_cmdline_prompt_user2/)
>
>> Fix:
>> Hyrum, who identified the problem on IRC, also suggested the solution of
>> adding a lock file to 'svnadmin pack'. That looks doable to me, although
>> it should require passing an FS object (svn_fs_t) to svn_fs_fs__pack (so
>> there is a mutex to pass to with_some_lock()).
>
>> I intend to look into a patch along these lines, but posting here for
>> comments on the approach.
>
>> Daniel
>
>> (For example, another way might be to abort if '5.pack' already exists
>> (rather than remove it and proceed). But would that work?)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
iEYEARECAAYFAkliMiEACgkQCwOubk4kUXzwxQCgrsZeZU9mo4ynGTlmnQs8G/Yk
0ekAnjJzkHomUbZU6XakJ2H2tELldmzz
=szwS
-----END PGP SIGNATURE-----
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005516
Re: packing race condition
Posted by David Glasser <gl...@davidglasser.net>.
FSFS has no read-write locks. It is a fundamental design goal of FSFS goal
that reads should never block; anything that changes that should have a very
very strong justification. (Not that you are here, AFAICT.)
--dave
On Jan 5, 2009 5:31 AM, "Hyrum K. Wright" <hy...@mail.utexas.edu>
wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
It doesn't use the write lock currently, but modifying it to do so would
probably solve the problem. When I originally wrote packing, I intended it
to
be single-threaded, and if somebody attempted to run two instances in
parallel,
well, Don't Do That. Using the write lock won't let 'svnadmin pack' run any
quicker, (i.e., the process isn't parallelized between two concurrent
instances), but it would prevent the race condition Daniel describes.
When we were chatting on IRC the other day, I wasn't sure if upgrade used
the
write lock, or a read-write lock (does FSFS even have such a thing?) and I
didn't have time to check. If we can get away with using the write-lock,
which
I believe we can, it seems to be the most maintainable solution going
forward.
As an aside, I've been thinking that letting pack notify of progress would
be
useful, especially for the big repositories on which it was intended to run.
Thoughts?
- -Hyrum
David Glasser wrote: > You mean svnadmin pack doesn't just work within the
FSFS write lock? That > ...
>
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004537
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkliC7kACgkQCwOubk4kUXzxuwCg2AMzpKrU00Gk3Dxd7YNqMy9M
ep4AoJ0HddavnWaeXtQtCleJhBO7uIyq
=+PGP
-----END PGP SIGNATURE-----
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1006002
Re: packing race condition
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Jan 05, 2009 at 07:31:38AM -0600, Hyrum K. Wright wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> It doesn't use the write lock currently, but modifying it to do so would
> probably solve the problem. When I originally wrote packing, I intended it to
> be single-threaded, and if somebody attempted to run two instances in parallel,
> well, Don't Do That. Using the write lock won't let 'svnadmin pack' run any
> quicker, (i.e., the process isn't parallelized between two concurrent
> instances), but it would prevent the race condition Daniel describes.
>
> When we were chatting on IRC the other day, I wasn't sure if upgrade used the
> write lock, or a read-write lock (does FSFS even have such a thing?) and I
> didn't have time to check. If we can get away with using the write-lock, which
> I believe we can, it seems to be the most maintainable solution going forward.
>
> As an aside, I've been thinking that letting pack notify of progress would be
> useful, especially for the big repositories on which it was intended to run.
> Thoughts?
Just a quick thought:
If this is something people might end up running from cron,
then locking should definitely be used.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005268
Re: packing race condition
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
It doesn't use the write lock currently, but modifying it to do so would
probably solve the problem. When I originally wrote packing, I intended it to
be single-threaded, and if somebody attempted to run two instances in parallel,
well, Don't Do That. Using the write lock won't let 'svnadmin pack' run any
quicker, (i.e., the process isn't parallelized between two concurrent
instances), but it would prevent the race condition Daniel describes.
When we were chatting on IRC the other day, I wasn't sure if upgrade used the
write lock, or a read-write lock (does FSFS even have such a thing?) and I
didn't have time to check. If we can get away with using the write-lock, which
I believe we can, it seems to be the most maintainable solution going forward.
As an aside, I've been thinking that letting pack notify of progress would be
useful, especially for the big repositories on which it was intended to run.
Thoughts?
- -Hyrum
David Glasser wrote:
> You mean svnadmin pack doesn't just work within the FSFS write lock? That
> would seem logical to me... fits with svnadmin upgrade, eg.
>
> --dave
>
> On Jan 5, 2009 12:22 AM, "Daniel Shahaf" <d....@daniel.shahaf.name> wrote:
>
> Problem:
> Race condition in 'svnadmin pack' may cause corruption (data loss).
>
> Details:
> See pack_shard() in fs_fs.c. Note the two svn_io_remove_dir2() calls.
> Suppose two 'svnadmin pack' run in parallel, and that one of them
> reaches the first call (which removes the pack dir) just after the first
> of them reaches the second call (which removes the non-packed shard dir).
>
> Result:
> The whole shard (that the two 'svnadmin pack' worked on) is physically
> deleted from the (OS) filesystem. Effectively, it does:
>
> % rm -rf db/revs/5/
> % rm -rf db/revs/5.pack/
>
> and then (one of the svnadmin's) throws an error (because it tries to
> pack the shard that had just been removed).
>
> Proof:
> I can reproduce this (on a testing shard_size=4 repository) by placing
> breakpoints in the function around the remove_dir2() calls.
>
> (s/place breakpoints/call svn_cmdline_prompt_user2/)
>
> Fix:
> Hyrum, who identified the problem on IRC, also suggested the solution of
> adding a lock file to 'svnadmin pack'. That looks doable to me, although
> it should require passing an FS object (svn_fs_t) to svn_fs_fs__pack (so
> there is a mutex to pass to with_some_lock()).
>
> I intend to look into a patch along these lines, but posting here for
> comments on the approach.
>
> Daniel
>
> (For example, another way might be to abort if '5.pack' already exists
> (rather than remove it and proceed). But would that work?)
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004476
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004537
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkliC7kACgkQCwOubk4kUXzxuwCg2AMzpKrU00Gk3Dxd7YNqMy9M
ep4AoJ0HddavnWaeXtQtCleJhBO7uIyq
=+PGP
-----END PGP SIGNATURE-----
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1005192
Re: packing race condition
Posted by David Glasser <gl...@davidglasser.net>.
You mean svnadmin pack doesn't just work within the FSFS write lock? That
would seem logical to me... fits with svnadmin upgrade, eg.
--dave
On Jan 5, 2009 12:22 AM, "Daniel Shahaf" <d....@daniel.shahaf.name> wrote:
Problem:
Race condition in 'svnadmin pack' may cause corruption (data loss).
Details:
See pack_shard() in fs_fs.c. Note the two svn_io_remove_dir2() calls.
Suppose two 'svnadmin pack' run in parallel, and that one of them
reaches the first call (which removes the pack dir) just after the first
of them reaches the second call (which removes the non-packed shard dir).
Result:
The whole shard (that the two 'svnadmin pack' worked on) is physically
deleted from the (OS) filesystem. Effectively, it does:
% rm -rf db/revs/5/
% rm -rf db/revs/5.pack/
and then (one of the svnadmin's) throws an error (because it tries to
pack the shard that had just been removed).
Proof:
I can reproduce this (on a testing shard_size=4 repository) by placing
breakpoints in the function around the remove_dir2() calls.
(s/place breakpoints/call svn_cmdline_prompt_user2/)
Fix:
Hyrum, who identified the problem on IRC, also suggested the solution of
adding a lock file to 'svnadmin pack'. That looks doable to me, although
it should require passing an FS object (svn_fs_t) to svn_fs_fs__pack (so
there is a mutex to pass to with_some_lock()).
I intend to look into a patch along these lines, but posting here for
comments on the approach.
Daniel
(For example, another way might be to abort if '5.pack' already exists
(rather than remove it and proceed). But would that work?)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004476
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1004537