Skip to content

Commit 4915f89

Browse files
siyingfacebook-github-bot
siying
authored and
facebook-github-bot
committedAug 24, 2022
WritableFileWriter to allow operation after failure when SyncWithoutFlush() is involved (facebook#10555)
Summary: facebook#10489 adds an assertion in most functions in WritableFileWriter to check no previous error. However, it only works without calling SyncWithoutFlush(). The nature of SyncWithoutFlush() makes two concurrent call fails to check status code of each other and causing assertion failure. Fix the problem by skipping the check after SyncWithoutFlush() is called and not check status code in SyncWithoutFlush(). Since the original change was not officially released yet, the fix isn't added to HISTORY.md. Pull Request resolved: facebook#10555 Test Plan: Make sure existing tests still pass Reviewed By: anand1976 Differential Revision: D38946208 fbshipit-source-id: 63566732d3f25c8a8342840499cf7b7d745f27c2
1 parent 198e5d8 commit 4915f89

File tree

2 files changed

+57
-48
lines changed

2 files changed

+57
-48
lines changed
 

‎file/writable_file_writer.cc

+36-45
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,6 @@
2424
#include "util/rate_limiter.h"
2525

2626
namespace ROCKSDB_NAMESPACE {
27-
namespace {
28-
IOStatus AssertFalseAndGetStatusForPrevError() {
29-
assert(false);
30-
return IOStatus::IOError("Writer has previous error.");
31-
}
32-
} // namespace
33-
3427
IOStatus WritableFileWriter::Create(const std::shared_ptr<FileSystem>& fs,
3528
const std::string& fname,
3629
const FileOptions& file_opts,
@@ -51,7 +44,7 @@ IOStatus WritableFileWriter::Create(const std::shared_ptr<FileSystem>& fs,
5144

5245
IOStatus WritableFileWriter::Append(const Slice& data, uint32_t crc32c_checksum,
5346
Env::IOPriority op_rate_limiter_priority) {
54-
if (seen_error_) {
47+
if (seen_error()) {
5548
return AssertFalseAndGetStatusForPrevError();
5649
}
5750

@@ -97,7 +90,7 @@ IOStatus WritableFileWriter::Append(const Slice& data, uint32_t crc32c_checksum,
9790
if (buf_.CurrentSize() > 0) {
9891
s = Flush(op_rate_limiter_priority);
9992
if (!s.ok()) {
100-
seen_error_ = true;
93+
set_seen_error();
10194
return s;
10295
}
10396
}
@@ -179,14 +172,14 @@ IOStatus WritableFileWriter::Append(const Slice& data, uint32_t crc32c_checksum,
179172
uint64_t cur_size = filesize_.load(std::memory_order_acquire);
180173
filesize_.store(cur_size + data.size(), std::memory_order_release);
181174
} else {
182-
seen_error_ = true;
175+
set_seen_error();
183176
}
184177
return s;
185178
}
186179

187180
IOStatus WritableFileWriter::Pad(const size_t pad_bytes,
188181
Env::IOPriority op_rate_limiter_priority) {
189-
if (seen_error_) {
182+
if (seen_error()) {
190183
return AssertFalseAndGetStatusForPrevError();
191184
}
192185
assert(pad_bytes < kDefaultPageSize);
@@ -204,7 +197,7 @@ IOStatus WritableFileWriter::Pad(const size_t pad_bytes,
204197
if (left > 0) {
205198
IOStatus s = Flush(op_rate_limiter_priority);
206199
if (!s.ok()) {
207-
seen_error_ = true;
200+
set_seen_error();
208201
return s;
209202
}
210203
}
@@ -222,7 +215,7 @@ IOStatus WritableFileWriter::Pad(const size_t pad_bytes,
222215
}
223216

224217
IOStatus WritableFileWriter::Close() {
225-
if (seen_error_) {
218+
if (seen_error()) {
226219
IOStatus interim;
227220
if (writable_file_.get() != nullptr) {
228221
interim = writable_file_->Close(IOOptions(), nullptr);
@@ -333,7 +326,7 @@ IOStatus WritableFileWriter::Close() {
333326
checksum_finalized_ = true;
334327
}
335328
} else {
336-
seen_error_ = true;
329+
set_seen_error();
337330
}
338331

339332
return s;
@@ -342,7 +335,7 @@ IOStatus WritableFileWriter::Close() {
342335
// write out the cached data to the OS cache or storage if direct I/O
343336
// enabled
344337
IOStatus WritableFileWriter::Flush(Env::IOPriority op_rate_limiter_priority) {
345-
if (seen_error_) {
338+
if (seen_error()) {
346339
return AssertFalseAndGetStatusForPrevError();
347340
}
348341

@@ -370,7 +363,7 @@ IOStatus WritableFileWriter::Flush(Env::IOPriority op_rate_limiter_priority) {
370363
}
371364
}
372365
if (!s.ok()) {
373-
seen_error_ = true;
366+
set_seen_error();
374367
return s;
375368
}
376369
}
@@ -399,7 +392,7 @@ IOStatus WritableFileWriter::Flush(Env::IOPriority op_rate_limiter_priority) {
399392
}
400393

401394
if (!s.ok()) {
402-
seen_error_ = true;
395+
set_seen_error();
403396
return s;
404397
}
405398

@@ -427,7 +420,7 @@ IOStatus WritableFileWriter::Flush(Env::IOPriority op_rate_limiter_priority) {
427420
offset_sync_to - last_sync_size_ >= bytes_per_sync_) {
428421
s = RangeSync(last_sync_size_, offset_sync_to - last_sync_size_);
429422
if (!s.ok()) {
430-
seen_error_ = true;
423+
set_seen_error();
431424
}
432425
last_sync_size_ = offset_sync_to;
433426
}
@@ -455,20 +448,20 @@ const char* WritableFileWriter::GetFileChecksumFuncName() const {
455448
}
456449

457450
IOStatus WritableFileWriter::Sync(bool use_fsync) {
458-
if (seen_error_) {
451+
if (seen_error()) {
459452
return AssertFalseAndGetStatusForPrevError();
460453
}
461454

462455
IOStatus s = Flush();
463456
if (!s.ok()) {
464-
seen_error_ = true;
457+
set_seen_error();
465458
return s;
466459
}
467460
TEST_KILL_RANDOM("WritableFileWriter::Sync:0");
468461
if (!use_direct_io() && pending_sync_) {
469462
s = SyncInternal(use_fsync);
470463
if (!s.ok()) {
471-
seen_error_ = true;
464+
set_seen_error();
472465
return s;
473466
}
474467
}
@@ -478,10 +471,9 @@ IOStatus WritableFileWriter::Sync(bool use_fsync) {
478471
}
479472

480473
IOStatus WritableFileWriter::SyncWithoutFlush(bool use_fsync) {
481-
if (seen_error_) {
474+
if (seen_error()) {
482475
return AssertFalseAndGetStatusForPrevError();
483476
}
484-
485477
if (!writable_file_->IsSyncThreadSafe()) {
486478
return IOStatus::NotSupported(
487479
"Can't WritableFileWriter::SyncWithoutFlush() because "
@@ -491,16 +483,16 @@ IOStatus WritableFileWriter::SyncWithoutFlush(bool use_fsync) {
491483
IOStatus s = SyncInternal(use_fsync);
492484
TEST_SYNC_POINT("WritableFileWriter::SyncWithoutFlush:2");
493485
if (!s.ok()) {
494-
seen_error_ = true;
486+
#ifndef NDEBUG
487+
sync_without_flush_called_ = true;
488+
#endif // NDEBUG
489+
set_seen_error();
495490
}
496491
return s;
497492
}
498493

499494
IOStatus WritableFileWriter::SyncInternal(bool use_fsync) {
500-
if (seen_error_) {
501-
return AssertFalseAndGetStatusForPrevError();
502-
}
503-
495+
// Caller is supposed to check seen_error_
504496
IOStatus s;
505497
IOSTATS_TIMER_GUARD(fsync_nanos);
506498
TEST_SYNC_POINT("WritableFileWriter::SyncInternal:0");
@@ -536,14 +528,13 @@ IOStatus WritableFileWriter::SyncInternal(bool use_fsync) {
536528
}
537529
#endif
538530
SetPerfLevel(prev_perf_level);
539-
if (!s.ok()) {
540-
seen_error_ = true;
541-
}
531+
532+
// The caller will be responsible to call set_seen_error() if s is not OK.
542533
return s;
543534
}
544535

545536
IOStatus WritableFileWriter::RangeSync(uint64_t offset, uint64_t nbytes) {
546-
if (seen_error_) {
537+
if (seen_error()) {
547538
return AssertFalseAndGetStatusForPrevError();
548539
}
549540

@@ -559,7 +550,7 @@ IOStatus WritableFileWriter::RangeSync(uint64_t offset, uint64_t nbytes) {
559550
io_options.rate_limiter_priority = writable_file_->GetIOPriority();
560551
IOStatus s = writable_file_->RangeSync(offset, nbytes, io_options, nullptr);
561552
if (!s.ok()) {
562-
seen_error_ = true;
553+
set_seen_error();
563554
}
564555
#ifndef ROCKSDB_LITE
565556
if (ShouldNotifyListeners()) {
@@ -578,7 +569,7 @@ IOStatus WritableFileWriter::RangeSync(uint64_t offset, uint64_t nbytes) {
578569
// limiter if available
579570
IOStatus WritableFileWriter::WriteBuffered(
580571
const char* data, size_t size, Env::IOPriority op_rate_limiter_priority) {
581-
if (seen_error_) {
572+
if (seen_error()) {
582573
return AssertFalseAndGetStatusForPrevError();
583574
}
584575

@@ -653,7 +644,7 @@ IOStatus WritableFileWriter::WriteBuffered(
653644
}
654645
#endif
655646
if (!s.ok()) {
656-
seen_error_ = true;
647+
set_seen_error();
657648
return s;
658649
}
659650
}
@@ -669,14 +660,14 @@ IOStatus WritableFileWriter::WriteBuffered(
669660
buf_.Size(0);
670661
buffered_data_crc32c_checksum_ = 0;
671662
if (!s.ok()) {
672-
seen_error_ = true;
663+
set_seen_error();
673664
}
674665
return s;
675666
}
676667

677668
IOStatus WritableFileWriter::WriteBufferedWithChecksum(
678669
const char* data, size_t size, Env::IOPriority op_rate_limiter_priority) {
679-
if (seen_error_) {
670+
if (seen_error()) {
680671
return AssertFalseAndGetStatusForPrevError();
681672
}
682673

@@ -751,7 +742,7 @@ IOStatus WritableFileWriter::WriteBufferedWithChecksum(
751742
// and let caller determine error handling.
752743
buf_.Size(0);
753744
buffered_data_crc32c_checksum_ = 0;
754-
seen_error_ = true;
745+
set_seen_error();
755746
return s;
756747
}
757748
}
@@ -766,7 +757,7 @@ IOStatus WritableFileWriter::WriteBufferedWithChecksum(
766757
uint64_t cur_size = flushed_size_.load(std::memory_order_acquire);
767758
flushed_size_.store(cur_size + left, std::memory_order_release);
768759
if (!s.ok()) {
769-
seen_error_ = true;
760+
set_seen_error();
770761
}
771762
return s;
772763
}
@@ -801,7 +792,7 @@ void WritableFileWriter::Crc32cHandoffChecksumCalculation(const char* data,
801792
#ifndef ROCKSDB_LITE
802793
IOStatus WritableFileWriter::WriteDirect(
803794
Env::IOPriority op_rate_limiter_priority) {
804-
if (seen_error_) {
795+
if (seen_error()) {
805796
assert(false);
806797

807798
return IOStatus::IOError("Writer has previous error.");
@@ -873,7 +864,7 @@ IOStatus WritableFileWriter::WriteDirect(
873864
}
874865
if (!s.ok()) {
875866
buf_.Size(file_advance + leftover_tail);
876-
seen_error_ = true;
867+
set_seen_error();
877868
return s;
878869
}
879870
}
@@ -898,14 +889,14 @@ IOStatus WritableFileWriter::WriteDirect(
898889
// behind
899890
next_write_offset_ += file_advance;
900891
} else {
901-
seen_error_ = true;
892+
set_seen_error();
902893
}
903894
return s;
904895
}
905896

906897
IOStatus WritableFileWriter::WriteDirectWithChecksum(
907898
Env::IOPriority op_rate_limiter_priority) {
908-
if (seen_error_) {
899+
if (seen_error()) {
909900
return AssertFalseAndGetStatusForPrevError();
910901
}
911902

@@ -986,7 +977,7 @@ IOStatus WritableFileWriter::WriteDirectWithChecksum(
986977
buf_.Size(file_advance + leftover_tail);
987978
buffered_data_crc32c_checksum_ =
988979
crc32c::Value(buf_.BufferStart(), buf_.CurrentSize());
989-
seen_error_ = true;
980+
set_seen_error();
990981
return s;
991982
}
992983
}
@@ -1011,7 +1002,7 @@ IOStatus WritableFileWriter::WriteDirectWithChecksum(
10111002
// behind
10121003
next_write_offset_ += file_advance;
10131004
} else {
1014-
seen_error_ = true;
1005+
set_seen_error();
10151006
}
10161007
return s;
10171008
}

‎file/writable_file_writer.h

+21-3
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,14 @@ class WritableFileWriter {
151151
uint64_t next_write_offset_;
152152
#endif // ROCKSDB_LITE
153153
bool pending_sync_;
154-
bool seen_error_;
154+
std::atomic<bool> seen_error_;
155+
#ifndef NDEBUG
156+
// SyncWithoutFlush() is the function that is allowed to be called
157+
// concurrently with other function. One of the concurrent call
158+
// could set seen_error_, and the other one would hit assertion
159+
// in debug mode.
160+
std::atomic<bool> sync_without_flush_called_ = false;
161+
#endif // NDEBUG
155162
uint64_t last_sync_size_;
156163
uint64_t bytes_per_sync_;
157164
RateLimiter* rate_limiter_;
@@ -290,10 +297,21 @@ class WritableFileWriter {
290297

291298
const char* GetFileChecksumFuncName() const;
292299

293-
bool seen_error() const { return seen_error_; }
300+
bool seen_error() const {
301+
return seen_error_.load(std::memory_order_relaxed);
302+
}
294303
// For options of relaxed consistency, users might hope to continue
295304
// operating on the file after an error happens.
296-
void reset_seen_error() { seen_error_ = false; }
305+
void reset_seen_error() {
306+
seen_error_.store(false, std::memory_order_relaxed);
307+
}
308+
void set_seen_error() { seen_error_.store(true, std::memory_order_relaxed); }
309+
310+
IOStatus AssertFalseAndGetStatusForPrevError() {
311+
// This should only happen if SyncWithoutFlush() was called.
312+
assert(sync_without_flush_called_);
313+
return IOStatus::IOError("Writer has previous error.");
314+
}
297315

298316
private:
299317
// Decide the Rate Limiter priority.

0 commit comments

Comments
 (0)
Please sign in to comment.