Fix blocked DB::open on multiprocess access on exFAT filesystem #6959

kiburtse · 2023-09-07T21:12:22Z

What, How & Why?

Opening same realm with multiple processes on fat32 and exfat file systems may hang. Every open initializes multiple management files for interprocess communication. Truncate on open on exfat leads to the new inode number being assigned to the file, hence mixing internal cache of shared lock data within process. Avoid doing so in InterprocessMutex.

Fixes #6739

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
* [ ] C-API, if public C++ API changed.

realm access on fat32 and exfat file systems.

test/test_file.cpp

kiburtse · 2023-09-08T16:08:33Z

src/realm/util/interprocess_mutex.hpp

    // exFAT does not allocate a unique id for the file until it's non-empty
    m_lock_info->m_file.resize(1);
    m_fileuid = m_lock_info->m_file.get_unique_id();

+    REALM_ASSERT_RELEASE(s_info_map->find(m_fileuid) == s_info_map->end());


Are there some other places where this could be a problem? We do use open with truncate in a few other places.

Haven't found anything anywhere else.

This implies that the fix in #3883 was insufficient and that lines 267-268 are now unnecessary. Can you add some stronger checks on an InterprocessMutex to make sure that its file handle is allocated correctly on open?

How it is so? Other fix was targeting different issue but essentially dealing with the same peculiarity.

'Append' mode doesn't resize newly created file (it just avoids truncating it on open and ultimately making uid available for other files). It's still needed to resize file to 1 to get real uid.

Which stronger checks do you have in mind?

I see, I misunderstood what the problem was here.
I was wondering if it might be possible to come up with a deterministic test case using InterprocessMutex and the SpawnedProcess test helper. But unless you can come up with something creative, I think it is just inherently racy. I suppose our existing multiprocess tests are actually hitting this and causing the hang which is reassuring, so technically it is covered.

Ok, i've looked at it a bit again and added somewhat reliable test using just InterprocessMutex and spawn_process. Seems like we don't really have something like this at all. Should be beneficial. The logic is similar with how this is used in DB and it mimics almost the same pattern as with multiprocess tests from lang bindings test cases. It's still racy but somewhat reliably hits added assertions for me on exfat sdcard on macos, but needs a lot of iterations (>1000 sometimes) which takes tens of seconds. I've put just 10 to keep added time to milliseconds. If it hits an assertion or hangs on some run, we should be better equipped in the future to investigate as this time around. Would it be fine to merge?

test/test_file.cpp

ironage · 2023-10-12T20:57:50Z

src/realm/util/interprocess_mutex.hpp

    // exFAT does not allocate a unique id for the file until it's non-empty
    m_lock_info->m_file.resize(1);
    m_fileuid = m_lock_info->m_file.get_unique_id();

+    REALM_ASSERT_RELEASE(s_info_map->find(m_fileuid) == s_info_map->end());


This implies that the fix in #3883 was insufficient and that lines 267-268 are now unnecessary. Can you add some stronger checks on an InterprocessMutex to make sure that its file handle is allocated correctly on open?

coveralls-official · 2023-10-12T21:03:33Z

Pull Request Test Coverage Report for Build kirill.burtsev_95

59 of 82 (71.95%) changed or added relevant lines in 5 files are covered.
51 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.01%) to 91.605%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test/test_file.cpp	9	13	69.23%
test/util/test_path.cpp	5	9	55.56%
test/test_shared.cpp	39	54	72.22%

Files with Coverage Reduction	New Missed Lines	%
test/util/test_path.cpp	1	79.77%
src/realm/array_string.cpp	2	87.78%
src/realm/table_view.cpp	2	94.18%
src/realm/sync/client.cpp	3	90.75%
src/realm/query_expression.hpp	4	93.43%
src/realm/sync/network/websocket.cpp	4	75.26%
src/realm/sync/network/network.cpp	6	89.86%
src/realm/sync/noinst/server/server.cpp	6	76.41%
test/fuzz_group.cpp	10	54.68%
src/realm/sync/network/default_socket.cpp	13	76.24%

Totals
Change from base Build 1752:	0.01%
Covered Lines:	230632
Relevant Lines:	251769

💛 - Coveralls

src/realm/util/interprocess_mutex.hpp

ironage · 2023-10-13T22:05:22Z

src/realm/util/interprocess_mutex.hpp

    // exFAT does not allocate a unique id for the file until it's non-empty
    m_lock_info->m_file.resize(1);
    m_fileuid = m_lock_info->m_file.get_unique_id();

+    REALM_ASSERT_RELEASE(s_info_map->find(m_fileuid) == s_info_map->end());


I see, I misunderstood what the problem was here.
I was wondering if it might be possible to come up with a deterministic test case using InterprocessMutex and the SpawnedProcess test helper. But unless you can come up with something creative, I think it is just inherently racy. I suppose our existing multiprocess tests are actually hitting this and causing the hang which is reassuring, so technically it is covered.

Co-authored-by: James Stone <[email protected]>

finnschiermer

Just a little thing with the test :-), otherwise looking good.

finnschiermer · 2023-10-19T09:38:40Z

test/test_shared.cpp

+            spawned.emplace_back(
+                test_util::spawn_process(test_context.test_details.test_name, util::format("child [%1]", i)));
+
+            if (spawned.back()->is_child()) {


Isn't it timing dependent what spawned.back() refers to when it executes in a child while the parent is still spawning other children?

Isn't test_util::spawn_process essentially no-op in child process? It checks if REALM_CHILD_IDENT is present in env for a process and returns uninitialized SpawnedProcess. So first iteration in child process should get it, check the same thing again, execute and quit. That's the pattern in other test from what i can tell. May be i don't understand something here, but how parent process would influence this?

I think I just (mis)read the spawn_process as a fork() system call...

finnschiermer

LGTM

Fix double file lock and DB::open being blocked with multiple concurrent

22a63fd

realm access on fat32 and exfat file systems.

cla-bot bot added the cla: yes label Sep 7, 2023

github-actions bot assigned kiburtse Sep 7, 2023

kiburtse requested review from ironage and finnschiermer September 8, 2023 16:06

kiburtse commented Sep 8, 2023

View reviewed changes

kiburtse added 4 commits October 12, 2023 22:07

Adjust test checks for fat32/exfat

ad7823f

Adjust the comment for open mode to reflect the issue

82a1970

Merge branch 'master' into kb/fix-interprocess-lock-exfat-multiprocess

8024b87

Add changelog

326f7b8

kiburtse marked this pull request as ready for review October 12, 2023 20:33

ironage reviewed Oct 12, 2023

View reviewed changes

ironage approved these changes Oct 13, 2023

View reviewed changes

kiburtse and others added 2 commits October 16, 2023 11:04

Fix comment typo

749d2aa

Co-authored-by: James Stone <[email protected]>

Add separate multiprocess test for InterprocessMutex init and locks

c584ef0

finnschiermer suggested changes Oct 19, 2023

View reviewed changes

kiburtse added 2 commits October 19, 2023 18:16

Fix build

d69e8ae

not needed?

f30dd24

finnschiermer approved these changes Oct 20, 2023

View reviewed changes

kiburtse merged commit 7820ded into master Oct 20, 2023
2 checks passed

kiburtse deleted the kb/fix-interprocess-lock-exfat-multiprocess branch October 20, 2023 08:58

kiburtse mentioned this pull request Nov 20, 2023

EncryptionKeyCheck test failure on exfat #7156

Closed

ironage mentioned this pull request Nov 23, 2023

Fix more encryption issues on exFAT filesystem #7162

Merged

4 tasks

nirinchev mentioned this pull request Mar 12, 2024

Crashes on attempting to read realm file from some external drives #7454

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix blocked DB::open on multiprocess access on exFAT filesystem #6959

Fix blocked DB::open on multiprocess access on exFAT filesystem #6959

kiburtse commented Sep 7, 2023 •

edited

Loading

kiburtse Sep 8, 2023

kiburtse Oct 12, 2023

ironage Oct 12, 2023

kiburtse Oct 13, 2023

ironage Oct 13, 2023

kiburtse Oct 18, 2023

ironage Oct 12, 2023

coveralls-official bot commented Oct 12, 2023 •

edited

Loading

ironage Oct 13, 2023

finnschiermer left a comment

finnschiermer Oct 19, 2023

kiburtse Oct 19, 2023

finnschiermer Oct 20, 2023

finnschiermer left a comment

Fix blocked DB::open on multiprocess access on exFAT filesystem #6959

Fix blocked DB::open on multiprocess access on exFAT filesystem #6959

Conversation

kiburtse commented Sep 7, 2023 • edited Loading

What, How & Why?

☑️ ToDos

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls-official bot commented Oct 12, 2023 • edited Loading

Pull Request Test Coverage Report for Build kirill.burtsev_95

💛 - Coveralls

Choose a reason for hiding this comment

finnschiermer left a 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

finnschiermer left a comment

Choose a reason for hiding this comment

kiburtse commented Sep 7, 2023 •

edited

Loading

coveralls-official bot commented Oct 12, 2023 •

edited

Loading