You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2020/06/10 16:10:45 UTC
FSFS commit failure should release txn proto-rev lock
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
Re: FSFS commit failure should release txn proto-rev lock
Posted by Julian Foad <ju...@apache.org>.
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
>
Re: FSFS commit failure should release txn proto-rev lock
Posted by Julian Foad <ju...@apache.org>.
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
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 29 juni 2021 kl 12:33 skrev Julian Foad <ju...@apache.org>:
> Daniel Shahaf wrote:
> > Julian Foad wrote on Fri, Jun 25, 2021 at 14:27:30 +0100:
> > > Now filed as: https://issues.apache.org/jira/browse/SVN-4877
> >
> > "Now"? The issue is a month old.
>
> You're absolutely right! That's when I was made aware this issue had come
> up again. First I noticed we had not previously filed an issue, so I
> started by filing it as #4877, and started drafting this email. With life
> getting in the way, it has taken me until now (ahem, I mean June 25th) to
> re-check everything and put this email together with the clearest
> explanation I could.
>
> Thank you for taking another look at this.
>
> There could be two angles to view this issue. One angle is looking at the
> specifics of why WD's replicator wants different behaviour, which I will go
> into further below.
>
> Another angle could be looking from a software design point of view and
> saying of course code like this should clean up like this. I am hopeful
> that someone may be able to see it from this angle and say of course this
> change is right in principle, and maybe point out if I got the specifics
> right or missed something.
>
This was my reflection when I read the first mail - although I don't yet
know enough of the specifics of Subversion to say if it is reasonable. My
philosophy is that whenever something fails, the code should try to roll
back as much as possible so the caller could retry. For example if I
fopen() two files and the second fail, I tend to close the first one before
returning an error message. Then the caller can figure out why the second
failed - potentially recovering and retrying.
> > > 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.
>
catch:
set a flag to abort after finally
(but maybe that was implict)
> > > finally:
> > > 4. Unlock the proto-rev file.
> > > 5. Continue with the rest of the commit finalization.
> > >
> >
> > Why would this be correct? Why would this have no downside?
>
> Would it be easier to see from this angle: that would make the behaviour
> for a re-try within one calling process equivalent to the existing
> behaviour that would occur if the FSFS machine restarts and a new calling
> process re-tries.
Could there be other parts where "state" is set in someway that makes it
impossible to restart with the same calling process?
Kind regards,
Daniel Sahlberg
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, 06 Jul 2021 01:23 +00:00:
> Julian Foad wrote on Tue, Jun 29, 2021 at 11:32:24 +0100:
> > Daniel Shahaf wrote:
> > > Julian Foad wrote on Fri, Jun 25, 2021 at 14:27:30 +0100:
> > There could be two angles to view this issue. One angle is looking at
> > the specifics of why WD's replicator wants different behaviour, which
> > I will go into further below.
> >
> > Another angle could be looking from a software design point of view
> > and saying of course code like this should clean up like this. I am
> > hopeful that someone may be able to see it from this angle and say of
> > course this change is right in principle, and maybe point out if I got
> > the specifics right or missed something.
> >
>
> I'm viewing this from the latter angle: as a matter of libsvn_fs_fs's
> behaviour, viewed as a single, reusable component. WD's needs are
> interesting as context but are not to be considered design constraints
> on potential fixes. (For one, I suspect designing an API to WD's needs
> would risk ASF's 501(c)3 status.)
>
> > > > 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.
> > >
> > > I think it's possible that the second try would legitimately fail with
> > > a different error message than the first try.
> >
> > Yes, that remains a third possibility, where the underlying cause of
> > the error changed. The problematic cases are the other two.
> >
> > In the case of a persistent underlying error condition, as best I
> > understand, the concern is that the replicator should be able to
> > report the error condition consistently. Why does it matter? Remember
> ⋮
> > Bear in mind that the replicator does not know about the specifics of
> > all the possible svn error codes, it needs to apply this logic
> > uniformly to all error responses (and success responses) from all
> > APIs.
>
> I decline to respond as that would constitute design discussion of
> non-FLOSS product(s).
…and as such, would be off-topic for this list, being an ASF list.
> > > Thus, it's not clear what the desired _change_ in behaviour is.
> >
> > Is that clearer now?
> >
>
> No.
>
> > > > 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.
> > > >
> > >
> > > Why would this be correct? Why would this have no downside?
> >
> > Would it be easier to see from this angle: that would make the behaviour for a re-try within one calling process equivalent to the existing behaviour that would occur if the FSFS machine restarts and a new calling process re-tries.
>
> And are we sure that codepath is crash-safe?
>
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Tue, Jun 29, 2021 at 11:32:24 +0100:
> Daniel Shahaf wrote:
> > Julian Foad wrote on Fri, Jun 25, 2021 at 14:27:30 +0100:
> There could be two angles to view this issue. One angle is looking at
> the specifics of why WD's replicator wants different behaviour, which
> I will go into further below.
>
> Another angle could be looking from a software design point of view
> and saying of course code like this should clean up like this. I am
> hopeful that someone may be able to see it from this angle and say of
> course this change is right in principle, and maybe point out if I got
> the specifics right or missed something.
>
I'm viewing this from the latter angle: as a matter of libsvn_fs_fs's
behaviour, viewed as a single, reusable component. WD's needs are
interesting as context but are not to be considered design constraints
on potential fixes. (For one, I suspect designing an API to WD's needs
would risk ASF's 501(c)3 status.)
> > > 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.
> >
> > I think it's possible that the second try would legitimately fail with
> > a different error message than the first try.
>
> Yes, that remains a third possibility, where the underlying cause of
> the error changed. The problematic cases are the other two.
>
> In the case of a persistent underlying error condition, as best I
> understand, the concern is that the replicator should be able to
> report the error condition consistently. Why does it matter? Remember
⋮
> Bear in mind that the replicator does not know about the specifics of
> all the possible svn error codes, it needs to apply this logic
> uniformly to all error responses (and success responses) from all
> APIs.
I decline to respond as that would constitute design discussion of
non-FLOSS product(s).
> > Thus, it's not clear what the desired _change_ in behaviour is.
>
> Is that clearer now?
>
No.
> > > 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.
> > >
> >
> > Why would this be correct? Why would this have no downside?
>
> Would it be easier to see from this angle: that would make the behaviour for a re-try within one calling process equivalent to the existing behaviour that would occur if the FSFS machine restarts and a new calling process re-tries.
And are we sure that codepath is crash-safe?
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Julian Foad wrote on Fri, Jun 25, 2021 at 14:27:30 +0100:
> > Now filed as: https://issues.apache.org/jira/browse/SVN-4877
>
> "Now"? The issue is a month old.
You're absolutely right! That's when I was made aware this issue had come up again. First I noticed we had not previously filed an issue, so I started by filing it as #4877, and started drafting this email. With life getting in the way, it has taken me until now (ahem, I mean June 25th) to re-check everything and put this email together with the clearest explanation I could.
Thank you for taking another look at this.
There could be two angles to view this issue. One angle is looking at the specifics of why WD's replicator wants different behaviour, which I will go into further below.
Another angle could be looking from a software design point of view and saying of course code like this should clean up like this. I am hopeful that someone may be able to see it from this angle and say of course this change is right in principle, and maybe point out if I got the specifics right or missed something.
[...]
> Okay, so it fails with a wrong error message.
In a case where the underlying cause of the error has remained the same, that would be a correct statement; in a case where the underlying cause was transient and has gone away, it could have succeeded on re-try, and the problem is it is failing where it should succeed. Both of these cases are problematic, as explained further below.
> > Subversion itself doesn't care. It never re-tries at this level. It always discards the transaction if an error is returned.
>
> Even under mod_dav_svn when the client is a generic DAV client?
Yes: in dav_svn__checkin(), if svn_repos_fs_commit_txn() fails before finalizing the commit, then -> dav_svn__abort_txn -> svn_fs_abort_txn.
> > 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.
>
> I think it's possible that the second try would legitimately fail with
> a different error message than the first try.
Yes, that remains a third possibility, where the underlying cause of the error changed. The problematic cases are the other two.
In the case of a persistent underlying error condition, as best I understand, the concern is that the replicator should be able to report the error condition consistently. Why does it matter? Remember that the client (of fsfs) in this case is managing replicated repositories. From one high-level commit operation, it creates several FSFS commit operations on the several replicas. When these replicas return errors, it needs to be able to consolidate these errors into a single result to return to the higher level svn code. If all the replicas have the same underlying cause of error, and they all return the same error code, the replicator can consolidate those into a single result and return that to the higher level caller which will then do what Subversion does, and that's fine. But if some replicas return different error codes from others, the replicator has to assume that replication has failed and the repositories are out of sync in some way, and that is a "hard error" condition requiring manual intervention. (Something like that; I don't know all the details.)
On top of that, before assuming that an error is persistent, the replicator needs be able to re-try operations, in general and at will, because at various levels there can be temporary conditions that are local to one replica which may go away on re-try and end up successful. Although we are focusing on the internals of a FSFS API here, in general these re-try scenarios extend to things like re-trying if the operation is interrupted by the machine crashing and restarting. The operations used need to be idempotent to guarantee this kind of re-trying and recovery.
So, if a FSFS operation returns an error in one replica, the replicator first re-tries on that replica in case it was a local and temporary error. If the re-try also fails, then it is time to report the error, and the replicator will compare this with the error (or success) result that the other replicas have encountered, to decide whether it is a consistent response that should be handed back to the higher level caller, or if it is inconsistent then the replicas have gone out of sync and manual intervention is necessary to repair the the system.
Bear in mind that the replicator does not know about the specifics of all the possible svn error codes, it needs to apply this logic uniformly to all error responses (and success responses) from all APIs.
> Thus, it's not clear what the desired _change_ in behaviour is.
Is that clearer now?
> > 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.
> >
>
> Why would this be correct? Why would this have no downside?
Would it be easier to see from this angle: that would make the behaviour for a re-try within one calling process equivalent to the existing behaviour that would occur if the FSFS machine restarts and a new calling process re-tries.
--
- Julian
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Fri, Jun 25, 2021 at 14:27:30 +0100:
> Now filed as: https://issues.apache.org/jira/browse/SVN-4877
>
"Now"? The issue is a month old.
> 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.
>
Okay, so it fails with a wrong error message.
> Subversion itself doesn't care. It never re-tries at this level. It always discards the transaction if an error is returned.
>
Even under mod_dav_svn when the client is a generic DAV client?
> 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.
>
I think it's possible that the second try would legitimately fail with
a different error message than the first try.
Thus, it's not clear what the desired _change_ in behaviour is.
> 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.
>
Why would this be correct? Why would this have no downside?
Re: SVN-4877 FSFS commit failure should release txn proto-rev lock
Posted by Julian Foad <ju...@apache.org>.
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
--
- Julian
Re: FSFS commit failure should release txn proto-rev lock
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, 30 Jul 2020 09:11 +00:00:
> (Off the top of my head, did sub tests 1-8 fail with "already locked"
> and 9 is the odd one out?)
In the file "output" inside the tarball you sent, yes. I haven't run
the tests on my machine.
Re: FSFS commit failure should release txn proto-rev lock
Posted by Julian Foad <ju...@apache.org>.
Thanks for the response, Daniel. I'm away and it will be a few days before I can check and respond completely.
(Off the top of my head, did sub tests 1-8 fail with "already locked" and 9 is the odd one out?)
30 Jul 2020 00:00:32 Daniel Shahaf <d....@daniel.shahaf.name>:
> (I wasn't going to comment on this since I don't know this part of the
> code too well, but given the crickets…)
>
> So, the undesired behaviour is this:
>
>> This commit failed unexpectedly:
>> Traceback (most recent call last):
>> File "/home/julianfoad/tmp/svn/wd-nv-7983/test-release-proto-rev-lock-7/test-commits.py", line 59, in <module>
>> fs.commit_txn(txn2)
>> File "/home/julianfoad/build/subversion-c/subversion/bindings/swig/python/libsvn/fs.py", line 324, in svn_fs_commit_txn
>> return _fs.svn_fs_commit_txn(*args)
>> svn.core.SubversionException:
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs/fs-loader.c:885
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/tree.c:2358
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:4010
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:368
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:240
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:228
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:3813
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:475
>> 2 - Can't open file '/home/julianfoad/tmp/svn/wd-nv-7983/repo/db/txn-protorevs/1-a.rev': No such file or directory
>> at /home/julianfoad/src/subversion-c/subversion/libsvn_subr/io.c:3931
>> + echo 'FAILED: sub-test 9'
>> FAILED: sub-test 9
>>
> First of all, you said the error message read "already locked". The
> error above is ENOENT. Can you clarify this?
>
> The change proposed is this:
>
> Julian Foad wrote on Wed, 10 Jun 2020 17:10 +0100:
>> @@ -3881,30 +3898,32 @@ commit_body(void *baton, apr_pool_t *poo
>> /* 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;
>> }
>>
>> Why is it correct to call unlock_proto_rev() without checking the error
> code svn_fs_fs__move_into_place() returned?
>
> HTH,
>
> Daniel
>
> P.S. Suggest to pass the name of the local variable ERR1 as
> a parameter to the macro to avoid action at a distance.
>
Re: FSFS commit failure should release txn proto-rev lock
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
(I wasn't going to comment on this since I don't know this part of the
code too well, but given the crickets…)
So, the undesired behaviour is this:
> This commit failed unexpectedly:
> Traceback (most recent call last):
> File "/home/julianfoad/tmp/svn/wd-nv-7983/test-release-proto-rev-lock-7/test-commits.py", line 59, in <module>
> fs.commit_txn(txn2)
> File "/home/julianfoad/build/subversion-c/subversion/bindings/swig/python/libsvn/fs.py", line 324, in svn_fs_commit_txn
> return _fs.svn_fs_commit_txn(*args)
> svn.core.SubversionException:
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs/fs-loader.c:885
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/tree.c:2358
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:4010
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:368
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:240
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/fs_fs.c:228
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:3813
> at /home/julianfoad/src/subversion-c/subversion/libsvn_fs_fs/transaction.c:475
> 2 - Can't open file '/home/julianfoad/tmp/svn/wd-nv-7983/repo/db/txn-protorevs/1-a.rev': No such file or directory
> at /home/julianfoad/src/subversion-c/subversion/libsvn_subr/io.c:3931
> + echo 'FAILED: sub-test 9'
> FAILED: sub-test 9
First of all, you said the error message read "already locked". The
error above is ENOENT. Can you clarify this?
The change proposed is this:
Julian Foad wrote on Wed, 10 Jun 2020 17:10 +0100:
> @@ -3881,30 +3898,32 @@ commit_body(void *baton, apr_pool_t *poo
> /* 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;
> }
>
Why is it correct to call unlock_proto_rev() without checking the error
code svn_fs_fs__move_into_place() returned?
HTH,
Daniel
P.S. Suggest to pass the name of the local variable ERR1 as
a parameter to the macro to avoid action at a distance.