protorev file handling in svn_fs_commit_txn

[Philip Martin <ph...@wandisco.com>]

Suppose svn_fs_commit_txn fails to complete, either due to a transient
error like disk full or file permissions, or because the process is
killed.  Further suppose that the transaction remains on disk.  Is it
valid to call svn_fs_commit_txn again on the same transaction?  There
doesn't seem to be anything to prohibit it but the FSFS implementation
can lead to revision files that are not correct.

Each transaction has a protorev file containing the file representations
built up during the transaction.  When svn_fs_commit_txn is called the
protorev file is expected to contain just the sequence of file reps.
The commit code appends the node reps, change lists, etc. to the
protorev file and then moves it to create the new revision file.  What
happens when svn_fs_commit_txn is interrupted and retried depends on
when the interrupt occurs.  An interrupt after appending but before
moving will leave the appended data in the protorev file, and then a
retry will add the appended data a second time.  The result is a
revision file that contains duplicate nodes, multiple change lists, etc.
The problem is likely even worse if changes are made to the transaction
between the calls to svn_fs_commit_txn.

So, is it valid to retry svn_fs_commit_txn?  Is it valid to modify the
transaction after svn_fs_commit_txn fails?  If it is valid we should
probably try to make the result independent of the number of attempts.
If it is not valid we should probably try to make the attempts fail.

This is mostly a theoretical problem at present: servers and clients
tend to delete transactions when svn_fs_commit_txn fail and so no retry
is possible.

In the long term perhaps we should dispense with the protorev file.  We
could stored file reps in individual files in the transaction directory
and build the revision file from scratch.  There would be an IO penalty,
writing the file reps twice, but we might gain the ability to write
file reps into the transaction in parallel.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: protorev file handling in svn_fs_commit_txn

["C. Michael Pilato" <cm...@collab.net>]

On 11/26/2013 06:53 AM, Philip Martin wrote:
> Suppose svn_fs_commit_txn fails to complete, either due to a transient
> error like disk full or file permissions, or because the process is
> killed.  Further suppose that the transaction remains on disk.  Is it
> valid to call svn_fs_commit_txn again on the same transaction?  There
> doesn't seem to be anything to prohibit it but the FSFS implementation
> can lead to revision files that are not correct.

[Just answering the API question here.]

Yes.  So long as the txn is neither aborted nor successfully promoted
into a new revision, you should be able to repeat the call to
svn_fs_commit_txn() without a corruption penalty.  If FSFS doesn't allow
this to happen, FSFS is buggy.

Re: protorev file handling in svn_fs_commit_txn

[Philip Martin <ph...@wandisco.com>]

Daniel Shahaf <d....@daniel.shahaf.name> writes:

> Philip Martin wrote on Tue, Nov 26, 2013 at 16:52:38 +0000:
>> [ pointed out several problems ]
>
> I agree.
>
> It's still not clear to me how the repository you posted managed to get
> rep-cache.db to refer to the file rep at offset 0 --- a rep which isn't
> reachable from the root-node-offset at the end of the file.  In your
> simulation of the failure, did commit_baton get re-initialized between
> the failed and successful commit_body() calls?  I think in a normal
> situation it would be.

The failed commit and the successful commit were made by two separate
processes.  The rep_cache offest points to the text representation at
the start of the file, not to either file node representation.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: protorev file handling in svn_fs_commit_txn

[Daniel Shahaf <d....@daniel.shahaf.name>]

Philip Martin wrote on Tue, Nov 26, 2013 at 16:52:38 +0000:
> [ pointed out several problems ]

I agree.

It's still not clear to me how the repository you posted managed to get
rep-cache.db to refer to the file rep at offset 0 --- a rep which isn't
reachable from the root-node-offset at the end of the file.  In your
simulation of the failure, did commit_baton get re-initialized between
the failed and successful commit_body() calls?  I think in a normal
situation it would be.

Re: protorev file handling in svn_fs_commit_txn

[Philip Martin <ph...@wandisco.com>]

Daniel Shahaf <d....@daniel.shahaf.name> writes:

> I don't think it depends on the implementation details; I think it's
> a valid revision file, full stop.  The revision file invariant is that
> everything reachable from the two offsets in the last line of the file
> be valid --- and that indeed is the case for the revs/0/1 file in your
> repository.
>
> Yes, there are some byte offsets, node-revs, reps, etc in the file which
> are not reachable from the two offsets in the last line, but that
> doesn't make the file invalid.

Yes, I agree.  Provided the parser only parses good offsets it will
ignore the extra data, and that's what happens when starting with the
root node indicated by the final offsets in the file.  I still think
it's unfortunate that the extra data gets into the revision file.

There is also a problem associated with transaction properties.  The
code removes the temporary properties SVN_FS__PROP_TXN_CHECK_OOD and
SVN_FS__PROP_TXN_CHECK_LOCKS that may have been set when the transaction
was created.  If the commit fails after removing these properties the
checks they trigger will not occur on any subsequent attempts to modify
the transaction.  CHECK_OOD isn't implemented yet and CHECK_LOCKS only
affects the timing of the checks, not the final result, so there should
be no effect on any revision that does get committed.  I still think
it's unfortunate that the properties are lost.

Finally, there is a window between moving the protorev file into place
and moving the revprop file into place.  If interrupted during this
period the revision does not exist because the revprop file is missing
but the transaction is broken because there is no protorev file.
Retrying the commit will fail.  The client will have to repeat the whole
commit and create a new transaction.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: protorev file handling in svn_fs_commit_txn

[Daniel Shahaf <d....@daniel.shahaf.name>]

Philip Martin wrote on Tue, Nov 26, 2013 at 12:36:34 +0000:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
> 
> > Philip Martin wrote on Tue, Nov 26, 2013 at 11:53:05 +0000:
> >> The result is a revision file that contains duplicate nodes, multiple
> >> change lists, etc.  The problem is likely even worse if changes are
> >> made to the transaction between the calls to svn_fs_commit_txn.
> >> 
> >
> > Assuming there are no changes to the transaction, is there a correctness
> > problem resulting?  Or is the problem simply one of wasted space, i.e.,
> > the resulting revision file would contain N copies of the change list
> > but only one copy would be pointed to?
> 
> I don't know, I expect it depends on implementation details of the FSFS
> code that reads revision files.

I don't think it depends on the implementation details; I think it's
a valid revision file, full stop.  The revision file invariant is that
everything reachable from the two offsets in the last line of the file
be valid --- and that indeed is the case for the revs/0/1 file in your
repository.

Yes, there are some byte offsets, node-revs, reps, etc in the file which
are not reachable from the two offsets in the last line, but that
doesn't make the file invalid.

Re: protorev file handling in svn_fs_commit_txn

[Philip Martin <ph...@wandisco.com>]

Daniel Shahaf <d....@daniel.shahaf.name> writes:

> Philip Martin wrote on Tue, Nov 26, 2013 at 11:53:05 +0000:
>> The result is a revision file that contains duplicate nodes, multiple
>> change lists, etc.  The problem is likely even worse if changes are
>> made to the transaction between the calls to svn_fs_commit_txn.
>> 
>
> Assuming there are no changes to the transaction, is there a correctness
> problem resulting?  Or is the problem simply one of wasted space, i.e.,
> the resulting revision file would contain N copies of the change list
> but only one copy would be pointed to?

I don't know, I expect it depends on implementation details of the FSFS
code that reads revision files.  I've attached a repository in which I
have triggered the problem on this commit:

   svnadmin create repo
   svn import repo/format file://`pwd`/repo/f

so it contains r1 with one file rep, two nodes 'f', two nodes '/', two
change lists and two final offset lines.  The two 'f' nodes refer to the
single file rep and are the same but the two '/' nodes refer to
different 'f' nodes and so are different.

"svnadmin verify" and "svnadmin dump" do not report errors.

"svnlook tree --show-ids" shows the second set of nodes and ignores the
first set.

Re: protorev file handling in svn_fs_commit_txn

[Daniel Shahaf <d....@daniel.shahaf.name>]

Philip Martin wrote on Tue, Nov 26, 2013 at 11:53:05 +0000:
> The result is a revision file that contains duplicate nodes, multiple
> change lists, etc.  The problem is likely even worse if changes are
> made to the transaction between the calls to svn_fs_commit_txn.
> 

Assuming there are no changes to the transaction, is there a correctness
problem resulting?  Or is the problem simply one of wasted space, i.e.,
the resulting revision file would contain N copies of the change list
but only one copy would be pointed to?

> So, is it valid to retry svn_fs_commit_txn?  Is it valid to modify the
> transaction after svn_fs_commit_txn fails?  If it is valid we should
> probably try to make the result independent of the number of attempts.
> If it is not valid we should probably try to make the attempts fail.
> 
> This is mostly a theoretical problem at present: servers and clients
> tend to delete transactions when svn_fs_commit_txn fail and so no retry
> is possible.
> 
> In the long term perhaps we should dispense with the protorev file.  We
> could stored file reps in individual files in the transaction directory
> and build the revision file from scratch.  There would be an IO penalty,
> writing the file reps twice, but we might gain the ability to write
> file reps into the transaction in parallel.

Hmm.  I wonder if we could just create a, say, 1MB file full of zeroes,
and than allocate a quarter MB to each of four threads?  And write to
all quarters in parallel?