Skip to content

Commit 296de8d

Browse files
committed
leveldb: Fix PosixWritableFile::Sync() on Apple systems.
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
1 parent b70493c commit 296de8d

File tree

3 files changed

+40
-12
lines changed

3 files changed

+40
-12
lines changed

CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ include(CheckCXXSymbolExists)
3737
# versions of do not expose fdatasync() in <unistd.h> in standard C mode
3838
# (-std=c11), but do expose the function in standard C++ mode (-std=c++11).
3939
check_cxx_symbol_exists(fdatasync "unistd.h" HAVE_FDATASYNC)
40+
check_cxx_symbol_exists(F_FULLFSYNC "fcntl.h" HAVE_FULLFSYNC)
4041

4142
include(CheckCXXSourceCompiles)
4243

port/port_config.h.in

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@
1010
#cmakedefine01 HAVE_FDATASYNC
1111
#endif // !defined(HAVE_FDATASYNC)
1212

13+
// Define to 1 if you have a definition for F_FULLFSYNC in <fcntl.h>.
14+
#if !defined(HAVE_FULLFSYNC)
15+
#cmakedefine01 HAVE_FULLFSYNC
16+
#endif // !defined(HAVE_FULLFSYNC)
17+
1318
// Define to 1 if you have Google CRC32C.
1419
#if !defined(HAVE_CRC32C)
1520
#cmakedefine01 HAVE_CRC32C

util/env_posix.cc

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,6 @@
3535
#include "util/posix_logger.h"
3636
#include "util/env_posix_test_helper.h"
3737

38-
// HAVE_FDATASYNC is defined in the auto-generated port_config.h, which is
39-
// included by port_stdcxx.h.
40-
#if !HAVE_FDATASYNC
41-
#define fdatasync fsync
42-
#endif // !HAVE_FDATASYNC
43-
4438
namespace leveldb {
4539

4640
namespace {
@@ -314,10 +308,11 @@ class PosixWritableFile final : public WritableFile {
314308
}
315309

316310
status = FlushBuffer();
317-
if (status.ok() && ::fdatasync(fd_) != 0) {
318-
status = PosixError(filename_, errno);
311+
if (!status.ok()) {
312+
return status;
319313
}
320-
return status;
314+
315+
return SyncFd(fd_, filename_);
321316
}
322317

323318
private:
@@ -352,14 +347,41 @@ class PosixWritableFile final : public WritableFile {
352347
if (fd < 0) {
353348
status = PosixError(dirname_, errno);
354349
} else {
355-
if (::fsync(fd) < 0) {
356-
status = PosixError(dirname_, errno);
357-
}
350+
status = SyncFd(fd, dirname_);
358351
::close(fd);
359352
}
360353
return status;
361354
}
362355

356+
// Ensures that all the caches associated with the given file descriptor's
357+
// data are flushed all the way to durable media, and can withstand power
358+
// failures.
359+
//
360+
// The path argument is only used to populate the description string in the
361+
// returned Status if an error occurs.
362+
static Status SyncFd(int fd, const std::string& fd_path) {
363+
#if HAVE_FULLFSYNC
364+
// On macOS and iOS, fsync() doesn't guarantee durability past power
365+
// failures. fcntl(F_FULLFSYNC) is required for that purpose. Some
366+
// filesystems don't support fcntl(F_FULLFSYNC), and require a fallback to
367+
// fsync().
368+
if (::fcntl(fd, F_FULLFSYNC) == 0) {
369+
return Status::OK();
370+
}
371+
#endif // HAVE_FULLFSYNC
372+
373+
#if HAVE_FDATASYNC
374+
bool sync_success = ::fdatasync(fd) == 0;
375+
#else
376+
bool sync_success = ::fsync(fd) == 0;
377+
#endif // HAVE_FDATASYNC
378+
379+
if (sync_success) {
380+
return Status::OK();
381+
}
382+
return PosixError(fd_path, errno);
383+
}
384+
363385
// Returns the directory name in a path pointing to a file.
364386
//
365387
// Returns "." if the path does not contain any directory separator.

0 commit comments

Comments
 (0)