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

vfs: enforce mutual exclusion in MemFS.Lock #2904

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Sep 14, 2023

Previously MemFS's Lock method did not implement mutual exclusion under the assumption that databases are given separate processes which do not share memory. This commit adapts Lock to enforce mutual exclusion, preventing accidentally using same FS concurrently from more than one Pebble instance. It's believed that cockroachdb/cockroach#110645 is the result of Cockroach acceptance tests opening a MemFS store twice, with one Pebble instance deleting files from beneath another Pebble instance.

@jbowens jbowens requested review from a team and itsbilal September 14, 2023 22:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @jbowens)


vfs/mem_fs.go line 468 at r1 (raw file):

	// close the same MemFS-backed database multiple times. We want mutual
	// exclusion in this case too. See cockroachdb/cockroach#110645.
	_, loaded := y.lockedFiles.Swap(fullname, true)

[nit] the value could be nil here (I initially wondered if the true value has some significance)


vfs/mem_fs.go line 474 at r1 (raw file):

		return nil, syscall.EAGAIN
	}
	// Otherwise, we successfully acquired the lock. Locks are visible in the

"locks are visible in the file" - did you mean "filesystem"?

Previously MemFS's Lock method did not implement mutual exclusion under the
assumption that databases are given separate processes which do not share
memory. This commit adapts Lock to enforce mutual exclusion, preventing
accidentally using same FS concurrently from more than one Pebble instance.
It's believed that cockroachdb/cockroach#110645 is the result of Cockroach
acceptance tests opening a MemFS store twice, with one Pebble instance deleting
files from beneath another Pebble instance.
Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @itsbilal)


vfs/mem_fs.go line 468 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] the value could be nil here (I initially wondered if the true value has some significance)

Done.


vfs/mem_fs.go line 474 at r1 (raw file):

Previously, RaduBerinde wrote…

"locks are visible in the file" - did you mean "filesystem"?

Done.

@jbowens jbowens merged commit 6d6570b into cockroachdb:master Sep 15, 2023
@jbowens jbowens deleted the memfs-lock branch September 15, 2023 15:22
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 this pull request may close these issues.

3 participants