On 28 May 2015 at 19:52, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 28 May 2015 at 19:25, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
>> On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>>
>>> On 28 May 2015 at 18:45, <stefan2_at_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.
I've prepared a patch that fixes svn_io_file_rename() to consistently
return error for cross volume renames on Windows using platform
specific code. See svn-rename-no-copy-allowed-v1.patch.txt.
Review/testing will be really appreciated since it contains platform
specific code.
While I think that we should generally avoid platform specific code,
in this case using the native API gives us an opportunity to use
MOVEFILE_WRITE_THROUGH flag for MoveFileEx later to avoid potential
problems on network shares and non-NTFS filesystems.
I've also prepared patch for APR that changes apr_file_rename()
behavior to fail for cross-volume renames on Windows (see
apr-file-rename-no-copy-allowed-v1.patch), but I'm not sure that it
could be committed to APR (and backported to release branches) since
it changes behavior and some APR users may depend on current behavior.
--
Ivan Zhakov
Received on 2015-06-23 19:30:18 CEST