[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: Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
Date: Thu, 28 May 2015 20:07:52 +0200

On Thu, May 28, 2015 at 6:52 PM, 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.
> 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.
Received on 2015-05-28 20:08:11 CEST

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