On 21 May 2015 at 16:25, Philip Martin <philip.martin_at_wandisco.com> wrote:
> ivan_at_apache.org writes:
>
>> Author: ivan
>> Date: Thu May 21 11:00:43 2015
>> New Revision: 1680819
>>
>> URL: http://svn.apache.org/r1680819
>> Log:
>> Prevent a possible FSFS repository corruption with power or network disk
>> failures when changing revision properties. Perform a hardware flush
>> after we finished writing to a temporary revprop file and before moving
>> it into place. The change doesn't affect commit operation behavior.
>>
>> The corruption can be easily reproduced by triggering a power loss while
>> performing svnsync.
>>
>> This change is somewhat similar to what we did in r1483781, but covers how
>> we write revision property files. See related discussion in dev_at_s.a.o
>> (Subject: "FSFS Repository corruption on high load on Windows [...] ") [1].
>>
>> Patch by: me
>> kotkov
>>
>> [1] http://svn.haxx.se/dev/archive-2013-05/0245.shtml
>>
>> * subversion/libsvn_fs_fs/revprops.c
>> (repack_stream_open): Rename the function ...
>> (repack_file_open): ...to this. Rework it to return files (apr_file_t)
>> instead of streams.
>> (repack_revprops): Work with a file instead of a stream. Flush any
>> unwritten data to disk before returning.
>> (write_non_packed_revprop): Flush any unwritten data to disk after
>> serializing the revision property list.
>> (write_packed_revprop): Cope with the changes in repack_file_open() and
>> repack_revprops() that now work with files. Flush the data to disk when
>> done writing to a temporary manifest file.
>
> Is that the best approach? In the past we have been moving away from
> file code to stream code. Can we make the flush part of the stream
> code? Perhaps we could create a "flushing stream" that just does flush
> and then simply insert the new stream into the old code.
>
We considered adding FLUSH_TO_DISK flag to svn_stream_from_apr_file(),
but decided to commit simple fix for now:
1. The current FSFS code uses explicit flush and this commit makes
code consistent with other parts
2. Adding flag to svn_stream_from_apr_file() will require revving API
etc, while this fix should be backported to 1.8.x and 1.9.x
3. It's unclear how FLUSH_TO_DISK flag should interact with DISOWN=TRUE.
Taking in account all above we decided to commit simple fix for now
and possibly implement FLUSH_TO_DISK flag later applying it to all
FSFS code where needed.
--
Ivan Zhakov
Received on 2015-05-21 15:48:53 CEST