On Mon, Apr 11, 2022 at 6:14 PM Evgeny Kotkov
<evgeny.kot...@visualsvn.com> wrote:
>
> svn-role <svn-r...@apache.org> writes:
>
> > Merge r1883355 from trunk:
> >
> > * r1883355
> > Use the APR-1.4+ API for flushing file contents to disk.
> > Justification:
> > Reduce code duplication between APR and SVN.
> > Votes:
> > +1: brane, jun66j5, markphip
> …
> > + do {
> > + apr_err = apr_file_datasync(file);
> > + } while(APR_STATUS_IS_EINTR(apr_err));
> > +
> > + /* If the file is in a memory filesystem, fsync() may return
> > + EINVAL. Presumably the user knows the risks, and we can just
> > + ignore the error. */
> > + if (APR_STATUS_IS_EINVAL(apr_err))
> > + return SVN_NO_ERROR;
> > +
> > + if (apr_err)
> > + return svn_error_wrap_apr(apr_err,
> > + _("Can't flush file '%s' to disk"),
> > + try_utf8_from_internal_style(fname, pool));
>
> A few thoughts on this change:
>
> 1) Previously, the check for EINVAL within the svn_io_file_flush_to_disk()
> function only happened in the #else block, so it did not affect the
> behavior on Windows. With the change, the check happens unconditionally
> on all platforms.
>
> On Windows, APR_STATUS_IS_EINVAL() is defined as follows:
>
> #define APR_STATUS_IS_EINVAL(s) ((s) == APR_EINVAL \
> || (s) == APR_OS_START_SYSERR + ERROR_INVALID_ACCESS \
> || (s) == APR_OS_START_SYSERR + ERROR_INVALID_DATA \
> || (s) == APR_OS_START_SYSERR + ERROR_INVALID_FUNCTION \
> || (s) == APR_OS_START_SYSERR + ERROR_INVALID_HANDLE \
> || (s) == APR_OS_START_SYSERR + ERROR_INVALID_PARAMETER \
> || (s) == APR_OS_START_SYSERR + ERROR_NEGATIVE_SEEK)
>
> So with this change, all of these error codes are now going to be ignored,
> and the svn_io_file_flush_to_disk() function will return SVN_NO_ERROR,
> indicating the success of flushing the data to disk. Some of these error
> codes, such as ERROR_INVALID_HANDLE, are quite generic.
> I would think that this change opens a data corruption possibility in
> the following case:
>
> - A real error happens during FlushFileBuffers().
> - It gets translated into one of the error codes above by the I/O stack.
> - The error is ignored in the implementation of
> svn_io_file_flush_to_disk().
> - The caller of svn_io_file_flush_to_disk() interprets this situation as a
> successful data flush.
> - He continues to work under an assumption that the data was successfully
> flushed to disk, whereas in fact it was not. For example, by committing
> a repository transaction.
> - A power loss after the transaction commit causes the data to be lost or
> corrupted.
>
> 2) Unless I am mistaken, this change replaces an fcntl(F_FULLFSYNC) with
> fdatasync() when the latter is supported. So assuming that both operations
> are supported, the new behavior of the svn_io_file_flush_to_disk() function
> has weaker integrity guarantees than before, because fdatasync() does not
> ensure that metadata such as the last modification time is written to disk.
>
> I haven't done a thorough check if we rely on mtime being flushed to the
> disk in our code, but even if we don't, svn_io_file_flush_to_disk() is part
> of the public API, so altering its integrity guarantees in a patch release
> might be unexpected for its callers.
>
> Thoughts?
>
> (A small disclaimer: these changes have slipped past my attention until now,
> when I tried updating to 1.14.2. But better late than sorry…)
I assume nothing has been done on /trunk since this change was added?
FWIW, if we decide we need a 1.14.3, I will do the RM work for it once
everything is backported.
Mark
