On Mon, Apr 11, 2022 at 6:14 PM Evgeny Kotkov <[email protected]> wrote: > > svn-role <[email protected]> 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
