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 2006/10/02 11:20:03 UTC
[PATCH] Fixing the FSFS corruption bug, again
Here's an updated patch to (hopefully) fix the FSFS corruption problem,
this time using a method that works on NFS filesystems.
See http://svn.haxx.se/dev/archive-2006-09/0078.shtml and issue #2467
for background.
Comments would be appreciated, as would test results from OS X or Windows.
Regards,
Malcolm
[[[
FSFS: Detect a particular class of FS API violation (encountered while
modifying a transaction) and return an error instead of corrupting the
eventual revision file.
(There is evidence that suggests that mod_dav_svn is currently committing
this API violation; see issue 2467, of which this may be the root cause)
Prevent concurrent writes to the prototype revision file (whether by
writing a representation or committing the transaction) by:
1. Introducing a framework for storing per-filesystem and per-transaction
shared data (i.e. data that is shared between all threads in a
process).
2. Using that framework to check whether any threads in the current
process are writing to the proto-revision file.
3. Creating a 'rev-lock' file in the transaction directory to check
whether any other processes are writing to the proto-revision file.
* subversion/include/svn_fs.h
(svn_fs_apply_text): Document the requirement that the caller close
the returned stream before further operating on the transaction,
using the same wording as svn_fs_apply_textdelta() already provides.
* subversion/include/svn_error_codes.h
(SVN_ERR_FS_REP_BEING_WRITTEN): New.
* subversion/libsvn_fs_fs/fs.h
(fs_fs_shared_txn_data_t): New. Shared (one per process) object to
hold shared data for an active transaction in a filesystem.
(fs_fs_shared_data_t): New. Shared (one per process) object to hold
shared data for a filesystem.
(fs_fs_data_t): Extend to contain a reference to an explicit
fs_fs_shared_data_t object rather that just the single write-lock
mutex.
* subversion/libsvn_fs_fs/fs.c
(SVN_FSFS_LOCK_USERDATA_PREFIX): Remove, in favour of the more general...
(SVN_FSFS_SHARED_USERDATA_PREFIX): New.
(fs_serialized_init): Allocate an explicit 'shared filesystem data'
object, and store that in the userdata pool rather than just the
write-lock mutex. Initialise the contents of that object, including
the mutexes to serialise write access to the filesystem and read/write
access to the transaction list.
* subversion/libsvn_fs_fs/structure
(Transaction layout) Document the new 'rev-lock' file and locking
mechanism.
* subversion/libsvn_fs_fs/fs_fs.c
(PATH_REV_LOCK, path_txn_proto_rev_lock): New.
(get_shared_txn, free_shared_txn): New. Add and remove transactions
from the shared transaction list.
(with_txnlist_lock): New. Run a function while holding the lock for
the shared transaction list.
(struct unlock_proto_rev_baton, unlock_proto_rev_body,
unlock_proto_rev):
New. Unlock the prototype revision file.
(unlock_proto_rev_list_locked): New. Ditto, but for the case where
the shared transaction list is already locked.
(struct get_writable_proto_rev_baton, get_writable_proto_rev_body,
get_writable_proto_rev): New. Lock the prototype revision file and
return a writable file handle to it.
(purge_shared_txn_body, purge_shared_txn): New. Remove a transaction
from the shared transaction list.
(svn_fs_fs__create_txn): Create an empty 'rev-lock' file when creating
a transaction.
(svn_fs_fs__purge_txn): Remove the shared transaction object before
removing the transaction directory.
(struct rep_write_baton): Keep track of the returned lock cookie from
get_writable_proto_rev() so that the proto-rev file can be unlocked.
(rep_write_get_baton): Call get_writable_proto_rev() to open the
proto-rev file, stashing the cookie returned into the rep_write_baton).
(rep_write_contents_close): Unlock the proto-rev file using the cookie.
(svn_fs_fs__with_write_lock): Account for the movement of the
filesystem write-lock mutex into the explicit per-filesystem object.
(commit_body): Call get_writable_proto_rev() to open the proto-rev file,
and call unlock_proto_rev() to unlock it after we've finalised it.
]]]
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Oct 02, 2006 at 10:54:58PM +0100, Malcolm Rowe wrote:
> Yes, I was going to ask: is it safe to split translated messages?
>
> _("That is, "
> "like this?")
>
Replying to myself: of course it is - we do it in some of the help
output, if nowhere else.
> I was worried that doing so might break the translation tools.
>
(I've no idea where I got that idea.)
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Oct 03, 2006 at 09:06:05AM -0400, Garrett Rooney wrote:
> Yeah, I just happened to miss that bit. Now that I've read it, I
> understand totally.
>
Excellent!
I committed a version of this patch (sans enormous lines) in r21738.
> >My only concern with the approach I've taken is the possibility of
> >accreting a collection of transactions that have neither been committed
> >nor aborted; these would lie around in memory in the same way they'd lie
> >around on disk. On the other hand, that shouldn't be that common a
> >problem, should it?
>
> I'm not sure there's a real work around for it anyway.
>
I agree; I can't see a simple way to solve this short of
reference-counting the shared transaction objects, and adding pool
cleanups to the svn_txn_t's pools. And even then, I'd be worried about
pool destruction ordering.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/2/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Mon, Oct 02, 2006 at 05:24:38PM -0400, Garrett Rooney wrote:
> > This seems to work just fine on OS X and Linux, FWIW, but there's at
> > least one thing that worries me. The locking scheme used for managing
> > the per-txn stuff really could use some commenting, since I've been
> > staring at it for at least half an hour now trying to make sure I've
> > got it right.
> >
>
> Heh. As I mentioned on IRC, that was part of the code I'd thought I'd
> _over_ commented :-)
>
> The relevant comments are in libsvn_fs_fs/fs.h, where the structure is
> defined:
>
> /* A list of shared transaction objects for each transaction that is
> currently active, or NULL if none are. All access to this list,
> including the contents of the objects stored in it, is synchronised
> under TXN_LIST_LOCK. */
> fs_fs_shared_txn_data_t *txns;
>
> I thought that was pretty clear, but I'm probably too close to the
> problem; can anyone else suggest a better wording to that or any of the
> other comments?
Yeah, I just happened to miss that bit. Now that I've read it, I
understand totally.
> > Just to be clear, there's a single per-filesystem mutex that's used
> > within the with_txnlist_lock function to serialize access to both the
> > list of transactions and some of the data within the transactions. We
> > grab that lock, grab the flock, then note "hey, i've got it locked",
> > then drop the mutex. Later on when someone else tries to modify the
> > txn they'll be able to lock the per-filesystem mutex, but at that
> > point they'll see that someone else is modifying the txn, and error
> > out before they try to grab the flock? Other processes will simply
> > block when trying to flock.
> >
>
> s/some of the data/all of the data in that structure/, but otherwise,
> that's entirely correct. Also note that 'access' in the comment above
> means reading as well as writing.
>
> > I guess the part that confused me was how we've got a single per-fs
> > mutex, but per-txn lock files. In retrospect it makes sense, but it
> > was a bit weird at first. Other than that, this all seems fine, and I
> > think the confusion could be taken care of by some comments that sum
> > up the overall locking strategy, especially the fact that the
> > txn_list_lock ends up protecting both the list and the being_written
> > field.
> >
>
> Well, the way I was thinking was:
>
> - there's ->txn_list_lock, which is a mutex that protects all access to
> the txns list (and also the free-txn member).
> - there's txn->being_written, which is the intra-process lock on the
> transaction. It doesn't have to be a heavyweight mutex, since we
> serialise all access to the list.
> - finally, there's the transaction lockfiles themselves, which we
> lock if it's safe to attempt to do so.
>
> > Oh, and you've also got some mega-long lines in there that need to be
> > wrapped, but I'm sure you would have taken care of that before
> > committing.
> >
>
> Yes, I was going to ask: is it safe to split translated messages?
>
> _("That is, "
> "like this?")
>
> I was worried that doing so might break the translation tools.
>
>
> My only concern with the approach I've taken is the possibility of
> accreting a collection of transactions that have neither been committed
> nor aborted; these would lie around in memory in the same way they'd lie
> around on disk. On the other hand, that shouldn't be that common a
> problem, should it?
I'm not sure there's a real work around for it anyway.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Oct 02, 2006 at 05:24:38PM -0400, Garrett Rooney wrote:
> This seems to work just fine on OS X and Linux, FWIW, but there's at
> least one thing that worries me. The locking scheme used for managing
> the per-txn stuff really could use some commenting, since I've been
> staring at it for at least half an hour now trying to make sure I've
> got it right.
>
Heh. As I mentioned on IRC, that was part of the code I'd thought I'd
_over_ commented :-)
The relevant comments are in libsvn_fs_fs/fs.h, where the structure is
defined:
/* A list of shared transaction objects for each transaction that is
currently active, or NULL if none are. All access to this list,
including the contents of the objects stored in it, is synchronised
under TXN_LIST_LOCK. */
fs_fs_shared_txn_data_t *txns;
I thought that was pretty clear, but I'm probably too close to the
problem; can anyone else suggest a better wording to that or any of the
other comments?
> Just to be clear, there's a single per-filesystem mutex that's used
> within the with_txnlist_lock function to serialize access to both the
> list of transactions and some of the data within the transactions. We
> grab that lock, grab the flock, then note "hey, i've got it locked",
> then drop the mutex. Later on when someone else tries to modify the
> txn they'll be able to lock the per-filesystem mutex, but at that
> point they'll see that someone else is modifying the txn, and error
> out before they try to grab the flock? Other processes will simply
> block when trying to flock.
>
s/some of the data/all of the data in that structure/, but otherwise,
that's entirely correct. Also note that 'access' in the comment above
means reading as well as writing.
> I guess the part that confused me was how we've got a single per-fs
> mutex, but per-txn lock files. In retrospect it makes sense, but it
> was a bit weird at first. Other than that, this all seems fine, and I
> think the confusion could be taken care of by some comments that sum
> up the overall locking strategy, especially the fact that the
> txn_list_lock ends up protecting both the list and the being_written
> field.
>
Well, the way I was thinking was:
- there's ->txn_list_lock, which is a mutex that protects all access to
the txns list (and also the free-txn member).
- there's txn->being_written, which is the intra-process lock on the
transaction. It doesn't have to be a heavyweight mutex, since we
serialise all access to the list.
- finally, there's the transaction lockfiles themselves, which we
lock if it's safe to attempt to do so.
> Oh, and you've also got some mega-long lines in there that need to be
> wrapped, but I'm sure you would have taken care of that before
> committing.
>
Yes, I was going to ask: is it safe to split translated messages?
_("That is, "
"like this?")
I was worried that doing so might break the translation tools.
My only concern with the approach I've taken is the possibility of
accreting a collection of transactions that have neither been committed
nor aborted; these would lie around in memory in the same way they'd lie
around on disk. On the other hand, that shouldn't be that common a
problem, should it?
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/2/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> Here's an updated patch to (hopefully) fix the FSFS corruption problem,
> this time using a method that works on NFS filesystems.
>
> See http://svn.haxx.se/dev/archive-2006-09/0078.shtml and issue #2467
> for background.
>
> Comments would be appreciated, as would test results from OS X or Windows.
This seems to work just fine on OS X and Linux, FWIW, but there's at
least one thing that worries me. The locking scheme used for managing
the per-txn stuff really could use some commenting, since I've been
staring at it for at least half an hour now trying to make sure I've
got it right.
Just to be clear, there's a single per-filesystem mutex that's used
within the with_txnlist_lock function to serialize access to both the
list of transactions and some of the data within the transactions. We
grab that lock, grab the flock, then note "hey, i've got it locked",
then drop the mutex. Later on when someone else tries to modify the
txn they'll be able to lock the per-filesystem mutex, but at that
point they'll see that someone else is modifying the txn, and error
out before they try to grab the flock? Other processes will simply
block when trying to flock.
I guess the part that confused me was how we've got a single per-fs
mutex, but per-txn lock files. In retrospect it makes sense, but it
was a bit weird at first. Other than that, this all seems fine, and I
think the confusion could be taken care of by some comments that sum
up the overall locking strategy, especially the fact that the
txn_list_lock ends up protecting both the list and the being_written
field.
Oh, and you've also got some mega-long lines in there that need to be
wrapped, but I'm sure you would have taken care of that before
committing.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixing the FSFS corruption bug, again
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Oct 02, 2006 at 12:20:03PM +0100, Malcolm Rowe wrote:
> Here's an updated patch to (hopefully) fix the FSFS corruption problem,
> this time using a method that works on NFS filesystems.
>
Oopsy. The attached additional patch is needed to fix compiles when
!APR_HAS_THREADS.
Regards,
Malcolm