{"thread":{"id":"56840","subject":"[PATCH] refs: sync loose refs to disk before committing them","startedAt":"2021-11-04T12:38:45Z","lastAt":"2021-11-11T12:14:30Z","messageCount":34,"participants":["Patrick Steinhardt","Ævar Arnfjörð Bjarmason","Junio C Hamano","Neeraj Singh","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440448","messageId":"dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@pks.im","threadId":"56840","inReplyTo":null,"subject":"[PATCH] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-04T12:38:22Z","receivedAt":"2021-11-04T12:38:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing loose refs, we first create a lockfile, write the new ref\ninto that lockfile, close it and then rename the lockfile into place\nsuch that the actual update is atomic for that single ref. While this\nworks as intended under normal circumstences, at GitLab we infrequently\nencounter corrupt loose refs in repositories after a machine encountered\na hard reset. The corruption is always of the same type: the ref has\nbeen committed into place, but it is completely empty.\n\nThe root cause of this is likely that we don't sync contents of the\nlockfile to disk before renaming it into place. As a result, it's not\nguaranteed that the contents are properly persisted and one may observe\nweird in-between states on hard resets. Quoting ext4 documentation [1]:\n\n Many broken applications don't use fsync() when replacing existing\n files via patterns such as fd =\n open(\"foo.new\")/write(fd,..)/close(fd)/ rename(\"foo.new\", \"foo\"), or\n worse yet, fd = open(\"foo\", O_TRUNC)/write(fd,..)/close(fd). If\n auto_da_alloc is enabled, ext4 will detect the replace-via-rename\n and replace-via-truncate patterns and force that any delayed\n allocation blocks are allocated such that at the next journal\n commit, in the default data=ordered mode, the data blocks of the new\n file are forced to disk before the rename() operation is committed.\n This provides roughly the same level of guarantees as ext3, and\n avoids the \"zero-length\" problem that can happen when a system\n crashes before the delayed allocation blocks are forced to disk.\n\nThis explicitly points out that one must call fsync(3P) before doing the\nrename(3P) call, or otherwise data may not be correctly persisted to\ndisk.\n\nFix this by always flushing refs to disk before committing them into\nplace to avoid this class of corruption.\n\n[1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 151b0056fe..06a3f0bdea 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1749,6 +1749,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \tfd = get_lock_file_fd(&lock->lk);\n \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t write_in_full(fd, &term, 1) < 0 ||\n+\t fsync(fd) < 0 ||\n \t close_ref_gently(lock) < 0) {\n \t\tstrbuf_addf(err,\n \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n-- \n2.33.1\n\n"},{"id":"440451","messageId":"211104.86ilx8hwvi.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@pks.im","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-04T13:14:54Z","receivedAt":"2021-11-04T13:29:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 04 2021, Patrick Steinhardt wrote:\n\n> When writing loose refs, we first create a lockfile, write the new ref\n> into that lockfile, close it and then rename the lockfile into place\n> such that the actual update is atomic for that single ref. While this\n> works as intended under normal circumstences, at GitLab we infrequently\n> encounter corrupt loose refs in repositories after a machine encountered\n> a hard reset. The corruption is always of the same type: the ref has\n> been committed into place, but it is completely empty.\n>\n> The root cause of this is likely that we don't sync contents of the\n> lockfile to disk before renaming it into place. As a result, it's not\n> guaranteed that the contents are properly persisted and one may observe\n> weird in-between states on hard resets. Quoting ext4 documentation [1]:\n>\n> Many broken applications don't use fsync() when replacing existing\n> files via patterns such as fd =\n> open(\"foo.new\")/write(fd,..)/close(fd)/ rename(\"foo.new\", \"foo\"), or\n> worse yet, fd = open(\"foo\", O_TRUNC)/write(fd,..)/close(fd). If\n> auto_da_alloc is enabled, ext4 will detect the replace-via-rename\n> and replace-via-truncate patterns and force that any delayed\n> allocation blocks are allocated such that at the next journal\n> commit, in the default data=ordered mode, the data blocks of the new\n> file are forced to disk before the rename() operation is committed.\n> This provides roughly the same level of guarantees as ext3, and\n> avoids the \"zero-length\" problem that can happen when a system\n> crashes before the delayed allocation blocks are forced to disk.\n>\n> This explicitly points out that one must call fsync(3P) before doing the\n> rename(3P) call, or otherwise data may not be correctly persisted to\n> disk.\n>\n> Fix this by always flushing refs to disk before committing them into\n> place to avoid this class of corruption.\n>\n> [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/files-backend.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 151b0056fe..06a3f0bdea 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1749,6 +1749,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n> \tfd = get_lock_file_fd(&lock->lk);\n> \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> \t write_in_full(fd, &term, 1) < 0 ||\n> +\t fsync(fd) < 0 ||\n> \t close_ref_gently(lock) < 0) {\n> \t\tstrbuf_addf(err,\n> \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n\nYeah, that really does seem like it's the cause of such zeroing out\nissues.\n\nThis has a semantic conflict with some other changes in flight, see:\n\n git log -p origin/master..origin/seen -- write-or-die.c\n\nI.e. here you do want to not die, so fsync_or_die() doesn't make sense\nper-se, but in those changes that function has grown to mean\nfsync_with_configured_strategy_or_die().\n\nAlso we need the loop around fsync, see cccdfd22436 (fsync(): be\nprepared to see EINTR, 2021-06-04).\n\nI think it would probably be best to create a git_fsync_fd() function\nwhich is non-fatal and has that config/while loop, and have\nfsync_or_die() be a \"..or die()\" wrapper around that, then you could\ncall that git_fsync_fd() here.\n\nOn the change more generally there's some performance numbers quoted at,\nso re the recent discussions about fsync() performance I wonder how this\nchanges things.\n\nI've also noted in those threads recently that our overall use of fsync\nis quite, bad, and especially when it comes to assuming that we don't\nneed to fsync dir entries, which we still don't do here.\n\nThe ext4 docs seem to suggest that this will be the right thing to do in\neither case, but I wonder if this won't increase the odds of corruption\non some other fs's.\n\nI.e. before we'd write() && rename() without the fsync(), so on systems\nthat deferred fsync() until some global sync point we might have been\nable to rely on those happening atomically (although clearly not on\nothers, e.g. ext4).\n\nBut now we'll fsync() the data explicitly, then do a rename(), but we\ndon't fsync the dir entry, so per POSIX an external application can't\nrely on seeing that rename yet. Will that bite us still, but just in\nanother way on some other systems?\n\n1. https://stackoverflow.com/questions/7433057/is-rename-without-fsync-safe\n"},{"id":"440457","messageId":"YYPzXEJ5L5vjjtJp@ncase","threadId":"56840","inReplyTo":"211104.86ilx8hwvi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-04T14:51:08Z","receivedAt":"2021-11-04T14:51:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 04, 2021 at 02:14:54PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Nov 04 2021, Patrick Steinhardt wrote:\n> \n> > When writing loose refs, we first create a lockfile, write the new ref\n> > into that lockfile, close it and then rename the lockfile into place\n> > such that the actual update is atomic for that single ref. While this\n> > works as intended under normal circumstences, at GitLab we infrequently\n> > encounter corrupt loose refs in repositories after a machine encountered\n> > a hard reset. The corruption is always of the same type: the ref has\n> > been committed into place, but it is completely empty.\n> >\n> > The root cause of this is likely that we don't sync contents of the\n> > lockfile to disk before renaming it into place. As a result, it's not\n> > guaranteed that the contents are properly persisted and one may observe\n> > weird in-between states on hard resets. Quoting ext4 documentation [1]:\n> >\n> > Many broken applications don't use fsync() when replacing existing\n> > files via patterns such as fd =\n> > open(\"foo.new\")/write(fd,..)/close(fd)/ rename(\"foo.new\", \"foo\"), or\n> > worse yet, fd = open(\"foo\", O_TRUNC)/write(fd,..)/close(fd). If\n> > auto_da_alloc is enabled, ext4 will detect the replace-via-rename\n> > and replace-via-truncate patterns and force that any delayed\n> > allocation blocks are allocated such that at the next journal\n> > commit, in the default data=ordered mode, the data blocks of the new\n> > file are forced to disk before the rename() operation is committed.\n> > This provides roughly the same level of guarantees as ext3, and\n> > avoids the \"zero-length\" problem that can happen when a system\n> > crashes before the delayed allocation blocks are forced to disk.\n> >\n> > This explicitly points out that one must call fsync(3P) before doing the\n> > rename(3P) call, or otherwise data may not be correctly persisted to\n> > disk.\n> >\n> > Fix this by always flushing refs to disk before committing them into\n> > place to avoid this class of corruption.\n> >\n> > [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs/files-backend.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 151b0056fe..06a3f0bdea 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1749,6 +1749,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n> > \tfd = get_lock_file_fd(&lock->lk);\n> > \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> > \t write_in_full(fd, &term, 1) < 0 ||\n> > +\t fsync(fd) < 0 ||\n> > \t close_ref_gently(lock) < 0) {\n> > \t\tstrbuf_addf(err,\n> > \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n> \n> Yeah, that really does seem like it's the cause of such zeroing out\n> issues.\n> \n> This has a semantic conflict with some other changes in flight, see:\n> \n> git log -p origin/master..origin/seen -- write-or-die.c\n> \n> I.e. here you do want to not die, so fsync_or_die() doesn't make sense\n> per-se, but in those changes that function has grown to mean\n> fsync_with_configured_strategy_or_die().\n> \n> Also we need the loop around fsync, see cccdfd22436 (fsync(): be\n> prepared to see EINTR, 2021-06-04).\n> \n> I think it would probably be best to create a git_fsync_fd() function\n> which is non-fatal and has that config/while loop, and have\n> fsync_or_die() be a \"..or die()\" wrapper around that, then you could\n> call that git_fsync_fd() here.\n\nThanks for pointing it out, I'll base v2 on next in that case.\n\n> On the change more generally there's some performance numbers quoted at,\n> so re the recent discussions about fsync() performance I wonder how this\n> changes things.\n\nYeah, good question. I punted on doing benchmarks for this given that I\nwasn't completely sure whether there's any preexisting ones which would\nfit best here.\n\nNo matter the results, I'd still take the stance that we should by\ndefault try to do the right thing and try hard to not end up with\ncorrupt data, and if filesystem docs explicitly say we must fsync(3P)\nthen that's what we should be doing. That being said, I wouldn't mind\nintroducing something like `core.fsyncObjectFiles` for refs, too, so\nthat folks who want an escape hatch have one.\n\n> I've also noted in those threads recently that our overall use of fsync\n> is quite, bad, and especially when it comes to assuming that we don't\n> need to fsync dir entries, which we still don't do here.\n\nYeah. I also thought about not putting the fsync(3P) logic into ref\nlogic, but instead into our lockfiles. In theory, we should always be\ndoing this before committing lockfiles into place, so it would fit in\nthere quite naturally.\n\n> The ext4 docs seem to suggest that this will be the right thing to do in\n> either case, but I wonder if this won't increase the odds of corruption\n> on some other fs's.\n> \n> I.e. before we'd write() && rename() without the fsync(), so on systems\n> that deferred fsync() until some global sync point we might have been\n> able to rely on those happening atomically (although clearly not on\n> others, e.g. ext4).\n> \n> But now we'll fsync() the data explicitly, then do a rename(), but we\n> don't fsync the dir entry, so per POSIX an external application can't\n> rely on seeing that rename yet. Will that bite us still, but just in\n> another way on some other systems?\n>\n> 1. https://stackoverflow.com/questions/7433057/is-rename-without-fsync-safe\n\nGood point. I'd happy to extend this patch to also fsync(3P) the dir\nentry. But it does sound like even more of a reason to move the logic\ninto the lockfiles such that we don't have to duplicate it wherever we\nreally don't want to end up with corrupted data.\n\nPatrick\n"},{"id":"440486","messageId":"xmqqk0hn1unp.fsf@gitster.g","threadId":"56840","inReplyTo":"211104.86ilx8hwvi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-04T21:24:26Z","receivedAt":"2021-11-04T21:24:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think it would probably be best to create a git_fsync_fd() function\n> which is non-fatal and has that config/while loop, and have\n> fsync_or_die() be a \"..or die()\" wrapper around that, then you could\n> call that git_fsync_fd() here.\n\nAdding git_fsync_fd() smells like a poor taste, as git_fsync() takes\nan fd already. How about making the current one into a static helper\n\n\t-int git_fsync(int fd, enum fsync_action action)\n\t+static int git_fsync_once(int fd, enum fsync_action action)\n\t ...\n\nand then hide the looping behavior behind git_fsync() proper?\n\n int git_fsync(int fd, enum fsync_action action)\n {\n while (git_fsync_once(fd, action) < 0)\n if (errno != EINTR)\n return -1;\n return 0;\n }\n\nfsync_or_die() can be simplified by getting rid of its loop.\n\nNone of that needs to block Patrick's work, I think. A version that\nuses raw fsync() and punts on EINTR can graduate first, which makes\nthe situation better than the status quo, and all the ongoing work\nthat deal with fsync can be extended with an eye to make it also\nusable to replace the fsync() call Patrick's fix adds.\n"},{"id":"440488","messageId":"20211104223600.GA4069@neerajsi-x1.localdomain","threadId":"56840","inReplyTo":"xmqqk0hn1unp.fsf@gitster.g","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-11-04T22:36:00Z","receivedAt":"2021-11-04T22:36:06Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Thu, Nov 04, 2021 at 02:24:26PM -0700, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > I think it would probably be best to create a git_fsync_fd() function\n> > which is non-fatal and has that config/while loop, and have\n> > fsync_or_die() be a \"..or die()\" wrapper around that, then you could\n> > call that git_fsync_fd() here.\n> \n> Adding git_fsync_fd() smells like a poor taste, as git_fsync() takes\n> an fd already. How about making the current one into a static helper\n> \n> \t-int git_fsync(int fd, enum fsync_action action)\n> \t+static int git_fsync_once(int fd, enum fsync_action action)\n> \t ...\n> \n> and then hide the looping behavior behind git_fsync() proper?\n> \n> int git_fsync(int fd, enum fsync_action action)\n> {\n> while (git_fsync_once(fd, action) < 0)\n> if (errno != EINTR)\n> return -1;\n> return 0;\n> }\n> \n> fsync_or_die() can be simplified by getting rid of its loop.\n> \n> None of that needs to block Patrick's work, I think. A version that\n> uses raw fsync() and punts on EINTR can graduate first, which makes\n> the situation better than the status quo, and all the ongoing work\n> that deal with fsync can be extended with an eye to make it also\n> usable to replace the fsync() call Patrick's fix adds.\n\nIs there some reason we shouldn't die if writing the ref fails? We are\nalready accustomed to dying if fsyncing a packfile or the index fails.\n\nI assume the number of refs updated is not that high on any given git\noperation, so it's not worth having a batch mode for this eventually.\n\nThanks,\nNeeraj\n"},{"id":"440499","messageId":"xmqqfssbz8ft.fsf@gitster.g","threadId":"56840","inReplyTo":"20211104223600.GA4069@neerajsi-x1.localdomain","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T01:40:22Z","receivedAt":"2021-11-05T01:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neeraj Singh writes:\n\n> Is there some reason we shouldn't die if writing the ref fails? We are\n> already accustomed to dying if fsyncing a packfile or the index fails.\n\nIf we look at the surrounding code and the callers of the function,\nthis caller of fsync() is about to signal a failure to its callers\nby returning an error(). The callers are prepared to see an error\nand cope with it.\n\nIntroducing die() to such a library-ish part of the code deep in the\ncallchain is not exactly a good change, especially when it is\nobvious how we can avoid it.\n"},{"id":"440503","messageId":"YYTQ//+8OnsR60k1@coredump.intra.peff.net","threadId":"56840","inReplyTo":"xmqqfssbz8ft.fsf@gitster.g","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T06:36:47Z","receivedAt":"2021-11-05T06:36:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 04, 2021 at 06:40:22PM -0700, Junio C Hamano wrote:\n\n> Neeraj Singh writes:\n> \n> > Is there some reason we shouldn't die if writing the ref fails? We are\n> > already accustomed to dying if fsyncing a packfile or the index fails.\n> \n> If we look at the surrounding code and the callers of the function,\n> this caller of fsync() is about to signal a failure to its callers\n> by returning an error(). The callers are prepared to see an error\n> and cope with it.\n> \n> Introducing die() to such a library-ish part of the code deep in the\n> callchain is not exactly a good change, especially when it is\n> obvious how we can avoid it.\n\nAnd here's a good concrete example that relies on it: when receive-pack\nis taking in a push, if the ref fails it should report that via the\nprotocol back to the client, rather than just hanging up the connection.\nThat leaves the client less confused, and gives it the opportunity to\nattempt and report status on other ref updates.\n\n-Peff\n"},{"id":"440504","messageId":"YYTYJpyrxtyR8yYZ@coredump.intra.peff.net","threadId":"56840","inReplyTo":"dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@pks.im","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T07:07:18Z","receivedAt":"2021-11-05T07:07:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 04, 2021 at 01:38:22PM +0100, Patrick Steinhardt wrote:\n\n> When writing loose refs, we first create a lockfile, write the new ref\n> into that lockfile, close it and then rename the lockfile into place\n> such that the actual update is atomic for that single ref. While this\n> works as intended under normal circumstences, at GitLab we infrequently\n> encounter corrupt loose refs in repositories after a machine encountered\n> a hard reset. The corruption is always of the same type: the ref has\n> been committed into place, but it is completely empty.\n\nYes, I've seen this quite a bit, too, and I agree that more fsync()-ing\nwill probably help.\n\nThere are two things that have made me hesitate, though:\n\n 1. This doesn't quite make the write durable. Doing that would also\n require fsyncing the surrounding directory (and so on up the tree).\n This is a much trickier problem, because we often don't even have\n open descriptors for those.\n\n I think we can ignore that, though. Doing an fsync() here makes\n things strictly better with respect to robustness (especially if\n you care less about \"has my ref update been committed to disk\" and\n more about \"does this end up corrupt after a power failure\").\n\n 2. It's not clear what the performance implications will be,\n especially on a busy server doing a lot of ref updates, or on a\n filesystem where fsync() ends up syncing everything, not just the\n one file (my impression is ext3 is such a system, but not ext4).\n Whereas another solution may be journaling data and metadata writes\n in order without worrying about the durability of writing them to\n disk.\n\n I suspect for small updates (say, a push of one or two refs), this\n will have little impact. We'd generally fsync the incoming packfile\n and its idx anyway, so we're adding may one or two fsyncs on top of\n that. But if you're pushing 100 refs, that will be 100 sequential\n fsyncs, which may add up to quite a bit of latency. It would be\n nice if we could batch these by somehow (e.g., by opening up all of\n the lockfiles, writing and fsyncing them, and then renaming one by\n one).\n\n So I think we want to at least add a config knob here. That would\n make it easier to experiment, and provides an escape hatch.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 151b0056fe..06a3f0bdea 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1749,6 +1749,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n> \tfd = get_lock_file_fd(&lock->lk);\n> \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> \t write_in_full(fd, &term, 1) < 0 ||\n> +\t fsync(fd) < 0 ||\n> \t close_ref_gently(lock) < 0) {\n> \t\tstrbuf_addf(err,\n> \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n\nI agree with the sentiment elsewhere that this would probably make sense\nfor any lockfile. That does probably make batching a bit more difficult,\nthough it could be layered on top (i.e., we could still fsync in the ref\ncode, and the lockfile fsync-ing should then become a noop).\n\n-Peff\n"},{"id":"440505","messageId":"YYTaiIlEKxHRVdCy@coredump.intra.peff.net","threadId":"56840","inReplyTo":"YYTYJpyrxtyR8yYZ@coredump.intra.peff.net","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T07:17:28Z","receivedAt":"2021-11-05T07:17:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 03:07:18AM -0400, Jeff King wrote:\n\n> 2. It's not clear what the performance implications will be,\n> especially on a busy server doing a lot of ref updates, or on a\n> filesystem where fsync() ends up syncing everything, not just the\n> one file (my impression is ext3 is such a system, but not ext4).\n> Whereas another solution may be journaling data and metadata writes\n> in order without worrying about the durability of writing them to\n> disk.\n> \n> I suspect for small updates (say, a push of one or two refs), this\n> will have little impact. We'd generally fsync the incoming packfile\n> and its idx anyway, so we're adding may one or two fsyncs on top of\n> that. But if you're pushing 100 refs, that will be 100 sequential\n> fsyncs, which may add up to quite a bit of latency. It would be\n> nice if we could batch these by somehow (e.g., by opening up all of\n> the lockfiles, writing and fsyncing them, and then renaming one by\n> one).\n\nSo here's a quick experiment that shows a worst case: a small push that\nupdates a bunch of refs. After building Git with and without your patch,\nI set up a small repo like:\n\n git init\n git commit --allow-empty -m foo\n for i in $(seq 100); do\n git update-ref refs/heads/$i HEAD\n done\n\nTo give a clean slate between runs, I stuck this in a script called\n\"setup\":\n\n #!/bin/sh\n rm -rf dst.git\n git init --bare dst.git\n sync\n\nAnd then ran:\n\n $ hyperfine -L v orig,fsync -p ./setup '/tmp/{v}/bin/git push dst.git refs/heads/*'\n Benchmark 1: /tmp/orig/bin/git push dst.git refs/heads/*\n Time (mean ± σ): 9.9 ms ± 0.2 ms [User: 6.3 ms, System: 4.7 ms]\n Range (min … max): 9.5 ms … 10.5 ms 111 runs\n \n Benchmark 2: /tmp/fsync/bin/git push dst.git refs/heads/*\n Time (mean ± σ): 401.0 ms ± 7.7 ms [User: 9.4 ms, System: 15.2 ms]\n Range (min … max): 389.4 ms … 412.4 ms 10 runs\n \n Summary\n '/tmp/orig/bin/git push dst.git refs/heads/*' ran\n 40.68 ± 1.16 times faster than '/tmp/fsync/bin/git push dst.git refs/heads/*'\n\nSo it really does produce a noticeable impact (this is on a system with\na decent SSD and no other disk load, so I'd expect it to be about\naverage for modern hardware).\n\nNow this test isn't entirely fair. 100 refs is a larger than average\nnumber to be pushing, and the effect is out-sized because there's\nvirtually no time spent dealing with the objects themselves, nor is\nthere any network latency. But 400ms feels like a non-trivial amount of\ntime just in absolute numbers.\n\nThe numbers scale pretty linearly, as you'd expect. Pushing 10 refs\ntakes ~40ms, 100 takes ~400ms, and 1000 takes ~4s. The non-fsyncing\nversion gets slower, too (there's more work to do), but much more slowly\n(6ms, 10ms, and 50ms respectively).\n\nSo this will definitely hurt at edge / pathological cases.\n\n-Peff\n"},{"id":"440512","messageId":"211105.865yt7hu7x.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"20211104223600.GA4069@neerajsi-x1.localdomain","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-05T08:35:24Z","receivedAt":"2021-11-05T08:39:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 04 2021, Neeraj Singh wrote:\n\n> On Thu, Nov 04, 2021 at 02:24:26PM -0700, Junio C Hamano wrote:\n>> Ævar Arnfjörð Bjarmason writes:\n>> \n>> > I think it would probably be best to create a git_fsync_fd() function\n>> > which is non-fatal and has that config/while loop, and have\n>> > fsync_or_die() be a \"..or die()\" wrapper around that, then you could\n>> > call that git_fsync_fd() here.\n>> \n>> Adding git_fsync_fd() smells like a poor taste, as git_fsync() takes\n>> an fd already. How about making the current one into a static helper\n>> \n>> \t-int git_fsync(int fd, enum fsync_action action)\n>> \t+static int git_fsync_once(int fd, enum fsync_action action)\n>> \t ...\n>> \n>> and then hide the looping behavior behind git_fsync() proper?\n>> \n>> int git_fsync(int fd, enum fsync_action action)\n>> {\n>> while (git_fsync_once(fd, action) < 0)\n>> if (errno != EINTR)\n>> return -1;\n>> return 0;\n>> }\n>> \n>> fsync_or_die() can be simplified by getting rid of its loop.\n>> \n>> None of that needs to block Patrick's work, I think. A version that\n>> uses raw fsync() and punts on EINTR can graduate first, which makes\n>> the situation better than the status quo, and all the ongoing work\n>> that deal with fsync can be extended with an eye to make it also\n>> usable to replace the fsync() call Patrick's fix adds.\n>\n> Is there some reason we shouldn't die if writing the ref fails? We are\n> already accustomed to dying if fsyncing a packfile or the index fails.\n>\n> I assume the number of refs updated is not that high on any given git\n> operation, so it's not worth having a batch mode for this eventually.\n\nOthers have mostly touched on this, but usually (always?) we're about to\ndie anyway, but by not calling die() here we'll die with a less crappy\nmessage.\n\nI.e. this bit:\n\n strbuf_addf(err, [...]\n\nIs effectively a deferred die() in most (all?) cases, and be passed up\nto the ref update code. because instead of a nondescript message like:\n\n fsync: failed somewhere\n\nWe want:\n\n couldn't update ref xyz to OID abc: fsync failed\n\nOr similar. \n\nHrm, actually having written the above there's a really good reason not\nto die, which is that fsync can return ENOSPC.\n\nSo if we're in the middle of a transaction and have created and written\nthe lockfile we might only notice that the disk has is full when we do\nthe fsync().\n\nIn that case we'll (or should, I didn't check just now) unroll the ref\ntransaction, and delete the *.lock files we created, which presumably\nwill succeed in that scenario.\n\nSo calling die() at this level is the difference between leaving the\nrepo in an inconsistent state due to a disk error, and something like\n\"git fetch --atomic\" gracefully failing.\n"},{"id":"440515","messageId":"YYTzmWzAaqvGFu/v@coredump.intra.peff.net","threadId":"56840","inReplyTo":"211105.865yt7hu7x.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T09:04:25Z","receivedAt":"2021-11-05T09:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 09:35:24AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> So if we're in the middle of a transaction and have created and written\n> the lockfile we might only notice that the disk has is full when we do\n> the fsync().\n> \n> In that case we'll (or should, I didn't check just now) unroll the ref\n> transaction, and delete the *.lock files we created, which presumably\n> will succeed in that scenario.\n> \n> So calling die() at this level is the difference between leaving the\n> repo in an inconsistent state due to a disk error, and something like\n> \"git fetch --atomic\" gracefully failing.\n\nWe should rollback the lockfiles even if we call die(), via the atexit()\nhandler. Ditto if we receive a fatal signal.\n\n(But I completely agree that if we have the opportunity to just pass the\nerror up the stack, we should do so).\n\n-Peff\n"},{"id":"440517","messageId":"nycvar.QRO.7.76.6.2111051010070.56@tvgsbejvaqbjf.bet","threadId":"56840","inReplyTo":"YYTaiIlEKxHRVdCy@coredump.intra.peff.net","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-05T09:12:25Z","receivedAt":"2021-11-05T09:12:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Patrick,\n\nOn Fri, 5 Nov 2021, Jeff King wrote:\n\n> On Fri, Nov 05, 2021 at 03:07:18AM -0400, Jeff King wrote:\n>\n> > 2. It's not clear what the performance implications will be,\n> > especially on a busy server doing a lot of ref updates, or on a\n> > filesystem where fsync() ends up syncing everything, not just the\n> > one file (my impression is ext3 is such a system, but not ext4).\n> > Whereas another solution may be journaling data and metadata writes\n> > in order without worrying about the durability of writing them to\n> > disk.\n> >\n> > I suspect for small updates (say, a push of one or two refs), this\n> > will have little impact. We'd generally fsync the incoming packfile\n> > and its idx anyway, so we're adding may one or two fsyncs on top of\n> > that. But if you're pushing 100 refs, that will be 100 sequential\n> > fsyncs, which may add up to quite a bit of latency. It would be\n> > nice if we could batch these by somehow (e.g., by opening up all of\n> > the lockfiles, writing and fsyncing them, and then renaming one by\n> > one).\n>\n> So here's a quick experiment that shows a worst case: a small push that\n> updates a bunch of refs. After building Git with and without your patch,\n> I set up a small repo like:\n>\n> git init\n> git commit --allow-empty -m foo\n> for i in $(seq 100); do\n> git update-ref refs/heads/$i HEAD\n> done\n>\n> To give a clean slate between runs, I stuck this in a script called\n> \"setup\":\n>\n> #!/bin/sh\n> rm -rf dst.git\n> git init --bare dst.git\n> sync\n>\n> And then ran:\n>\n> $ hyperfine -L v orig,fsync -p ./setup '/tmp/{v}/bin/git push dst.git refs/heads/*'\n> Benchmark 1: /tmp/orig/bin/git push dst.git refs/heads/*\n> Time (mean ± σ): 9.9 ms ± 0.2 ms [User: 6.3 ms, System: 4.7 ms]\n> Range (min … max): 9.5 ms … 10.5 ms 111 runs\n>\n> Benchmark 2: /tmp/fsync/bin/git push dst.git refs/heads/*\n> Time (mean ± σ): 401.0 ms ± 7.7 ms [User: 9.4 ms, System: 15.2 ms]\n> Range (min … max): 389.4 ms … 412.4 ms 10 runs\n>\n> Summary\n> '/tmp/orig/bin/git push dst.git refs/heads/*' ran\n> 40.68 ± 1.16 times faster than '/tmp/fsync/bin/git push dst.git refs/heads/*'\n>\n> So it really does produce a noticeable impact (this is on a system with\n> a decent SSD and no other disk load, so I'd expect it to be about\n> average for modern hardware).\n>\n> Now this test isn't entirely fair. 100 refs is a larger than average\n> number to be pushing, and the effect is out-sized because there's\n> virtually no time spent dealing with the objects themselves, nor is\n> there any network latency. But 400ms feels like a non-trivial amount of\n> time just in absolute numbers.\n>\n> The numbers scale pretty linearly, as you'd expect. Pushing 10 refs\n> takes ~40ms, 100 takes ~400ms, and 1000 takes ~4s. The non-fsyncing\n> version gets slower, too (there's more work to do), but much more slowly\n> (6ms, 10ms, and 50ms respectively).\n>\n> So this will definitely hurt at edge / pathological cases.\n\nOuch.\n\nI wonder whether this could be handled similarly to the\n`core.fsyncObjectFiles=batch` mode that has been proposed in\nhttps://lore.kernel.org/git/pull.1076.v8.git.git.1633366667.gitgitgadget@gmail.com/\n\nEssentially, we would have to find a better layer to do this, where we\ncan synchronize after a potentially quite large number of ref updates has\nhappened. That would definitely be a different layer than the file-based\nrefs backend, of course, and would probably apply in a different way to\nother refs backends.\n\nCiao,\nDscho\n"},{"id":"440518","messageId":"YYT3zx+P5maUAu5K@ncase","threadId":"56840","inReplyTo":"nycvar.QRO.7.76.6.2111051010070.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-05T09:22:23Z","receivedAt":"2021-11-05T09:22:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 05, 2021 at 10:12:25AM +0100, Johannes Schindelin wrote:\n> Hi Peff & Patrick,\n> \n> On Fri, 5 Nov 2021, Jeff King wrote:\n> \n> > On Fri, Nov 05, 2021 at 03:07:18AM -0400, Jeff King wrote:\n> >\n> > > 2. It's not clear what the performance implications will be,\n> > > especially on a busy server doing a lot of ref updates, or on a\n> > > filesystem where fsync() ends up syncing everything, not just the\n> > > one file (my impression is ext3 is such a system, but not ext4).\n> > > Whereas another solution may be journaling data and metadata writes\n> > > in order without worrying about the durability of writing them to\n> > > disk.\n> > >\n> > > I suspect for small updates (say, a push of one or two refs), this\n> > > will have little impact. We'd generally fsync the incoming packfile\n> > > and its idx anyway, so we're adding may one or two fsyncs on top of\n> > > that. But if you're pushing 100 refs, that will be 100 sequential\n> > > fsyncs, which may add up to quite a bit of latency. It would be\n> > > nice if we could batch these by somehow (e.g., by opening up all of\n> > > the lockfiles, writing and fsyncing them, and then renaming one by\n> > > one).\n> >\n> > So here's a quick experiment that shows a worst case: a small push that\n> > updates a bunch of refs. After building Git with and without your patch,\n> > I set up a small repo like:\n> >\n> > git init\n> > git commit --allow-empty -m foo\n> > for i in $(seq 100); do\n> > git update-ref refs/heads/$i HEAD\n> > done\n> >\n> > To give a clean slate between runs, I stuck this in a script called\n> > \"setup\":\n> >\n> > #!/bin/sh\n> > rm -rf dst.git\n> > git init --bare dst.git\n> > sync\n> >\n> > And then ran:\n> >\n> > $ hyperfine -L v orig,fsync -p ./setup '/tmp/{v}/bin/git push dst.git refs/heads/*'\n> > Benchmark 1: /tmp/orig/bin/git push dst.git refs/heads/*\n> > Time (mean ± σ): 9.9 ms ± 0.2 ms [User: 6.3 ms, System: 4.7 ms]\n> > Range (min … max): 9.5 ms … 10.5 ms 111 runs\n> >\n> > Benchmark 2: /tmp/fsync/bin/git push dst.git refs/heads/*\n> > Time (mean ± σ): 401.0 ms ± 7.7 ms [User: 9.4 ms, System: 15.2 ms]\n> > Range (min … max): 389.4 ms … 412.4 ms 10 runs\n> >\n> > Summary\n> > '/tmp/orig/bin/git push dst.git refs/heads/*' ran\n> > 40.68 ± 1.16 times faster than '/tmp/fsync/bin/git push dst.git refs/heads/*'\n> >\n> > So it really does produce a noticeable impact (this is on a system with\n> > a decent SSD and no other disk load, so I'd expect it to be about\n> > average for modern hardware).\n> >\n> > Now this test isn't entirely fair. 100 refs is a larger than average\n> > number to be pushing, and the effect is out-sized because there's\n> > virtually no time spent dealing with the objects themselves, nor is\n> > there any network latency. But 400ms feels like a non-trivial amount of\n> > time just in absolute numbers.\n> >\n> > The numbers scale pretty linearly, as you'd expect. Pushing 10 refs\n> > takes ~40ms, 100 takes ~400ms, and 1000 takes ~4s. The non-fsyncing\n> > version gets slower, too (there's more work to do), but much more slowly\n> > (6ms, 10ms, and 50ms respectively).\n> >\n> > So this will definitely hurt at edge / pathological cases.\n> \n> Ouch.\n> \n> I wonder whether this could be handled similarly to the\n> `core.fsyncObjectFiles=batch` mode that has been proposed in\n> https://lore.kernel.org/git/pull.1076.v8.git.git.1633366667.gitgitgadget@gmail.com/\n> \n> Essentially, we would have to find a better layer to do this, where we\n> can synchronize after a potentially quite large number of ref updates has\n> happened. That would definitely be a different layer than the file-based\n> refs backend, of course, and would probably apply in a different way to\n> other refs backends.\n> \n> Ciao,\n> Dscho\n\nGood question. We'd likely have to make this part of the ref_transaction\nlayer to make this work without having to update all callers. It would\nlikely work something like the following:\n\n 1. The caller queues up all ref updates.\n\n 2. The caller moves the transaction into prepared state, which\n creates all the lockfiles for us.\n\n 3. After all lockfiles have been created, we must sync them to disk.\n This may either happen when preparing the transaction, or when\n committing it. I think it's better located in prepare though so\n that the likelihood that the transaction succeeds after a\n successful prepare is high.\n\n 4. Now we can rename all files into place.\n\nNaturally, we'd only benefit from this batching in case the caller\nactually queues up all ref updates into a single transaction. And\nthere's going to be at least some which don't by default. The first that\ncomes to my mind is git-fetch(1), which explicitly requires the user to\nuse `--atomic` to queue them all up into a single transaction. And I\nguess there's going to be others which use one transaction per ref.\n\nPatrick\n"},{"id":"440519","messageId":"YYT6tDyfBbwot2br@coredump.intra.peff.net","threadId":"56840","inReplyTo":"nycvar.QRO.7.76.6.2111051010070.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T09:34:44Z","receivedAt":"2021-11-05T09:34:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 10:12:25AM +0100, Johannes Schindelin wrote:\n\n> > So this will definitely hurt at edge / pathological cases.\n> \n> Ouch.\n> \n> I wonder whether this could be handled similarly to the\n> `core.fsyncObjectFiles=batch` mode that has been proposed in\n> https://lore.kernel.org/git/pull.1076.v8.git.git.1633366667.gitgitgadget@gmail.com/\n\nYeah, that was along the lines I was thinking.\n\nI hadn't really looked at the details of the batch-fsync there. The big\ntrick seems to be doing a pagecache writeback for each file, and then\nstimulating an actual disk write (by fsyncing a tempfile) after the\nbatch is done.\n\nThat would be pretty easy to do for the refs (it's git_fsync() while\nclosing each file where Patrick is calling fsync(), followed by a single\ndo_batch_fsync() after everything is closed but before we rename).\n\n> Essentially, we would have to find a better layer to do this, where we\n> can synchronize after a potentially quite large number of ref updates has\n> happened. That would definitely be a different layer than the file-based\n> refs backend, of course, and would probably apply in a different way to\n> other refs backends.\n\nWe do have the concept of a ref_transaction, so that would be the\nnatural place for it. Not every caller uses it, though, because it\nimplies atomicity of the transaction (so some may do a sequence of N\nindependent transactions, because they don't want failure of one to\nimpact others). I think that could be changed, if the ref_transaction\nlearned about non-atomicity, but it may take some surgery.\n\nI expect that reftables would similarly benefit; it is probably much\nmore efficient to write a table slice with N entries than it is to write\nN slices, even before accounting for fsync(). And once doing that, then\nthe fsync() part becomes trivial.\n\n-Peff\n"},{"id":"440738","messageId":"YYpauqpBDVzOo+Px@ncase","threadId":"56840","inReplyTo":"YYT6tDyfBbwot2br@coredump.intra.peff.net","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-09T11:25:46Z","receivedAt":"2021-11-09T11:26:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 05, 2021 at 05:34:44AM -0400, Jeff King wrote:\n> On Fri, Nov 05, 2021 at 10:12:25AM +0100, Johannes Schindelin wrote:\n> \n> > > So this will definitely hurt at edge / pathological cases.\n> > \n> > Ouch.\n> > \n> > I wonder whether this could be handled similarly to the\n> > `core.fsyncObjectFiles=batch` mode that has been proposed in\n> > https://lore.kernel.org/git/pull.1076.v8.git.git.1633366667.gitgitgadget@gmail.com/\n> \n> Yeah, that was along the lines I was thinking.\n> \n> I hadn't really looked at the details of the batch-fsync there. The big\n> trick seems to be doing a pagecache writeback for each file, and then\n> stimulating an actual disk write (by fsyncing a tempfile) after the\n> batch is done.\n> \n> That would be pretty easy to do for the refs (it's git_fsync() while\n> closing each file where Patrick is calling fsync(), followed by a single\n> do_batch_fsync() after everything is closed but before we rename).\n> \n> > Essentially, we would have to find a better layer to do this, where we\n> > can synchronize after a potentially quite large number of ref updates has\n> > happened. That would definitely be a different layer than the file-based\n> > refs backend, of course, and would probably apply in a different way to\n> > other refs backends.\n> \n> We do have the concept of a ref_transaction, so that would be the\n> natural place for it. Not every caller uses it, though, because it\n> implies atomicity of the transaction (so some may do a sequence of N\n> independent transactions, because they don't want failure of one to\n> impact others). I think that could be changed, if the ref_transaction\n> learned about non-atomicity, but it may take some surgery.\n> \n> I expect that reftables would similarly benefit; it is probably much\n> more efficient to write a table slice with N entries than it is to write\n> N slices, even before accounting for fsync(). And once doing that, then\n> the fsync() part becomes trivial.\n> \n> -Peff\n\nSo I've finally found the time to have another look at massaging this\ninto the ref_transaction mechanism. If we do want to batch the fsync(3P)\ncalls, then we basically have two different alternatives:\n\n 1. We first lock all loose refs by creating the respective lock\n files and writing the updated ref into that file. We keep the\n file descriptor open such that we can then flush them all in one\n go.\n\n 2. Same as before, we lock all refs and write the updated pointers\n into the lockfiles, but this time we close each lockfile after\n having written to it. Later, we reopen them all to fsync(3P) them\n to disk.\n\nI'm afraid both alternatives aren't any good: the first alternative\nrisks running out of file descriptors if you queue up lots of refs. And\nthe second alternative is going to be slow, especially so on Windows if\nI'm not mistaken.\n\nSo with both not being feasible, we'll likely have to come up with a\nmore complex scheme if we want to batch-sync files. One idea would be to\nfsync(3P) all lockfiles every $n refs, but it adds complexity in a place\nwhere I'd really like to have things as simple as possible. It also\nraises the question what $n would have to be.\n\nDoes anybody else have better ideas than I do?\n\nPatrick\n"},{"id":"440817","messageId":"YYuEq42toR6mycem@coredump.intra.peff.net","threadId":"56840","inReplyTo":"YYpauqpBDVzOo+Px@ncase","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T08:36:59Z","receivedAt":"2021-11-10T08:37:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 09, 2021 at 12:25:46PM +0100, Patrick Steinhardt wrote:\n\n> So I've finally found the time to have another look at massaging this\n> into the ref_transaction mechanism. If we do want to batch the fsync(3P)\n> calls, then we basically have two different alternatives:\n> \n> 1. We first lock all loose refs by creating the respective lock\n> files and writing the updated ref into that file. We keep the\n> file descriptor open such that we can then flush them all in one\n> go.\n> \n> 2. Same as before, we lock all refs and write the updated pointers\n> into the lockfiles, but this time we close each lockfile after\n> having written to it. Later, we reopen them all to fsync(3P) them\n> to disk.\n> \n> I'm afraid both alternatives aren't any good: the first alternative\n> risks running out of file descriptors if you queue up lots of refs. And\n> the second alternative is going to be slow, especially so on Windows if\n> I'm not mistaken.\n\nI agree the first is a dead end. I had imagined something like the\nsecond, but I agree that we'd have to measure the cost of re-opening.\nIt's too bad there is not a syscall to sync a particular set of paths\n(or even better, a directory tree recursively).\n\nThere is another option, though: the batch-fsync code for objects does a\n\"cheap\" fsync while we have the descriptor open, and then later triggers\na to-disk sync on a separate file. My understanding is that this works\nbecause modern filesystems will make sure the data write is in the\njournal on the cheap sync, and then the separate-file sync makes sure\nthe journal goes to disk.\n\nWe could do something like that here. In fact, if you don't care about\ndurability and just filesystem corruption, then you only care about the\nfirst sync (because the bad case is when the rename gets journaled but\nthe data write doesn't).\n\nIn fact, even if you did choose to re-open and fsync each one, that's\nstill sequential. We'd need some way to tell the kernel to sync them all\nat once. The batch-fsync trickery above is one such way (I haven't\ntried, but I wonder if making a bunch of fsync calls in parallel would\nwork similarly).\n\n> So with both not being feasible, we'll likely have to come up with a\n> more complex scheme if we want to batch-sync files. One idea would be to\n> fsync(3P) all lockfiles every $n refs, but it adds complexity in a place\n> where I'd really like to have things as simple as possible. It also\n> raises the question what $n would have to be.\n\nI do think syncing every $n would not be too hard to implement. It could\nall be hidden behind a state machine API that collects lock files and\nflushes when it sees fit. You'd just call a magic \"batch_fsync_and_close\"\ninstead of \"fsync() and close()\", though you would have to remember to\ndo a final flush call to tell it there are no more coming.\n\n-Peff\n"},{"id":"440819","messageId":"YYuN971dStHrlJh3@ncase","threadId":"56840","inReplyTo":"YYuEq42toR6mycem@coredump.intra.peff.net","subject":"Re: [PATCH] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-10T09:16:39Z","receivedAt":"2021-11-10T09:17:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 10, 2021 at 03:36:59AM -0500, Jeff King wrote:\n> On Tue, Nov 09, 2021 at 12:25:46PM +0100, Patrick Steinhardt wrote:\n> \n> > So I've finally found the time to have another look at massaging this\n> > into the ref_transaction mechanism. If we do want to batch the fsync(3P)\n> > calls, then we basically have two different alternatives:\n> > \n> > 1. We first lock all loose refs by creating the respective lock\n> > files and writing the updated ref into that file. We keep the\n> > file descriptor open such that we can then flush them all in one\n> > go.\n> > \n> > 2. Same as before, we lock all refs and write the updated pointers\n> > into the lockfiles, but this time we close each lockfile after\n> > having written to it. Later, we reopen them all to fsync(3P) them\n> > to disk.\n> > \n> > I'm afraid both alternatives aren't any good: the first alternative\n> > risks running out of file descriptors if you queue up lots of refs. And\n> > the second alternative is going to be slow, especially so on Windows if\n> > I'm not mistaken.\n> \n> I agree the first is a dead end. I had imagined something like the\n> second, but I agree that we'd have to measure the cost of re-opening.\n> It's too bad there is not a syscall to sync a particular set of paths\n> (or even better, a directory tree recursively).\n> \n> There is another option, though: the batch-fsync code for objects does a\n> \"cheap\" fsync while we have the descriptor open, and then later triggers\n> a to-disk sync on a separate file. My understanding is that this works\n> because modern filesystems will make sure the data write is in the\n> journal on the cheap sync, and then the separate-file sync makes sure\n> the journal goes to disk.\n> \n> We could do something like that here. In fact, if you don't care about\n> durability and just filesystem corruption, then you only care about the\n> first sync (because the bad case is when the rename gets journaled but\n> the data write doesn't).\n\nAh, interesting. That does sound like a good way forward to me, thanks\nfor the pointers!\n\nPatrick\n\n> In fact, even if you did choose to re-open and fsync each one, that's\n> still sequential. We'd need some way to tell the kernel to sync them all\n> at once. The batch-fsync trickery above is one such way (I haven't\n> tried, but I wonder if making a bunch of fsync calls in parallel would\n> work similarly).\n> \n> > So with both not being feasible, we'll likely have to come up with a\n> > more complex scheme if we want to batch-sync files. One idea would be to\n> > fsync(3P) all lockfiles every $n refs, but it adds complexity in a place\n> > where I'd really like to have things as simple as possible. It also\n> > raises the question what $n would have to be.\n> \n> I do think syncing every $n would not be too hard to implement. It could\n> all be hidden behind a state machine API that collects lock files and\n> flushes when it sees fit. You'd just call a magic \"batch_fsync_and_close\"\n> instead of \"fsync() and close()\", though you would have to remember to\n> do a final flush call to tell it there are no more coming.\n> \n> -Peff\n"},{"id":"440826","messageId":"cover.1636544377.git.ps@pks.im","threadId":"56840","inReplyTo":"dd65718814011eb93ccc4428f9882e0f025224a6.1636029491.git.ps@pks.im","subject":"[PATCH v2 0/3] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-10T11:40:50Z","receivedAt":"2021-11-10T11:41:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\n[This is a resend, I initially used the wrong mailing list address.]\n\nthis is the second version of this patch series to implement syncing of\nloose refs to avoid a class of ref corruption.\n\nThis series is implements the same batching as Neeraj's series [1]. It\nintroduces a new config switch \"core.fsyncRefFiles\" with the same three\ntoggles \"on\", \"off\" and \"batch\". Given that it reuses funcitonality\nprovided by the batched object flushing this series is based on \"next\".\n\nPlease note that I didn't yet add any performance numbers or tests.\nPerformance tests didn't show any conclusive results on my machine given\nthat I couldn't observe any noticeable impact at all, and I didn't write\ntests yet given that I first wanted to get some feedback on this series.\nIf we agree that this is the correct way to go forward I'll of course\nput in some more time to add them.\n\nPatrick\n\n[1]: \n\nPatrick Steinhardt (3):\n wrapper: handle EINTR in `git_fsync()`\n wrapper: provide function to sync directories\n refs: add configuration to enable flushing of refs\n\n Documentation/config/core.txt | 7 ++++\n bulk-checkin.c | 13 ++------\n cache.h | 7 ++++\n config.c | 10 ++++++\n environment.c | 1 +\n git-compat-util.h | 7 ++++\n refs/files-backend.c | 62 ++++++++++++++++++++++++++++++++++-\n wrapper.c | 30 ++++++++++++++++-\n write-or-die.c | 6 ++--\n 9 files changed, 127 insertions(+), 16 deletions(-)\n\n-- \n2.33.1\n\n"},{"id":"440827","messageId":"0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im","threadId":"56840","inReplyTo":"cover.1636544377.git.ps@pks.im","subject":"[PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-10T11:40:54Z","receivedAt":"2021-11-10T11:41:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While we already have a wrapper around `git_fsync()`, this wrapper\ndoesn't handle looping around `EINTR`. Convert it to do so such that\ncallers don't have to remember doing this everywhere.\n\nSigned-off-by: Patrick Steinhardt \n---\n wrapper.c | 9 ++++++++-\n write-or-die.c | 6 ++----\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex ece3d2ca10..e20df4f3a6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)\n \treturn fd;\n }\n \n-int git_fsync(int fd, enum fsync_action action)\n+static int git_fsync_once(int fd, enum fsync_action action)\n {\n \tswitch (action) {\n \tcase FSYNC_WRITEOUT_ONLY:\n@@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)\n \tdefault:\n \t\tBUG(\"unexpected git_fsync(%d) call\", action);\n \t}\n+}\n \n+int git_fsync(int fd, enum fsync_action action)\n+{\n+\twhile (git_fsync_once(fd, action) < 0)\n+\t\tif (errno != EINTR)\n+\t\t\treturn -1;\n+\treturn 0;\n }\n \n static int warn_if_unremovable(const char *op, const char *file, int rc)\ndiff --git a/write-or-die.c b/write-or-die.c\nindex cc8291d979..e61220aa2d 100644\n--- a/write-or-die.c\n+++ b/write-or-die.c\n@@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n \n void fsync_or_die(int fd, const char *msg)\n {\n-\twhile (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) {\n-\t\tif (errno != EINTR)\n-\t\t\tdie_errno(\"fsync error on '%s'\", msg);\n-\t}\n+\tif (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)\n+\t\tdie_errno(\"fsync error on '%s'\", msg);\n }\n \n void write_or_die(int fd, const void *buf, size_t count)\n-- \n2.33.1\n\n"},{"id":"440828","messageId":"3ac9d4d7abd224a4c0991f1036f2d95eedb9ceac.1636544377.git.ps@pks.im","threadId":"56840","inReplyTo":"cover.1636544377.git.ps@pks.im","subject":"[PATCH v2 2/3] wrapper: provide function to sync directories","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-10T11:40:59Z","receivedAt":"2021-11-10T11:41:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04),\nwe have introduced batched syncing of object files. This mode works by\nonly requesting a writeback of the page cache backing the file on\nwritten files, followed by a single hardware-flush via a temporary file\ncreated in the directory we want to flush. Given modern journaling file\nsystems, this pattern is expected to be durable.\n\nWhile it's possible to reuse the `git_fsync()` helper to synchronize the\npage cache only, there is no helper which would allow for doing a\nhardware flush of a directory by creating a temporary file. Other\ncallers which want to follow the same pattern would thus have to repeat\nthis logic.\n\nExtract a new helper `git_fsync_dir()` from the object files code which\nneatly encapsulates this logic such that it can be reused.\n\nSigned-off-by: Patrick Steinhardt \n---\n bulk-checkin.c | 13 +++----------\n git-compat-util.h | 7 +++++++\n wrapper.c | 21 +++++++++++++++++++++\n 3 files changed, 31 insertions(+), 10 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 4deee1af46..e6ebdd1db5 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -98,16 +98,9 @@ static void do_batch_fsync(void)\n \t * hardware.\n \t */\n \n-\tif (needs_batch_fsync) {\n-\t\tstruct strbuf temp_path = STRBUF_INIT;\n-\t\tstruct tempfile *temp;\n-\n-\t\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", get_object_directory());\n-\t\ttemp = xmks_tempfile(temp_path.buf);\n-\t\tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n-\t\tdelete_tempfile(&temp);\n-\t\tstrbuf_release(&temp_path);\n-\t}\n+\tif (needs_batch_fsync &&\n+\t git_fsync_dir(get_object_directory()) < 0)\n+\t\tdie_errno(\"fsyncing object directory\");\n \n \tif (bulk_fsync_objdir)\n \t\ttmp_objdir_migrate(bulk_fsync_objdir);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 97f97178e7..f890bd07fd 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1221,6 +1221,13 @@ enum fsync_action {\n \n int git_fsync(int fd, enum fsync_action action);\n \n+/*\n+ * Issue a full hardware flush against a temporary file in the given directory\n+ * to ensure that all files inside that directory are durable before any renames\n+ * occur.\n+ */\n+int git_fsync_dir(const char *path);\n+\n /*\n * Preserves errno, prints a message, but gives no warning for ENOENT.\n * Returns 0 on success, which includes trying to unlink an object that does\ndiff --git a/wrapper.c b/wrapper.c\nindex e20df4f3a6..6c6cc8b74f 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -3,6 +3,7 @@\n */\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"tempfile.h\"\n \n static int memory_limit_check(size_t size, int gentle)\n {\n@@ -601,6 +602,26 @@ int git_fsync(int fd, enum fsync_action action)\n \treturn 0;\n }\n \n+int git_fsync_dir(const char *path)\n+{\n+\tstruct strbuf temp_path = STRBUF_INIT;\n+\tstruct tempfile *temp;\n+\n+\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", path);\n+\n+\ttemp = mks_tempfile(temp_path.buf);\n+\tif (!temp)\n+\t\treturn -1;\n+\n+\tif (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0)\n+\t\treturn -1;\n+\n+\tdelete_tempfile(&temp);\n+\tstrbuf_release(&temp_path);\n+\n+\treturn 0;\n+}\n+\n static int warn_if_unremovable(const char *op, const char *file, int rc)\n {\n \tint err;\n-- \n2.33.1\n\n"},{"id":"440829","messageId":"d9aa96913b1730f1d0c238d7d52e27c20bc55390.1636544377.git.ps@pks.im","threadId":"56840","inReplyTo":"cover.1636544377.git.ps@pks.im","subject":"[PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-10T11:41:03Z","receivedAt":"2021-11-10T11:41:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing loose refs, we first create a lockfile, write the new ref\ninto that lockfile, close it and then rename the lockfile into place\nsuch that the actual update is atomic for that single ref. While this\nworks as intended under normal circumstences, at GitLab we infrequently\nencounter corrupt loose refs in repositories after a machine encountered\na hard reset. The corruption is always of the same type: the ref has\nbeen committed into place, but it is completely empty.\n\nThe root cause of this is likely that we don't sync contents of the\nlockfile to disk before renaming it into place. As a result, it's not\nguaranteed that the contents are properly persisted and one may observe\nweird in-between states on hard resets. Quoting ext4 documentation [1]:\n\n Many broken applications don't use fsync() when replacing existing\n files via patterns such as fd =\n open(\"foo.new\")/write(fd,..)/close(fd)/ rename(\"foo.new\", \"foo\"), or\n worse yet, fd = open(\"foo\", O_TRUNC)/write(fd,..)/close(fd). If\n auto_da_alloc is enabled, ext4 will detect the replace-via-rename\n and replace-via-truncate patterns and force that any delayed\n allocation blocks are allocated such that at the next journal\n commit, in the default data=ordered mode, the data blocks of the new\n file are forced to disk before the rename() operation is committed.\n This provides roughly the same level of guarantees as ext3, and\n avoids the \"zero-length\" problem that can happen when a system\n crashes before the delayed allocation blocks are forced to disk.\n\nThis explicitly points out that one must call fsync(3P) before doing the\nrename(3P) call, or otherwise data may not be correctly persisted to\ndisk.\n\nFix this by introducing a new configuration \"core.fsyncRefFiles\". This\nconfig matches behaviour of \"core.fsyncObjectFiles\" in that it provides\nthree different modes:\n\n - \"off\" disables calling fsync on ref files. This is the default\n behaviour previous to this change and remains the default after\n this change.\n\n - \"on\" enables calling fsync on ref files, where each reference is\n flushed to disk before it is being committed. This is the safest\n setting, but may incur significant performance overhead.\n\n - \"batch\" will flush the page cache of each file as it is written to\n ensure its data is persisted. After all refs have been written,\n the directories which host refs are flushed.\n\nWith this change in place and when \"core.fsyncRefFiles\" is set to either\n\"on\" or \"batch\", this kind of corruption shouldn't happen anymore.\n\n[1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/config/core.txt | 7 ++++\n cache.h | 7 ++++\n config.c | 10 ++++++\n environment.c | 1 +\n refs/files-backend.c | 62 ++++++++++++++++++++++++++++++++++-\n 5 files changed, 86 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 200b4d9f06..e2fd0d8c90 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -572,6 +572,13 @@ core.fsyncObjectFiles::\n stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or\n ReFS.\n \n+core.fsyncRefFiles::\n+\tA value indicating the level of effort Git will expend in trying to make\n+\trefs added to the repo durable in the event of an unclean system\n+\tshutdown. This setting currently only controls loose refs in the object\n+\tstore, so updates to packed refs may not be equally durable. Takes the\n+\tsame parameters as `core.fsyncObjectFiles`.\n+\n core.preloadIndex::\n \tEnable parallel index preload for operations like 'git diff'\n +\ndiff --git a/cache.h b/cache.h\nindex 6d6e6770ec..14c8fab6b4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -991,7 +991,14 @@ enum fsync_object_files_mode {\n FSYNC_OBJECT_FILES_BATCH\n };\n \n+enum fsync_ref_files_mode {\n+ FSYNC_REF_FILES_OFF,\n+ FSYNC_REF_FILES_ON,\n+ FSYNC_REF_FILES_BATCH\n+};\n+\n extern enum fsync_object_files_mode fsync_object_files;\n+extern enum fsync_ref_files_mode fsync_ref_files;\n extern int core_preload_index;\n extern int precomposed_unicode;\n extern int protect_hfs;\ndiff --git a/config.c b/config.c\nindex 5eb36ecd77..4cbad5a29d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1500,6 +1500,16 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.fsyncreffiles\")) {\n+\t\tif (value && !strcmp(value, \"batch\"))\n+\t\t\tfsync_ref_files = FSYNC_REF_FILES_BATCH;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\tfsync_ref_files = FSYNC_REF_FILES_ON;\n+\t\telse\n+\t\t\tfsync_ref_files = FSYNC_REF_FILES_OFF;\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.preloadindex\")) {\n \t\tcore_preload_index = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex aeafe80235..1514ac9384 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -43,6 +43,7 @@ const char *git_hooks_path;\n int zlib_compression_level = Z_BEST_SPEED;\n int pack_compression_level = Z_DEFAULT_COMPRESSION;\n enum fsync_object_files_mode fsync_object_files;\n+enum fsync_ref_files_mode fsync_ref_files;\n size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;\n size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;\n size_t delta_base_cache_limit = 96 * 1024 * 1024;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4b14f30d48..31d7456266 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1360,6 +1360,57 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t const struct object_id *oid, const char *logmsg,\n \t\t\t struct strbuf *err);\n \n+static int sync_loose_ref(int fd)\n+{\n+\tswitch (fsync_ref_files) {\n+\tcase FSYNC_REF_FILES_OFF:\n+\t\treturn 0;\n+\tcase FSYNC_REF_FILES_ON:\n+\t\treturn git_fsync(fd, FSYNC_HARDWARE_FLUSH);\n+\tcase FSYNC_REF_FILES_BATCH:\n+\t\treturn git_fsync(fd, FSYNC_WRITEOUT_ONLY);\n+\tdefault:\n+\t\tBUG(\"invalid fsync mode %d\", fsync_ref_files);\n+\t}\n+}\n+\n+#define SYNC_LOOSE_REF_GITDIR (1 << 0)\n+#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)\n+\n+static int sync_loose_refs_flags(const char *refname)\n+{\n+\tswitch (ref_type(refname)) {\n+\tcase REF_TYPE_PER_WORKTREE:\n+\tcase REF_TYPE_PSEUDOREF:\n+\t\treturn SYNC_LOOSE_REF_GITDIR;\n+\tcase REF_TYPE_MAIN_PSEUDOREF:\n+\tcase REF_TYPE_OTHER_PSEUDOREF:\n+\tcase REF_TYPE_NORMAL:\n+\t\treturn SYNC_LOOSE_REF_COMMONDIR;\n+\tdefault:\n+\t\tBUG(\"unknown ref type %d of ref %s\",\n+\t\t ref_type(refname), refname);\n+\t}\n+}\n+\n+static int sync_loose_refs(struct files_ref_store *refs,\n+\t\t\t int flags,\n+\t\t\t struct strbuf *err)\n+{\n+\tif (fsync_ref_files != FSYNC_REF_FILES_BATCH)\n+\t\treturn 0;\n+\n+\tif ((flags & SYNC_LOOSE_REF_GITDIR) &&\n+\t git_fsync_dir(refs->base.gitdir) < 0)\n+\t\treturn -1;\n+\n+\tif ((flags & SYNC_LOOSE_REF_COMMONDIR) &&\n+\t git_fsync_dir(refs->gitcommondir) < 0)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n /*\n * Emit a better error message than lockfile.c's\n * unable_to_lock_message() would in case there is a D/F conflict with\n@@ -1502,6 +1553,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \toidcpy(&lock->old_oid, &orig_oid);\n \n \tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n+\t sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||\n \t commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", newrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -1522,6 +1574,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = LOG_REFS_NONE;\n \tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n+\t sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||\n \t commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -1781,6 +1834,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \tfd = get_lock_file_fd(&lock->lk);\n \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n \t write_in_full(fd, &term, 1) < 0 ||\n+\t sync_loose_ref(fd) < 0 ||\n \t close_ref_gently(lock) < 0) {\n \t\tstrbuf_addf(err,\n \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n@@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n \t\t\t \"ref_transaction_prepare\");\n \tsize_t i;\n-\tint ret = 0;\n+\tint ret = 0, sync_flags = 0;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \tchar *head_ref = NULL;\n \tint head_type;\n@@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t&update->new_oid, NULL,\n \t\t\t\t\tNULL);\n \t\t}\n+\n+\t\tsync_flags |= sync_loose_refs_flags(update->refname);\n \t}\n \n+\tret = sync_loose_refs(refs, sync_flags, err);\n+\tif (ret)\n+\t\tgoto cleanup;\n+\n \tif (packed_transaction) {\n \t\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n \t\t\tret = TRANSACTION_GENERIC_ERROR;\n-- \n2.33.1\n\n"},{"id":"440842","messageId":"nycvar.QRO.7.76.6.2111101530490.21127@tvgsbejvaqbjf.bet","threadId":"56840","inReplyTo":"0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T14:33:32Z","receivedAt":"2021-11-10T14:33:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\njust one observation, below.\n\nOn Wed, 10 Nov 2021, Patrick Steinhardt wrote:\n\n> diff --git a/wrapper.c b/wrapper.c\n> index ece3d2ca10..e20df4f3a6 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)\n> \treturn fd;\n> }\n>\n> -int git_fsync(int fd, enum fsync_action action)\n> +static int git_fsync_once(int fd, enum fsync_action action)\n> {\n> \tswitch (action) {\n> \tcase FSYNC_WRITEOUT_ONLY:\n> @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)\n> \tdefault:\n> \t\tBUG(\"unexpected git_fsync(%d) call\", action);\n> \t}\n> +}\n>\n> +int git_fsync(int fd, enum fsync_action action)\n> +{\n> +\twhile (git_fsync_once(fd, action) < 0)\n> +\t\tif (errno != EINTR)\n> +\t\t\treturn -1;\n> +\treturn 0;\n> }\n\nMy immediate reaction was: why not fold `git_fsync_once()` into\n`git_fsync()`?\n\nAnd then I had a look at the function (which is sadly not in the diff\ncontext of this email, one of the occasions when I would prefer a proper\nUI for reviewing), and I agree that indenting the code even one level\nwould make it harder to read.\n\nAll this is to say: I agree with the approach you took here.\n\nCiao,\nDscho\n"},{"id":"440844","messageId":"211110.864k8khy5m.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"0c8e98295e91b656a89e1db53bfe02e7c7fc8432.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T14:39:31Z","receivedAt":"2021-11-10T14:40:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> While we already have a wrapper around `git_fsync()`, this wrapper\n> doesn't handle looping around `EINTR`. Convert it to do so such that\n> callers don't have to remember doing this everywhere.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> wrapper.c | 9 ++++++++-\n> write-or-die.c | 6 ++----\n> 2 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index ece3d2ca10..e20df4f3a6 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)\n> \treturn fd;\n> }\n> \n> -int git_fsync(int fd, enum fsync_action action)\n> +static int git_fsync_once(int fd, enum fsync_action action)\n> {\n> \tswitch (action) {\n> \tcase FSYNC_WRITEOUT_ONLY:\n> @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)\n> \tdefault:\n> \t\tBUG(\"unexpected git_fsync(%d) call\", action);\n> \t}\n> +}\n> \n> +int git_fsync(int fd, enum fsync_action action)\n> +{\n> +\twhile (git_fsync_once(fd, action) < 0)\n> +\t\tif (errno != EINTR)\n> +\t\t\treturn -1;\n> +\treturn 0;\n> }\n> \n> static int warn_if_unremovable(const char *op, const char *file, int rc)\n> diff --git a/write-or-die.c b/write-or-die.c\n> index cc8291d979..e61220aa2d 100644\n> --- a/write-or-die.c\n> +++ b/write-or-die.c\n> @@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)\n> \n> void fsync_or_die(int fd, const char *msg)\n> {\n> -\twhile (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) {\n> -\t\tif (errno != EINTR)\n> -\t\t\tdie_errno(\"fsync error on '%s'\", msg);\n> -\t}\n> +\tif (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)\n> +\t\tdie_errno(\"fsync error on '%s'\", msg);\n\nNit: While at it maybe change it to: \"fsync() error syncing file '%s'\"\n\nMaybe having a xgit_fsync() convenience wrapper would make subsequent\nsteps easier?\n"},{"id":"440845","messageId":"nycvar.QRO.7.76.6.2111101534370.21127@tvgsbejvaqbjf.bet","threadId":"56840","inReplyTo":"cover.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T14:44:36Z","receivedAt":"2021-11-10T14:44:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 10 Nov 2021, Patrick Steinhardt wrote:\n\n> This series is implements the same batching as Neeraj's series [1]. It\n> introduces a new config switch \"core.fsyncRefFiles\" with the same three\n> toggles \"on\", \"off\" and \"batch\". Given that it reuses funcitonality\n> provided by the batched object flushing this series is based on \"next\".\n\nExcellent, I am glad that my idea was helpful.\n\nI read through the patches and found them easy to follow. Even 3/3, which\nwas a bit complex because of the need to support both transacted ref\nupdates as well as atomic renames/copies, is clear and well-written.\n\nFWIW you could base your patches directly on `ns/batched-fsync`, I would\nthink.\n\nThank you,\nDscho\n"},{"id":"440847","messageId":"211110.86zgqcgj98.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"3ac9d4d7abd224a4c0991f1036f2d95eedb9ceac.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 2/3] wrapper: provide function to sync directories","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T14:40:24Z","receivedAt":"2021-11-10T14:47:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04),\n> we have introduced batched syncing of object files. This mode works by\n> only requesting a writeback of the page cache backing the file on\n> written files, followed by a single hardware-flush via a temporary file\n> created in the directory we want to flush. Given modern journaling file\n> systems, this pattern is expected to be durable.\n>\n> While it's possible to reuse the `git_fsync()` helper to synchronize the\n> page cache only, there is no helper which would allow for doing a\n> hardware flush of a directory by creating a temporary file. Other\n> callers which want to follow the same pattern would thus have to repeat\n> this logic.\n>\n> Extract a new helper `git_fsync_dir()` from the object files code which\n> neatly encapsulates this logic such that it can be reused.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> bulk-checkin.c | 13 +++----------\n> git-compat-util.h | 7 +++++++\n> wrapper.c | 21 +++++++++++++++++++++\n> 3 files changed, 31 insertions(+), 10 deletions(-)\n>\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 4deee1af46..e6ebdd1db5 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -98,16 +98,9 @@ static void do_batch_fsync(void)\n> \t * hardware.\n> \t */\n> \n> -\tif (needs_batch_fsync) {\n> -\t\tstruct strbuf temp_path = STRBUF_INIT;\n> -\t\tstruct tempfile *temp;\n> -\n> -\t\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", get_object_directory());\n> -\t\ttemp = xmks_tempfile(temp_path.buf);\n> -\t\tfsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));\n> -\t\tdelete_tempfile(&temp);\n> -\t\tstrbuf_release(&temp_path);\n> -\t}\n> +\tif (needs_batch_fsync &&\n> +\t git_fsync_dir(get_object_directory()) < 0)\n> +\t\tdie_errno(\"fsyncing object directory\");\n\nNit: Similar to 1/3, but this message is new: We say \"fsyncing object\ndirectory\", but it would be better to pass in some \"verbose\" flag to\ngit_fsync_dir() so we can say e.g.:\n\n error_errno(_(\"couldn't create core.fsyncRefFiles=batch tempfile '%s' in '%s'\"), ...)\n error_errno(_(\"couldn't fsync() core.fsyncRefFiles=batch tempfile '%s' in '%s'\"), ...)\n\nI.e. being able to say specifically why we failed, permission error or\nthe tempfile? fsync() didn't work etc?\n\nLooking at the underlying APIs maybe they already have a mode to \"die\"\nor \"warn\" appropriately? Or...\n\n> +int git_fsync_dir(const char *path)\n> +{\n> +\tstruct strbuf temp_path = STRBUF_INIT;\n> +\tstruct tempfile *temp;\n> +\n> +\tstrbuf_addf(&temp_path, \"%s/bulk_fsync_XXXXXX\", path);\n> +\n> +\ttemp = mks_tempfile(temp_path.buf);\n> +\tif (!temp)\n> +\t\treturn -1;\n> +\n> +\tif (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0)\n> +\t\treturn -1;\n\n...if they do maybe we should use their non-fatal mode, because\nwith/without that these \"return -1\" need to be \"goto cleanup\" so we can\nattempt to clean up after ourselves here.\n\nI think this whole thing would be better if we generalized tmp-objdir.h\na bit, so it could create and manage an arbitrary file in an arbitrary\ndirectory, and that API should really be generalized to a user of\ntempfile.c.\n\nI.e. we'd then create this file, sync it optionally, whine if it does't\nwork, and be guaranteed to try to clean anything that goes wrong up\natexit().\n"},{"id":"440849","messageId":"211110.86v910gi9a.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"d9aa96913b1730f1d0c238d7d52e27c20bc55390.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T14:49:02Z","receivedAt":"2021-11-10T15:09:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Patrick Steinhardt wrote:\n\n> [...]\n> Fix this by introducing a new configuration \"core.fsyncRefFiles\". This\n> config matches behaviour of \"core.fsyncObjectFiles\" in that it provides\n> three different modes:\n>\n> - \"off\" disables calling fsync on ref files. This is the default\n> behaviour previous to this change and remains the default after\n> this change.\n>\n> - \"on\" enables calling fsync on ref files, where each reference is\n> flushed to disk before it is being committed. This is the safest\n> setting, but may incur significant performance overhead.\n>\n> - \"batch\" will flush the page cache of each file as it is written to\n> ensure its data is persisted. After all refs have been written,\n> the directories which host refs are flushed.\n>\n> With this change in place and when \"core.fsyncRefFiles\" is set to either\n> \"on\" or \"batch\", this kind of corruption shouldn't happen anymore.\n>\n> [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n\nWith the understanding that my grokking of this approach is still\nsomewhere between \"uh, that works?\" and \"wow, voodoo FS magic!\". ....\n\nI haven't looked at these changes in much daiter, or Neeraj's recent\nrelated changes but...\n\n> +core.fsyncRefFiles::\n> +\tA value indicating the level of effort Git will expend in trying to make\n> +\trefs added to the repo durable in the event of an unclean system\n> +\tshutdown. This setting currently only controls loose refs in the object\n> +\tstore, so updates to packed refs may not be equally durable. Takes the\n> +\tsame parameters as `core.fsyncObjectFiles`.\n> +\n\n...my understanding of it is basically a way of going back to what Linus\npointed out way back in aafe9fbaf4f (Add config option to enable\n'fsync()' of object files, 2008-06-18).\n\nI.e. we've got files x and y. POSIX sayeth we'd need to fsync them all\nand the directory entry, but on some FS's it would be sufficient to\nfsync() just y if they're created in that order. It'll imply an fsync of\nboth x and y, is that accurate?\n\nIf not you can probably discard the rest, but forging on:\n\nWhy do we then need to fsync() a \"z\" file in get_object_directory()\n(i.e. .git/objects) then? That's a new \"z\", wasn't flushing \"y\" enough?\n\nOr if you've written .git/objects/x and .git/refs/y I can imagine\nwanting to create and sync a .git/z if the FS's semantics are to then\nflush all remaining updates from that tree up, but here it's\n.git/objects, not .git. That also seems to contract this above:\n\n> ensure its data is persisted. After all refs have been written,\n> the directories which host refs are flushed.\n\nI.e. that would be .git/refs (let's ignore .git/HEAD and the like for\nnow), not .git/objects or .git?\n\nAnd again, forging on but more generally [continued below]...\n\n> +\tif (!strcmp(var, \"core.fsyncreffiles\")) {\n\nUX side: now we've got a core.fsyncRefFiles and\ncore.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those\nwork together like say \"fsck.*\" and \"fetch.fsck.*\" do, i.e. you'd be\nable to configure this once for objects and refs, or in two variables,\none for objects, one for refs...\n\n\n> +static int sync_loose_ref(int fd)\n> +{\n> +\tswitch (fsync_ref_files) {\n> +\tcase FSYNC_REF_FILES_OFF:\n> +\t\treturn 0;\n> +\tcase FSYNC_REF_FILES_ON:\n> +\t\treturn git_fsync(fd, FSYNC_HARDWARE_FLUSH);\n> +\tcase FSYNC_REF_FILES_BATCH:\n> +\t\treturn git_fsync(fd, FSYNC_WRITEOUT_ONLY);\n> +\tdefault:\n> +\t\tBUG(\"invalid fsync mode %d\", fsync_ref_files);\n> +\t}\n> +}\n> +\n> +#define SYNC_LOOSE_REF_GITDIR (1 << 0)\n> +#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)\n\nnit: make this an enum and ...\n\n> +static int sync_loose_refs_flags(const char *refname)\n> +{\n> +\tswitch (ref_type(refname)) {\n> +\tcase REF_TYPE_PER_WORKTREE:\n> +\tcase REF_TYPE_PSEUDOREF:\n> +\t\treturn SYNC_LOOSE_REF_GITDIR;\n> +\tcase REF_TYPE_MAIN_PSEUDOREF:\n> +\tcase REF_TYPE_OTHER_PSEUDOREF:\n> +\tcase REF_TYPE_NORMAL:\n> +\t\treturn SYNC_LOOSE_REF_COMMONDIR;\n> +\tdefault:\n> +\t\tBUG(\"unknown ref type %d of ref %s\",\n> +\t\t ref_type(refname), refname);\n\n... you won't need this default case...\n\n> [...]\n> /*\n> * Emit a better error message than lockfile.c's\n> * unable_to_lock_message() would in case there is a D/F conflict with\n> @@ -1502,6 +1553,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n> \toidcpy(&lock->old_oid, &orig_oid);\n> \n> \tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n> +\t sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||\n> \t commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {\n> \t\terror(\"unable to write current sha1 into %s: %s\", newrefname, err.buf);\n> \t\tstrbuf_release(&err);\n> @@ -1522,6 +1574,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n> \tflag = log_all_ref_updates;\n> \tlog_all_ref_updates = LOG_REFS_NONE;\n> \tif (write_ref_to_lockfile(lock, &orig_oid, &err) ||\n> +\t sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||\n> \t commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {\n> \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n> \t\tstrbuf_release(&err);\n> @@ -1781,6 +1834,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n> \tfd = get_lock_file_fd(&lock->lk);\n> \tif (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> \t write_in_full(fd, &term, 1) < 0 ||\n> +\t sync_loose_ref(fd) < 0 ||\n> \t close_ref_gently(lock) < 0) {\n> \t\tstrbuf_addf(err,\n> \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n> @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n> \t\t\t \"ref_transaction_prepare\");\n> \tsize_t i;\n> -\tint ret = 0;\n> +\tint ret = 0, sync_flags = 0;\n> \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n> \tchar *head_ref = NULL;\n> \tint head_type;\n> @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t\t\t&update->new_oid, NULL,\n> \t\t\t\t\tNULL);\n> \t\t}\n> +\n> +\t\tsync_flags |= sync_loose_refs_flags(update->refname);\n> \t}\n> \n> +\tret = sync_loose_refs(refs, sync_flags, err);\n> +\tif (ret)\n> +\t\tgoto cleanup;\n> +\n> \tif (packed_transaction) {\n> \t\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n> \t\t\tret = TRANSACTION_GENERIC_ERROR;\n\n...[continued from above]: Again, per my potentially wrong understanding\nof syncing a \"x\" and \"y\" via an fsync of a subsequent \"z\" that's\nadjacent on the FS to those two.\n\nIsn't this setting us up for a really bad interaction between this\nseries and Neeraj's work? Well \"bad\" as in \"bad for performance\".\n\nI.e. you'll turn on \"use the batch thing for objects and refs\" and we'll\ndo two fsyncs, one for the object update, and one for refs. The common\ncase is that we'll have both in play.\n\nSo shouldn't this go to a higher level for both so we only create a \"z\"\n.git/sync-it-now-please.txt or whatever once we do all pending updates\non the .git/ directory?\n\nI can also imagine that we'd want that at an even higher level, e.g. for\n\"git pull\" surely we'd want it not for refs or objects, but in\nbuiltin/pull.c somewhere because we'll be updating the .git/index after\nwe do both refs and objects, and you'd want to fsync at the very end,\nno?\n\nNone of the above should mean we can't pursue a more narrow approach for\nnow. I'm just:\n\n 1) Seeing if I understand what we're trying to do here, maybe not.\n\n 2) Encouraging you two to think about a holistic way to configure some\n logical conclusion to this topic at large, so we won't end up with\n core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,\n core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)\n\nI'll send another more generic follow-up E-Mail for #2.\n"},{"id":"440863","messageId":"20211110191533.GA484@neerajsi-x1.localdomain","threadId":"56840","inReplyTo":"211110.86v910gi9a.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-11-10T19:15:33Z","receivedAt":"2021-11-10T19:15:42Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 10 2021, Patrick Steinhardt wrote:\n> \n> > +\tshutdown. This setting currently only controls loose refs in the object\n> > +\tstore, so updates to packed refs may not be equally durable. Takes the\n> > +\tsame parameters as `core.fsyncObjectFiles`.\n> > +\n> \n> ...my understanding of it is basically a way of going back to what Linus\n> pointed out way back in aafe9fbaf4f (Add config option to enable\n> 'fsync()' of object files, 2008-06-18).\n> \n> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all\n> and the directory entry, but on some FS's it would be sufficient to\n> fsync() just y if they're created in that order. It'll imply an fsync of\n> both x and y, is that accurate?\n> \n> If not you can probably discard the rest, but forging on:\n> \n> Why do we then need to fsync() a \"z\" file in get_object_directory()\n> (i.e. .git/objects) then? That's a new \"z\", wasn't flushing \"y\" enough?\n> \n> Or if you've written .git/objects/x and .git/refs/y I can imagine\n> wanting to create and sync a .git/z if the FS's semantics are to then\n> flush all remaining updates from that tree up, but here it's\n> .git/objects, not .git. That also seems to contract this above:\n\nWe're breaking through the abstraction provided by POSIX in 'batch' mode,\nsince the POSIX abstraction does not give us any option to be both safe\nand reasonably fast on hardware that does something expensive upon fsync().\n\nWe need to ensure that 'x' and 'y' are handed off to the storage device via\na non-flushing pagecache cleaning call (e.g. sync_file_ranges or macOS fsync).\nThen creating and flushing 'z' ensures that 'x' and 'y' will be visible after\nsuccessful completion. On FSes with a single journal that is flushed on fsync,\nthis should also ensure that any other files that have been cleaned out of the\npagecache and any other metadata updates are also persisted. The fsync provides\na barrier in addition to the durability.\n\n\n> > ensure its data is persisted. After all refs have been written,\n> > the directories which host refs are flushed.\n> \n> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for\n> now), not .git/objects or .git?\n> \n> And again, forging on but more generally [continued below]...\n> \n> > +\tif (!strcmp(var, \"core.fsyncreffiles\")) {\n> \n> UX side: now we've got a core.fsyncRefFiles and\n> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those\n> work together like say \"fsck.*\" and \"fetch.fsck.*\" do, i.e. you'd be\n> able to configure this once for objects and refs, or in two variables,\n> one for objects, one for refs...\n> \n\nI agree with this feedback. We should have a core.fsync flag to control everything\nthat might be synced in the repo. However, we'd have to decide what we want\nto do with packfiles and the index which are currently always fsynced.\n\n> ...[continued from above]: Again, per my potentially wrong understanding\n> of syncing a \"x\" and \"y\" via an fsync of a subsequent \"z\" that's\n> adjacent on the FS to those two.\n\nI suspect Patrick is concerned about the case where the worktree is on\na separate filesystem from the main repo, so there might be a motivation\nto sync the worktree refs separately. Is that right, Patrick?\n\n> \n> Isn't this setting us up for a really bad interaction between this\n> series and Neeraj's work? Well \"bad\" as in \"bad for performance\".\n> \n> I.e. you'll turn on \"use the batch thing for objects and refs\" and we'll\n> do two fsyncs, one for the object update, and one for refs. The common\n> case is that we'll have both in play.\n> \n> So shouldn't this go to a higher level for both so we only create a \"z\"\n> .git/sync-it-now-please.txt or whatever once we do all pending updates\n> on the .git/ directory?\n> \n> I can also imagine that we'd want that at an even higher level, e.g. for\n> \"git pull\" surely we'd want it not for refs or objects, but in\n> builtin/pull.c somewhere because we'll be updating the .git/index after\n> we do both refs and objects, and you'd want to fsync at the very end,\n> no?\n> \n> None of the above should mean we can't pursue a more narrow approach for\n> now. I'm just:\n> \n> 1) Seeing if I understand what we're trying to do here, maybe not.\n> \n> 2) Encouraging you two to think about a holistic way to configure some\n> logical conclusion to this topic at large, so we won't end up with\n> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,\n> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)\n> \n> I'll send another more generic follow-up E-Mail for #2.\n\nIn my view there are two separable concerns:\n\n 1) What durability do we want for the user's data when an entire 'git'\n command completes? I.e. if I run 'git add <1000 files>; git commit' and the\n system loses power after both commands return, should I see all of those files\n in the committed state of the repo?\n\n 2) What internal consistency do we want in the git databases (ODB and REFS) if\n we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes\n before returning, can there be an invalid object or a torn ref state?\n\nIf were only concerned with (1), then doing a single fsync at the end of the high-level\ngit command would be sufficient. However, (2) requires more fsyncs to provide barriers\nbetween different phases internal to the git commands. For instance, we need a barrier\nbetween creating the ODB state for a commit (blobs, trees, commit obj) and the refs\npointing to it.\n\nI am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds\neach fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not\nbe user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)\nwould become apparent to the user.\n\nThe more optimal way to handle consistency and durability is Write-ahead-logging with log\nreplay on crash recovery. That approach can reduce the number of fsyncs in the active workload\nto at most two (one to sync the log with a commit record and then one before truncating the\nlog after updating the main database). At that point, however, I think it would be best to\nuse an existing database engine with some modifications to create a good data layout for git.\n\nThanks,\nNeeraj\n"},{"id":"440870","messageId":"YYwvVy6AWDjkWazn@coredump.intra.peff.net","threadId":"56840","inReplyTo":"cover.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T20:45:11Z","receivedAt":"2021-11-10T20:45:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 12:40:50PM +0100, Patrick Steinhardt wrote:\n\n> Please note that I didn't yet add any performance numbers or tests.\n> Performance tests didn't show any conclusive results on my machine given\n> that I couldn't observe any noticeable impact at all, and I didn't write\n> tests yet given that I first wanted to get some feedback on this series.\n> If we agree that this is the correct way to go forward I'll of course\n> put in some more time to add them.\n\nHere's a mild adjustment of the quick test I showed earlier in [1]. It\nuses your version for all cases, but swaps between the three config\noptions, and it uses --atomic to put everything into a single\ntransaction:\n\n $ hyperfine -L v false,true,batch -p ./setup 'git -C dst.git config core.fsyncreffiles {v}; git.compile push --atomic dst.git refs/heads/*'\n Benchmark 1: git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*\n Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 7.8 ms, System: 3.9 ms]\n Range (min … max): 10.1 ms … 11.0 ms 108 runs\n \n Benchmark 2: git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*\n Time (mean ± σ): 402.5 ms ± 6.2 ms [User: 13.9 ms, System: 10.7 ms]\n Range (min … max): 387.3 ms … 408.9 ms 10 runs\n \n Benchmark 3: git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*\n Time (mean ± σ): 21.4 ms ± 0.6 ms [User: 8.0 ms, System: 5.2 ms]\n Range (min … max): 20.7 ms … 24.9 ms 99 runs\n \n Summary\n 'git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*' ran\n 2.05 ± 0.07 times faster than 'git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*'\n 38.51 ± 1.06 times faster than 'git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*'\n\nSo yes, the \"batch\" case is much improved. It's still more expensive\nthan skipping the fsync entirely, but it's within reason. The trick,\nthough is the --atomic. If I drop that flag, then \"batch\" takes as long\nas \"true\". And of course that's the default.\n\nI wonder if that means we'd want an extra mode: do the WRITEOUT_ONLY\nflush, but _don't_ do a HARDWARE_FLUSH for each transaction. That\ndoesn't give you durability, but it (in theory) solves the out-of-order\njournal problem without paying any performance cost.\n\nI say \"in theory\" because I'm just assuming that the WRITEOUT thing\nworks as advertised. I don't have an easy way to test the outcome on a\npower failure at the right moment here.\n\n-Peff\n\n[1] https://lore.kernel.org/git/YYTaiIlEKxHRVdCy@coredump.intra.peff.net/\n"},{"id":"440871","messageId":"211110.8635o3rbdc.gmgdl@evledraar.gmail.com","threadId":"56840","inReplyTo":"20211110191533.GA484@neerajsi-x1.localdomain","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-10T20:23:04Z","receivedAt":"2021-11-10T20:45:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 10 2021, Neeraj Singh wrote:\n\n> On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Nov 10 2021, Patrick Steinhardt wrote:\n>> \n>> > +\tshutdown. This setting currently only controls loose refs in the object\n>> > +\tstore, so updates to packed refs may not be equally durable. Takes the\n>> > +\tsame parameters as `core.fsyncObjectFiles`.\n>> > +\n>> \n>> ...my understanding of it is basically a way of going back to what Linus\n>> pointed out way back in aafe9fbaf4f (Add config option to enable\n>> 'fsync()' of object files, 2008-06-18).\n>> \n>> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all\n>> and the directory entry, but on some FS's it would be sufficient to\n>> fsync() just y if they're created in that order. It'll imply an fsync of\n>> both x and y, is that accurate?\n>> \n>> If not you can probably discard the rest, but forging on:\n>> \n>> Why do we then need to fsync() a \"z\" file in get_object_directory()\n>> (i.e. .git/objects) then? That's a new \"z\", wasn't flushing \"y\" enough?\n>> \n>> Or if you've written .git/objects/x and .git/refs/y I can imagine\n>> wanting to create and sync a .git/z if the FS's semantics are to then\n>> flush all remaining updates from that tree up, but here it's\n>> .git/objects, not .git. That also seems to contract this above:\n>\n> We're breaking through the abstraction provided by POSIX in 'batch' mode,\n> since the POSIX abstraction does not give us any option to be both safe\n> and reasonably fast on hardware that does something expensive upon fsync().\n>\n> We need to ensure that 'x' and 'y' are handed off to the storage device via\n> a non-flushing pagecache cleaning call (e.g. sync_file_ranges or macOS fsync).\n> Then creating and flushing 'z' ensures that 'x' and 'y' will be visible after\n> successful completion. On FSes with a single journal that is flushed on fsync,\n> this should also ensure that any other files that have been cleaned out of the\n> pagecache and any other metadata updates are also persisted. The fsync provides\n> a barrier in addition to the durability.\n\nYes. I understand that we're not doing POSIX fsyncing().\n\nI'm asking about something else, i.e. with this not-like-POSIX-sync why\nit is that when you have a directory:\n\n A\n\nand files:\n\n A/{X,Y}\n\nThat you'd write those two, and then proceed to do the \"batch flush\" by\ncreating and fsync()-ing a:\n\n B/Z\n\nAs opposed to either of:\n\n A/Z\n\nOr:\n\n Z\n\nI.e. why update .git/refs/* and create a flush file in .git/object/* and\nnot .git/refs/* or .git/*?\n\nMaybe the answer is just that this is WIP code copied from your\n.git/objects/ fsync() implementation, or maybe it's more subtle and I'm\nmissing something.\n\n>> > ensure its data is persisted. After all refs have been written,\n>> > the directories which host refs are flushed.\n>> \n>> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for\n>> now), not .git/objects or .git?\n>> \n>> And again, forging on but more generally [continued below]...\n>> \n>> > +\tif (!strcmp(var, \"core.fsyncreffiles\")) {\n>> \n>> UX side: now we've got a core.fsyncRefFiles and\n>> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those\n>> work together like say \"fsck.*\" and \"fetch.fsck.*\" do, i.e. you'd be\n>> able to configure this once for objects and refs, or in two variables,\n>> one for objects, one for refs...\n>> \n>\n> I agree with this feedback. We should have a core.fsync flag to control everything\n> that might be synced in the repo. However, we'd have to decide what we want\n> to do with packfiles and the index which are currently always fsynced.\n\n*nod*. See this if you haven't yet:\nhttps://lore.kernel.org/git/211110.86r1bogg27.gmgdl@evledraar.gmail.com/T/#u\n\n>> ...[continued from above]: Again, per my potentially wrong understanding\n>> of syncing a \"x\" and \"y\" via an fsync of a subsequent \"z\" that's\n>> adjacent on the FS to those two.\n>\n> I suspect Patrick is concerned about the case where the worktree is on\n> a separate filesystem from the main repo, so there might be a motivation\n> to sync the worktree refs separately. Is that right, Patrick?\n\nBut he's syncing .git/objects, not .git/worktrees//refs/, no?\n\n>> \n>> Isn't this setting us up for a really bad interaction between this\n>> series and Neeraj's work? Well \"bad\" as in \"bad for performance\".\n>> \n>> I.e. you'll turn on \"use the batch thing for objects and refs\" and we'll\n>> do two fsyncs, one for the object update, and one for refs. The common\n>> case is that we'll have both in play.\n>> \n>> So shouldn't this go to a higher level for both so we only create a \"z\"\n>> .git/sync-it-now-please.txt or whatever once we do all pending updates\n>> on the .git/ directory?\n>> \n>> I can also imagine that we'd want that at an even higher level, e.g. for\n>> \"git pull\" surely we'd want it not for refs or objects, but in\n>> builtin/pull.c somewhere because we'll be updating the .git/index after\n>> we do both refs and objects, and you'd want to fsync at the very end,\n>> no?\n>> \n>> None of the above should mean we can't pursue a more narrow approach for\n>> now. I'm just:\n>> \n>> 1) Seeing if I understand what we're trying to do here, maybe not.\n>> \n>> 2) Encouraging you two to think about a holistic way to configure some\n>> logical conclusion to this topic at large, so we won't end up with\n>> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,\n>> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)\n>> \n>> I'll send another more generic follow-up E-Mail for #2.\n>\n> In my view there are two separable concerns:\n>\n> 1) What durability do we want for the user's data when an entire 'git'\n> command completes? I.e. if I run 'git add <1000 files>; git commit' and the\n> system loses power after both commands return, should I see all of those files\n> in the committed state of the repo?\n>\n> 2) What internal consistency do we want in the git databases (ODB and REFS) if\n> we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes\n> before returning, can there be an invalid object or a torn ref state?\n>\n> If were only concerned with (1), then doing a single fsync at the end of the high-level\n> git command would be sufficient. However, (2) requires more fsyncs to provide barriers\n> between different phases internal to the git commands. For instance, we need a barrier\n> between creating the ODB state for a commit (blobs, trees, commit obj) and the refs\n> pointing to it.\n>\n> I am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds\n> each fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not\n> be user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)\n> would become apparent to the user.\n>\n> The more optimal way to handle consistency and durability is Write-ahead-logging with log\n> replay on crash recovery. That approach can reduce the number of fsyncs in the active workload\n> to at most two (one to sync the log with a commit record and then one before truncating the\n> log after updating the main database). At that point, however, I think it would be best to\n> use an existing database engine with some modifications to create a good data layout for git.\n\nI think that git should safely store your data by default, we clearly\ndon't do that well enough in some cases, and should fix it.\n\nBut there's also cases where people would much rather have performance,\nand I think we should support that. E.g. running git in CI, doing a\none-off import/fetch that you'll invoke \"sync(1)\" yourself after\netc. This is the direction Eric Wong's patches are going into.\n\nI understand from his patches/comments that you're not correct that just\n1-3 fsyncs are OK, i.e. for some use-cases 0 is OK, and any >0 is too\nmany, since it'll force a flush of the whole disk or something.\n\nEven when git is is operating in a completely safe mode I think we'd\nstill have license to play it fast and loose with /some/ user data,\nbecause users don't really care about all of their data in the .git/\ndirectory.\n\nI.e. you do care about the *.pack file, but an associated *.bitmap is a\nderived file, so we probably don't need to fsync that too. Is it worth\nworrying about? Probably not, but that's what I had in mind with the\nabove-linked proposed config schema.\n\nSimilarly for say writing 1k loose objects I think it would be\ncompletely fine to end up with a corrupt object during a \"git fetch\", as\nlong as we also guarantee that we can gracefully recover from that\ncorruption.\n\nI.e. 1) we didn't update the ref(s) involved 2) we know the FS has the\nsorts of properties you're aiming for with \"batch\".\n\nNow, as some patches I've had to object*.c recently show we don't handle\nthose cases gracefully either. I.e. if we find a short loose object\nwe'll panic, even if we'd be perfectly capable of getting it from\nelsewhere, and nothing otherwise references it.\n\nBut if we fixed that and gc/fsck/fetch etc. learned to deal with that\ncontent I don't see why wouldn't e.g. default to not fsyncing loose\nobjects when invoked from say \"fetch\" by default, depending on FS/OS\ndetection, and if we couldn't say it was safe defaulted to some \"POSIX\"\nmode that would be pedantic but slow and safe.\n"},{"id":"440889","messageId":"20211111000349.GA703@neerajsi-x1.localdomain","threadId":"56840","inReplyTo":"211110.8635o3rbdc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-11-11T00:03:49Z","receivedAt":"2021-11-11T00:03:58Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Wed, Nov 10, 2021 at 09:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> Yes. I understand that we're not doing POSIX fsyncing().\n> \n> I'm asking about something else, i.e. with this not-like-POSIX-sync why\n> it is that when you have a directory:\n> \n> A\n> \n> and files:\n> \n> A/{X,Y}\n> \n> That you'd write those two, and then proceed to do the \"batch flush\" by\n> creating and fsync()-ing a:\n> \n> B/Z\n> \n> As opposed to either of:\n> \n> A/Z\n> \n> Or:\n> \n> Z\n> \n> I.e. why update .git/refs/* and create a flush file in .git/object/* and\n> not .git/refs/* or .git/*?\n> \n> Maybe the answer is just that this is WIP code copied from your\n> .git/objects/ fsync() implementation, or maybe it's more subtle and I'm\n> missing something.\n\nIt looks I didn't really answer your actual question before. On the filesystems\nwhich I'm familiar with at a code level (NTFS and ReFS on Windows), fsyncing a file\nor dir anywhere on the filesystem should ensure that metadata operations completed\nbefore the fsync starts are durable when the fsync returns. So which specific directory\nwe put the file in shouldn't matter as long as it's on the same filesystem as the other\nfiles we're interested in.\n\n> \n> *nod*. See this if you haven't yet:\n> https://lore.kernel.org/git/211110.86r1bogg27.gmgdl@evledraar.gmail.com/T/#u\n\nI'll respond on that thread with my opinion. Thanks for reviewing this and pushing\nfor a good holistic approach.\n\nThanks,\nNeeraj\n"},{"id":"440892","messageId":"20211111001803.GA787@neerajsi-x1.localdomain","threadId":"56840","inReplyTo":"d9aa96913b1730f1d0c238d7d52e27c20bc55390.1636544377.git.ps@pks.im","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-11-11T00:18:03Z","receivedAt":"2021-11-11T00:18:11Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Wed, Nov 10, 2021 at 12:41:03PM +0100, Patrick Steinhardt wrote:\n> \n> +static int sync_loose_ref(int fd)\n> +{\n> +\tswitch (fsync_ref_files) {\n> +\tcase FSYNC_REF_FILES_OFF:\n> +\t\treturn 0;\n> +\tcase FSYNC_REF_FILES_ON:\n> +\t\treturn git_fsync(fd, FSYNC_HARDWARE_FLUSH);\n> +\tcase FSYNC_REF_FILES_BATCH:\n> +\t\treturn git_fsync(fd, FSYNC_WRITEOUT_ONLY);\n> +\tdefault:\n> +\t\tBUG(\"invalid fsync mode %d\", fsync_ref_files);\n> +\t}\n> +}\n> +\n> +#define SYNC_LOOSE_REF_GITDIR (1 << 0)\n> +#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)\n> +\n> +static int sync_loose_refs_flags(const char *refname)\n> +{\n> +\tswitch (ref_type(refname)) {\n> +\tcase REF_TYPE_PER_WORKTREE:\n> +\tcase REF_TYPE_PSEUDOREF:\n> +\t\treturn SYNC_LOOSE_REF_GITDIR;\n> +\tcase REF_TYPE_MAIN_PSEUDOREF:\n> +\tcase REF_TYPE_OTHER_PSEUDOREF:\n> +\tcase REF_TYPE_NORMAL:\n> +\t\treturn SYNC_LOOSE_REF_COMMONDIR;\n> +\tdefault:\n> +\t\tBUG(\"unknown ref type %d of ref %s\",\n> +\t\t ref_type(refname), refname);\n> +\t}\n> +}\n> +\n> +static int sync_loose_refs(struct files_ref_store *refs,\n> +\t\t\t int flags,\n> +\t\t\t struct strbuf *err)\n> +{\n> +\tif (fsync_ref_files != FSYNC_REF_FILES_BATCH)\n> +\t\treturn 0;\n> +\n> +\tif ((flags & SYNC_LOOSE_REF_GITDIR) &&\n> +\t git_fsync_dir(refs->base.gitdir) < 0)\n> +\t\treturn -1;\n> +\n> +\tif ((flags & SYNC_LOOSE_REF_COMMONDIR) &&\n> +\t git_fsync_dir(refs->gitcommondir) < 0)\n> +\t\treturn -1;\n> +\n> +\treturn 0;\n> +}\n> +\n\nNow that I understand it better, I agree with Ævar's feedback. I think\nonly a single sync is needed to cover everything in .git/. \n\nI'd also suggest renaming 'sync_loose_refs' to something which makes\nit clearer that it's the batch-flush step. Maybe s/do_batch_fsync/do_batch_objects_fsync\nand s/sync_loose_refs/do_batch_refs_fsync.\n\nThanks\nNeeraj\n"},{"id":"440907","messageId":"YY0C2ES1aGpSAW4v@ncase","threadId":"56840","inReplyTo":"YYwvVy6AWDjkWazn@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-11T11:47:36Z","receivedAt":"2021-11-11T11:48:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 10, 2021 at 03:45:11PM -0500, Jeff King wrote:\n> On Wed, Nov 10, 2021 at 12:40:50PM +0100, Patrick Steinhardt wrote:\n> \n> > Please note that I didn't yet add any performance numbers or tests.\n> > Performance tests didn't show any conclusive results on my machine given\n> > that I couldn't observe any noticeable impact at all, and I didn't write\n> > tests yet given that I first wanted to get some feedback on this series.\n> > If we agree that this is the correct way to go forward I'll of course\n> > put in some more time to add them.\n> \n> Here's a mild adjustment of the quick test I showed earlier in [1]. It\n> uses your version for all cases, but swaps between the three config\n> options, and it uses --atomic to put everything into a single\n> transaction:\n> \n> $ hyperfine -L v false,true,batch -p ./setup 'git -C dst.git config core.fsyncreffiles {v}; git.compile push --atomic dst.git refs/heads/*'\n> Benchmark 1: git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*\n> Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 7.8 ms, System: 3.9 ms]\n> Range (min … max): 10.1 ms … 11.0 ms 108 runs\n> \n> Benchmark 2: git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*\n> Time (mean ± σ): 402.5 ms ± 6.2 ms [User: 13.9 ms, System: 10.7 ms]\n> Range (min … max): 387.3 ms … 408.9 ms 10 runs\n> \n> Benchmark 3: git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*\n> Time (mean ± σ): 21.4 ms ± 0.6 ms [User: 8.0 ms, System: 5.2 ms]\n> Range (min … max): 20.7 ms … 24.9 ms 99 runs\n> \n> Summary\n> 'git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*' ran\n> 2.05 ± 0.07 times faster than 'git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*'\n> 38.51 ± 1.06 times faster than 'git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*'\n> \n> So yes, the \"batch\" case is much improved. It's still more expensive\n> than skipping the fsync entirely, but it's within reason. The trick,\n> though is the --atomic. If I drop that flag, then \"batch\" takes as long\n> as \"true\". And of course that's the default.\n\nGreat, thanks for doing these tests.\n\n> I wonder if that means we'd want an extra mode: do the WRITEOUT_ONLY\n> flush, but _don't_ do a HARDWARE_FLUSH for each transaction. That\n> doesn't give you durability, but it (in theory) solves the out-of-order\n> journal problem without paying any performance cost.\n> \n> I say \"in theory\" because I'm just assuming that the WRITEOUT thing\n> works as advertised. I don't have an easy way to test the outcome on a\n> power failure at the right moment here.\n> \n> -Peff\n> \n> [1] https://lore.kernel.org/git/YYTaiIlEKxHRVdCy@coredump.intra.peff.net/\n\nYeah, it's also one of the things I'm currently wondering about. I just\npiggy-backed on the preexisting patch series which claim that it at\nleast ought to work on macOS and Windows, but left out of the equation\nwere Linux filesystems. I'm hoping that we'll get an answer to this\nquestion via <211111.86pmr7pk9o.gmgdl@evledraar.gmail.com>, where Ævar\nput Linus and Christopher Hellwig on Cc.\n\nIn any case, if the outcome of a WRITEOUT_ONLY flush would be that we\nmay not see all renames, but that every refs' contents would be\nwell-defined, then this is a tradeoff we'd probably want to make at\nGitLab.\n\nPatrick\n"},{"id":"440909","messageId":"YY0HWCgg2e/cAc5y@ncase","threadId":"56840","inReplyTo":"211110.86v910gi9a.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-11T12:06:48Z","receivedAt":"2021-11-11T12:07:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 10 2021, Patrick Steinhardt wrote:\n> \n> > [...]\n> > Fix this by introducing a new configuration \"core.fsyncRefFiles\". This\n> > config matches behaviour of \"core.fsyncObjectFiles\" in that it provides\n> > three different modes:\n> >\n> > - \"off\" disables calling fsync on ref files. This is the default\n> > behaviour previous to this change and remains the default after\n> > this change.\n> >\n> > - \"on\" enables calling fsync on ref files, where each reference is\n> > flushed to disk before it is being committed. This is the safest\n> > setting, but may incur significant performance overhead.\n> >\n> > - \"batch\" will flush the page cache of each file as it is written to\n> > ensure its data is persisted. After all refs have been written,\n> > the directories which host refs are flushed.\n> >\n> > With this change in place and when \"core.fsyncRefFiles\" is set to either\n> > \"on\" or \"batch\", this kind of corruption shouldn't happen anymore.\n> >\n> > [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt\n> \n> With the understanding that my grokking of this approach is still\n> somewhere between \"uh, that works?\" and \"wow, voodoo FS magic!\". ....\n> \n> I haven't looked at these changes in much daiter, or Neeraj's recent\n> related changes but...\n> \n> > +core.fsyncRefFiles::\n> > +\tA value indicating the level of effort Git will expend in trying to make\n> > +\trefs added to the repo durable in the event of an unclean system\n> > +\tshutdown. This setting currently only controls loose refs in the object\n> > +\tstore, so updates to packed refs may not be equally durable. Takes the\n> > +\tsame parameters as `core.fsyncObjectFiles`.\n> > +\n> \n> ...my understanding of it is basically a way of going back to what Linus\n> pointed out way back in aafe9fbaf4f (Add config option to enable\n> 'fsync()' of object files, 2008-06-18).\n> \n> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all\n> and the directory entry, but on some FS's it would be sufficient to\n> fsync() just y if they're created in that order. It'll imply an fsync of\n> both x and y, is that accurate?\n> \n> If not you can probably discard the rest, but forging on:\n> \n> Why do we then need to fsync() a \"z\" file in get_object_directory()\n> (i.e. .git/objects) then? That's a new \"z\", wasn't flushing \"y\" enough?\n> \n> Or if you've written .git/objects/x and .git/refs/y I can imagine\n> wanting to create and sync a .git/z if the FS's semantics are to then\n> flush all remaining updates from that tree up, but here it's\n> .git/objects, not .git. That also seems to contract this above:\n> \n> > ensure its data is persisted. After all refs have been written,\n> > the directories which host refs are flushed.\n> \n> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for\n> now), not .git/objects or .git?\n\nYeah, .git/refs. Note that we need to flush refs in two locations though\ngiven that there are common refs shared across worktrees, and then there\nare worktree-specific refs. These may not even share the filesystem, so\nthere's not much we can do about this. We could play games with the\nfsid, but I'm not sure it's worth it.\n\n> And again, forging on but more generally [continued below]...\n> \n> > +\tif (!strcmp(var, \"core.fsyncreffiles\")) {\n> \n> UX side: now we've got a core.fsyncRefFiles and\n> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those\n> work together like say \"fsck.*\" and \"fetch.fsck.*\" do, i.e. you'd be\n> able to configure this once for objects and refs, or in two variables,\n> one for objects, one for refs...\n\nHonestly, I'd prefer to just have a global variable with per-subsystem\noverrides so that you can say \"Please batch-sync all files by default,\nbut I really don't care for files in the worktree\". Kind of goes into\nthe same direction as your RFC.\n\n[snip]\n> > @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> > \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n> > \t\t\t \"ref_transaction_prepare\");\n> > \tsize_t i;\n> > -\tint ret = 0;\n> > +\tint ret = 0, sync_flags = 0;\n> > \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n> > \tchar *head_ref = NULL;\n> > \tint head_type;\n> > @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> > \t\t\t\t\t&update->new_oid, NULL,\n> > \t\t\t\t\tNULL);\n> > \t\t}\n> > +\n> > +\t\tsync_flags |= sync_loose_refs_flags(update->refname);\n> > \t}\n> > \n> > +\tret = sync_loose_refs(refs, sync_flags, err);\n> > +\tif (ret)\n> > +\t\tgoto cleanup;\n> > +\n> > \tif (packed_transaction) {\n> > \t\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n> > \t\t\tret = TRANSACTION_GENERIC_ERROR;\n> \n> ...[continued from above]: Again, per my potentially wrong understanding\n> of syncing a \"x\" and \"y\" via an fsync of a subsequent \"z\" that's\n> adjacent on the FS to those two.\n> \n> Isn't this setting us up for a really bad interaction between this\n> series and Neeraj's work? Well \"bad\" as in \"bad for performance\".\n> \n> I.e. you'll turn on \"use the batch thing for objects and refs\" and we'll\n> do two fsyncs, one for the object update, and one for refs. The common\n> case is that we'll have both in play.\n> \n> So shouldn't this go to a higher level for both so we only create a \"z\"\n> .git/sync-it-now-please.txt or whatever once we do all pending updates\n> on the .git/ directory?\n\nGood question. Taking a different stance would be to say \"I don't ever\nwant to write a ref referring to an object which isn't yet guaranteed to\nbe persistent\". In that case, writing and syncing objects first and then\nupdating refs would be the correct thing to do and wouldn't need\ncoordination on a higher level.\n\n> I can also imagine that we'd want that at an even higher level, e.g. for\n> \"git pull\" surely we'd want it not for refs or objects, but in\n> builtin/pull.c somewhere because we'll be updating the .git/index after\n> we do both refs and objects, and you'd want to fsync at the very end,\n> no?\n> \n> None of the above should mean we can't pursue a more narrow approach for\n> now. I'm just:\n> \n> 1) Seeing if I understand what we're trying to do here, maybe not.\n\nPersonally, my most important bullet point is that I want to get rid of\nthe corrupt refs we see in production every now and then. As Peff\npointed out in another email, it _might_ be sufficient to just flush out\nthe page cache for those without synchronizing any metadata at all. But\nnone of us seems to be sure that this really works the way we think it\ndoes.\n\n> 2) Encouraging you two to think about a holistic way to configure some\n> logical conclusion to this topic at large, so we won't end up with\n> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,\n> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)\n\nYeah, I agree with you on this point. We definitely don't want to end up\nwith twenty different knobs to sync objects, refs, packed-refs, configs,\nreflogs et al, and we're currently steering into that direction. Having\na central knob core.fsync which directs defaults for all subsystems\nwould be very much preferable if you ask me, where specific subsystems\ncan then be changed as the user sees fit.\n\nWhat I think is important though is that we should have the right\ndefaults in place. It doesn't help to say \"fsync is slow\" as an excuse\nto not do them at all by default, where the result is that users get a\ncorrupt repository.\n\nPatrick\n"},{"id":"440910","messageId":"YY0JC/zS/2rqbZIp@ncase","threadId":"56840","inReplyTo":"211110.8635o3rbdc.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-11T12:14:03Z","receivedAt":"2021-11-11T12:14:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 10, 2021 at 09:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 10 2021, Neeraj Singh wrote:\n> \n> > On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> >> \n> >> On Wed, Nov 10 2021, Patrick Steinhardt wrote:\n[snip]\n> >> ...[continued from above]: Again, per my potentially wrong understanding\n> >> of syncing a \"x\" and \"y\" via an fsync of a subsequent \"z\" that's\n> >> adjacent on the FS to those two.\n> >\n> > I suspect Patrick is concerned about the case where the worktree is on\n> > a separate filesystem from the main repo, so there might be a motivation\n> > to sync the worktree refs separately. Is that right, Patrick?\n> \n> But he's syncing .git/objects, not .git/worktrees//refs/, no?\n\nThat'd be a bug then ;) My intent was to sync .git/refs and the\nper-worktree refs.\n\n[snip]\n> > In my view there are two separable concerns:\n> >\n> > 1) What durability do we want for the user's data when an entire 'git'\n> > command completes? I.e. if I run 'git add <1000 files>; git commit' and the\n> > system loses power after both commands return, should I see all of those files\n> > in the committed state of the repo?\n> >\n> > 2) What internal consistency do we want in the git databases (ODB and REFS) if\n> > we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes\n> > before returning, can there be an invalid object or a torn ref state?\n> >\n> > If were only concerned with (1), then doing a single fsync at the end of the high-level\n> > git command would be sufficient. However, (2) requires more fsyncs to provide barriers\n> > between different phases internal to the git commands. For instance, we need a barrier\n> > between creating the ODB state for a commit (blobs, trees, commit obj) and the refs\n> > pointing to it.\n> >\n> > I am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds\n> > each fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not\n> > be user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)\n> > would become apparent to the user.\n> >\n> > The more optimal way to handle consistency and durability is Write-ahead-logging with log\n> > replay on crash recovery. That approach can reduce the number of fsyncs in the active workload\n> > to at most two (one to sync the log with a commit record and then one before truncating the\n> > log after updating the main database). At that point, however, I think it would be best to\n> > use an existing database engine with some modifications to create a good data layout for git.\n> \n> I think that git should safely store your data by default, we clearly\n> don't do that well enough in some cases, and should fix it.\n> \n> But there's also cases where people would much rather have performance,\n> and I think we should support that. E.g. running git in CI, doing a\n> one-off import/fetch that you'll invoke \"sync(1)\" yourself after\n> etc. This is the direction Eric Wong's patches are going into.\n> \n> I understand from his patches/comments that you're not correct that just\n> 1-3 fsyncs are OK, i.e. for some use-cases 0 is OK, and any >0 is too\n> many, since it'll force a flush of the whole disk or something.\n> \n> Even when git is is operating in a completely safe mode I think we'd\n> still have license to play it fast and loose with /some/ user data,\n> because users don't really care about all of their data in the .git/\n> directory.\n\nI think we need to discern two important cases: the first case is us\nlosing data, and the second case is us leaving the repository in a\ncorrupt state. I'm okay-ish with the first case: if your machine crashes\nit's not completely unexpected that losing data would be a natural\nconsequence (well, losing the data that's currently in transit at least).\n\nBut we should make sure that we're not leaving the repo in a corrupt\nstate under any circumstance, and that's exactly what can happen right\nnow because we don't flush refs to disk before doing the renames.\nAssuming we've got semantics correct, we are thus forced to do a page\ncache flush to make sure that data is on disk before doing a rename to\nnot corrupt the repo. But in the case where we're fine with losing some\ndata, we may skip doing the final fsync to also synchronize the rename\nto disk.\n\nPatrick\n\n> I.e. you do care about the *.pack file, but an associated *.bitmap is a\n> derived file, so we probably don't need to fsync that too. Is it worth\n> worrying about? Probably not, but that's what I had in mind with the\n> above-linked proposed config schema.\n> \n> Similarly for say writing 1k loose objects I think it would be\n> completely fine to end up with a corrupt object during a \"git fetch\", as\n> long as we also guarantee that we can gracefully recover from that\n> corruption.\n> \n> I.e. 1) we didn't update the ref(s) involved 2) we know the FS has the\n> sorts of properties you're aiming for with \"batch\".\n> \n> Now, as some patches I've had to object*.c recently show we don't handle\n> those cases gracefully either. I.e. if we find a short loose object\n> we'll panic, even if we'd be perfectly capable of getting it from\n> elsewhere, and nothing otherwise references it.\n> \n> But if we fixed that and gc/fsck/fetch etc. learned to deal with that\n> content I don't see why wouldn't e.g. default to not fsyncing loose\n> objects when invoked from say \"fetch\" by default, depending on FS/OS\n> detection, and if we couldn't say it was safe defaulted to some \"POSIX\"\n> mode that would be pedantic but slow and safe.\n"}]}