Apple doesn't follow POSIX specifications for fsync(). Instead, fsync() guarantees to flush the buffer cache to the device, which means the data will survive kernel panics, but may not survive power outages. Applications that need stronger guarantees (like databases) need to use fcntl(F_FULLFSYNC).
This CL switches PosixWritableFile::Sync() to get the stronger guarantees on Apple systems. The improved implementation follows the same principles as SQLite [1] and node.js [2].
Research for the fcntl() to fsync() fallback strategy:
Apple's released source code at https://opensource.apple.com/ shows at least three different error codes being returned when a filesystem does not support F_FULLFSYNC.
fcntl() is implemented in xnu-4903.221.2 in bsd/kern/kern_descrip.c, where it delegates to fcntl_nocancel(). The documentation for fcntl_nocancel() mentions error codes for some operations, but does not include F_FULLFSYNC. The F_FULLSYNC branch in fcntl_nocancel() calls VNOP_IOCTL(_, F_FULLSYNC, NULL, 0, _), whose return value sets the error
code.
VNOP_IOCTL() is implemented in bsd/vfs/kpi_vfs.c and calls the ioctl function in the vnode's operation vector. The per-filesystem function names follow the pattern _vnop_ioctl() for all the instances in opensource code: {hfs,msdosfs,nfs,ntfs,smbfs,webdav,zfs}_vnop_ioctl().
hfs-407.30.1, msdosfs-229.200.3, and nfs in xnu-4903.221.2 handle F_FULLFSYNC. ntfs-94.200.1 and smb-759.40.1 do not handle F_FULLFSYNC, and the default branch returns ENOSUP. webdav-380.200.1 also does not handle F_FULLFSYNC, but the default branch returns EINVAL. zfs-59 also does not handle F_FULLSYNC, and its default branch returns ENOTTY.
From a different angle, Apple's ntfs-94.200.1 includes utility code that uses fcntl(F_FULLFSYNC) and falls back to fsync() just like we do, supporting the hypothesis that there is no good way to detect lack of F_FULLFSYNC support. Also, Apple's fcntl() man page [3] does not mention a way to detect lack of F_FULLFSYNC support.
[1] https://www.sqlite.org/src/doc/trunk/src/os_unix.c
[2] https://github.com/libuv/libuv/blob/master/src/unix/fs.c
[3] https://developer.apple.com/library/archive/documentatiVon/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html
Tested:
https://travis-ci.org/pwnall/leveldb/builds/477318498
TAP global presubmit
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228593729
The CMake feature-detection code used check_symbol_exists(), which
invokes the C compiler. However, some glibc versions don't expose the
fdatasync() declaration when compiled with -std=c11, but do expose it
when compiled with -std=c++11. This most likely comes down to how
_POSIX_SOURCE is defined -- it needs to be >= 201112L for <unistd.h> to
expose fdatasync().
This CL switches to check_cxx_symbol_exists(), which uses the C++
compiler. Asides from fixing the problem above, this is the right thing
to do, because we use <unistd.h> in env_posix.cc, which is compiled with
the C++ compiler.
This CL also fixes a previously introduced inconsistency, where the
macro indicating the fdatasync() feature detection result was referred
to as HAVE_FDATASYNC and HAVE_FUNC_FDATASYNC. The former appears to be
used in other libraries, so this CL switches all our references to
HAVE_FDATASYNC.
Fixes https://github.com/google/leveldb/issues/629
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228392612
This is not an API-breaking change, because it reduces the API that the
leveldb embedder must implement. The project will build just fine
against ports that still implement InitOnce.
C++11 guarantees thread-safe initialization of static variables inside
functions. This is a more restricted form of std::call_once or
pthread_once_t (e.g., single call site), so the compiler might be able
to generate better code [1]. Equally important, having less code in
port_example.h makes it easier to port to other platforms.
Due to the change above, this CL introduces a new approach for storing
the singleton BytewiseComparatorImpl instance returned by
BytewiseComparator(). The new approach avoids a dynamic memory
allocation, which eliminates the false positive from LeakSanitizer
reported in https://github.com/google/leveldb/issues/200
[1] https://stackoverflow.com/a/27206650/
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212348004
The porting layer implements threading primitives: atomic pointers,
condition variables, mutexes, thread-safe initialization. These are all
specified in C++11, so the reference open source port implementation can
become platform-independent.
The porting layer will remain in place to allow the use of other
implementations with more features, such as the built-in deadlock
detection in abseil's Mutex.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=193245934
ConsumeDecimalNumber has fairly non-trivial logic, and a previous
version has crashed inexplicably on Android. Having some test coverage
will make it easier to tweak / simplify the function later on.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192821751
The CMake-based build relies on __has_include, which is standardized in
C++17. Unfortunately, __has_include is available without requiring
--std=c++17 on all the compilers on CI, so this problem was not caught.
Fixes https://github.com/google/leveldb/issues/572
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192208842
C++11 requires <atomic>. This lets us remove the header detection
(LEVELDB_ATOMIC_PRESENT) and simplify port/atomic_pointer.h.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189919098
This CL switches the public headers to C++11 default and deleted constructors, and adds override to the relevant leveldb::EnvWrapper methods. This should be a good test for C++11 compiler support.
Once this CL settles, the rest of the codebase can be safely modernized to C++11.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189873212
costan
Hyzeta