-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fsync all the things #4030
fsync all the things #4030
Conversation
Fixed this up for windows; should be ready for review. |
d9f8dec
to
dd80d55
Compare
Fixed up the conflicts. |
Are you calling fsync on the parent directory when creating new files? |
No - good call. On at least POSIX-type systems this is necessary when creating new files. As far as I can tell, you cannot |
Once you commit this into master and expose it via rugged (Ruby) somehow, I will test it and let you know how it affects performance. |
One more case - after calling |
I'd like to see this PR get merged, but I'd also like to start an extended discussion on how to make this more efficient. Of course, calling I think it would be nice to review how Apache Lucene implements durability: http://blog.mikemccandless.com/2014/04/testing-lucenes-index-durability-after.html In particular, the way they batch changes, and test that fsync works seem like good ideas suitable for the odb backend. I'm not suggesting we defer this PR at all - correct is more important than performance. But, we should probably also consider performance too. If we could make the The data git manages is too critical for corruption and/or failures. |
This is why git doesn't call fsync, it kills performance. Databases solves this by having a single file for the undo/redo logs, which is truly append only (the SATA NCQ really likes to optimize writes for these logs) I guess this is why git is not a concurrent database system, and never will be. NOTE: do_fsync will always be default off, right? |
@ethomson Don't use FlushFileBuffers, it can be extremely slow (by Microsoft's own admission in the CreateFile docs). Use FILE_FLAG_WRITE_THROUGH | FILE_FLAG_NO_BUFFERING whenever you can instead. |
I agree with @pureconfig here. This is why we have a pluggable odb backend. Our default is to mimic git; if they change the way they write object files, we will change to match. But if you want some guarantees that git doesn't offer, then I would recommend putting your objects in a proper database.
I haven't read this paper, but my knee-jerk reaction is that any time we differ from git, we are incorrect. Generally speaking, we aim for bug-for-bug compatibility with git.
Yes. If git changes this default, so will we, but I can't imagine them doing so. |
/cc @whoisj 👋 I had it in my head that your filesystem team explicitly recommended that we use If |
Absolutely, but the git ODB with fsync would be good enough for my use case. Even without fsync, it's good enough actually, but it's a nice feature to have to improve durability. |
But you also said
So "nice feature" is an understatement. As mentioned by someone else, it sounds like you want guarantees implying the use of a proper database :) |
@ethomson this sounds correct, but it was about a year ago (or more?) that we had the discussion with them... and man, I'm getting old. My memory isn't what it used to be. So, on that note I've sent them a query re: this topic and hopefully will have a concrete answer for you in a day or so. 😄 |
Yeah, I still believe this and I think libgit2 should aim for this by default, but I also appreciate the "bug-for-bug" compatibility goal.
End to end durability isn't as simple as just calling |
@ethomson Do you guys use |
@ioquatix We use libgit2 and git both on our fileservers, which are all in Spokes. I don't want to spend a lot of time discussing GitHub's products in the libgit2 issue tracker, but the point of Spokes itself is to deal with problems like this. You can learn more at https://www.youtube.com/watch?v=f7ecUqHxD7o and https://www.youtube.com/watch?v=DY0yNRNkYb0 |
c5e72a3
to
15f92da
Compare
@ethomson the file system experts advise to not use the Per the experts, the extra time spent not buffering and forcing the write to block on IO availability, almost guarantees your data won't be get to persistent media if there's a catastrophe. Instead, the recommend that the application flush data at appropriate times. To quote them exactly:
|
Thanks @whoisj for the definitive answer. I've updated this to I've also changed the name of the option to |
15f92da
to
f4b7712
Compare
We've had an fsync option for a long time, but it was "ignored". Stop ignoring it.
Allow users to enable `SYNCHRONIZED_OBJECT_CREATION` with a setting.
Introduce a simple counter that `p_fsync` implements. This is useful for ensuring that `p_fsync` is called when we expect it to be, for example when we have enabled an odb backend to perform `fsync`s when writing objects.
Honor `git_object__synchronized_writing` when creating a packfile and corresponding index.
Add a custom `O_FSYNC` bit (if it's not been defined by the operating system`) so that `git_futils_writebuffer` can optionally do an `fsync` when it's done writing. We call `fsync` ourselves, even on systems that define `O_FSYNC` because its definition is no guarantee of its actual support. Mac, for instance, defines it but doesn't support it in an `open(2)` call.
Only use defaults for `git_futils_writebuffer` when flags == 0, lest (1 << 31) be treated as the defaults.
When fsync'ing files, fsync the parent directory in the case where we rename a file into place, or create a new file, to ensure that the directory entry is flushed correctly.
Rename `GIT_OPT_ENABLE_SYNCHRONIZED_OBJECT_CREATION` -> `GIT_OPT_ENABLE_SYNCHRONOUS_OBJECT_CREATION`.
Windows doesn't support it.
cff175b
to
10a7e54
Compare
I added support for This seems to be failing on Linux, though, after that. I'll take a look. |
The question is more like why did it work on Windows:
Two issues:
So this leaves me wondering why it did not fail on Windows. |
Haha; it succeeded on Mac and Windows explicitly because of the typo. Their Thanks for catching that, I'll fix it tonight. |
Yep; I did the same thing as your patch and will verify that the CI builds are happy before squashing it into the previous commit. |
If your question was inquiring why we didn't fail on Windows because it was fsync'ing when it shouldn't be, then the answer is that we didn't actually test that case. We ensured that we didn't fsync given an odb that isn't attached to a repo, but I never tested that given a repo, so we were simply always fsync'ing on platforms that were case insensitive. Anyway, I also added a test to ensure that we don't do that, since that would be for reals bad to regress. 😛 |
|
|
This has been hanging out for a while without any objections and since it doesn't change any default behavior (and some people seem interested in it) I'm going to go ahead and merge it. |
@ethomson this is rather a big deal, congrats on landing this and cross-platform no doubt. Well done sir. Well done indeed. 😄 |
If I update rugged from git, will it pull this code, or do you need to make a new release? I'd like to measure performance. |
It is open-source and use of cmake makes it very flexible. Why not compile and drop into existing framework to test? |
Because I use the rugged gem and my preference is that it just works without changing the existing build process. |
@ioquatix Rugged is tied to an exact version of libgit2, so it won't be updated automatically. I will send a PR to Rugged shortly. |
@ethomson That's awesome, thanks. Then, I'll report back. It will be really interesting to see what the impact is. |
Okay here is my report. I have two tests: https://github.com/ioquatix/relaxo/blob/9db8348fdeec8b5e3572bc543ab02af1bf0055b2/spec/relaxo/performance_spec.rb#L57-L77 One test creates lots of objects for each commit, and the other creates lots of separate commits with one object. Without fsync, on an oldish i7, running on an Samsung 840 PRO:
With fsync enabled:
|
My conclusion, is that for real world usage, using fsync should have no appreciable difference for general day to day git usage, but for high performance services, it might be an issue. |
Optionally
fsync
loose objects, packfiles and their indexes, loose references and packed reference files. This can be enabled by setting the optionGIT_OPT_ENABLE_SYNCHRONIZED_OBJECT_CREATION
, and it is not enabled by default.Additionally, enable the
do_fsync
option to the loose odb backend to perform similar calls.At present, we do not honor
core.fsyncObjectFiles
, though this would be a logical next step.