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

Fix Issue 21627 - macOS: std.stdio.File.sync does not guarantee to be… #7789

Merged
merged 1 commit into from Mar 1, 2021

Conversation

kubo39
Copy link
Contributor

@kubo39 kubo39 commented Feb 12, 2021

… written to disk

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kubo39! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21627 normal macOS: std.stdio.File.sync does not guarantee to be written to disk

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7789"

@CyberShadow
Copy link
Member

Looking around a bit, it looks like F_FULLFSYNC actually does something else than fsync on other POSIX platforms:

https://www.postgresql.org/message-id/a06010200be3da9564694@%5B17.202.21.231%5D

Important bit:

The behavior
of fsync() on MacOS X is the same as it is on every other version of
Unix since the dawn of time (well, since the introduction of fsync
anyway :-).

and

https://news.ycombinator.com/item?id=6792669

I'm actually surprised Apple is even willing to make the guarantees above about F_FULLFSYNC; I'd read it as, at best, only applying to Apple hardware, as if a third-party controller is doing something silly they can't really do much about that.

So, it looks like there is no equivalent to F_FULLFSYNC for other platforms.

Whatever we choose to do, we should document that as the intent of the function, so that it can be updated consistently with new APIs / platforms. One of two things:

  • Do we want the function to (vaguely) submit the data to disk? In which case we should keep using fsync.
  • Do we want the function to provide a best-effort maximum guarantee (as far as the OS API allows) that the data has been permanently saved? In which case this is a change in that direction but the documentation ought to be updated to reflect that.

Finally, there is the question if there are circumstances where D programs wouldn't want to use F_FULLFSYNC, in which case this change would be a performance regression for them.

@n8sh
Copy link
Member

n8sh commented Feb 12, 2021

there is the question if there are circumstances where D programs wouldn't want to use F_FULLFSYNC, in which case this change would be a performance regression for them.

The one circumstance I can think of is if power loss to the storage device is believed to be impossible. Are there others?

@@ -982,7 +986,9 @@ Call $(LREF flush) before calling this function to flush the C `FILE` buffers fi

This function calls
$(HTTP msdn.microsoft.com/en-us/library/windows/desktop/aa364439%28v=vs.85%29.aspx,
`FlushFileBuffers`) on Windows and
`FlushFileBuffers`) on Windows,
$(HTTP developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link does not exist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it works now. I must have had a connection issue.

@RazvanN7
Copy link
Collaborator

At the minimum, I think that we should specify that fcntl is called using FULLSYNC. @kubo39 What is your stance on this?

@CyberShadow
Copy link
Member

At the minimum, I think that we should specify that fcntl is called using FULLSYNC.

Not sure if that's how you meant it, but just in case I'll clarify that it already does (the text says "..., F_FULLFSYNC fcntl on Darwin, and ...".

@dlang-bot dlang-bot merged commit 11ec2bf into dlang:master Mar 1, 2021
@kubo39 kubo39 deleted the use-FULLFSYNC-fcntl branch March 1, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants