[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Tue, 23 Jun 2015 20:29:46 +0300

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.
> 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

This is an archived mail posted to the Subversion Dev mailing list.