Fix hang in MultiRead with O_DIRECT and io_uring #10368

akankshamahajan15 · 2022-07-15T15:04:06Z

Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read =
0 because of wrong check, it got added into incomplete list and gets stuck in an infinite loop as it will always return bytes_read = 0. The bug was introduced by PR #10197 and that PR is not released yet in any release branch.

Test Plan: Added new unit test

Reviewers:

Subscribers:

Tasks:

Tags:

facebook-github-bot · 2022-07-15T15:09:06Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

akankshamahajan15 · 2022-07-15T15:44:00Z

I am working on adding a unit test also to catch this hang.

facebook-github-bot · 2022-07-15T16:27:38Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

siying · 2022-07-15T16:34:15Z

LGTM. Please add a unit test for this scenario. I'm surprised that it is not covered in EnvPosixTestWithParam.MultiRead where we explicitly generate cases where bytes_read=0. We might need to investigate whether the test doesn't do the job.

mdcallag · 2022-07-15T17:23:39Z

I am trying to repro the hang with this patch applied. It will take a few hours to get a signal. Otherwise I don't have an opinion on the diff because I don't know this part of RocksDB that well.

akankshamahajan15 · 2022-07-15T17:30:14Z

I am trying to repro the hang with this patch applied. It will take a few hours to get a signal. Otherwise I don't have an opinion on the diff because I don't know this part of RocksDB that well.

Thanks for the testing with this patch Mark, I am also trying to reproduce it with unit tests as well(currently this exact case is not covered).

akankshamahajan15 · 2022-07-15T18:52:18Z

In our unit tests,

rocksdb/env/env_test.cc

Line 1434 in a543773

// In this test we don't do aligned read, so it doesn't work for

we don't do direct_reads with nonalignment because of which this scenario was never hit. I will try to see if I can do the alignment reads.

akankshamahajan15 · 2022-07-15T19:21:53Z

In other test, sync point is not enabled, because of which partial results were never injected.

rocksdb/env/env_test.cc

Line 1404 in a543773

ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();

siying · 2022-07-15T19:26:02Z

In other test, sync point is not enabled, because of which partial results were never injected.

rocksdb/env/env_test.cc

Line 1404 in a543773

ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();

This test bug was introduced only 8 days ago, after the bug was introduced: #10278
It should definitely be fixed but before the bug was introduced, it didn't catch the bug either.

facebook-github-bot · 2022-07-15T20:40:25Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

mdcallag · 2022-07-15T22:38:32Z

I was unable to reproduce it in 15 attempts. Will let it run overnight but without the patch I can repro it in 1 or 2 attempts.

Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read = 0 because of wrong check, it gets stuck in an infinite loop. Test Plan: CircleCI Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-17T20:18:51Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

akankshamahajan15 · 2022-07-17T20:19:21Z

I am able to reproduce the issue (hang) in the unit test.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-17T20:25:20Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-17T22:24:06Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-17T22:38:56Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-17T23:05:40Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T00:24:13Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-18T01:32:41Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T15:33:56Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

akankshamahajan15 · 2022-07-18T15:53:17Z

@siying Do you know which platforms, direct_io is supported and which not? I am getting error

 env/env_test.cc:1538
env_->NewWritableFile(fname, &wfile, soptions)
IO error: While open a file for appending: /dev/shm/rocksdb.7OId/testfile_1370631_5849386736499158703: Invalid argument

whereas it works fine on my devserver.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T16:39:44Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

siying · 2022-07-18T17:02:37Z

@siying Do you know which platforms, direct_io is supported and which not? I am getting error
 env/env_test.cc:1538
env_->NewWritableFile(fname, &wfile, soptions)
IO error: While open a file for appending: /dev/shm/rocksdb.7OId/testfile_1370631_5849386736499158703: Invalid argument
whereas it works fine on my devserver.

This is ramfs, so direct I/O is not supported. I think the best way of testing direct I/O scenario is to emulate the behavior. I think many tests in env_test do to test Direct I/O is to turn off direct I/O when creating the fd, and rely on the alignment assertion in the code to validate some behavior like here:

rocksdb/env/env_test.cc

Lines 1836 to 1844 in 9620653

    
           #if !defined(OS_MACOSX) && !defined(OS_WIN) && !defined(OS_SOLARIS) && !defined(OS_AIX) && !defined(OS_OPENBSD) && !defined(OS_FREEBSD) 
        
               if (soptions.use_direct_writes) { 
        
                 ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( 
        
                     "NewWritableFile:O_DIRECT", [&](void* arg) { 
        
                       int* val = static_cast<int*>(arg); 
        
                       *val &= ~O_DIRECT; 
        
                     }); 
        
               } 
        
           #endif

This may or may not work in this case though.

siying · 2022-07-18T17:09:20Z

Is it true that the bug would happen in non-Direct/IO case too? I feel that if we try to read beyond the file size, we will fall into this infinite look, no matter it is direct I/O. It's just that unless it is direct I/O, we never read beyond the file size. If that is the case, can we also try to reproduce it too?

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T18:22:57Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

env/env_test.cc

facebook-github-bot · 2022-07-18T18:43:55Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

env/env_test.cc

siying · 2022-07-18T18:47:04Z

env/env_test.cc

@@ -1522,6 +1522,71 @@ TEST_F(EnvPosixTest, MultiReadNonAlignedLargeNum) {
  }
 }

+TEST_F(EnvPosixTest, MultiReadDirectIONonAlignedLargeNum) {
+  EnvOptions soptions;
+  soptions.use_direct_reads = true;


I suspect that non-direct case also has the bug and either way we should add the coverage.

The issue was with direct_io and in case of non_direct_io we do the read again and update the results there without adding to incomplete list in case bytes_reads are 0. We do have existing test for that MultiReadNonAlignedLargeNum

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T19:02:52Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

facebook-github-bot · 2022-07-18T19:32:31Z

@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-18T20:07:07Z

@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

siying · 2022-07-18T21:54:03Z

env/env_test.cc

+
+  // Validate results
+  for (size_t i = 0; i < num_reads; ++i) {
+    ASSERT_OK(reqs[i].status);


We could validate result too.

Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read = 0 because of wrong check, it got added into incomplete list and gets stuck in an infinite loop as it will always return bytes_read = 0. The bug was introduced by PR #10197 and that PR is not released yet in any release branch. Pull Request resolved: #10368 Test Plan: Added new unit test Reviewed By: siying Differential Revision: D37885184 Pulled By: akankshamahajan15 fbshipit-source-id: 35b36a44b696d29b2f6f25301aa1b19547b4e03b

facebook-github-bot added the CLA Signed label Jul 15, 2022

akankshamahajan15 requested review from mdcallag and siying July 15, 2022 15:04

akankshamahajan15 force-pushed the fix_io_uring branch from 06905e0 to d54b32b Compare July 15, 2022 16:27

Fix multirandom hang with O_DIRECT and io_uring

6c08e9f

Summary: Fix bug in O_DIRECT and io_uring when its EOF and bytes_read = 0 because of wrong check, it gets stuck in an infinite loop. Test Plan: CircleCI Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the fix_io_uring branch from 02725be to 09cd71f Compare July 17, 2022 20:18

Add unit test

89ee33e

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the fix_io_uring branch from 09cd71f to 89ee33e Compare July 17, 2022 20:25

akankshamahajan15 force-pushed the fix_io_uring branch from 7668797 to 3f2f585 Compare July 17, 2022 22:38

Fix build failure

4b369b9

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the fix_io_uring branch from 3f2f585 to 4b369b9 Compare July 18, 2022 00:24

Enable unit test

8d0f065

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Try fixing the build failure related to alignment

733c18e

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Make unit test simple

5fdf588

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

siying reviewed Jul 18, 2022

View reviewed changes

env/env_test.cc Outdated Show resolved Hide resolved

env/env_test.cc Outdated Show resolved Hide resolved

env/env_test.cc Outdated Show resolved Hide resolved

siying reviewed Jul 18, 2022

View reviewed changes

Trying fixing the build failures

733e145

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 force-pushed the fix_io_uring branch from 1238cd4 to 733e145 Compare July 18, 2022 19:02

Addressed comments

5ab7125

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

akankshamahajan15 requested a review from siying July 18, 2022 20:28

siying approved these changes Jul 18, 2022

View reviewed changes

facebook-github-bot closed this in f6c4d7a Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hang in MultiRead with O_DIRECT and io_uring #10368

Fix hang in MultiRead with O_DIRECT and io_uring #10368

akankshamahajan15 commented Jul 15, 2022 •

edited

Loading

facebook-github-bot commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

siying commented Jul 15, 2022

mdcallag commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

siying commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

mdcallag commented Jul 15, 2022

facebook-github-bot commented Jul 17, 2022

akankshamahajan15 commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

akankshamahajan15 commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying commented Jul 18, 2022 •

edited

Loading

siying commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying Jul 18, 2022

akankshamahajan15 Jul 18, 2022 •

edited

Loading

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying Jul 18, 2022

Fix hang in MultiRead with O_DIRECT and io_uring #10368

Fix hang in MultiRead with O_DIRECT and io_uring #10368

Conversation

akankshamahajan15 commented Jul 15, 2022 • edited Loading

facebook-github-bot commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

siying commented Jul 15, 2022

mdcallag commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

akankshamahajan15 commented Jul 15, 2022

siying commented Jul 15, 2022

facebook-github-bot commented Jul 15, 2022

mdcallag commented Jul 15, 2022

facebook-github-bot commented Jul 17, 2022

akankshamahajan15 commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 17, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

akankshamahajan15 commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying commented Jul 18, 2022 • edited Loading

siying commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying Jul 18, 2022

Choose a reason for hiding this comment

akankshamahajan15 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

facebook-github-bot commented Jul 18, 2022

siying Jul 18, 2022

Choose a reason for hiding this comment

akankshamahajan15 commented Jul 15, 2022 •

edited

Loading

siying commented Jul 18, 2022 •

edited

Loading

akankshamahajan15 Jul 18, 2022 •

edited

Loading