andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Dynamic test whether sync_file_range returns ENOSYS (#5416)

Browse Source 
Summary:
`sync_file_range` returns `ENOSYS` on Windows Subsystem for Linux even
when using a supposedly supported filesystem like ext4. To handle this
case we can do a dynamic check that a no-op `sync_file_range`
invocation, which is accomplished by passing zero for the `flags`
argument, succeeds.

Also I rearranged the function and comments to hopefully make it more
easily understandable.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5416

Differential Revision: D15807061

fbshipit-source-id: d31d94e1f228b7850ea500e6199f8b5daf8cfbd3
This commit is contained in:
Andrew Kryczka 2019-06-13 13:52:43 -07:00 committed by Facebook Github Bot
parent ec8111c5a4
commit 2c9df9f9e5
1 changed files with 19 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
env/io_posix.cc vendored
View File

@ -186,28 +186,34 @@ size_t GetLogicalBufferSize(int __attribute__((__unused__)) fd) {
#define ZFS_SUPER_MAGIC 0x2fc12fc1
#endif
bool IsSyncFileRangeSupported(int __attribute__((__unused__)) fd) {
// `fstatfs` is only available on Linux, but so is `sync_file_range`, so
// `defined(ROCKSDB_RANGESYNC_PRESENT)` should imply `defined(OS_LINUX)`.
bool IsSyncFileRangeSupported(int fd) {
// The approach taken in this function is to build a blacklist of cases where
// we know `sync_file_range` definitely will not work properly despite passing
// the compile-time check (`ROCKSDB_RANGESYNC_PRESENT`). If we are unsure, or
// if any of the checks fail in unexpected ways, we allow `sync_file_range` to
// be used. This way should minimize risk of impacting existing use cases.
struct statfs buf;
int ret = fstatfs(fd, &buf);
assert(ret == 0);
if (ret != 0) {
// We don't know whether the filesystem properly supports `sync_file_range`.
// Even if it doesn't, we don't know of any safety issue with trying to call
// it anyways. So, to preserve the same behavior as before this `fstatfs`
// check was introduced, we assume `sync_file_range` is usable.
return true;
}
if (buf.f_type == ZFS_SUPER_MAGIC) {
if (ret == 0 && buf.f_type == ZFS_SUPER_MAGIC) {
// Testing on ZFS showed the writeback did not happen asynchronously when
// `sync_file_range` was called, even though it returned success. Avoid it
// and use `fdatasync` instead to preserve the contract of `bytes_per_sync`,
// even though this'll incur extra I/O for metadata.
return false;
}
// No known problems with other filesystems' implementations of
// `sync_file_range`, so allow them to use it.
ret = sync_file_range(fd, 0 /* offset */, 0 /* nbytes */, 0 /* flags */);
assert(!(ret == -1 && errno != ENOSYS));
if (ret == -1 && errno == ENOSYS) {
// `sync_file_range` is not implemented on all platforms even if
// compile-time checks pass and a supported filesystem is in-use. For
// example, using ext4 on WSL (Windows Subsystem for Linux),
// `sync_file_range()` returns `ENOSYS`
// ("Function not implemented").
return false;
}
// None of the cases on the blacklist matched, so allow `sync_file_range` use.
return true;
}