Skip to content

Commit 226a894

Browse files
committed
Fix a really nontrivial RemoveRange bug
_, rm := split(rm) never changes `rm` itself, so `rm != b` wasn't triggered... But split was still working 99% of the time! How? The answer is that `changed = true` also wasn't triggered so ascendChange() thought that Btree didn't change and fell through to the next item, and MOST of the time Btree returned `rm` again because an item was just inserted before it which changed the iterator, after which it was removed just fine as fully contained by the removed range. HOWEVER, as Btree is not guaranteed to resume iteration correctly after changes, and SOMETIMES it was actually skipping our item, and it wasn't removed from the list. Reproducing it in tests is rather hard, but I suppose you'd have to fill the Btree to some extent.
1 parent 5a34b20 commit 226a894

File tree

1 file changed

+15
-7
lines changed

1 file changed

+15
-7
lines changed

internal/buffer_list.go

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -375,7 +375,7 @@ func (l *BufferList) delete(b *FileBuffer) (allocated int64) {
375375
// Remove buffers in range (offset..size)
376376
func (l *BufferList) RemoveRange(removeOffset, removeSize uint64, filter func(b *FileBuffer) bool) (allocated int64) {
377377
endOffset := removeOffset + removeSize
378-
ascendChange(&l.at, removeOffset, func(end uint64, b *FileBuffer) (cont bool, changed bool) {
378+
ascendChange(&l.at, removeOffset+1, func(end uint64, b *FileBuffer) (cont bool, changed bool) {
379379
if b.offset >= endOffset {
380380
return false, false
381381
}
@@ -387,16 +387,17 @@ func (l *BufferList) RemoveRange(removeOffset, removeSize uint64, filter func(b
387387
changed = true
388388
} else {
389389
rm := b
390+
bufStart := b.offset
390391
// split-delete is simpler than cut regarding the dirty part reference count
391392
if endOffset < bufEnd {
392393
// remove beginning
393394
rm, _ = l.split(rm, endOffset)
394395
}
395-
if removeOffset > rm.offset {
396+
if removeOffset > bufStart {
396397
// remove end
397398
_, rm = l.split(rm, removeOffset)
398399
}
399-
if rm != b {
400+
if removeOffset > bufStart || endOffset < bufEnd {
400401
allocated += l.delete(rm)
401402
changed = true
402403
}
@@ -636,6 +637,16 @@ func (l *BufferList) split(b *FileBuffer, offset uint64) (left, right *FileBuffe
636637
}
637638

638639
// Left here for the ease of debugging
640+
func (l *BufferList) Dump(offset, size uint64) {
641+
l.at.Ascend(offset+1, func(end uint64, b *FileBuffer) bool {
642+
if b.offset >= offset+size {
643+
return false
644+
}
645+
fmt.Printf("%x-%x s%v l%v z%v d%v\n", b.offset, b.offset+b.length, b.state, b.loading, b.zero, b.dirtyID)
646+
return true
647+
})
648+
}
649+
639650
func (l *BufferList) DebugCheckHoles(s string) {
640651
var eof uint64
641652
l.at.Descend(0xFFFFFFFFFFFFFFFF, func(end uint64, b *FileBuffer) bool {
@@ -645,10 +656,7 @@ func (l *BufferList) DebugCheckHoles(s string) {
645656
h, _, _ := l.GetHoles(0, eof)
646657
if len(h) > 0 {
647658
fmt.Printf("Debug: holes detected%s: %#v\n", s, h)
648-
l.at.Ascend(0, func(end uint64, b *FileBuffer) bool {
649-
fmt.Printf("%x-%x s%v z%v d%v\n", b.offset, b.offset+b.length, b.state, b.zero, b.dirtyID)
650-
return true
651-
})
659+
l.Dump(0, 0xFFFFFFFFFFFFFFFF)
652660
panic("holes detected")
653661
}
654662
}

0 commit comments

Comments
 (0)