From 268db5933c2955bdd0277821ffa1219fbccc6829 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Fri, 10 Jul 2020 16:29:06 +0530 Subject: [PATCH] Revert "fix: Fix race condition in block.incRef (#1337)" (#1407) This reverts commit 21735af853dab8942e187dddbeeb41f3ecb04071. The commit is reverted because we have seen some crashes which could be caused by this change. We haven't been able to reproduce the crashes yet. Related to #1389, #1388, #1387 Also, see https://discuss.dgraph.io/t/current-state-of-badger-crashes/7602 --- badger/cmd/bank.go | 29 +++----------------------- table/iterator.go | 2 ++ table/table.go | 52 +++++++--------------------------------------- 3 files changed, 13 insertions(+), 70 deletions(-) diff --git a/badger/cmd/bank.go b/badger/cmd/bank.go index a312ca9e4..5491eebd6 100644 --- a/badger/cmd/bank.go +++ b/badger/cmd/bank.go @@ -125,7 +125,7 @@ func toSlice(bal uint64) []byte { } func getBalance(txn *badger.Txn, account int) (uint64, error) { - item, err := get(txn, key(account)) + item, err := txn.Get(key(account)) if err != nil { return 0, err } @@ -197,25 +197,6 @@ func diff(a, b []account) string { var errFailure = errors.New("test failed due to balance mismatch") -// get function will fetch the value for the key "k" either by using the -// txn.Get API or the iterator.Seek API. -func get(txn *badger.Txn, k []byte) (*badger.Item, error) { - if rand.Int()%2 == 0 { - return txn.Get(k) - } - - iopt := badger.DefaultIteratorOptions - // PrefectValues is expensive. We don't need it here. - iopt.PrefetchValues = false - it := txn.NewIterator(iopt) - defer it.Close() - it.Seek(k) - if it.Valid() { - return it.Item(), nil - } - return nil, badger.ErrKeyNotFound -} - // seekTotal retrives the total of all accounts by seeking for each account key. func seekTotal(txn *badger.Txn) ([]account, error) { expected := uint64(numAccounts) * uint64(initialBal) @@ -223,7 +204,7 @@ func seekTotal(txn *badger.Txn) ([]account, error) { var total uint64 for i := 0; i < numAccounts; i++ { - item, err := get(txn, key(i)) + item, err := txn.Get(key(i)) if err != nil { log.Printf("Error for account: %d. err=%v. key=%q\n", i, err, key(i)) return accounts, err @@ -362,11 +343,7 @@ func runTest(cmd *cobra.Command, args []string) error { WithNumMemtables(2). // Do not GC any versions, because we need them for the disect.. WithNumVersionsToKeep(int(math.MaxInt32)). - WithValueThreshold(1). // Make all values go to value log - WithCompression(options.ZSTD). - WithKeepL0InMemory(false). - WithMaxCacheSize(10 << 20) - + WithValueThreshold(1) // Make all values go to value log if mmap { opts = opts.WithTableLoadingMode(options.MemoryMap) } diff --git a/table/iterator.go b/table/iterator.go index d48e58138..43709b370 100644 --- a/table/iterator.go +++ b/table/iterator.go @@ -44,6 +44,8 @@ func (itr *blockIterator) setBlock(b *block) { // Decrement the ref for the old block. If the old block was compressed, we // might be able to reuse it. itr.block.decrRef() + // Increment the ref for the new block. + b.incrRef() itr.block = b itr.err = nil diff --git a/table/table.go b/table/table.go index e66fa63d0..29807bd72 100644 --- a/table/table.go +++ b/table/table.go @@ -202,46 +202,25 @@ type block struct { ref int32 } -// incrRef increments the ref of a block and return a bool indicating if the -// increment was successful. A true value indicates that the block can be used. -func (b *block) incrRef() bool { - for { - // We can't blindly add 1 to ref. We need to check whether it has - // reached zero first, because if it did, then we should absolutely not - // use this block. - ref := atomic.LoadInt32(&b.ref) - // The ref would not be equal to 0 unless the existing - // block get evicted before this line. If the ref is zero, it means that - // the block is already added the the blockPool and cannot be used - // anymore. The ref of a new block is 1 so the following condition will - // be true only if the block got reused before we could increment its - // ref. - if ref == 0 { - return false - } - // Increment the ref only if it is not zero and has not changed between - // the time we read it and we're updating it. - // - if atomic.CompareAndSwapInt32(&b.ref, ref, ref+1) { - return true - } - } +func (b *block) incrRef() { + atomic.AddInt32(&b.ref, 1) } func (b *block) decrRef() { if b == nil { return } + p := atomic.AddInt32(&b.ref, -1) // Insert the []byte into pool only if the block is resuable. When a block // is reusable a new []byte is used for decompression and this []byte can // be reused. // In case of an uncompressed block, the []byte is a reference to the // table.mmap []byte slice. Any attempt to write data to the mmap []byte // will lead to SEGFAULT. - if atomic.AddInt32(&b.ref, -1) == 0 && b.isReusable { + if p == 0 && b.isReusable { blockPool.Put(&b.data) } - y.AssertTrue(atomic.LoadInt32(&b.ref) >= 0) + y.AssertTrue(p >= 0) } func (b *block) size() int64 { return int64(3*intSize /* Size of the offset, entriesIndexStart and chkLen */ + @@ -499,9 +478,6 @@ func calculateOffsetsSize(offsets []*pb.BlockOffset) int64 { return totalSize + 3*8 } -// block function return a new block. Each block holds a ref and the byte -// slice stored in the block will be reused when the ref becomes zero. The -// caller should release the block by calling block.decrRef() on it. func (t *Table) block(idx int) (*block, error) { y.AssertTruef(idx >= 0, "idx=%d", idx) if idx >= t.noOfBlocks { @@ -511,12 +487,7 @@ func (t *Table) block(idx int) (*block, error) { key := t.blockCacheKey(idx) blk, ok := t.opt.Cache.Get(key) if ok && blk != nil { - // Use the block only if the increment was successful. The block - // could get evicted from the cache between the Get() call and the - // incrRef() call. - if b := blk.(*block); b.incrRef() { - return b, nil - } + return blk.(*block), nil } } @@ -524,7 +495,6 @@ func (t *Table) block(idx int) (*block, error) { ko := t.blockOffsets()[idx] blk := &block{ offset: int(ko.Offset), - ref: 1, } var err error if blk.data, err = t.read(blk.offset, int(ko.Len)); err != nil { @@ -581,14 +551,8 @@ func (t *Table) block(idx int) (*block, error) { } if t.opt.Cache != nil && t.opt.KeepBlocksInCache { key := t.blockCacheKey(idx) - // incrRef should never return false here because we're calling it on a - // new block with ref=1. - y.AssertTrue(blk.incrRef()) - - // Decrement the block ref if we could not insert it in the cache. - if !t.opt.Cache.Set(key, blk, blk.size()) { - blk.decrRef() - } + blk.incrRef() + t.opt.Cache.Set(key, blk, blk.size()) } return blk, nil }