On 21 May 2015 at 16:25, Philip Martin <philip.mar...@wandisco.com> wrote: > i...@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@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
