Perform unmap when mlock fails or both meta pages corrupted #433

ahrtr · 2023-03-21T08:19:27Z

When mlock somehow fails, then bbolt may run into nil pointer error in rollback. The new added test case also fails. The solution is to perform unmap when mlock fails or both meta pages corrupted.

Linked to #382

$ go test -run TestFailpoint_mLockFail_When_remap -timeout 20s
panic: test timed out after 20s

goroutine 50 [running]:
testing.(*M).startAlarm.func1()
	/Users/wachao/software/go/src/testing/testing.go:2036 +0x8e
created by time.goFunc
	/Users/wachao/software/go/src/time/sleep.go:176 +0x32

goroutine 1 [chan receive]:
testing.(*T).Run(0xc0000936c0, {0x131d78b?, 0x1bebc6605467?}, 0x1343410)
	/Users/wachao/software/go/src/testing/testing.go:1494 +0x37a
testing.runTests.func1(0xc0000936c0?)
	/Users/wachao/software/go/src/testing/testing.go:1846 +0x6e
testing.tRunner(0xc0000936c0, 0xc0000f9cd8)
	/Users/wachao/software/go/src/testing/testing.go:1446 +0x10b
testing.runTests(0xc0000d30e0?, {0x154dd40, 0x4, 0x4}, {0x16655b8?, 0x40?, 0x15550e0?})
	/Users/wachao/software/go/src/testing/testing.go:1844 +0x456
testing.(*M).Run(0xc0000d30e0)
	/Users/wachao/software/go/src/testing/testing.go:1726 +0x5d9
main.main()
	_testmain.go:53 +0x1aa

goroutine 21 [semacquire]:
sync.runtime_SemacquireMutex(0xd0?, 0x0?, 0x14d16a0?)
	/Users/wachao/software/go/src/runtime/sema.go:77 +0x25
sync.(*Mutex).lockSlow(0xc000102ab8)
	/Users/wachao/software/go/src/sync/mutex.go:171 +0x165
sync.(*Mutex).Lock(...)
	/Users/wachao/software/go/src/sync/mutex.go:90
go.etcd.io/bbolt.(*DB).beginRWTx(0xc000102900)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:728 +0x77
go.etcd.io/bbolt.(*DB).Begin(0x14d16a0?, 0x20?)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:670 +0x1d
go.etcd.io/bbolt.(*DB).Update(0x151e520?, 0xc0000eb7c0)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:819 +0x3e
go.etcd.io/bbolt/internal/btesting.(*DB).MustCheck(0xc0000b5620)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/internal/btesting/btesting.go:120 +0x45
go.etcd.io/bbolt/internal/btesting.(*DB).PostTestCleanup(0xc0000b5620)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/internal/btesting/btesting.go:70 +0x25
testing.(*common).Cleanup.func1()
	/Users/wachao/software/go/src/testing/testing.go:1041 +0x11f
testing.(*common).runCleanup(0xc000093860, 0xc0000ebf30?)
	/Users/wachao/software/go/src/testing/testing.go:1210 +0x95
testing.tRunner.func2()
	/Users/wachao/software/go/src/testing/testing.go:1440 +0x29
panic({0x12c7620, 0x1546f10})
	/Users/wachao/software/go/src/runtime/panic.go:884 +0x212
go.etcd.io/bbolt/internal/common.(*Meta).Txid(...)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/internal/common/meta.go:118
go.etcd.io/bbolt.(*DB).meta(0xc000118380?)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:1043 +0x30
go.etcd.io/bbolt.(*DB).hasSyncedFreelist(...)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:386
go.etcd.io/bbolt.(*Tx).rollback(0xc00013e000)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/tx.go:283 +0x6e
go.etcd.io/bbolt.(*DB).Update.func1()
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:827 +0x25
panic({0x12c7620, 0x1546f10})
	/Users/wachao/software/go/src/runtime/panic.go:884 +0x212
go.etcd.io/bbolt/internal/common.(*Meta).Txid(...)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/internal/common/meta.go:118
go.etcd.io/bbolt.(*DB).meta(0xc000118380?)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:1043 +0x30
go.etcd.io/bbolt.(*DB).hasSyncedFreelist(...)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:386
go.etcd.io/bbolt.(*Tx).rollback(0xc00013e000)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/tx.go:283 +0x6e
go.etcd.io/bbolt.(*Tx).Commit(0xc00013e000)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/tx.go:163 +0x310
go.etcd.io/bbolt.(*DB).Update(0x16655b8?, 0xc000066e88)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/db.go:842 +0xe5
go.etcd.io/bbolt/internal/btesting.(*DB).Fill(0xc0000b5620, {0xc0000baa18, 0x4, 0x4}, 0x1, 0x2710, 0x1343400, 0x1343408)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/internal/btesting/btesting.go:159 +0x105
go.etcd.io/bbolt/tests/failpoint.TestFailpoint_mLockFail_When_remap(0xc000093860)
	/Users/wachao/go/src/github.com/ahrtr/bbolt/tests/failpoint/db_failpoint_test.go:77 +0x12e
testing.tRunner(0xc000093860, 0x1343410)
	/Users/wachao/software/go/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/Users/wachao/software/go/src/testing/testing.go:1493 +0x35f
exit status 2
FAIL	go.etcd.io/bbolt/tests/failpoint	20.688s

Signed-off-by: Benjamin Wang <[email protected]>

ahrtr · 2023-03-21T11:37:57Z

cc @ptabor @tjungblu @cenkalti

tjungblu · 2023-03-21T12:04:07Z

db.go

+	defer func() {
+		if err != nil {
+			// TODO(ahrtr): add log to print the error if unmap somehow fails.
+			_ = db.munmap()


Thanks @ahrtr, overall looks good!

I'm wondering about the failure modes here reading through the man page: https://linux.die.net/man/2/munmap

it kinda looks to me that most of these errors should also be fatal, so I wouldn't even just log them but directly error out of the process. wdyt?

or at the very least, wrap the already existing err with the new one

cenkalti · 2023-03-21T19:46:38Z

LGTM

fuweid · 2023-03-22T03:57:38Z

tests/failpoint/db_failpoint_test.go

@@ -1,6 +1,8 @@
 package failpoint

 import (
+	"fmt"
+	"go.etcd.io/bbolt/internal/btesting"


minor issue: please move it into third group.

Good catch. Thank you @fuweid

We may want to enhance the workflow to detect such error automatically.

raised #435

Signed-off-by: Benjamin Wang <[email protected]>

ahrtr · 2023-03-23T02:06:19Z

This should be a safe change, and already been reviewed by three people, so let me merging it for now.

ptabor · 2023-03-23T10:49:24Z

db.go

+	defer func() {
+		if err != nil {
+			if unmapErr := db.munmap(); unmapErr != nil {
+				err = fmt.Errorf("%w; unmap failed: %v", err, unmapErr)


@ahrtr

I would be explicit in that error that it's 'rollback unmap also failed'.

I had some doubts whether we should execute here unmap or just db.invalidate.
Ideally cleanup for failing mmap() (like rollback unmap) should be handled within the mmap() function's logic, rather than assumed externally.

If we suspect the failing rollbacks might be reason for the double-linking issues,
by submitting this patch, we are making it more difficult to build repro (i.e. we need to test with & without that patch). It would be good to split failpoints (helping with repro) from the 'fix' PR.

Next time please ping me or @serathius a little longer for business logic change - to not create precedences.

I would be explicit in that error that it's 'rollback unmap also failed'.

Thanks for the suggestion. Will fix it separately.

I had some doubts whether we should execute here unmap or just db.invalidate.
Ideally cleanup for failing mmap() (like rollback unmap) should be handled within the mmap() function's logic, rather than assumed externally.

Definitely we should execute db.unmap instead of db.invalidate. Note that the deferred function will only be executed when mmap already succeeded but (1) mlock somehow failed or (2) both meta pages are corrupted.

If we suspect the failing rollbacks might be reason for the double-linking issues,
by submitting this patch, we are making it more difficult to build repro (i.e. we need to test with & without that patch). It would be good to split failpoints (helping with repro) from the 'fix' PR.

Not sure what do you mean by "double-linking issues", could you clarify this? I found the issue with the new added test cases in this PR, so I fixed it in the same PR. I plan to backport both failpoints and all bug fixes to release-1.3, please see #436. Please let me know if you still have any concern.

Next time please ping me or @serathius a little longer for business logic change - to not create precedences.

Sorry to merge this PR without another maintainer's approval. I do think it's a safe change, and It's already reviewed by three experienced contributors. But anyway, will try to follow the process next time.

thx

You are right.

I mean corruptions like Branch page items link to already released pages #402. There is slim chance there are caused by transaction rollback and follow up commit. If we find repro without this PR, we have a higher confidence we found the root issue.
If the PR indeed fixes the problem. You might struggle to repro problem that got already fixed.
I'm not saying I have concerns about this PR, just flagging.

Thx.

If the PR indeed fixes the problem. You might struggle to repro problem that got already fixed.
I'm not saying I have concerns about this PR, just flagging.

Thanks for the reminder.

ahrtr added 2 commits March 20, 2023 15:34

test: add failpoint for mlock operation

c00862a

Signed-off-by: Benjamin Wang <[email protected]>

test: add test cases to similate mlock failure

d8ac759

Signed-off-by: Benjamin Wang <[email protected]>

ahrtr added the backport/v1.3 label Mar 21, 2023

ahrtr added this to the v1.4.0 milestone Mar 21, 2023

tjungblu reviewed Mar 21, 2023

View reviewed changes

fuweid reviewed Mar 22, 2023

View reviewed changes

Perform unmap when failing to mlock or both meta pages corrupted

b2f3dd5

Signed-off-by: Benjamin Wang <[email protected]>

ahrtr mentioned this pull request Mar 22, 2023

Improve the workflow to check goimport items #435

Closed

ahrtr merged commit b027d19 into etcd-io:master Mar 23, 2023

ahrtr mentioned this pull request Mar 23, 2023

[1.3] Support injecting failpoints to mimic syscall errors #436

Closed

3 tasks

ahrtr removed this from the v1.4.0 milestone Mar 23, 2023

ptabor reviewed Mar 23, 2023

View reviewed changes

ahrtr mentioned this pull request Mar 23, 2023

update the error message when rollback unmap also fails #438

Merged

jmhbnz mentioned this pull request Mar 30, 2023

[1.3] Backport perform unmap when mlock fails or both meta pages corrupted #444

Merged

ahrtr mentioned this pull request Apr 14, 2023

Nominate @tjungblu @cenkalti and @pavelkalinnikov as members etcd-io/etcd#15714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform unmap when mlock fails or both meta pages corrupted #433

Perform unmap when mlock fails or both meta pages corrupted #433

ahrtr commented Mar 21, 2023 •

edited

Loading

ahrtr commented Mar 21, 2023

tjungblu Mar 21, 2023

tjungblu Mar 21, 2023

ahrtr Mar 22, 2023

cenkalti commented Mar 21, 2023

fuweid Mar 22, 2023

ahrtr Mar 22, 2023

ahrtr Mar 22, 2023

ahrtr commented Mar 23, 2023

ptabor Mar 23, 2023

ahrtr Mar 23, 2023

ahrtr Mar 23, 2023

ptabor Mar 24, 2023

ahrtr Mar 24, 2023

Perform unmap when mlock fails or both meta pages corrupted #433

Perform unmap when mlock fails or both meta pages corrupted #433

Conversation

ahrtr commented Mar 21, 2023 • edited Loading

ahrtr commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cenkalti commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Mar 21, 2023 •

edited

Loading