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

refactor: catch errno EINTR when reading and writing files instead of EAGAIN #242

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

guoxiangCN
Copy link
Contributor

@guoxiangCN guoxiangCN commented Jul 13, 2022

EAGAIN error code Appears only when the O_NONBLOCK option is set and there is no more content in the file to read.
If a read() is interrupted by a signal before it reads any data, it shall return -1 with errno set to [EINTR].

… EAGAIN

EAGAIN error code Appears only when the O_NONBLOCK option is set
and there is no more content in the file to read.

Signed-off-by: guoxiangCN <[email protected]>
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #242 (d2b41e0) into master (ee0f6cf) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   97.18%   97.19%   +0.01%     
==========================================
  Files          30       30              
  Lines        9368     9368              
==========================================
+ Hits         9104     9105       +1     
+ Misses        264      263       -1     
Impacted Files Coverage Δ
src/env/default.rs 92.13% <0.00%> (ø)
src/codec.rs 98.67% <0.00%> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee0f6cf...d2b41e0. Read the comment docs.

@tabokie
Copy link
Member

tabokie commented Jul 14, 2022

Thanks for the attention.

According to rust, a Write::write shouldn't write any bytes if an error is returned:

If an error is returned then no bytes in the buffer were written to this writer.

So it appears we should remove the retry looping entirely. For consistency do the same for the read as well.

FYI we already have retry logic at a higher level, so this change won't affect the IO results.

Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => {}

self.writer.write_all(buf)?;

@LykxSassinator
Copy link
Contributor

LykxSassinator commented Jul 14, 2022

How about direclty add an extra option OFlag::O_NONBLOCK for the open api rather than modify the Errno ? I think the original design on it is to use OFlag::O_NONBLOCK as the default option for fd.

@guoxiangCN
Copy link
Contributor Author

Okay, I didn't notice the higher judgment. However, according to the constraints of the write method you mentioned, retrying the entire while loop will also lead to a semi successful and semi failed state of data writing, that is, partial success and partial failure.
However, I'm not sure whether I need to cross the loop to really write. I don't know much about the underlying file system writing.

@tabokie
Copy link
Member

tabokie commented Jul 14, 2022

Having looked closely, I think the implementation here is not very important. We maintain a file offset and use it to write, so a partial successful write can be overwritten correctly afterwards:

let len = self.inner.write(self.offset, buf)?;
self.offset += len;

And the pwrite interface doesn't guarantee atomicity at all. (In fact non of the unix syscalls guarantees that, considering direct IO). So your current changes look okay.

@tabokie
Copy link
Member

tabokie commented Jul 14, 2022

I think the original design on it is to use OFlag::O_NONBLOCK as the default option for fd.

@LykxSassinator No, non block is only useful for network fd.

@tabokie tabokie merged commit be26141 into tikv:master Jul 14, 2022
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