Branko Čibej <br...@apache.org> writes: > > 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?
Yes, I think it would be fine if we restored the pre-r1883355 way of handling EINVAL under an #ifdef. > > 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). I think that since fdatasync() has lower integrity guarantees, starting to unconditionally use it in the existing API might be problematic. For example, the API users (ourselves and third-party) may implicitly rely on the current integrity guarantees and assume that metadata is flushed to disk. And in this case it's not limited to just the svn_io_file_flush_to_disk() function, because there are other parts of the public API that call svn_io_file_flush_to_disk() internally — such as svn_io_file_rename2() or svn_io_write_atomic2(). If the original intention of this change was to save I/O in cases where we would be fine with just flushing the file contents, then perhaps we could do something like svn_io_file_flush_to_disk2(…, svn_boolean_t sync_metadata) that uses apr_file_datasync() if sync_metadata is false — and incrementally start passing sync_metadata=FALSE on the calling sites where it's safe to do so. But if the original intention was to fully switch to APR functions, I am not too sure if we can do so, because both apr_file_datasync() and apr_file_sync() have lower integrity guarantees in some cases, compared to the previous code. (apr_file_datasync() — due to calling fdatasync() in some cases and apr_file_sync() due to always doing just an fsync() instead of F_FULLFSYNC when it's supported). Thanks, Evgeny Kotkov
