On 28 May 2015 at 19:52, Ivan Zhakov <i...@visualsvn.com> wrote:
> 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.
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
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1687052)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -4024,7 +4024,25 @@
return SVN_NO_ERROR;
}
+#if defined(WIN32)
+/* Compatibility wrapper around apr_file_rename() to workaround
+ APR problems on Windows. */
+static apr_status_t
+win32_file_rename(const WCHAR *from_path_w,
+ const WCHAR *to_path_w,
+ apr_pool_t *pool)
+{
+ /* APR calls MoveFileExW() with MOVEFILE_COPY_ALLOWED, while we rely
+ * that rename is atomic operation. Use MoveFileEx directly on Windows
+ * without MOVEFILE_COPY_ALLOWED flag to workaround it.
+ */
+ if (!MoveFileExW(from_path_w, to_path_w, MOVEFILE_REPLACE_EXISTING))
+ return apr_get_os_error();
+ return APR_SUCCESS;
+}
+#endif
+
svn_error_t *
svn_io_file_rename(const char *from_path, const char *to_path,
apr_pool_t *pool)
@@ -4031,13 +4049,19 @@
{
apr_status_t status = APR_SUCCESS;
const char *from_path_apr, *to_path_apr;
+#if defined(WIN32)
+ WCHAR *from_path_w;
+ WCHAR *to_path_w;
+#endif
SVN_ERR(cstring_from_utf8(&from_path_apr, from_path, pool));
SVN_ERR(cstring_from_utf8(&to_path_apr, to_path, pool));
- status = apr_file_rename(from_path_apr, to_path_apr, pool);
+#if defined(WIN32)
+ SVN_ERR(svn_io__utf8_to_unicode_longpath(&from_path_w, from_path_apr, pool));
+ SVN_ERR(svn_io__utf8_to_unicode_longpath(&to_path_w, to_path_apr, pool));
+ status = win32_file_rename(from_path_w, to_path_w, pool);
-#if defined(WIN32) || defined(__OS2__)
/* If the target file is read only NTFS reports EACCESS and
FAT/FAT32 reports EEXIST */
if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
@@ -4047,9 +4071,23 @@
allow renaming when from_path is read only. */
SVN_ERR(svn_io_set_file_read_write(to_path, TRUE, pool));
+ status = win32_file_rename(from_path_w, to_path_w, pool);
+ }
+ WIN32_RETRY_LOOP(status, win32_file_rename(from_path_w, to_path_w, pool));
+#elif defined(__OS2__)
+ /* If the target file is read only NTFS reports EACCESS and
+ FAT/FAT32 reports EEXIST */
+ if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
+ {
+ /* Set the destination file writable because OS/2 will not
+ allow us to rename when to_path is read-only, but will
+ allow renaming when from_path is read only. */
+ SVN_ERR(svn_io_set_file_read_write(to_path, TRUE, pool));
+
status = apr_file_rename(from_path_apr, to_path_apr, pool);
}
- WIN32_RETRY_LOOP(status, apr_file_rename(from_path_apr, to_path_apr, pool));
+#else
+ status = apr_file_rename(from_path_apr, to_path_apr, pool);
#endif /* WIN32 || __OS2__ */
if (status)
Index: file_io/win32/open.c
===================================================================
--- file_io/win32/open.c (revision 1687097)
+++ file_io/win32/open.c (working copy)
@@ -548,15 +548,13 @@
return rv;
}
#ifndef _WIN32_WCE
- if (MoveFileExW(wfrompath, wtopath, MOVEFILE_REPLACE_EXISTING |
- MOVEFILE_COPY_ALLOWED))
+ if (MoveFileExW(wfrompath, wtopath, MOVEFILE_REPLACE_EXISTING))
#else
if (MoveFileW(wfrompath, wtopath))
#endif
return APR_SUCCESS;
#else
- if (MoveFileEx(frompath, topath, MOVEFILE_REPLACE_EXISTING |
- MOVEFILE_COPY_ALLOWED))
+ if (MoveFileEx(frompath, topath, MOVEFILE_REPLACE_EXISTING))
return APR_SUCCESS;
#endif
}
