Add file sync shims #1418

divergentdave · 2020-05-18T03:25:45Z

This PR adds shim implementations for these related file syncing functions.

fsync, for POSIX targets, backed by File::sync_all()
fdatasync, for POSIX targets, backed by File::sync_data()
fcntl with command F_FULLFSYNC, for macOS targets, backed by File::sync_all()
sync_file_range, for Linux targets, backed by File::sync_data()

Adds implementations for fsync, fdatasync, and sync_file_range

RalfJung · 2020-05-19T09:13:36Z

I am not familiar with the detailed semantics of all these variants of sync. Are these all adequate implementations, or are some of them using sync_all because a more precise syncing is not possible with Rust's cross-platform APIs?

src/shims/foreign_items/posix.rs

src/shims/fs.rs

src/shims/foreign_items/posix/linux.rs

tests/run-pass/fs.rs

src/shims/fs.rs

divergentdave · 2020-05-19T13:08:51Z

Whoops, I misspoke above, sync_file_range uses File::sync_data(). I've edited the comment.

On non-ancient versions of Linux, fsync lines up well with File::sync_all(), as it writes out all data for a file and its file metadata, and then waits for the disk to finish writing. (on "older kernels", it did not flush disk caches) Likewise, fdatasync lines up well with File::sync_data(), as it skips writing out some file metadata. sync_file_range doesn't have an analogue in the Rust standard library, but it is closest to File::sync_data(), because it doesn't write out file metadata, and while sync_file_range allows selecting what range of the file should be written out, File::sync_data() writes out the whole file, so it's a stronger operation, and just loses out on performance.

On macOS, fsync writes out file contents and metadata, but it does not block until the disk has finished writing. fcntl with F_FULLFSYNC writes out both and then waits for the disk to write. While File::sync_all() looks like it could describe either operation, as its docs don't mention flushing the drive's buffers, (only the filesystem's) fcntl with F_FULLFSYNC is the stronger of the two operations on macOS, and that's what the stdlib uses to implement File. And yes, I put the fsync shim in the posix.rs file since it's a POSIX function, even though it may not be used on macOS.

RalfJung · 2020-05-19T13:12:42Z

sync_file_range doesn't have an analogue in the Rust standard library, but it is closest to File::sync_data(), because it doesn't write out file metadata, and while sync_file_range allows selecting what range of the file should be written out, File::sync_data() writes out the whole file, so it's a stronger operation, and just loses out on performance.

Thanks for the explanation! I see the code already has a comment for this, so this seems fine.

On macOS, fsync writes out file contents and metadata, but it does not block until the disk has finished writing. fcntl with F_FULLFSYNC writes out both and then waits for the disk to write. While File::sync_all() looks like it could describe either operation, as its docs don't mention flushing the drive's buffers, (only the filesystem's) fcntl with F_FULLFSYNC is the stronger of the two operations on macOS, and that's what the stdlib uses to implement File. And yes, I put the fsync shim in the posix.rs file since it's a POSIX function, even though it may not be used on macOS.

I see. So on macOS, the Miri implementation is stronger than what the actual system does. That's fine, but please leave a comment along those lines.

src/shims/fs.rs

tests/run-pass/libc.rs

src/shims/fs.rs

RalfJung · 2020-05-25T06:44:57Z

Thanks a lot! @bors r+

bors · 2020-05-25T06:44:58Z

📌 Commit 3092b9e has been approved by RalfJung

Add file sync shims This PR adds shim implementations for these related file syncing functions. * `fsync`, for POSIX targets, backed by `File::sync_all()` * `fdatasync`, for POSIX targets, backed by `File::sync_data()` * `fcntl` with command `F_FULLFSYNC`, for macOS targets, backed by `File::sync_all()` * `sync_file_range`, for Linux targets, backed by `File::sync_data()`

bors · 2020-05-25T06:45:07Z

⌛ Testing commit 3092b9e with merge 5f55f0e...

bors · 2020-05-25T06:57:57Z

💔 Test failed - status-appveyor

RalfJung · 2020-05-25T07:04:21Z

Looks like file.sync_data() is failing on a Windows host / Linux target (and then, as usual, we run into strerror_r when printing the error messages of the failure).

RalfJung · 2020-05-25T07:06:56Z

Hm, the code should work... maybe this is an AppVeyor thing?

src/shims/fs.rs

RalfJung · 2020-05-25T07:10:48Z

Let's see if this reproduces.
@bors try

bors · 2020-05-25T07:10:55Z

⌛ Trying commit 3092b9e with merge ba32813...

Add file sync shims This PR adds shim implementations for these related file syncing functions. * `fsync`, for POSIX targets, backed by `File::sync_all()` * `fdatasync`, for POSIX targets, backed by `File::sync_data()` * `fcntl` with command `F_FULLFSYNC`, for macOS targets, backed by `File::sync_all()` * `sync_file_range`, for Linux targets, backed by `File::sync_data()`

bors · 2020-05-26T01:03:22Z

⌛ Trying commit 889640b with merge d0cb89d...

Add file sync shims This PR adds shim implementations for these related file syncing functions. * `fsync`, for POSIX targets, backed by `File::sync_all()` * `fdatasync`, for POSIX targets, backed by `File::sync_data()` * `fcntl` with command `F_FULLFSYNC`, for macOS targets, backed by `File::sync_all()` * `sync_file_range`, for Linux targets, backed by `File::sync_data()`

bors · 2020-05-26T01:24:53Z

☀️ Try build successful - checks-travis, status-appveyor
Build commit: d0cb89d (d0cb89df165c9dbd5633b42c38a16569ade9c490)

divergentdave · 2020-05-26T03:04:50Z

@bors try

bors · 2020-05-26T03:04:57Z

⌛ Trying commit c01bc14 with merge 57b5acb...

Add file sync shims This PR adds shim implementations for these related file syncing functions. * `fsync`, for POSIX targets, backed by `File::sync_all()` * `fdatasync`, for POSIX targets, backed by `File::sync_data()` * `fcntl` with command `F_FULLFSYNC`, for macOS targets, backed by `File::sync_all()` * `sync_file_range`, for Linux targets, backed by `File::sync_data()`

bors · 2020-05-26T03:27:39Z

☀️ Try build successful - checks-travis, status-appveyor
Build commit: 57b5acb (57b5acb948954e846aa6d3d05a5736130242bd76)

RalfJung · 2020-05-26T07:05:07Z

Argh, GH doesn't let me view the force-pushd diff. :/
In the future, please avoid force pushes until review is done.

src/shims/fs.rs

tests/run-pass/fs.rs

divergentdave · 2020-06-06T17:43:20Z

I added similar logic to fcntl(F_FULLFSYNC) and sync_file_range, and added read-only files to both tests.

@bors try

bors · 2020-06-06T17:43:27Z

⌛ Trying commit e352d4f with merge e8ff19e...

Add file sync shims This PR adds shim implementations for these related file syncing functions. * `fsync`, for POSIX targets, backed by `File::sync_all()` * `fdatasync`, for POSIX targets, backed by `File::sync_data()` * `fcntl` with command `F_FULLFSYNC`, for macOS targets, backed by `File::sync_all()` * `sync_file_range`, for Linux targets, backed by `File::sync_data()`

bors · 2020-06-06T18:05:48Z

☀️ Try build successful - checks-travis, status-appveyor
Build commit: e8ff19e (e8ff19eed88eb639641ae8929aec1a57f052c198)

RalfJung · 2020-06-08T16:15:15Z

src/shims/fs.rs

+        }
+
+        if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get_mut(&fd) {
+            if !*writable && cfg!(windows) {


We have this same conditional 4 times now... can you factor this into a helper function?

RalfJung · 2020-06-08T23:29:49Z

I guess we can always clean up later. Thanks for the PR @divergentdave!
@bors r+

bors · 2020-06-08T23:29:50Z

📌 Commit e352d4f has been approved by RalfJung

divergentdave · 2020-06-08T23:31:22Z

Hang on, I just got the refactor done

RalfJung · 2020-06-08T23:34:20Z

Oh okay, that's quick. :)
@bors r-
I'm going to sleep now, will check this out the next morning.

RalfJung · 2020-06-09T08:48:36Z

Thanks. :)
@bors r+

bors · 2020-06-09T08:48:37Z

📌 Commit a60c130 has been approved by RalfJung

bors · 2020-06-09T08:48:45Z

⌛ Testing commit a60c130 with merge ffd03b3...

bors · 2020-06-09T09:13:37Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing ffd03b3 to master...

Add file sync shims

87c4694

Adds implementations for fsync, fdatasync, and sync_file_range

RalfJung reviewed May 19, 2020

View reviewed changes

src/shims/foreign_items/posix.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 19, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 19, 2020

View reviewed changes

src/shims/fs.rs Show resolved Hide resolved

RalfJung reviewed May 19, 2020

View reviewed changes

src/shims/foreign_items/posix/linux.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 19, 2020

View reviewed changes

tests/run-pass/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 19, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Review comments

0b060c7

RalfJung reviewed May 20, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 20, 2020

View reviewed changes

tests/run-pass/libc.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 20, 2020

View reviewed changes

tests/run-pass/libc.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 24, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

divergentdave force-pushed the file-sync branch from 4c6fdbd to fe8db0e Compare May 24, 2020 21:02

RalfJung reviewed May 24, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 24, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

divergentdave force-pushed the file-sync branch from fe8db0e to 3092b9e Compare May 24, 2020 22:45

RalfJung reviewed May 25, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Fix fsync shim for Windows hosts with RO files

c01bc14

divergentdave force-pushed the file-sync branch from 889640b to c01bc14 Compare May 26, 2020 02:46

RalfJung reviewed May 26, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 26, 2020

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 26, 2020

View reviewed changes

tests/run-pass/fs.rs Show resolved Hide resolved

Finish fixing Windows host support

e352d4f

RalfJung reviewed Jun 8, 2020

View reviewed changes

Extract common logic for Windows host workaround

a60c130

bors merged commit ffd03b3 into rust-lang:master Jun 9, 2020

divergentdave deleted the file-sync branch June 9, 2020 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file sync shims #1418

Add file sync shims #1418

divergentdave commented May 18, 2020 •

edited

RalfJung commented May 19, 2020

divergentdave commented May 19, 2020

RalfJung commented May 19, 2020

RalfJung commented May 25, 2020

bors commented May 25, 2020

bors commented May 25, 2020

bors commented May 25, 2020

RalfJung commented May 25, 2020

RalfJung commented May 25, 2020

RalfJung commented May 25, 2020

bors commented May 25, 2020

bors commented May 26, 2020

bors commented May 26, 2020

divergentdave commented May 26, 2020

bors commented May 26, 2020

bors commented May 26, 2020

RalfJung commented May 26, 2020

divergentdave commented Jun 6, 2020

bors commented Jun 6, 2020

bors commented Jun 6, 2020

RalfJung Jun 8, 2020

RalfJung commented Jun 8, 2020

bors commented Jun 8, 2020

divergentdave commented Jun 8, 2020

RalfJung commented Jun 8, 2020

RalfJung commented Jun 9, 2020

bors commented Jun 9, 2020

bors commented Jun 9, 2020

bors commented Jun 9, 2020

Add file sync shims #1418

Add file sync shims #1418

Conversation

divergentdave commented May 18, 2020 • edited

RalfJung commented May 19, 2020

divergentdave commented May 19, 2020

RalfJung commented May 19, 2020

RalfJung commented May 25, 2020

bors commented May 25, 2020

bors commented May 25, 2020

bors commented May 25, 2020

RalfJung commented May 25, 2020

RalfJung commented May 25, 2020

RalfJung commented May 25, 2020

bors commented May 25, 2020

bors commented May 26, 2020

bors commented May 26, 2020

divergentdave commented May 26, 2020

bors commented May 26, 2020

bors commented May 26, 2020

RalfJung commented May 26, 2020

divergentdave commented Jun 6, 2020

bors commented Jun 6, 2020

bors commented Jun 6, 2020

RalfJung Jun 8, 2020

Choose a reason for hiding this comment

RalfJung commented Jun 8, 2020

bors commented Jun 8, 2020

divergentdave commented Jun 8, 2020

RalfJung commented Jun 8, 2020

RalfJung commented Jun 9, 2020

bors commented Jun 9, 2020

bors commented Jun 9, 2020

bors commented Jun 9, 2020

divergentdave commented May 18, 2020 •

edited