Now filed as: https://issues.apache.org/jira/browse/SVN-4877
I am returning to this issue, which I first posted last year, as it has been
encountered it again in real life. I will explain from the beginning. (With
in-line responses to last year's questions.)
Please could anyone give the whole thing a read through and give me a sanity
check.
THE UNWANTED BEHAVIOUR
The code we are concerned with is a section of the FSFS commit finalization
code, in commit_body() called from svn_fs_fs__commit(), in
subversion/libsvn_fs_fs/transaction.c, around line 3800.
Within the following code section:
1. Get a write handle on the proto revision file.
2. Complete the construction of the proto-rev file.
3. Move the finished rev file into place.
4. Unlock the proto-rev file.
5. Continue with the rest of the commit finalization.
If an error is thrown during steps 2 and 3, and the calling process re-tries
the commit, the second try fails with:
160044 - Cannot write to the prototype revision file of transaction '1-2'
because a previous representation is currently being written by this process
at subversion/libsvn_fs_fs/transaction.c:312
What is happening:
- when the initial error is encountered, FSFS returns the error;
- FSFS does not call unlock_proto_rev()...
- ... so the proto-rev lock file remains on disk, with a file-lock held on it
and the "being_written" flag remaining set in the transaction object in memory;
- on re-try, when the code tries to acquire a proto-rev write lock, it finds
the lock is already held, and throws SVN_ERR_FS_REP_BEING_WRITTEN.
Subversion itself doesn't care. It never re-tries at this level. It always
discards the transaction if an error is returned.
Suppose it is not Subversion directly calling libsvn_fs_fs, but a third-party
intermediary process on the server. This process may have good reason to want
to re-try the operation. The WD replicator acts as an intermediary in this way
between libsvn_fs_fs and the higher layers of Subversion. When a call to FSFS
returns an error, it may re-try.
Plain FSFS suffers from a problem here, noted in comments in the commit code.
There is a window of opportunity for the commit to fail after its on-disk state
has been partly destroyed in the conversion to a final revision. In such a
case, a re-try could not succeed.
However, the replicator takes care to restore the txn files on disk to their
previous state, before re-trying the function call. By restoring the on-disk
state, the replicator should not suffer from this problem, and it should be
valid to re-try if it wants to.
This unexpected failure mode was discovered in a particular real-life case
where an error occurred during this phase of the commit. The replicator
re-tried, and unexpectedly got the "being written" error.
(In reply to my post last year, Daniel asked why the error message at the end
of the posted test output was ENOENT. That was the output of an additional test
case, one that was throwing an error after the move-into-place step, and
re-trying without restoring the on-disk txn files. That scenario is not in
scope for fixing this issue within svn. It is a case that will still remain
unrecoverable. I have removed that test case now.)
REPRODUCTION RECIPE
Attachment [1] instruments the commit code to produce an error in the relevant
section, on demand, controlled by an environment variable. With this, the test
output is "output-fakeerror-buggy" [3]. See "SUMMARY OF RESULTS" below.
DESIRED BEHAVIOUR
The commit re-try should fail in exactly the same way as the first try, if the
underlying cause of the error persists.
Or, if the underlying cause of the error was transient and has gone away, then
the commit should succeed.
PROPOSED FIX
My proposed fix is to unlock the lock when exiting that code section, no matter
whether successful or throwing an error.
Pseudo-Python:
try:
1. Get a write handle on the proto revision file.
2. Complete the construction of the proto-rev file.
3. Move the finished rev file into place.
finally:
4. Unlock the proto-rev file.
5. Continue with the rest of the commit finalization.
The patch I posted previously contained a proof-of-concept implementation using
alternatives for the SVN_ERR macros. I would suggest instead implementing this
by moving steps (1, 2, 3) into a sub-function, or, since step 1 already cleans
up if it fails, just steps 2 and 3.
The attached "i4877-unlock-protorev-fix-1.patch" [2] implements this, including
instrumentation similar to [1]. The test output is "output-fakeerror-fixed" [4].
(On my proof-of-concept patch last year, Daniel asked "Why is it correct to
call unlock_proto_rev() without checking the error
code svn_fs_fs__move_into_place() returned?". Hopefully this is now clear: it
is time to release the lock at that point, whether successful or throwing an
error.)
SUMMARY OF RESULTS
>From 'output-fakeerror-buggy':
try 1: 160004 - ### Fake failure point 2: Corrupt node-revision
try 2: 160044 - Cannot write to the prototype revision file of transaction
'1-2' because a previous representation is currently being written by this
process
>From 'output-fakeerror-fixed':
try 1: 160004 - ### Fake failure point 2: Corrupt node-revision
try 2: 160004 - ### Fake failure point 2: Corrupt node-revision
TODO:
1. The test case needs coding into svn's test suite.
2. There should be a test case for a transient error, a case where the re-try
succeeds.
Thoughts?
[1] i4877-simulated-fail-1.patch
[2] i4877-unlock-protorev-fix-1.patch
[3] output-fakeerror-buggy
[4] output-fakeerror-fixed
--
- JulianTHIS PATCH IS NOT INTENDED TO BE COMMITTED
For issue #4877 "FSFS commit failure should release txn proto-rev lock"
This patch is part of a reproduction recipe. It causes a simulated failure
in a FSFS commit, if en environment variable 'COMMIT_FAIL' is set to '1' or
'2' (choosing two different places where it may throw the error).
* subversion/libsvn_fs_fs/transaction.c
(SIMULATED_ERROR): New macro.
(commit_body): Conditionally, at two different points, throw an error
simulating a 'corrupt node-revision' condition.
--This line, and those below, will be ignored--
Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c (revision 1890876)
+++ subversion/libsvn_fs_fs/transaction.c (working copy)
@@ -3731,12 +3731,17 @@ struct commit_baton {
svn_fs_txn_t *txn;
apr_array_header_t *reps_to_cache;
apr_hash_t *reps_hash;
apr_pool_t *reps_pool;
};
+/* ### TESTING: Fake an error, if an environment variable is set */
+#define SIMULATED_ERROR(n) \
+ ((getenv("COMMIT_FAIL") && strcmp(getenv("COMMIT_FAIL"), #n) == 0) \
+ ? svn_error_createf(SVN_ERR_FS_CORRUPT, NULL, "### Fake failure point "#n": Corrupt node-revision") : SVN_NO_ERROR)
+
/* The work-horse for svn_fs_fs__commit, called with the FS write lock.
This implements the svn_fs_fs__with_write_lock() 'body' callback
type. BATON is a 'struct commit_baton *'. */
static svn_error_t *
commit_body(void *baton, apr_pool_t *pool)
{
@@ -3799,12 +3804,14 @@ commit_body(void *baton, apr_pool_t *poo
/* We are going to be one better than this puny old revision. */
new_rev = old_rev + 1;
/* Get a write handle on the proto revision file. */
SVN_ERR(get_writable_proto_rev(&proto_file, &proto_file_lockcookie,
cb->fs, txn_id, pool));
+ SVN_ERR(SIMULATED_ERROR(1));
+
SVN_ERR(svn_io_file_get_offset(&initial_offset, proto_file, pool));
/* Write out all the node-revisions and directory contents. */
root_id = svn_fs_fs__id_txn_create_root(txn_id, pool);
SVN_ERR(write_final_rev(&new_root_id, proto_file, new_rev, cb->fs, root_id,
start_node_id, start_copy_id, initial_offset,
@@ -3879,12 +3886,14 @@ commit_body(void *baton, apr_pool_t *poo
PATH_REVPROPS_DIR,
pool),
new_dir, pool));
}
}
+ SVN_ERR(SIMULATED_ERROR(2));
+
/* Move the finished rev file into place.
### This "breaks" the transaction by removing the protorev file
### but the revision is not yet complete. If this commit does
### not complete for any reason the transaction will be lost. */
old_rev_filename = svn_fs_fs__path_rev_absolute(cb->fs, old_rev, pool);
* subversion/libsvn_fs_fs/transaction.c
(commit_body):
--This line, and those below, will be ignored--
Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c (revision 1890835)
+++ subversion/libsvn_fs_fs/transaction.c (working copy)
@@ -3731,80 +3731,42 @@ struct commit_baton {
svn_fs_txn_t *txn;
apr_array_header_t *reps_to_cache;
apr_hash_t *reps_hash;
apr_pool_t *reps_pool;
};
-/* The work-horse for svn_fs_fs__commit, called with the FS write lock.
- This implements the svn_fs_fs__with_write_lock() 'body' callback
- type. BATON is a 'struct commit_baton *'. */
+/* ### TESTING: Fake an error, if an environment variable is set */
+#define TEST_COMMIT_FAIL(n) \
+ if (getenv("COMMIT_FAIL") && strcmp(getenv("COMMIT_FAIL"), #n) == 0) \
+ return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL, "### Fake failure point "#n": Corrupt node-revision");
+
+/* Write the proto-rev file, and move it into place.
+ *
+ * In this section of the commit, PROTO_FILE is writable and the caller
+ * holds a write lock on it.
+ *
+ */
static svn_error_t *
-commit_body(void *baton, apr_pool_t *pool)
+commit_write_proto_rev(apr_file_t *proto_file,
+ svn_revnum_t new_rev,
+ apr_uint64_t start_node_id,
+ apr_uint64_t start_copy_id,
+ apr_array_header_t *directory_ids,
+ const svn_fs_fs__id_part_t *txn_id,
+ apr_hash_t *changed_paths,
+ const char *old_rev_filename,
+ const struct commit_baton *cb,
+ apr_pool_t *pool)
{
- struct commit_baton *cb = baton;
fs_fs_data_t *ffd = cb->fs->fsap_data;
- const char *old_rev_filename, *rev_filename, *proto_filename;
- const char *revprop_filename;
+ const char *rev_filename, *proto_filename;
const svn_fs_id_t *root_id, *new_root_id;
- apr_uint64_t start_node_id;
- apr_uint64_t start_copy_id;
- svn_revnum_t old_rev, new_rev;
- apr_file_t *proto_file;
- void *proto_file_lockcookie;
apr_off_t initial_offset, changed_path_offset;
- const svn_fs_fs__id_part_t *txn_id = svn_fs_fs__txn_get_id(cb->txn);
- apr_hash_t *changed_paths;
- apr_array_header_t *directory_ids = apr_array_make(pool, 4,
- sizeof(pair_cache_key_t));
-
- /* Re-Read the current repository format. All our repo upgrade and
- config evaluation strategies are such that existing information in
- FS and FFD remains valid.
-
- Although we don't recommend upgrading hot repositories, people may
- still do it and we must make sure to either handle them gracefully
- or to error out.
-
- Committing pre-format 3 txns will fail after upgrade to format 3+
- because the proto-rev cannot be found; no further action needed.
- Upgrades from pre-f7 to f7+ means a potential change in addressing
- mode for the final rev. We must be sure to detect that cause because
- the failure would only manifest once the new revision got committed.
- */
- SVN_ERR(svn_fs_fs__read_format_file(cb->fs, pool));
-
- /* Read the current youngest revision and, possibly, the next available
- node id and copy id (for old format filesystems). Update the cached
- value for the youngest revision, because we have just checked it. */
- SVN_ERR(svn_fs_fs__read_current(&old_rev, &start_node_id, &start_copy_id,
- cb->fs, pool));
- ffd->youngest_rev_cache = old_rev;
- /* Check to make sure this transaction is based off the most recent
- revision. */
- if (cb->txn->base_rev != old_rev)
- return svn_error_create(SVN_ERR_FS_TXN_OUT_OF_DATE, NULL,
- _("Transaction out of date"));
-
- /* We need the changes list for verification as well as for writing it
- to the final rev file. */
- SVN_ERR(svn_fs_fs__txn_changes_fetch(&changed_paths, cb->fs, txn_id,
- pool));
+ TEST_COMMIT_FAIL(1);
- /* Locks may have been added (or stolen) between the calling of
- previous svn_fs.h functions and svn_fs_commit_txn(), so we need
- to re-examine every changed-path in the txn and re-verify all
- discovered locks. */
- SVN_ERR(verify_locks(cb->fs, txn_id, changed_paths, pool));
-
- /* We are going to be one better than this puny old revision. */
- new_rev = old_rev + 1;
-
- /* Get a write handle on the proto revision file. */
- SVN_ERR(get_writable_proto_rev(&proto_file, &proto_file_lockcookie,
- cb->fs, txn_id, pool));
SVN_ERR(svn_io_file_get_offset(&initial_offset, proto_file, pool));
/* Write out all the node-revisions and directory contents. */
root_id = svn_fs_fs__id_txn_create_root(txn_id, pool);
SVN_ERR(write_final_rev(&new_root_id, proto_file, new_rev, cb->fs, root_id,
start_node_id, start_copy_id, initial_offset,
@@ -3879,29 +3841,116 @@ commit_body(void *baton, apr_pool_t *poo
PATH_REVPROPS_DIR,
pool),
new_dir, pool));
}
}
+ TEST_COMMIT_FAIL(2);
+
/* Move the finished rev file into place.
### This "breaks" the transaction by removing the protorev file
### but the revision is not yet complete. If this commit does
### not complete for any reason the transaction will be lost. */
- old_rev_filename = svn_fs_fs__path_rev_absolute(cb->fs, old_rev, pool);
rev_filename = svn_fs_fs__path_rev(cb->fs, new_rev, pool);
proto_filename = svn_fs_fs__path_txn_proto_rev(cb->fs, txn_id, pool);
SVN_ERR(svn_fs_fs__move_into_place(proto_filename, rev_filename,
old_rev_filename, ffd->flush_to_disk,
pool));
+ return SVN_NO_ERROR;
+}
+
+/* The work-horse for svn_fs_fs__commit, called with the FS write lock.
+ This implements the svn_fs_fs__with_write_lock() 'body' callback
+ type. BATON is a 'struct commit_baton *'. */
+static svn_error_t *
+commit_body(void *baton, apr_pool_t *pool)
+{
+ struct commit_baton *cb = baton;
+ fs_fs_data_t *ffd = cb->fs->fsap_data;
+ const char *old_rev_filename;
+ const char *revprop_filename;
+ apr_uint64_t start_node_id;
+ apr_uint64_t start_copy_id;
+ svn_revnum_t old_rev, new_rev;
+ apr_file_t *proto_file;
+ void *proto_file_lockcookie;
+ const svn_fs_fs__id_part_t *txn_id = svn_fs_fs__txn_get_id(cb->txn);
+ apr_hash_t *changed_paths;
+ apr_array_header_t *directory_ids = apr_array_make(pool, 4,
+ sizeof(pair_cache_key_t));
+ svn_error_t *err2;
+
+ /* Re-Read the current repository format. All our repo upgrade and
+ config evaluation strategies are such that existing information in
+ FS and FFD remains valid.
+
+ Although we don't recommend upgrading hot repositories, people may
+ still do it and we must make sure to either handle them gracefully
+ or to error out.
+
+ Committing pre-format 3 txns will fail after upgrade to format 3+
+ because the proto-rev cannot be found; no further action needed.
+ Upgrades from pre-f7 to f7+ means a potential change in addressing
+ mode for the final rev. We must be sure to detect that cause because
+ the failure would only manifest once the new revision got committed.
+ */
+ SVN_ERR(svn_fs_fs__read_format_file(cb->fs, pool));
+
+ /* Read the current youngest revision and, possibly, the next available
+ node id and copy id (for old format filesystems). Update the cached
+ value for the youngest revision, because we have just checked it. */
+ SVN_ERR(svn_fs_fs__read_current(&old_rev, &start_node_id, &start_copy_id,
+ cb->fs, pool));
+ ffd->youngest_rev_cache = old_rev;
+
+ /* Check to make sure this transaction is based off the most recent
+ revision. */
+ if (cb->txn->base_rev != old_rev)
+ return svn_error_create(SVN_ERR_FS_TXN_OUT_OF_DATE, NULL,
+ _("Transaction out of date"));
+
+ /* We need the changes list for verification as well as for writing it
+ to the final rev file. */
+ SVN_ERR(svn_fs_fs__txn_changes_fetch(&changed_paths, cb->fs, txn_id,
+ pool));
+
+ /* Locks may have been added (or stolen) between the calling of
+ previous svn_fs.h functions and svn_fs_commit_txn(), so we need
+ to re-examine every changed-path in the txn and re-verify all
+ discovered locks. */
+ SVN_ERR(verify_locks(cb->fs, txn_id, changed_paths, pool));
+
+ /* We are going to be one better than this puny old revision. */
+ new_rev = old_rev + 1;
+
+ old_rev_filename = svn_fs_fs__path_rev_absolute(cb->fs, old_rev, pool);
+
+ /* Get a write handle on the proto revision file. */
+ SVN_ERR(get_writable_proto_rev(&proto_file, &proto_file_lockcookie,
+ cb->fs, txn_id, pool));
+
+ /* Write the proto-rev file, and move it into place. */
+ err2 = svn_error_trace(commit_write_proto_rev(
+ proto_file, new_rev,
+ start_node_id, start_copy_id,
+ directory_ids, txn_id,
+ changed_paths, old_rev_filename,
+ cb, pool));
+
/* Now that we've moved the prototype revision file out of the way,
we can unlock it (since further attempts to write to the file
will fail as it no longer exists). We must do this so that we can
remove the transaction directory later. */
- SVN_ERR(unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool));
+ /* If any error occurred while the proto-rev file was writable (locked),
+ we unlock it before returning the error, to give the caller the
+ possibility to re-try if it wants to. While Subversion itself never
+ re-tries at this point, at least one third-party caller (WD) does. */
+ SVN_ERR(svn_error_compose_create(err2,
+ unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool)));
/* Write final revprops file. */
SVN_ERR_ASSERT(! svn_fs_fs__is_packed_revprop(cb->fs, new_rev));
revprop_filename = svn_fs_fs__path_revprops(cb->fs, new_rev, pool);
SVN_ERR(write_final_revprop(revprop_filename, old_rev_filename,
cb->txn, ffd->flush_to_disk, pool));
output-fakeerror-buggy
Description: Binary data
output-fakeerror-fixed
Description: Binary data
