On 12.04.2022 00:14, Evgeny Kotkov 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.
You're right, I hadn't considered that. And Windows behaves sufficiently
differently that the EINVAL check as it stands is not appropriate. Would
you consider putting that entire check in an #ifdef an appropriate fix?
[...]
2) Unless I am mistaken, this change replaces an fcntl(F_FULLFSYNC) with
fdatasync() when the latter is supported.
Depending platform yes, APR may pick fdatasync() over fcntl(F_FULLFSYNC).
-- Brane
