The practical applications of this change are rather obscure, which perhaps explains why it has not been needed before. In particular, it apparently is not needed for the way the rest of standard Subversion drives FSFS, but may be needed for other users of FSFS. I have come across this case in WANdisco's replicator, but as there are other peculiarities in how that drives FSFS, let us not confuse the issue by looking too closely at it. It appears the issue would apply to other users of FSFS too.
In the FSFS commit-transaction code path (in svn_fs_fs__commit) there is a region where it acquires an exclusive write lock on the prototype revision (proto-rev). There are cases where code in this region can fail, and there is no release of the lock in the error return path. That means if the calling process re-tries, the "writing" flag is still set in the transaction object in memory, and this causes an "already locked" error.
In regular Subversion we abandon a transaction if it fails at this stage, and so never hit the problem. There are failure modes where a re-try could not succeed, notably after we move the proto-rev file into its final location, breaking the transaction; this case is called out in comments in the function and will remain after this change. Abandoning the transaction is a safe and effective way to use FSFS. However, other users of FSFS may prefer to re-try in certain other cases.
The case WANdisco encountered is where some old repository corruption (SVN-4858) was detected and reported at some point in this code region. Although the commit would not be able to succeed, it was important to them that the same error should be reported again during a re-try, and what was observed instead was that the "already locked" error was thrown instead.
I suppose disk being temporarily inaccessible is one class of error where a re-try might be successful.
The attached test and patch demonstrate and fix the problem.This patch does not attempt to make it possible to re-try a failed commit in all cases. Some remaining cases are noted in the patch log message which is repeated here:
[[[Roll back the transaction lock state so re-trying a failing commit results in the same error again instead of an "already locked" error.
The problem was that when a commit returned a failure from within the code region where it held a transaction proto-rev lock, it did not unlock it. Although the FSFSWD replicator replaces the transaction files on disk, the lock status remained on the transaction object in memory and a subsequent retry then failed with "already locked" error instead of the same failure mode as the first attempt.
The solution here is to reset the lock status of the in-memory transaction object before returning a commit failure.
This implementation addresses cases where the commit fails and returns an error (e.g. due to detecting repository corruption at this point, as in the case reported in NV-7983), and the lock can be successfully unlocked using the regular unlock code path.
Cases not addressed:* There are conceivable failure modes where this regular unlocking would not succeed, e.g. disk files becoming inaccessible, and this patch would not address those cases. These could perhaps be addressed by adding a lock clean-up function that ignores errors in clean-up, and using that instead of the regular unlocking code.
* This implementation does not address cases where the process crashes in this code region. (In such cases the in-memory 'is writable' flag would not be preserved anyway so that is out of scope.)
### NOT YET TESTED:For FSFSWD, this implementation should also work where a failure occurs after moving the proto-rev file to the final revision-file location. FSFSWD re-copies the transaction directory before re-trying, and so this should succeed. For regular FSFS, it does not address this case: a re-try in this case will fail to open the transaction proto-rev file.
### This patch includes debugging code. * subversion/libsvn_fs_fs/transaction.c (TEST_COMMIT_FAIL): Debug code. (ERR_PROTO_REV_LOCKED): New macro.(commit_body): Use the new macro to handle errors in the region where a proto-rev lock is held, and unlock it in those cases.
]]]My question is, does this change (without the debugging code) make sense as an improvement to FSFS?
- Julian
test-release-proto-rev-lock-7.tgz
Description: application/compressed-tar
In FSFS commit, release the proto-rev lock on failure in the locked region.
This should allow a re-try of the commit to succeed, or to fail in the same
way as before, in certain cases where this was not previously possible.
Previously, a commit failure in this region would not release the proto-rev
lock and so any re-try attempt would return an "already locked" error.
The problem was that when a commit returned a failure from within the code
region where it held a transaction proto-rev lock, it did not unlock it.
Although the FSFSWD replicator replaces the transaction files on disk, the
lock status remained on the transaction object in memory and a subsequent
retry then failed with "already locked" error instead of the same failure
mode as the first attempt.
The solution here is to reset the lock status of the in-memory
transaction object before returning a commit failure.
This implementation addresses cases where the commit fails and returns an
error (e.g. due to detecting repository corruption at this point, as in the
case reported in NV-7983), and the lock can be successfully unlocked using
the regular unlock code path.
Cases not addressed:
* There are conceivable failure modes where this regular unlocking would
not succeed, e.g. disk files becoming inaccessible, and this patch would
not address those cases. These could perhaps be addressed by adding a
lock clean-up function that ignores errors in clean-up, and using that
instead of the regular unlocking code.
* This implementation does not address cases where the process crashes in
this code region. (In such cases the in-memory 'is writable' flag would
not be preserved anyway so that is out of scope.)
### NOT YET TESTED:
For FSFSWD, this implementation should also work where a failure occurs
after moving the proto-rev file to the final revision-file location. FSFSWD
re-copies the transaction directory before re-trying, and so this should
succeed. For regular FSFS, it does not address this case: a re-try in this
case will fail to open the transaction proto-rev file.
### This patch includes debugging code.
* subversion/libsvn_fs_fs/transaction.c
(TEST_COMMIT_FAIL): Debug code.
(ERR_PROTO_REV_LOCKED): New macro.
(commit_body): Use the new macro to handle errors in the region where a
proto-rev lock is held, and unlock it in those cases.
--This line, and those below, will be ignored--
Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c (revision 675)
+++ subversion/libsvn_fs_fs/transaction.c (working copy)
@@ -3740,12 +3740,20 @@ 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;
};
+/* ### TEST: let's say the commit errors out here... */
+#define TEST_COMMIT_FAIL(n) \
+ if (getenv("COMMIT_FAIL") && strcmp(getenv("COMMIT_FAIL"), #n) == 0) \
+ ERR_PROTO_REV_LOCKED(svn_error_createf(SVN_ERR_FS_CORRUPT, NULL, "### Supposed failure: Corrupt node-revision, line %d", __LINE__));
+
+#define ERR_PROTO_REV_LOCKED(expr) \
+ do { if ((err1 = svn_error_trace(expr))) goto on_err_proto_rev_locked; } while (0)
+
/* 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)
{
@@ -3761,12 +3769,13 @@ commit_body(void *baton, apr_pool_t *poo
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));
+ svn_error_t *err1;
/* 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
@@ -3808,30 +3817,34 @@ 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(svn_io_file_get_offset(&initial_offset, proto_file, pool));
+ TEST_COMMIT_FAIL(1);
+ ERR_PROTO_REV_LOCKED(svn_io_file_get_offset(&initial_offset, proto_file, pool));
+ TEST_COMMIT_FAIL(2);
/* 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,
+ ERR_PROTO_REV_LOCKED(write_final_rev(&new_root_id, proto_file, new_rev, cb->fs, root_id,
start_node_id, start_copy_id, initial_offset,
directory_ids, cb->reps_to_cache, cb->reps_hash,
cb->reps_pool, TRUE, pool));
+ TEST_COMMIT_FAIL(3);
/* Write the changed-path information. */
- SVN_ERR(write_final_changed_path_info(&changed_path_offset, proto_file,
+ ERR_PROTO_REV_LOCKED(write_final_changed_path_info(&changed_path_offset, proto_file,
cb->fs, txn_id, changed_paths,
pool));
+ TEST_COMMIT_FAIL(4);
if (svn_fs_fs__use_log_addressing(cb->fs))
{
/* Append the index data to the rev file. */
- SVN_ERR(svn_fs_fs__add_index_data(cb->fs, proto_file,
+ ERR_PROTO_REV_LOCKED(svn_fs_fs__add_index_data(cb->fs, proto_file,
svn_fs_fs__path_l2p_proto_index(cb->fs, txn_id, pool),
svn_fs_fs__path_p2l_proto_index(cb->fs, txn_id, pool),
new_rev, pool));
}
else
{
@@ -3839,19 +3852,23 @@ commit_body(void *baton, apr_pool_t *poo
svn_stringbuf_t *trailer
= svn_fs_fs__unparse_revision_trailer
((apr_off_t)svn_fs_fs__id_item(new_root_id),
changed_path_offset,
pool);
- SVN_ERR(svn_io_file_write_full(proto_file, trailer->data, trailer->len,
+ ERR_PROTO_REV_LOCKED(svn_io_file_write_full(proto_file, trailer->data, trailer->len,
NULL, pool));
}
+ TEST_COMMIT_FAIL(5);
if (ffd->flush_to_disk)
- SVN_ERR(svn_io_file_flush_to_disk(proto_file, pool));
- SVN_ERR(svn_io_file_close(proto_file, pool));
+ ERR_PROTO_REV_LOCKED(svn_io_file_flush_to_disk(proto_file, pool));
+ TEST_COMMIT_FAIL(6);
+
+ ERR_PROTO_REV_LOCKED(svn_io_file_close(proto_file, pool));
+ TEST_COMMIT_FAIL(7);
/* We don't unlock the prototype revision file immediately to avoid a
race with another caller writing to the prototype revision file
before we commit it. */
/* Create the shard for the rev and revprop file, if we're sharding and
@@ -3865,13 +3882,13 @@ commit_body(void *baton, apr_pool_t *poo
= svn_fs_fs__path_rev_shard(cb->fs, new_rev, pool);
svn_error_t *err
= svn_io_dir_make(new_dir, APR_OS_DEFAULT, pool);
if (err && !APR_STATUS_IS_EEXIST(err->apr_err))
return svn_error_trace(err);
svn_error_clear(err);
- SVN_ERR(svn_io_copy_perms(svn_dirent_join(cb->fs->path,
+ ERR_PROTO_REV_LOCKED(svn_io_copy_perms(svn_dirent_join(cb->fs->path,
PATH_REVS_DIR,
pool),
new_dir, pool));
}
/* Create the revprops shard. */
@@ -3881,30 +3898,32 @@ commit_body(void *baton, apr_pool_t *poo
= svn_fs_fs__path_revprops_shard(cb->fs, new_rev, pool);
svn_error_t *err
= svn_io_dir_make(new_dir, APR_OS_DEFAULT, pool);
if (err && !APR_STATUS_IS_EEXIST(err->apr_err))
return svn_error_trace(err);
svn_error_clear(err);
- SVN_ERR(svn_io_copy_perms(svn_dirent_join(cb->fs->path,
+ ERR_PROTO_REV_LOCKED(svn_io_copy_perms(svn_dirent_join(cb->fs->path,
PATH_REVPROPS_DIR,
pool),
new_dir, pool));
}
}
+ TEST_COMMIT_FAIL(8);
/* 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,
+ ERR_PROTO_REV_LOCKED(svn_fs_fs__move_into_place(proto_filename, rev_filename,
old_rev_filename, ffd->flush_to_disk,
pool));
+ TEST_COMMIT_FAIL(9);
/* 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));
@@ -3939,12 +3958,20 @@ commit_body(void *baton, apr_pool_t *poo
SVN_ERR(promote_cached_directories(cb->fs, directory_ids, pool));
/* Remove this transaction directory. */
SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
return SVN_NO_ERROR;
+
+
+on_err_proto_rev_locked:
+ /* If any error occurred while the proto-rev file was writable (locked),
+ * unlock it before returning to clean up as best we can. */
+ err1 = svn_error_compose_create(err1,
+ unlock_proto_rev(cb->fs, txn_id, proto_file_lockcookie, pool));
+ return err1;
}
/* Add the representations in REPS_TO_CACHE (an array of representation_t *)
* to the rep-cache database of FS. */
static svn_error_t *
write_reps_to_cache(svn_fs_t *fs,
