You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2015/05/28 17:45:55 UTC
svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
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.
* subversion/libsvn_fs_fs/util.c
(svn_fs_fs__move_into_place): On Win32, always flush after rename.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/util.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/util.c?rev=1682265&r1=1682264&r2=1682265&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/util.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/util.c Thu May 28 15:45:55 2015
@@ -637,6 +637,22 @@ svn_fs_fs__move_into_place(const char *o
apr_pool_t *pool)
{
svn_error_t *err;
+ apr_file_t *file;
+
+#if defined(WIN32) || defined(__OS2__)
+
+ /* APR will *not* error out on Win32 if this requires a copy instead of
+ of a move. */
+ SVN_ERR(svn_io_file_rename(old_filename, new_filename, pool));
+
+ /* Flush the target of the copy to disk. */
+ SVN_ERR(svn_io_file_open(&file, new_filename, APR_WRITE,
+ APR_OS_DEFAULT, pool));
+ SVN_ERR(svn_io_file_flush_to_disk(file, pool));
+ SVN_ERR(svn_io_file_close(file, pool));
+
+ /* Copying permissions is a no-op on WIN32. */
+#else
SVN_ERR(svn_io_copy_perms(perms_reference, old_filename, pool));
@@ -644,8 +660,6 @@ svn_fs_fs__move_into_place(const char *o
err = svn_io_file_rename(old_filename, new_filename, pool);
if (err && APR_STATUS_IS_EXDEV(err->apr_err))
{
- apr_file_t *file;
-
/* Can't rename across devices; fall back to copying. */
svn_error_clear(err);
err = SVN_NO_ERROR;
@@ -654,11 +668,6 @@ svn_fs_fs__move_into_place(const char *o
/* Flush the target of the copy to disk. */
SVN_ERR(svn_io_file_open(&file, new_filename, APR_READ,
APR_OS_DEFAULT, pool));
- /* ### BH: Does this really guarantee a flush of the data written
- ### via a completely different handle on all operating systems?
- ###
- ### Maybe we should perform the copy ourselves instead of making
- ### apr do that and flush the real handle? */
SVN_ERR(svn_io_file_flush_to_disk(file, pool));
SVN_ERR(svn_io_file_close(file, pool));
}
@@ -672,7 +681,6 @@ svn_fs_fs__move_into_place(const char *o
On other operating systems, we'd only be asking for trouble
by trying to open and fsync a directory. */
const char *dirname;
- apr_file_t *file;
dirname = svn_dirent_dirname(new_filename, pool);
SVN_ERR(svn_io_file_open(&file, dirname, APR_READ, APR_OS_DEFAULT,
@@ -682,6 +690,8 @@ svn_fs_fs__move_into_place(const char *o
}
#endif
+#endif /* defined(WIN32) || defined(__OS2__) */
+
return SVN_NO_ERROR;
}
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 23:55, Bert Huijben <be...@qqmail.nl> wrote:
> In this case the log message documented that it fixed ‘a problem on
> Windows’, which would suggest at least some Windows testing
> [[
> 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.
> ]]
>
> The original code worked just fine in 1.7 and 1.8 before this patch, so some
> explanation on what problem is fixed, how to reproduce the problem and how
> this impacts performance would be welcome.
>
+1.
[...]
>> >> 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.
>
> I really hope you're joking. We've never required or expected testing on all
> platforms, from anyone.
>
Do you say that testing platform specific fix on the platform affected
is not good practice
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, May 29, 2015 at 4:54 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Stefan Fuhrmann <st...@wandisco.com> writes:
>
> > 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.
>
> [...]
>
> > The two problems are mentioned: The first one is that rename itself is
> > obviously not persistet, e.g. the 'current' file may show the old
> contents
> > even after the server send a "commit done, new rev available" to the
> client.
> >
> > To reproduce: Have a "busy" server setup, do lots of commits, record HEAD
> > revs returned to the client on the client, pull the plug on the server
> and
> > compare HEAD on server vs. HEAD on client. In some cases, the latter
> should
> > be higher.
>
> [...]
>
> This reproduction contradicts with what I see in code. Is it a blind
> guess?
> Did you try it with this patch and without it? Our current code doesn't
> use
> the svn_fs_fs__move_into_place() when updating the db/current file
> contents,
> so I don't see how it could possibly fix the aforementioned problem.
You are right, the 'current' file uses yet another instance
of that non-sync'ing code. I hadn't checked that before
I wrote the reply.
If that would be fixed, the same issue with the revision
data file and the revprops file still exists. The resulting
behaviour would be the one described (error due to
missing file).
> Do we
> *really* have a reproducible problem with single-volume renames on Windows?
>
Let me ask you this: Is a rename under Windows (past
and future releases) an immediately persistent operation?
IOW, do you observe 10+ms delays for your RAID to
complete the I/O before the call returns?
If there is a problem with how we handle cross-volume moves, then we should
> fix it, but perhaps not with doing a huge amount of unnecessary
> CreateFile()
> and FlushFileBuffers() calls in a common case in order to solve an edge
> case.
>
For FSX, I'll revamp the commit file handling such that
cross-volume renames should no longer be an issue
while reducing the overhead to a single fsync wait time.
It should also do away with the plethora of "sync here"
and "rename there" calls currently sprinkled over the code.
Once that works, it should be straight-forward to port it
to FSFS.
-- Stefan^2.
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> 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.
[...]
> The two problems are mentioned: The first one is that rename itself is
> obviously not persistet, e.g. the 'current' file may show the old contents
> even after the server send a "commit done, new rev available" to the client.
>
> To reproduce: Have a "busy" server setup, do lots of commits, record HEAD
> revs returned to the client on the client, pull the plug on the server and
> compare HEAD on server vs. HEAD on client. In some cases, the latter should
> be higher.
[...]
This reproduction contradicts with what I see in code. Is it a blind guess?
Did you try it with this patch and without it? Our current code doesn't use
the svn_fs_fs__move_into_place() when updating the db/current file contents,
so I don't see how it could possibly fix the aforementioned problem. Do we
*really* have a reproducible problem with single-volume renames on Windows?
If there is a problem with how we handle cross-volume moves, then we should
fix it, but perhaps not with doing a huge amount of unnecessary CreateFile()
and FlushFileBuffers() calls in a common case in order to solve an edge case.
Regards,
Evgeny Kotkov
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, May 28, 2015 at 10:55 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
> In this case the log message documented that it fixed ‘a problem on Windows’, which would suggest at least some Windows testing
No, it does not. The problems were sublte but could be
deduced from reading the source code alone (I wondered
why the code did not error out on Windows, so I started
reading through the called functions). In fact, without the
flush, the Win32 code (just call rename) is equivalent to
what was implemented previously.
The kind of testing Ivan has been asking for is to actually
demonstrate a data loss due to power failure. While I do
have access to a Windows VM for ordinary testing in our
data center, I don't have one that I can simply pull the
plug on.
> [[
> 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.
> ]]
>
> The original code worked just fine in 1.7 and 1.8 before this patch, so some explanation on what problem is fixed, how to reproduce the problem and how this impacts performance would be welcome.
The two problems are mentioned: The first one is that
rename itself is obviously not persistet, e.g. the 'current'
file may show the old contents even after the server
send a "commit done, new rev available" to the client.
To reproduce: Have a "busy" server setup, do lots of
commits, record HEAD revs returned to the client on
the client, pull the plug on the server and compare
HEAD on server vs. HEAD on client. In some cases,
the latter should be higher.
Secondly, if the rename degrades to a copy, the file
contents (e.g. revision data) may be incomplete upon
power failure. This one is arguably the more severe
problem as it may leave the repo in a corrupted state.
To reproduce: Have a "busy" server setup, put the repo's
protorev folder on a separate volume, do lots of commits,
pull the plug on the server and verify the repos. In some
cases, the revision data should be corrupt, e.g. just be
empty files.
Ideally, you place the repo on some SAN and keep that
running such that HW buffer effects don't play a role here.
-- Stefan^2.
> On 28 May 2015 6:54 pm, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
> >
>
> > >> 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.
>
> I really hope you're joking. We've never required or expected testing on all platforms, from anyone.
>
> -- Brane
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
In this case the log message documented that it fixed ‘a problem on Windows’, which would suggest at least some Windows testing
[[
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.
]]
The original code worked just fine in 1.7 and 1.8 before this patch, so some explanation on what problem is fixed, how to reproduce the problem and how this impacts performance would be welcome.
Bert
Sent from Surface
From: Branko Čibej
Sent: Thursday, May 28, 2015 10:27 PM
To: Ivan Zhakov
Cc: dev@subversion.apache.org, Stefan Fuhrmann, Stefan Fuhrmann
On 28 May 2015 6:54 pm, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
>
> >> 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.
I really hope you're joking. We've never required or expected testing on all platforms, from anyone.
-- Brane
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 28 May 2015 6:54 pm, "Ivan Zhakov" <iv...@visualsvn.com> wrote:
>
> >> 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.
I really hope you're joking. We've never required or expected testing on
all platforms, from anyone.
-- Brane
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, May 28, 2015 at 6:52 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 28 May 2015 at 19:25, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 28 May 2015 at 18:45, <st...@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.
>
I just posted a piece about how we should change our
usage of fsync strategy on the server side from "implicit
as part of some individual file operation" to "explicitly".
Depending on how the discussion on that turns out,
priorities might shift. But of course, having correctly
implemented public I/O APIs is important. So, we should
fix it. However, we might end up rev'ing the API b/c
internally we don't need such strong guarantees anymore.
> >>
> >> 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?
>
> Yes. Since we test for the platform anyway, we might as
well call directly into the OS API. Also, I think we should
support 3 platforms: WIn32, POSIX and "other".
-- Stefan^2.
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 20:50, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Philip Martin [mailto:philip.martin@wandisco.com]
>> Sent: donderdag 28 mei 2015 19:34
>> To: Ivan Zhakov
>> Cc: Stefan Fuhrmann; Subversion Development; Stefan Fuhrman
>> Subject: Re: svn commit: r1682265 -
>> /subversion/trunk/subversion/libsvn_fs_fs/util.c
>>
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>> > I meant to add platform specific code to svn_io_file_rename() to also
>> > fail with EXDEV on Windows for cross-volume copies.
>>
>> Will that go as far as supporting all the MoveFile, MoveFileW,
>> MoveFileEx, MoveFileExW variants?
>
> Why should it?
>
I've the same question.
> Do we really want to support Windows '95?
>
> Windows NT always supports the wide versions of functions (of which the non
> wide is a wrapper) and MoveFileEx is supported since at least XP.
>
> As far as I know we broke Windows 2000 compatibility at least a few point
> versions ago.
>
We require Windows 2000 or later since Subversion 1.6.0 [1]. I also
don't see why we should support Windows 2000: extended support by
Microsoft ended on July 13, 2010 (almost 5 years ago) [2]
And de-facto current Subversion trunk doesn't support Windows 2000.
Serf also doesn't support Windows 2000 (due usage of SSPI)
[1] http://svn.apache.org/repos/asf/subversion/tags/1.6.0/CHANGES
[2] http://blogs.technet.com/b/education/archive/2009/11/10/windows-2000-end-of-life.aspx
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
RE: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: donderdag 28 mei 2015 19:34
> To: Ivan Zhakov
> Cc: Stefan Fuhrmann; Subversion Development; Stefan Fuhrman
> Subject: Re: svn commit: r1682265 -
> /subversion/trunk/subversion/libsvn_fs_fs/util.c
>
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
> > I meant to add platform specific code to svn_io_file_rename() to also
> > fail with EXDEV on Windows for cross-volume copies.
>
> Will that go as far as supporting all the MoveFile, MoveFileW,
> MoveFileEx, MoveFileExW variants?
Why should it?
Do we really want to support Windows '95?
Windows NT always supports the wide versions of functions (of which the non
wide is a wrapper) and MoveFileEx is supported since at least XP.
As far as I know we broke Windows 2000 compatibility at least a few point
versions ago.
Bert
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 20:33, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> I meant to add platform specific code to svn_io_file_rename() to also
>> fail with EXDEV on Windows for cross-volume copies.
>
> Will that go as far as supporting all the MoveFile, MoveFileW,
> MoveFileEx, MoveFileExW variants?
>
> Odd that the APR devs specifically requested Windows behaviour that does
> not match Unix. Perhaps it was unintentional?
>
>> It could be nice
>> to fix in APR, but they seem to ignore our patches :(
>
> Do they have a record of doing that? I've sent patches that have been
> applied.
>
Some examples that I remember:
1. [Patch] R/W lock slowness on Windows (Was: Windows R/W lock comment
/ Reader Writer lock performance on Windows)
http://mail-archives.apache.org/mod_mbox/apr-dev/201408.mbox/%3C31e101cfae30%248670db00%2493529100%24%40qqmail.nl%3E
2. [PATCH] Fix apr_file_trunc for buffered files on Unix
http://mail-archives.apache.org/mod_mbox/apr-dev/201406.mbox/%3CCA%2Bt0gk2p8EsfGf1UM0HEHY9AdifFLc81f5BX9hTTRH9T9R45Eg%40mail.gmail.com%3E
3. [PATCH] Speed up file I/O
https://bz.apache.org/bugzilla/show_bug.cgi?id=49085
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> I meant to add platform specific code to svn_io_file_rename() to also
> fail with EXDEV on Windows for cross-volume copies.
Will that go as far as supporting all the MoveFile, MoveFileW,
MoveFileEx, MoveFileExW variants?
Odd that the APR devs specifically requested Windows behaviour that does
not match Unix. Perhaps it was unintentional?
> It could be nice
> to fix in APR, but they seem to ignore our patches :(
Do they have a record of doing that? I've sent patches that have been
applied.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> The svn_io_file_moves() catches EXDEV and perform copy to temp +
> rename on all platforms, while svn_fs_fs__move_into_place() on Windows
> doesn't. It relies to the fact that svn_io_file_rename() never fails
> in Windows, which is should be fixed imo.
If we fix svn_io_file_rename() that also make svn_io_file_move() work
properly on Windows. Then if we can get svn_fs_fs__move_into_place() to
call svn_fs_file_move() we move all the EXDEV stuff to io.c.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 20:11, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> 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.
>
> We do most (all?) of that. svn_io_file_rename() calls apr_file_rename()
> and on Unix that calls rename() which can fail with EXDEV. When that
> happens svn_io_file_rename() fails.
I meant to add platform specific code to svn_io_file_rename() to also
fail with EXDEV on Windows for cross-volume copies. It could be nice
to fix in APR, but they seem to ignore our patches :(
> svn_io_file_move() catches EXDEV and copies the file to a temporary in
> the destination directory and then calls apr_file_rename().
>
The svn_io_file_moves() catches EXDEV and perform copy to temp +
rename on all platforms, while svn_fs_fs__move_into_place() on Windows
doesn't. It relies to the fact that svn_io_file_rename() never fails
in Windows, which is should be fixed imo.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> 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.
We do most (all?) of that. svn_io_file_rename() calls apr_file_rename()
and on Unix that calls rename() which can fail with EXDEV. When that
happens svn_io_file_rename() fails.
svn_io_file_move() catches EXDEV and copies the file to a temporary in
the destination directory and then calls apr_file_rename().
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
RE: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: woensdag 24 juni 2015 09:41
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1682265 -
> /subversion/trunk/subversion/libsvn_fs_fs/util.c
> The rest of the patch looks good. I wonder though if we shouldn't just
> rip out OS/2-specific bits ... is that thing still alive and is it
> remotely possible that someone's running Subversion on OS/2?
OS/2 still lives in some very specific use cases as 'eComStation'.
I have absolutely no idea if anybody is using Subversion on it. (Last time I heard something about that on the mailinglists was a few years ago)
But I'm guessing that OS/2 is used in more places than the Windows '95 support that is somehow still used as fallback in apr.
Bert
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 24 June 2015 at 10:41, Branko Čibej <br...@wandisco.com> wrote:
> On 23.06.2015 19:29, Ivan Zhakov wrote:
>>
>> 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.
>
>> 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. */
>
> This isn't really a wrapper for apr_file_rename at all, is it?
>
You're right: I didn't updated doc-string from previous approach.
>> +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
>> +
>
> The rest of the patch looks good. I wonder though if we shouldn't just
> rip out OS/2-specific bits ... is that thing still alive and is it
> remotely possible that someone's running Subversion on OS/2?
>
Ripping OS/2 specific code is unrelated and I wanted to keep code for
other platform unchanged, since I don't have OS/2 build environment :)
Thanks for review. I've committed slightly fixed patch in r1687583.
--
Ivan Zhakov
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 24.06.2015 09:41, Branko Čibej wrote:
> On 23.06.2015 19:29, Ivan Zhakov wrote:
>> 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.
>> 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. */
> This isn't really a wrapper for apr_file_rename at all, is it?
>
>> +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
>> +
> The rest of the patch looks good. I wonder though if we shouldn't just
> rip out OS/2-specific bits ... is that thing still alive and is it
> remotely possible that someone's running Subversion on OS/2?
>
>
> Regarding the APR patch: IMO it should go into APR 2.0 (that's trunk)
> but, as you say, can't be backported to the 1.x series.
Actually: we could revise the function, similarly to what we do in the
SVN API; for example, write apr_file_move that takes a boolean parameter
that says whether or not the move should be guaranteed to be atomic, and
rewrite apr_file_rename in as a wrapper to that: this new function could
go into APR trunk (2.0) and onto the 1.6.x branch.
But this is a bit more work because we'd need implementations not just
for Windows but also for Unix and possibly Netware.
-- Brane
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 23.06.2015 19:29, Ivan Zhakov wrote:
>
> 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.
> 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. */
This isn't really a wrapper for apr_file_rename at all, is it?
> +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
> +
The rest of the patch looks good. I wonder though if we shouldn't just
rip out OS/2-specific bits ... is that thing still alive and is it
remotely possible that someone's running Subversion on OS/2?
Regarding the APR patch: IMO it should go into APR 2.0 (that's trunk)
but, as you say, can't be backported to the 1.x series.
-- Brane
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 19:52, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 28 May 2015 at 19:25, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>
>>> On 28 May 2015 at 18:45, <st...@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
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 19:25, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 28 May 2015 at 18:45, <st...@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
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, May 28, 2015 at 5:54 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 28 May 2015 at 18:45, <st...@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.
> 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.
> Did
> you able to reproduce data corruption on VM?
>
No. But I did not even try.
> 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.
-- Stefan^2.
Re: svn commit: r1682265 - /subversion/trunk/subversion/libsvn_fs_fs/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 May 2015 at 18:45, <st...@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? 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. Did
you able to reproduce data corruption on VM?
This change should significantly degrade performance of commit
operation, so it should be done only if we are really sure that this
code required.
[1] http://svn.haxx.se/dev/archive-2013-05/0245.shtml
--
Ivan Zhakov