Return error instead of panicking if rewriting fails #343

v01dstar · 2023-11-03T01:50:07Z

Return the error instead of panicking if the error won't cause inconsistency while rotating log files.

Ref #131, this PR modifies the decision 3 made in the issue. After this PR, create no longer panics. truncate will be retry-able while appending but non-retry-able while closing.

Also updated Cargo.toml to remove the TODO.

Signed-off-by: v01dstar <[email protected]>

tabokie

Most if not all panics in this codebase is necessary. fsync error on certain filesystems could cause all buffered write to be lost, and we (currently) have no way to check if those buffered writes belong to current write request or older ones. (#131).

I thought the is_no_space_err has already fixed this issue? @LykxSassinator

LykxSassinator · 2023-11-03T04:20:31Z

Nope, I think I missed some works.
When we got the is_no_space_err, we will try to rotate one new file again (For multi-directory configuration). If it failed, it still throw the panic to users, reminding users to extend the capacity of disk. The related case can be reviewed in:

raft-engine/tests/failpoints/test_engine.rs

Line 1169 in 385182b

// Case 3: no prefill and no spare space for new log files.

IMO, maybe when error == is_no_space_err, it's safe to return the error to users, as the previous work on flush has been done safely.

src/file_pipe_log/pipe.rs

v01dstar · 2023-11-03T06:39:22Z

Most if not all panics in this codebase is necessary. fsync error on certain filesystems could cause all buffered write to be lost, and we (currently) have no way to check if those buffered writes belong to current write request or older ones. (#131).

The issue seems indicating that fasync may return OK even if it actually failed. While, IIUC, your concern was that failed fsync may clear the buffer? Which, I don't think it is the case here? Could you please clarify? @tabokie

I thought the is_no_space_err has already fixed this issue? @LykxSassinator

LykxSassinator · 2023-11-03T07:21:51Z

FYI, the panic may also happen in the purge progress. U should check the following callings:

must_rewrite_append_queue
must_purge_all_stale
rewrite_rewrite_queue

/cc @v01dstar

tabokie · 2023-11-04T02:03:04Z

The case we are concerned with is after the first fsync fails and clears the buffer, the second fsync returns success, producing the false impression that what hasn't been flushed out in the first fsync is persisted in the second one.

As long as you don't bubble a fsync error, things should be fine. The case @LykxSassinator mentioned is pwrite fails first before fsync. I think it probably makes more sense to push down the panic close to fsync call.

src/file_pipe_log/pipe.rs

v01dstar · 2023-11-07T04:00:57Z

FYI, the panic may also happen in the purge progress. U should check the following callings:
* `must_rewrite_append_queue`

* `must_purge_all_stale`

Are above 2 functions being called in non-test code path? If not, I don't think we need to propagate the error up.

* `rewrite_rewrite_queue`

This is already covered, isn't it.

/cc @v01dstar

Co-authored-by: lucasliang <[email protected]> Signed-off-by: v01dstar <[email protected]>

Signed-off-by: v01dstar <[email protected]>

codecov · 2023-11-07T06:23:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (385182b) 98.21% compared to head (bb27b29) 98.21%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #343   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files          33       33           
  Lines       12446    12457   +11     
=======================================
+ Hits        12224    12235   +11     
  Misses        222      222

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tabokie · 2023-11-07T06:40:26Z

I think it probably makes more sense to push down the panic close to fsync call.

Need to at least try to do this. AFAIK there's no guarantee that pwrite will fail before fsync in the case of disk full. Especially when raft-engine disk is shared with other parties.

LykxSassinator · 2023-11-07T08:24:38Z

I think it probably makes more sense to push down the panic close to fsync call.

Need to at least try to do this. AFAIK there's no guarantee that pwrite will fail before fsync in the case of disk full.

IMO, specify whether the returned error from fdatasync is a nospace error is a better choice. Users/Callers can decide panic directly or bubble the error to upper calls. And if it was a nospace error, rotate could return this error.

    // src/env/log_fd/unix.rs
    #[inline]
    fn sync(&self) -> IoResult<()> {
        fail_point!("log_fd::sync::err", |_| {
            Err(from_nix_error(nix::Error::EINVAL, "fp"))
        });
        #[cfg(target_os = "linux")]
        {
            nix::unistd::fdatasync(self.0).map_err(|e| match e {
                Errno::ENOSPC => from_nix_error(e, "nospace"),
                _ => from_nix_error(e, "fdatasync"),
            })
        }
        #[cfg(not(target_os = "linux"))]
        {
            nix::unistd::fsync(self.0).map_err(|e| match e {
                Errno::ENOSPC => from_nix_error(e, "nospace"),
                _ => from_nix_error(e, "fsync"),
            })
        }
    }

/cc @v01dstar

tabokie · 2023-11-07T09:08:27Z

Why? fsync returning NoSpace does not guarantee you anything. The idea is to still always panic when fsync fails, but we can be smarter and not panic when pwrite fails.

LykxSassinator · 2023-11-07T12:42:08Z

Why? fsync returning NoSpace does not guarantee you anything. The idea is to still always panic when fsync fails, but we can be smarter and not panic when pwrite fails.

Returning the NOSPC error can make callers know the disk just reach the limit of capacity, rather than just bubbling an error and directly panic with this error.

tabokie · 2023-11-08T14:01:12Z

The case we are concerned with is after the first fsync fails and clears the buffer, the second fsync returns success, producing the false impression that what hasn't been flushed out in the first fsync is persisted in the second one.

Please refer to this line. Panics are here to avoid silent data loss.

Signed-off-by: Yang Zhang <[email protected]>

Signed-off-by: v01dstar <[email protected]>

v01dstar · 2023-11-09T07:08:28Z

Why? fsync returning NoSpace does not guarantee you anything. The idea is to still always panic when fsync fails, but we can be smarter and not panic when pwrite fails.

PTAL. I move panic inside LogFileWriter::sync() and LogFileWriter::truncate(), 2 operations that may cause inconsistency. However, at the same time, I make raft-engine not panic if create(), write_header() fail during rotating files, which changes the decisions made in #131 a little bit. I think, it is safe to return the error (no matter what the error is) in such cases, because no inconsistency would happen, since raft-engine will recycle the broken file (if any) next time rotating the file.

src/file_pipe_log/log_file.rs

Signed-off-by: Yang Zhang <[email protected]>

LykxSassinator

Rest LGTM

src/file_pipe_log/log_file.rs

src/file_pipe_log/pipe.rs

src/file_pipe_log/log_file.rs

src/file_pipe_log/pipe.rs

tabokie · 2023-11-16T06:09:54Z

src/file_pipe_log/pipe.rs

@@ -272,7 +270,7 @@ impl<F: FileSystem> SinglePipe<F> {
        };
        // File header must be persisted. This way we can recover gracefully if power
        // loss before a new entry is written.
-        new_file.writer.sync()?;
+        new_file.writer.sync();
        self.sync_dir(path_id)?;


This error needs to be handled carefully now. (e.g. remove the newly created file and make sure the old writer is okay to write again) Better just unwrap it as well.

build_file_writer above is the same.

Made sync_dir panic if it fails.

But build_file_writer should be fine, right? It is the type of panic this PR trying to avoid (this can be confirmed by test_no_space_write_error). If it fails, the new file won't be used for writing and will be recycled the next time rotate_impl is called. So, it already meet your expectation?

Probably.. I suggest add a few restart in test_file_rotate_error.

Added a few more verifications in test_file_rotate_error test, should be able to address your concern? PTAL

Signed-off-by: Yang Zhang <[email protected]>

src/file_pipe_log/log_file.rs

src/file_pipe_log/pipe.rs

Signed-off-by: Yang Zhang <[email protected]>

Connor1996

LGTM

tabokie · 2023-11-30T14:09:49Z

src/file_pipe_log/pipe.rs

-        self.sync_dir(path_id)?;
+        // Panic if sync calls fail, keep consistent with the behavior of
+        // `LogFileWriter::sync()`.
+        self.sync_dir(path_id).unwrap();


panic inside sync_dir as well.

tabokie · 2023-11-30T14:10:15Z

src/file_pipe_log/pipe.rs

@@ -248,7 +248,7 @@ impl<F: FileSystem> SinglePipe<F> {
        let new_seq = writable_file.seq + 1;
        debug_assert!(new_seq > DEFAULT_FIRST_FILE_SEQ);

-        writable_file.writer.close()?;
+        writable_file.writer.close().unwrap();


No need to unwrap now.

tabokie · 2023-11-30T14:12:27Z

src/file_pipe_log/log_file.rs

@@ -67,7 +67,7 @@ impl<F: FileSystem> LogFileWriter<F> {
    }


Add a comment to this struct stating it should be fail-safe, i.e. user can still use the writer without breaking data consistency if any operation has failed.

tabokie · 2023-11-30T14:12:40Z

src/filter.rs

@@ -333,7 +333,7 @@ impl RhaiFilterMachine {
                    )?;
                    log_batch.drain();
                }
-                writer.close()?;
+                writer.close().unwrap();


tabokie · 2023-11-30T14:12:55Z

src/purge.rs

@@ -273,7 +273,7 @@ where
    // Rewrites the entire rewrite queue into new log files.
    fn rewrite_rewrite_queue(&self) -> Result<Vec<u64>> {
        let _t = StopWatch::new(&*ENGINE_REWRITE_REWRITE_DURATION_HISTOGRAM);
-        self.pipe_log.rotate(LogQueue::Rewrite)?;
+        self.pipe_log.rotate(LogQueue::Rewrite).unwrap();


why unwrap this?

tabokie · 2023-11-30T14:16:15Z

tests/failpoints/test_io_error.rs

@@ -165,20 +165,24 @@ fn test_file_rotate_error() {
    {


Make two versions of this test: fn test_file_rotate_error(restart: bool)

// case 1 if restart { let engine = Engine::open_with_file_system(cfg.clone(), fs.clone()).unwrap(); } // case 2 // ...

Signed-off-by: Yang Zhang <[email protected]>

tabokie

Rest LG

tabokie · 2023-12-05T01:54:48Z

tests/failpoints/test_io_error.rs

-    let engine = Engine::open_with_file_system(cfg.clone(), fs.clone()).unwrap();
-    engine
+    let mut engine = Some(Engine::open_with_file_system(cfg.clone(), fs.clone()).unwrap());
+    let mut engine_ref = engine.as_ref().unwrap();


No need, you can re-assign a variable after it's moved, e.g. drop(engine); engine = Engine::new();

Signed-off-by: Yang Zhang <[email protected]>

v01dstar · 2023-12-06T22:28:16Z

/test

Signed-off-by: Yang Zhang <[email protected]>

LykxSassinator · 2023-12-07T02:03:10Z

/cc @Connor1996 Can u help to merge this pr? THx

v01dstar added 2 commits November 2, 2023 14:52

Return error instead of panicing if rewriting fails

3a75b78

Signed-off-by: v01dstar <[email protected]>

Update rust version

77f8beb

Signed-off-by: v01dstar <[email protected]>

LykxSassinator self-requested a review November 3, 2023 02:21

Update rust version in github workflow

34db5d0

Signed-off-by: v01dstar <[email protected]>

tabokie reviewed Nov 3, 2023

View reviewed changes

LykxSassinator reviewed Nov 3, 2023

View reviewed changes

src/file_pipe_log/pipe.rs Show resolved Hide resolved

LykxSassinator reviewed Nov 3, 2023

View reviewed changes

src/file_pipe_log/pipe.rs Show resolved Hide resolved

LykxSassinator reviewed Nov 7, 2023

View reviewed changes

src/file_pipe_log/pipe.rs Outdated Show resolved Hide resolved

src/file_pipe_log/pipe.rs Outdated Show resolved Hide resolved

LykxSassinator reviewed Nov 7, 2023

View reviewed changes

src/file_pipe_log/pipe.rs Outdated Show resolved Hide resolved

v01dstar and others added 4 commits November 6, 2023 21:52

Update src/file_pipe_log/pipe.rs

452d57e

Co-authored-by: lucasliang <[email protected]> Signed-off-by: v01dstar <[email protected]>

Update src/file_pipe_log/pipe.rs

a20cd43

Co-authored-by: lucasliang <[email protected]> Signed-off-by: v01dstar <[email protected]>

Address comments, fix test cases

43b25ca

Signed-off-by: v01dstar <[email protected]>

Fix format error

6fcb077

Signed-off-by: v01dstar <[email protected]>

v01dstar force-pushed the panic-to-error branch from cbb8a6b to 6fcb077 Compare November 7, 2023 05:52

v01dstar added 5 commits November 8, 2023 11:46

Move panic inside

8cc474d

Signed-off-by: Yang Zhang <[email protected]>

Fix clippy

1fd5416

Signed-off-by: Yang Zhang <[email protected]>

Propagate error if writing header fails

c606f51

Signed-off-by: Yang Zhang <[email protected]>

Adjust write header fail expectation, from panic to error

61fbdb6

Signed-off-by: v01dstar <[email protected]>

Panic if write header fails since we do not truncate

862fe0b

Signed-off-by: v01dstar <[email protected]>

Failure other than sync should be returned

0d2924b

Signed-off-by: v01dstar <[email protected]>

Connor1996 reviewed Nov 10, 2023

View reviewed changes

src/file_pipe_log/log_file.rs Outdated Show resolved Hide resolved

v01dstar added 3 commits November 15, 2023 12:34

Address comments

a59609a

Signed-off-by: Yang Zhang <[email protected]>

Fix test failures

0554cd1

Signed-off-by: Yang Zhang <[email protected]>

Change test exepectations

2c81285

Signed-off-by: Yang Zhang <[email protected]>

LykxSassinator reviewed Nov 16, 2023

View reviewed changes

src/file_pipe_log/log_file.rs Outdated Show resolved Hide resolved

src/file_pipe_log/log_file.rs Outdated Show resolved Hide resolved

src/file_pipe_log/log_file.rs Outdated Show resolved Hide resolved

tabokie reviewed Nov 16, 2023

View reviewed changes

v01dstar added 3 commits November 24, 2023 23:42

Address comments

bd2c3b4

Signed-off-by: Yang Zhang <[email protected]>

Fix format

005418f

Signed-off-by: Yang Zhang <[email protected]>

Revert sync() signature

2c8d59a

Signed-off-by: Yang Zhang <[email protected]>

LykxSassinator approved these changes Nov 28, 2023

View reviewed changes

Connor1996 reviewed Nov 28, 2023

View reviewed changes

src/file_pipe_log/log_file.rs Outdated Show resolved Hide resolved

src/file_pipe_log/pipe.rs Outdated Show resolved Hide resolved

v01dstar added 2 commits November 28, 2023 13:42

Add more details to rotate test

395d530

Signed-off-by: Yang Zhang <[email protected]>

Fix style

cee2d8f

Signed-off-by: Yang Zhang <[email protected]>

Connor1996 approved these changes Nov 30, 2023

View reviewed changes

tabokie reviewed Nov 30, 2023

View reviewed changes

Address comments

8c2eb45

Signed-off-by: Yang Zhang <[email protected]>

tabokie approved these changes Dec 5, 2023

View reviewed changes

v01dstar added 2 commits December 5, 2023 22:38

Address comments

445fd1e

Signed-off-by: Yang Zhang <[email protected]>

Fix clippy

3106c04

Signed-off-by: Yang Zhang <[email protected]>

Trigger Github actions

bb27b29

Signed-off-by: Yang Zhang <[email protected]>

LykxSassinator assigned Connor1996 Dec 7, 2023

tonyxuqqi merged commit e8de5d7 into tikv:master Dec 7, 2023
7 checks passed

v01dstar deleted the panic-to-error branch December 7, 2023 02:58

v01dstar mentioned this pull request Dec 12, 2023

Revert rustc version #345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error instead of panicking if rewriting fails #343

Return error instead of panicking if rewriting fails #343

v01dstar commented Nov 3, 2023 •

edited

Loading

tabokie left a comment

LykxSassinator commented Nov 3, 2023 •

edited

Loading

v01dstar commented Nov 3, 2023

LykxSassinator commented Nov 3, 2023

tabokie commented Nov 4, 2023 •

edited

Loading

v01dstar commented Nov 7, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

tabokie commented Nov 7, 2023

LykxSassinator commented Nov 7, 2023

tabokie commented Nov 7, 2023

LykxSassinator commented Nov 7, 2023

tabokie commented Nov 8, 2023

v01dstar commented Nov 9, 2023 •

edited

Loading

LykxSassinator left a comment

tabokie Nov 16, 2023

tabokie Nov 16, 2023

v01dstar Nov 25, 2023

tabokie Nov 27, 2023

v01dstar Nov 29, 2023

Connor1996 left a comment

tabokie Nov 30, 2023

tabokie Nov 30, 2023

tabokie Nov 30, 2023

tabokie Nov 30, 2023

tabokie Nov 30, 2023

tabokie Nov 30, 2023

tabokie left a comment

tabokie Dec 5, 2023

v01dstar commented Dec 6, 2023

LykxSassinator commented Dec 7, 2023 •

edited

Loading

Return error instead of panicking if rewriting fails #343

Return error instead of panicking if rewriting fails #343

Conversation

v01dstar commented Nov 3, 2023 • edited Loading

tabokie left a comment

Choose a reason for hiding this comment

LykxSassinator commented Nov 3, 2023 • edited Loading

v01dstar commented Nov 3, 2023

LykxSassinator commented Nov 3, 2023

tabokie commented Nov 4, 2023 • edited Loading

v01dstar commented Nov 7, 2023

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

tabokie commented Nov 7, 2023

LykxSassinator commented Nov 7, 2023

tabokie commented Nov 7, 2023

LykxSassinator commented Nov 7, 2023

tabokie commented Nov 8, 2023

v01dstar commented Nov 9, 2023 • edited Loading

LykxSassinator 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabokie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v01dstar commented Dec 6, 2023

LykxSassinator commented Dec 7, 2023 • edited Loading

v01dstar commented Nov 3, 2023 •

edited

Loading

LykxSassinator commented Nov 3, 2023 •

edited

Loading

tabokie commented Nov 4, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

v01dstar commented Nov 9, 2023 •

edited

Loading

LykxSassinator commented Dec 7, 2023 •

edited

Loading