Ping: can anyone comment on this proposed patch?
- Julian
Julian Foad wrote:
CC'ing more possible experts Dmitry, Evgeny: any thoughts about whether
this change makes sense for FSFS commit?
- Julian
Julian Foad wrote:
TL;DR: I propose a change to the FSFS commit-transaction function, to
release the proto-rev write lock if an error occurs while it has this
lock.
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
