On 28 May 2015 at 19:25, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote:
> On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
>>
>> On 28 May 2015 at 18:45, <stef...@apache.org> wrote:
>> > Author: stefan2
>> > Date: Thu May 28 15:45:55 2015
>> > New Revision: 1682265
>> >
>> > URL: http://svn.apache.org/r1682265
>> > Log:
>> > Correctly fsync() after renames in FSFS on Win32. We must flush the
>> > disk
>> > buffers after the rename, otherwise the metadata may not be persistent.
>> > Moreover, if the rename is degraded to a copy by Win32, we won't even
>> > have
>> > the complete file contents on disk without a buffer flush.
>> >
>> Are you sure about this change?
>
>
> Yes. APR calls MoveFileEx with MOVEFILE_COPY_ALLOWED
> but not with MOVEFILE_WRITE_THROUGH. If your txn folder
> points to a different volume than the repo, *no* data will be fsync'ed.
>
Using MOVEFILE_COPY_ALLOWED without MOVEFILE_WRITE_THROUGH is a
problem. But it could be easily fixed, without open+flush+close file
on every move.
Also moving txn folder on different volume already creates risk of
repository corruption, because svn_io_copy_file() is not atomic
operation, i.e. destination file could be deleted/empty/contain
partial content when svn_io_file_rename() degrades to copy. But we
need to improve svn_fs_fs__move_into_place() code in different way:
1. Make svn_io_file_name() consistently return error for cross volume
renames by fixing APR or using platform specific code.
2. For all platforms handle APR_STATUS_IS_EXDEV() like this: copy file
to temporary file in the same folder as destination and then rename
it.
I'm ready to implement it if nobody object.
>>
>> From my analysis that I posted two
>> years ago [1] metadata changes are persistent fine on Windows, the
>> only thing we need to make sure to flush data changes before move.
>
> I don't see analysis of your's concerning metadata in that thread.
> It is well possible that metadata changes are nicely serialized /
> atomic in NTFS, so it may be hard to produce inconsistent
> data within a given volume.
>
> However, it does not tell you anything about consistency with
> outside parties, say some svnsync'ed repository. The problem
> is that Windows may end up not persisting the rename (of e.g.
> the 'current' file) after telling the client that the commit got through
> and a new revision number is available.
>
> So, in your analysis, you would have to compare the process
> output / state ("I completed data for iteration X") with the on-disk
> contents after the power-off.
>
OK, but I think we could fix it without performance degradation:
introduce svn_io_file_rename2(src, dst, svn_boolean_t sync) and use
MoveFileEx(MOVEFILE_WRITE_THROUGH) on Windows, while using
apr_rename() + flush on Posix. Does it make sense?
>> Did
>> you able to reproduce data corruption on VM?
>
>
> No. But I did not even try.
>
I really hope that you have tested your patch on Windows before commit.
>> This change should significantly degrade performance of commit
>> operation, so it should be done only if we are really sure that this
>> code required.
>
>
> We need to change to change the way we use fsync anyway.
> I have a patch set for FSX sitting around for weeks now that
> reduces the overall number of fsyncs per commit from 8 to 2.
>
> I'll explain it in a different post.
>
--
Ivan Zhakov
