Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]FlushMemTable races with each other and crashes under DEBUG mode #7340

Closed
wolfkdy opened this issue Sep 2, 2020 · 2 comments
Closed
Assignees

Comments

@wolfkdy
Copy link
Contributor

wolfkdy commented Sep 2, 2020

this issue is duplicated with #5126
I open another ticket as the previous one is opened one year ago and seems untracked now as no one contributed a unittest.
this issue won't be a problem in release mode as asserts are eliminated.
The two asserts won't be held.

Status DBImpl::FlushMemTableToOutputFile(
    ColumnFamilyData* cfd, const MutableCFOptions& mutable_cf_options,
    bool* made_progress, JobContext* job_context,
    SuperVersionContext* superversion_context, LogBuffer* log_buffer) {
  mutex_.AssertHeld();
  assert(cfd->imm()->NumNotFlushed() != 0);
  assert(cfd->imm()->IsFlushPending());

Steps to reproduce the behavior

  • flush thread 1 pick column family 0,1,2, 3 then flush 0 (release the mutex lock). This is constructed by Writing to all cfs and issuing a switch wal request.
  • when thread 1 is flushing cf 0(with no mutex lock), user issue a Put and Flush to cf 1. let's assume flush thread 2 handles this request.
  • flush thread 2 flushes all the memtables of cf 1 and returns OK.
  • thread 1 is re-scheduled and tries to flush cf 1 and asserts.

these steps have been constributed into the unittest below, which can reproduce the two asserts.
The test works under 5.18.3, I guess it should also work under master branch.

TEST_F(DBFlushTest, ParallelFlush) {                                                                                
  Options options;                                                                                                  
  options.disable_auto_compactions = true;                                                                          
  options.max_background_flushes = 20;                                                                              
  options.env = env_;                                                                                               
  CreateAndReopenWithCF({"1", "2", "3"}, options);                                                                  
  FlushOptions flushOpt;                                                                                            
  flushOpt.wait = true;                                                                                             
  flushOpt.allow_write_stall=true;                                                                                  
                                                                                                                    
  bool triggered = false;                                                                                           
  SyncPoint::GetInstance()->SetCallBack(                                                                            
      "FlushJob::WriteLevel0Table", [&](void* /*arg*/) {                                                            
         if (triggered) return;                                                                                     
         triggered = true;                                                                                          
         ASSERT_OK(Put(static_cast<int>(1) /*cf*/, "key", "value"));                                                
         ASSERT_OK(dbfull()->Flush(flushOpt, handles_[1]));                                                         
      });                                                                                                           
  SyncPoint::GetInstance()->EnableProcessing();                                                                     
                                                                                                                    
                                                                                                                    
  for (size_t i = 0; i != handles_.size(); ++i) {                                                                   
    ASSERT_OK(Put(static_cast<int>(i) /*cf*/, "key", "value"));                                                     
  }                                                                                                                 
                                                                                                                    
  dbfull()->TEST_SwitchWAL();                                                                                       
  sleep(10);                                                                                                        
} 
@riversand963
Copy link
Contributor

riversand963 commented Sep 2, 2020

Thanks @wolfkdy for reporting. We can schedule multiple bg threads and each of them flushes one column family. Will submit a fix for this.

update
Changes required on flush scheduling seems more complex. Need to think more.

@riversand963 riversand963 self-assigned this Sep 2, 2020
@riversand963 riversand963 linked a pull request Sep 9, 2020 that will close this issue
facebook-github-bot pushed a commit that referenced this issue Dec 2, 2020
Summary:
#7340 reports and reproduces an assertion failure caused by a combination of the following:
- atomic flush is disabled.
- a column family can appear multiple times in the flush queue at the same time. This behavior was introduced in release 5.17.

Consequently, it is possible that two flushes race with each other. One bg flush thread flushes all memtables. The other thread calls `FlushMemTableToOutputFile()` afterwards, and hits the assertion error below.

```
  assert(cfd->imm()->NumNotFlushed() != 0);
  assert(cfd->imm()->IsFlushPending());
```

Fix this by reverting the behavior. In non-atomic-flush case, a column family can appear in the flush queue at most once at the same time.

Pull Request resolved: #7362

Test Plan:
make check
Also run stress test successfully for 10 times.
```
make crash_test
```

Reviewed By: ajkr

Differential Revision: D25172996

Pulled By: riversand963

fbshipit-source-id: f1559b6366cc609e961e3fc83fae548f1fad08ce
codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this issue Mar 5, 2021
Summary:
facebook#7340 reports and reproduces an assertion failure caused by a combination of the following:
- atomic flush is disabled.
- a column family can appear multiple times in the flush queue at the same time. This behavior was introduced in release 5.17.

Consequently, it is possible that two flushes race with each other. One bg flush thread flushes all memtables. The other thread calls `FlushMemTableToOutputFile()` afterwards, and hits the assertion error below.

```
  assert(cfd->imm()->NumNotFlushed() != 0);
  assert(cfd->imm()->IsFlushPending());
```

Fix this by reverting the behavior. In non-atomic-flush case, a column family can appear in the flush queue at most once at the same time.

Pull Request resolved: facebook#7362

Test Plan:
make check
Also run stress test successfully for 10 times.
```
make crash_test
```

Reviewed By: ajkr

Differential Revision: D25172996

Pulled By: riversand963

fbshipit-source-id: f1559b6366cc609e961e3fc83fae548f1fad08ce
@riversand963
Copy link
Contributor

@wolfkdy , given #7362 , I believe the bug is fixed. Let me know if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants