]> git-server-git.apps.pok.os.sepia.ceph.com Git - rocksdb.git/commit
Fix serious FSDirectory use-after-Close bug (missing fsync) (#10460)
authorPeter Dillinger <peterd@fb.com>
Tue, 2 Aug 2022 17:54:32 +0000 (10:54 -0700)
committerPeter Dillinger <peterd@fb.com>
Tue, 2 Aug 2022 19:45:05 +0000 (12:45 -0700)
commited057748689c80517e60da125d41fd8fde2676dc
tree0e81fdf014246a3a0932a22abd4a100e4cc56e88
parente656fa3d196c5b4c8a77255db1e6cd36a7ded348
Fix serious FSDirectory use-after-Close bug (missing fsync) (#10460)

Summary:
TL;DR: due to a recent change, if you drop a column family,
often that DB will no longer fsync after writing new SST files
to remaining or new column families, which could lead to data
loss on power loss.

More bug detail:
The intent of https://github.com/facebook/rocksdb/issues/10049 was to Close FSDirectory objects at
DB::Close time rather than waiting for DB object destruction.
Unfortunately, it also closes shared FSDirectory objects on
DropColumnFamily (& destroy remaining handles), which can lead
to use-after-Close on FSDirectory shared with remaining column
families. Those "uses" are only Fsyncs (or redundant Closes). In
the default Posix filesystem, an Fsync on a closed FSDirectory is a
quiet no-op. Consequently (under most configurations), if you drop
a column family, that DB will no longer fsync after writing new SST
files to column families sharing the same directory (true under most
configurations).

More fix detail:
Basically, this removes unnecessary Close ops on destroying
ColumnFamilyData. We let `shared_ptr` take care of calling the
destructor at the right time. If the intent was to require Close be
called before destroying FSDirectory, that was not made clear by the
author of FileSystem and was not at all enforced by https://github.com/facebook/rocksdb/issues/10049, which
could have added `assert(fd_ == -1)` to `~PosixDirectory()` but did
not. To keep this fix simple, we relax the unit test for https://github.com/facebook/rocksdb/issues/10049 to allow
timely destruction of FSDirectory to suffice as Close (in
CountedFileSystem). Added a TODO to revisit that.

Also in this PR:
* Added a TODO to share FSDirectory instances between DB and its column
families. (Already shared among column families.)
* Made DB::Close attempt to close all its open FSDirectory objects even
if there is a failure in closing one. Also code clean-up around this
logic.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10460

Test Plan:
add an assert to check for use-after-Close. With that
existing tests can detect the misuse. With fix, tests pass (except noted
relaxing of unit test for https://github.com/facebook/rocksdb/issues/10049)

Reviewed By: ajkr

Differential Revision: D38357922

Pulled By: pdillinger

fbshipit-source-id: d42079cadbedf0a969f03389bf586b3b4e1f9137
HISTORY.md
db/column_family.cc
db/db_basic_test.cc
db/db_impl/db_impl.h
db/db_impl/db_impl_open.cc
env/io_posix.cc
utilities/counted_fs.cc