* [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`
2021-11-10 11:40 ` [PATCH v2 0/3] " Patrick Steinhardt
@ 2021-11-10 11:40 ` Patrick Steinhardt
2021-11-10 14:33 ` Johannes Schindelin
2021-11-10 14:39 ` Ævar Arnfjörð Bjarmason
2021-11-10 11:40 ` [PATCH v2 2/3] wrapper: provide function to sync directories Patrick Steinhardt
` (3 subsequent siblings)
4 siblings, 2 replies; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-10 11:40 UTC (permalink / raw)
To: git
Cc: Jeff King, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Eric Wong, Neeraj K. Singh
[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]
While we already have a wrapper around `git_fsync()`, this wrapper
doesn't handle looping around `EINTR`. Convert it to do so such that
callers don't have to remember doing this everywhere.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
wrapper.c | 9 ++++++++-
write-or-die.c | 6 ++----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/wrapper.c b/wrapper.c
index ece3d2ca10..e20df4f3a6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)
return fd;
}
-int git_fsync(int fd, enum fsync_action action)
+static int git_fsync_once(int fd, enum fsync_action action)
{
switch (action) {
case FSYNC_WRITEOUT_ONLY:
@@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)
default:
BUG("unexpected git_fsync(%d) call", action);
}
+}
+int git_fsync(int fd, enum fsync_action action)
+{
+ while (git_fsync_once(fd, action) < 0)
+ if (errno != EINTR)
+ return -1;
+ return 0;
}
static int warn_if_unremovable(const char *op, const char *file, int rc)
diff --git a/write-or-die.c b/write-or-die.c
index cc8291d979..e61220aa2d 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
void fsync_or_die(int fd, const char *msg)
{
- while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) {
- if (errno != EINTR)
- die_errno("fsync error on '%s'", msg);
- }
+ if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
+ die_errno("fsync error on '%s'", msg);
}
void write_or_die(int fd, const void *buf, size_t count)
--
2.33.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`
2021-11-10 11:40 ` [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()` Patrick Steinhardt
@ 2021-11-10 14:33 ` Johannes Schindelin
2021-11-10 14:39 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 34+ messages in thread
From: Johannes Schindelin @ 2021-11-10 14:33 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Wong, Neeraj K. Singh
Hi Patrick,
just one observation, below.
On Wed, 10 Nov 2021, Patrick Steinhardt wrote:
> diff --git a/wrapper.c b/wrapper.c
> index ece3d2ca10..e20df4f3a6 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)
> return fd;
> }
>
> -int git_fsync(int fd, enum fsync_action action)
> +static int git_fsync_once(int fd, enum fsync_action action)
> {
> switch (action) {
> case FSYNC_WRITEOUT_ONLY:
> @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)
> default:
> BUG("unexpected git_fsync(%d) call", action);
> }
> +}
>
> +int git_fsync(int fd, enum fsync_action action)
> +{
> + while (git_fsync_once(fd, action) < 0)
> + if (errno != EINTR)
> + return -1;
> + return 0;
> }
My immediate reaction was: why not fold `git_fsync_once()` into
`git_fsync()`?
And then I had a look at the function (which is sadly not in the diff
context of this email, one of the occasions when I would prefer a proper
UI for reviewing), and I agree that indenting the code even one level
would make it harder to read.
All this is to say: I agree with the approach you took here.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()`
2021-11-10 11:40 ` [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()` Patrick Steinhardt
2021-11-10 14:33 ` Johannes Schindelin
@ 2021-11-10 14:39 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-10 14:39 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Johannes Schindelin, Junio C Hamano, Eric Wong,
Neeraj K. Singh
On Wed, Nov 10 2021, Patrick Steinhardt wrote:
> [[PGP Signed Part:Undecided]]
> While we already have a wrapper around `git_fsync()`, this wrapper
> doesn't handle looping around `EINTR`. Convert it to do so such that
> callers don't have to remember doing this everywhere.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> wrapper.c | 9 ++++++++-
> write-or-die.c | 6 ++----
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index ece3d2ca10..e20df4f3a6 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -546,7 +546,7 @@ int xmkstemp_mode(char *filename_template, int mode)
> return fd;
> }
>
> -int git_fsync(int fd, enum fsync_action action)
> +static int git_fsync_once(int fd, enum fsync_action action)
> {
> switch (action) {
> case FSYNC_WRITEOUT_ONLY:
> @@ -591,7 +591,14 @@ int git_fsync(int fd, enum fsync_action action)
> default:
> BUG("unexpected git_fsync(%d) call", action);
> }
> +}
>
> +int git_fsync(int fd, enum fsync_action action)
> +{
> + while (git_fsync_once(fd, action) < 0)
> + if (errno != EINTR)
> + return -1;
> + return 0;
> }
>
> static int warn_if_unremovable(const char *op, const char *file, int rc)
> diff --git a/write-or-die.c b/write-or-die.c
> index cc8291d979..e61220aa2d 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -57,10 +57,8 @@ void fprintf_or_die(FILE *f, const char *fmt, ...)
>
> void fsync_or_die(int fd, const char *msg)
> {
> - while (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0) {
> - if (errno != EINTR)
> - die_errno("fsync error on '%s'", msg);
> - }
> + if (git_fsync(fd, FSYNC_HARDWARE_FLUSH) < 0)
> + die_errno("fsync error on '%s'", msg);
Nit: While at it maybe change it to: "fsync() error syncing file '%s'"
Maybe having a xgit_fsync() convenience wrapper would make subsequent
steps easier?
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 2/3] wrapper: provide function to sync directories
2021-11-10 11:40 ` [PATCH v2 0/3] " Patrick Steinhardt
2021-11-10 11:40 ` [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()` Patrick Steinhardt
@ 2021-11-10 11:40 ` Patrick Steinhardt
2021-11-10 14:40 ` Ævar Arnfjörð Bjarmason
2021-11-10 11:41 ` [PATCH v2 3/3] refs: add configuration to enable flushing of refs Patrick Steinhardt
` (2 subsequent siblings)
4 siblings, 1 reply; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-10 11:40 UTC (permalink / raw)
To: git
Cc: Jeff King, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Eric Wong, Neeraj K. Singh
[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]
In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04),
we have introduced batched syncing of object files. This mode works by
only requesting a writeback of the page cache backing the file on
written files, followed by a single hardware-flush via a temporary file
created in the directory we want to flush. Given modern journaling file
systems, this pattern is expected to be durable.
While it's possible to reuse the `git_fsync()` helper to synchronize the
page cache only, there is no helper which would allow for doing a
hardware flush of a directory by creating a temporary file. Other
callers which want to follow the same pattern would thus have to repeat
this logic.
Extract a new helper `git_fsync_dir()` from the object files code which
neatly encapsulates this logic such that it can be reused.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
bulk-checkin.c | 13 +++----------
git-compat-util.h | 7 +++++++
wrapper.c | 21 +++++++++++++++++++++
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 4deee1af46..e6ebdd1db5 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -98,16 +98,9 @@ static void do_batch_fsync(void)
* hardware.
*/
- if (needs_batch_fsync) {
- struct strbuf temp_path = STRBUF_INIT;
- struct tempfile *temp;
-
- strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory());
- temp = xmks_tempfile(temp_path.buf);
- fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
- delete_tempfile(&temp);
- strbuf_release(&temp_path);
- }
+ if (needs_batch_fsync &&
+ git_fsync_dir(get_object_directory()) < 0)
+ die_errno("fsyncing object directory");
if (bulk_fsync_objdir)
tmp_objdir_migrate(bulk_fsync_objdir);
diff --git a/git-compat-util.h b/git-compat-util.h
index 97f97178e7..f890bd07fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1221,6 +1221,13 @@ enum fsync_action {
int git_fsync(int fd, enum fsync_action action);
+/*
+ * Issue a full hardware flush against a temporary file in the given directory
+ * to ensure that all files inside that directory are durable before any renames
+ * occur.
+ */
+int git_fsync_dir(const char *path);
+
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success, which includes trying to unlink an object that does
diff --git a/wrapper.c b/wrapper.c
index e20df4f3a6..6c6cc8b74f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,6 +3,7 @@
*/
#include "cache.h"
#include "config.h"
+#include "tempfile.h"
static int memory_limit_check(size_t size, int gentle)
{
@@ -601,6 +602,26 @@ int git_fsync(int fd, enum fsync_action action)
return 0;
}
+int git_fsync_dir(const char *path)
+{
+ struct strbuf temp_path = STRBUF_INIT;
+ struct tempfile *temp;
+
+ strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", path);
+
+ temp = mks_tempfile(temp_path.buf);
+ if (!temp)
+ return -1;
+
+ if (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0)
+ return -1;
+
+ delete_tempfile(&temp);
+ strbuf_release(&temp_path);
+
+ return 0;
+}
+
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
int err;
--
2.33.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 2/3] wrapper: provide function to sync directories
2021-11-10 11:40 ` [PATCH v2 2/3] wrapper: provide function to sync directories Patrick Steinhardt
@ 2021-11-10 14:40 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-10 14:40 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Johannes Schindelin, Junio C Hamano, Eric Wong,
Neeraj K. Singh
On Wed, Nov 10 2021, Patrick Steinhardt wrote:
> [[PGP Signed Part:Undecided]]
> In ec983eb5d2 (core.fsyncobjectfiles: batched disk flushes, 2021-10-04),
> we have introduced batched syncing of object files. This mode works by
> only requesting a writeback of the page cache backing the file on
> written files, followed by a single hardware-flush via a temporary file
> created in the directory we want to flush. Given modern journaling file
> systems, this pattern is expected to be durable.
>
> While it's possible to reuse the `git_fsync()` helper to synchronize the
> page cache only, there is no helper which would allow for doing a
> hardware flush of a directory by creating a temporary file. Other
> callers which want to follow the same pattern would thus have to repeat
> this logic.
>
> Extract a new helper `git_fsync_dir()` from the object files code which
> neatly encapsulates this logic such that it can be reused.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> bulk-checkin.c | 13 +++----------
> git-compat-util.h | 7 +++++++
> wrapper.c | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 4deee1af46..e6ebdd1db5 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -98,16 +98,9 @@ static void do_batch_fsync(void)
> * hardware.
> */
>
> - if (needs_batch_fsync) {
> - struct strbuf temp_path = STRBUF_INIT;
> - struct tempfile *temp;
> -
> - strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", get_object_directory());
> - temp = xmks_tempfile(temp_path.buf);
> - fsync_or_die(get_tempfile_fd(temp), get_tempfile_path(temp));
> - delete_tempfile(&temp);
> - strbuf_release(&temp_path);
> - }
> + if (needs_batch_fsync &&
> + git_fsync_dir(get_object_directory()) < 0)
> + die_errno("fsyncing object directory");
Nit: Similar to 1/3, but this message is new: We say "fsyncing object
directory", but it would be better to pass in some "verbose" flag to
git_fsync_dir() so we can say e.g.:
error_errno(_("couldn't create core.fsyncRefFiles=batch tempfile '%s' in '%s'"), ...)
error_errno(_("couldn't fsync() core.fsyncRefFiles=batch tempfile '%s' in '%s'"), ...)
I.e. being able to say specifically why we failed, permission error or
the tempfile? fsync() didn't work etc?
Looking at the underlying APIs maybe they already have a mode to "die"
or "warn" appropriately? Or...
> +int git_fsync_dir(const char *path)
> +{
> + struct strbuf temp_path = STRBUF_INIT;
> + struct tempfile *temp;
> +
> + strbuf_addf(&temp_path, "%s/bulk_fsync_XXXXXX", path);
> +
> + temp = mks_tempfile(temp_path.buf);
> + if (!temp)
> + return -1;
> +
> + if (git_fsync(get_tempfile_fd(temp), FSYNC_HARDWARE_FLUSH) < 0)
> + return -1;
...if they do maybe we should use their non-fatal mode, because
with/without that these "return -1" need to be "goto cleanup" so we can
attempt to clean up after ourselves here.
I think this whole thing would be better if we generalized tmp-objdir.h
a bit, so it could create and manage an arbitrary file in an arbitrary
directory, and that API should really be generalized to a user of
tempfile.c.
I.e. we'd then create this file, sync it optionally, whine if it does't
work, and be guaranteed to try to clean anything that goes wrong up
atexit().
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 11:40 ` [PATCH v2 0/3] " Patrick Steinhardt
2021-11-10 11:40 ` [PATCH v2 1/3] wrapper: handle EINTR in `git_fsync()` Patrick Steinhardt
2021-11-10 11:40 ` [PATCH v2 2/3] wrapper: provide function to sync directories Patrick Steinhardt
@ 2021-11-10 11:41 ` Patrick Steinhardt
2021-11-10 14:49 ` Ævar Arnfjörð Bjarmason
2021-11-11 0:18 ` Neeraj Singh
2021-11-10 14:44 ` [PATCH v2 0/3] refs: sync loose refs to disk before committing them Johannes Schindelin
2021-11-10 20:45 ` Jeff King
4 siblings, 2 replies; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-10 11:41 UTC (permalink / raw)
To: git
Cc: Jeff King, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Eric Wong, Neeraj K. Singh
[-- Attachment #1: Type: text/plain, Size: 9268 bytes --]
When writing loose refs, we first create a lockfile, write the new ref
into that lockfile, close it and then rename the lockfile into place
such that the actual update is atomic for that single ref. While this
works as intended under normal circumstences, at GitLab we infrequently
encounter corrupt loose refs in repositories after a machine encountered
a hard reset. The corruption is always of the same type: the ref has
been committed into place, but it is completely empty.
The root cause of this is likely that we don't sync contents of the
lockfile to disk before renaming it into place. As a result, it's not
guaranteed that the contents are properly persisted and one may observe
weird in-between states on hard resets. Quoting ext4 documentation [1]:
Many broken applications don't use fsync() when replacing existing
files via patterns such as fd =
open("foo.new")/write(fd,..)/close(fd)/ rename("foo.new", "foo"), or
worse yet, fd = open("foo", O_TRUNC)/write(fd,..)/close(fd). If
auto_da_alloc is enabled, ext4 will detect the replace-via-rename
and replace-via-truncate patterns and force that any delayed
allocation blocks are allocated such that at the next journal
commit, in the default data=ordered mode, the data blocks of the new
file are forced to disk before the rename() operation is committed.
This provides roughly the same level of guarantees as ext3, and
avoids the "zero-length" problem that can happen when a system
crashes before the delayed allocation blocks are forced to disk.
This explicitly points out that one must call fsync(3P) before doing the
rename(3P) call, or otherwise data may not be correctly persisted to
disk.
Fix this by introducing a new configuration "core.fsyncRefFiles". This
config matches behaviour of "core.fsyncObjectFiles" in that it provides
three different modes:
- "off" disables calling fsync on ref files. This is the default
behaviour previous to this change and remains the default after
this change.
- "on" enables calling fsync on ref files, where each reference is
flushed to disk before it is being committed. This is the safest
setting, but may incur significant performance overhead.
- "batch" will flush the page cache of each file as it is written to
ensure its data is persisted. After all refs have been written,
the directories which host refs are flushed.
With this change in place and when "core.fsyncRefFiles" is set to either
"on" or "batch", this kind of corruption shouldn't happen anymore.
[1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/config/core.txt | 7 ++++
cache.h | 7 ++++
config.c | 10 ++++++
environment.c | 1 +
refs/files-backend.c | 62 ++++++++++++++++++++++++++++++++++-
5 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 200b4d9f06..e2fd0d8c90 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -572,6 +572,13 @@ core.fsyncObjectFiles::
stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or
ReFS.
+core.fsyncRefFiles::
+ A value indicating the level of effort Git will expend in trying to make
+ refs added to the repo durable in the event of an unclean system
+ shutdown. This setting currently only controls loose refs in the object
+ store, so updates to packed refs may not be equally durable. Takes the
+ same parameters as `core.fsyncObjectFiles`.
+
core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
+
diff --git a/cache.h b/cache.h
index 6d6e6770ec..14c8fab6b4 100644
--- a/cache.h
+++ b/cache.h
@@ -991,7 +991,14 @@ enum fsync_object_files_mode {
FSYNC_OBJECT_FILES_BATCH
};
+enum fsync_ref_files_mode {
+ FSYNC_REF_FILES_OFF,
+ FSYNC_REF_FILES_ON,
+ FSYNC_REF_FILES_BATCH
+};
+
extern enum fsync_object_files_mode fsync_object_files;
+extern enum fsync_ref_files_mode fsync_ref_files;
extern int core_preload_index;
extern int precomposed_unicode;
extern int protect_hfs;
diff --git a/config.c b/config.c
index 5eb36ecd77..4cbad5a29d 100644
--- a/config.c
+++ b/config.c
@@ -1500,6 +1500,16 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "core.fsyncreffiles")) {
+ if (value && !strcmp(value, "batch"))
+ fsync_ref_files = FSYNC_REF_FILES_BATCH;
+ else if (git_config_bool(var, value))
+ fsync_ref_files = FSYNC_REF_FILES_ON;
+ else
+ fsync_ref_files = FSYNC_REF_FILES_OFF;
+ return 0;
+ }
+
if (!strcmp(var, "core.preloadindex")) {
core_preload_index = git_config_bool(var, value);
return 0;
diff --git a/environment.c b/environment.c
index aeafe80235..1514ac9384 100644
--- a/environment.c
+++ b/environment.c
@@ -43,6 +43,7 @@ const char *git_hooks_path;
int zlib_compression_level = Z_BEST_SPEED;
int pack_compression_level = Z_DEFAULT_COMPRESSION;
enum fsync_object_files_mode fsync_object_files;
+enum fsync_ref_files_mode fsync_ref_files;
size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
size_t delta_base_cache_limit = 96 * 1024 * 1024;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4b14f30d48..31d7456266 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1360,6 +1360,57 @@ static int commit_ref_update(struct files_ref_store *refs,
const struct object_id *oid, const char *logmsg,
struct strbuf *err);
+static int sync_loose_ref(int fd)
+{
+ switch (fsync_ref_files) {
+ case FSYNC_REF_FILES_OFF:
+ return 0;
+ case FSYNC_REF_FILES_ON:
+ return git_fsync(fd, FSYNC_HARDWARE_FLUSH);
+ case FSYNC_REF_FILES_BATCH:
+ return git_fsync(fd, FSYNC_WRITEOUT_ONLY);
+ default:
+ BUG("invalid fsync mode %d", fsync_ref_files);
+ }
+}
+
+#define SYNC_LOOSE_REF_GITDIR (1 << 0)
+#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)
+
+static int sync_loose_refs_flags(const char *refname)
+{
+ switch (ref_type(refname)) {
+ case REF_TYPE_PER_WORKTREE:
+ case REF_TYPE_PSEUDOREF:
+ return SYNC_LOOSE_REF_GITDIR;
+ case REF_TYPE_MAIN_PSEUDOREF:
+ case REF_TYPE_OTHER_PSEUDOREF:
+ case REF_TYPE_NORMAL:
+ return SYNC_LOOSE_REF_COMMONDIR;
+ default:
+ BUG("unknown ref type %d of ref %s",
+ ref_type(refname), refname);
+ }
+}
+
+static int sync_loose_refs(struct files_ref_store *refs,
+ int flags,
+ struct strbuf *err)
+{
+ if (fsync_ref_files != FSYNC_REF_FILES_BATCH)
+ return 0;
+
+ if ((flags & SYNC_LOOSE_REF_GITDIR) &&
+ git_fsync_dir(refs->base.gitdir) < 0)
+ return -1;
+
+ if ((flags & SYNC_LOOSE_REF_COMMONDIR) &&
+ git_fsync_dir(refs->gitcommondir) < 0)
+ return -1;
+
+ return 0;
+}
+
/*
* Emit a better error message than lockfile.c's
* unable_to_lock_message() would in case there is a D/F conflict with
@@ -1502,6 +1553,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
oidcpy(&lock->old_oid, &orig_oid);
if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
+ sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||
commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {
error("unable to write current sha1 into %s: %s", newrefname, err.buf);
strbuf_release(&err);
@@ -1522,6 +1574,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
flag = log_all_ref_updates;
log_all_ref_updates = LOG_REFS_NONE;
if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
+ sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||
commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {
error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
strbuf_release(&err);
@@ -1781,6 +1834,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
fd = get_lock_file_fd(&lock->lk);
if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
write_in_full(fd, &term, 1) < 0 ||
+ sync_loose_ref(fd) < 0 ||
close_ref_gently(lock) < 0) {
strbuf_addf(err,
"couldn't write '%s'", get_lock_file_path(&lock->lk));
@@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
files_downcast(ref_store, REF_STORE_WRITE,
"ref_transaction_prepare");
size_t i;
- int ret = 0;
+ int ret = 0, sync_flags = 0;
struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
char *head_ref = NULL;
int head_type;
@@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,
&update->new_oid, NULL,
NULL);
}
+
+ sync_flags |= sync_loose_refs_flags(update->refname);
}
+ ret = sync_loose_refs(refs, sync_flags, err);
+ if (ret)
+ goto cleanup;
+
if (packed_transaction) {
if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
ret = TRANSACTION_GENERIC_ERROR;
--
2.33.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 11:41 ` [PATCH v2 3/3] refs: add configuration to enable flushing of refs Patrick Steinhardt
@ 2021-11-10 14:49 ` Ævar Arnfjörð Bjarmason
2021-11-10 19:15 ` Neeraj Singh
2021-11-11 12:06 ` Patrick Steinhardt
2021-11-11 0:18 ` Neeraj Singh
1 sibling, 2 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-10 14:49 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Johannes Schindelin, Junio C Hamano, Eric Wong,
Neeraj K. Singh, Linus Torvalds
On Wed, Nov 10 2021, Patrick Steinhardt wrote:
> [...]
> Fix this by introducing a new configuration "core.fsyncRefFiles". This
> config matches behaviour of "core.fsyncObjectFiles" in that it provides
> three different modes:
>
> - "off" disables calling fsync on ref files. This is the default
> behaviour previous to this change and remains the default after
> this change.
>
> - "on" enables calling fsync on ref files, where each reference is
> flushed to disk before it is being committed. This is the safest
> setting, but may incur significant performance overhead.
>
> - "batch" will flush the page cache of each file as it is written to
> ensure its data is persisted. After all refs have been written,
> the directories which host refs are flushed.
>
> With this change in place and when "core.fsyncRefFiles" is set to either
> "on" or "batch", this kind of corruption shouldn't happen anymore.
>
> [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt
With the understanding that my grokking of this approach is still
somewhere between "uh, that works?" and "wow, voodoo FS magic!". ....
I haven't looked at these changes in much daiter, or Neeraj's recent
related changes but...
> +core.fsyncRefFiles::
> + A value indicating the level of effort Git will expend in trying to make
> + refs added to the repo durable in the event of an unclean system
> + shutdown. This setting currently only controls loose refs in the object
> + store, so updates to packed refs may not be equally durable. Takes the
> + same parameters as `core.fsyncObjectFiles`.
> +
...my understanding of it is basically a way of going back to what Linus
pointed out way back in aafe9fbaf4f (Add config option to enable
'fsync()' of object files, 2008-06-18).
I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all
and the directory entry, but on some FS's it would be sufficient to
fsync() just y if they're created in that order. It'll imply an fsync of
both x and y, is that accurate?
If not you can probably discard the rest, but forging on:
Why do we then need to fsync() a "z" file in get_object_directory()
(i.e. .git/objects) then? That's a new "z", wasn't flushing "y" enough?
Or if you've written .git/objects/x and .git/refs/y I can imagine
wanting to create and sync a .git/z if the FS's semantics are to then
flush all remaining updates from that tree up, but here it's
.git/objects, not .git. That also seems to contract this above:
> ensure its data is persisted. After all refs have been written,
> the directories which host refs are flushed.
I.e. that would be .git/refs (let's ignore .git/HEAD and the like for
now), not .git/objects or .git?
And again, forging on but more generally [continued below]...
> + if (!strcmp(var, "core.fsyncreffiles")) {
UX side: now we've got a core.fsyncRefFiles and
core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those
work together like say "fsck.*" and "fetch.fsck.*" do, i.e. you'd be
able to configure this once for objects and refs, or in two variables,
one for objects, one for refs...
> +static int sync_loose_ref(int fd)
> +{
> + switch (fsync_ref_files) {
> + case FSYNC_REF_FILES_OFF:
> + return 0;
> + case FSYNC_REF_FILES_ON:
> + return git_fsync(fd, FSYNC_HARDWARE_FLUSH);
> + case FSYNC_REF_FILES_BATCH:
> + return git_fsync(fd, FSYNC_WRITEOUT_ONLY);
> + default:
> + BUG("invalid fsync mode %d", fsync_ref_files);
> + }
> +}
> +
> +#define SYNC_LOOSE_REF_GITDIR (1 << 0)
> +#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)
nit: make this an enum and ...
> +static int sync_loose_refs_flags(const char *refname)
> +{
> + switch (ref_type(refname)) {
> + case REF_TYPE_PER_WORKTREE:
> + case REF_TYPE_PSEUDOREF:
> + return SYNC_LOOSE_REF_GITDIR;
> + case REF_TYPE_MAIN_PSEUDOREF:
> + case REF_TYPE_OTHER_PSEUDOREF:
> + case REF_TYPE_NORMAL:
> + return SYNC_LOOSE_REF_COMMONDIR;
> + default:
> + BUG("unknown ref type %d of ref %s",
> + ref_type(refname), refname);
... you won't need this default case...
> [...]
> /*
> * Emit a better error message than lockfile.c's
> * unable_to_lock_message() would in case there is a D/F conflict with
> @@ -1502,6 +1553,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
> oidcpy(&lock->old_oid, &orig_oid);
>
> if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
> + sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||
> commit_ref_update(refs, lock, &orig_oid, logmsg, &err)) {
> error("unable to write current sha1 into %s: %s", newrefname, err.buf);
> strbuf_release(&err);
> @@ -1522,6 +1574,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
> flag = log_all_ref_updates;
> log_all_ref_updates = LOG_REFS_NONE;
> if (write_ref_to_lockfile(lock, &orig_oid, &err) ||
> + sync_loose_refs(refs, sync_loose_refs_flags(newrefname), &err) ||
> commit_ref_update(refs, lock, &orig_oid, NULL, &err)) {
> error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
> strbuf_release(&err);
> @@ -1781,6 +1834,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
> fd = get_lock_file_fd(&lock->lk);
> if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
> write_in_full(fd, &term, 1) < 0 ||
> + sync_loose_ref(fd) < 0 ||
> close_ref_gently(lock) < 0) {
> strbuf_addf(err,
> "couldn't write '%s'", get_lock_file_path(&lock->lk));
> @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> files_downcast(ref_store, REF_STORE_WRITE,
> "ref_transaction_prepare");
> size_t i;
> - int ret = 0;
> + int ret = 0, sync_flags = 0;
> struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> char *head_ref = NULL;
> int head_type;
> @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> &update->new_oid, NULL,
> NULL);
> }
> +
> + sync_flags |= sync_loose_refs_flags(update->refname);
> }
>
> + ret = sync_loose_refs(refs, sync_flags, err);
> + if (ret)
> + goto cleanup;
> +
> if (packed_transaction) {
> if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
...[continued from above]: Again, per my potentially wrong understanding
of syncing a "x" and "y" via an fsync of a subsequent "z" that's
adjacent on the FS to those two.
Isn't this setting us up for a really bad interaction between this
series and Neeraj's work? Well "bad" as in "bad for performance".
I.e. you'll turn on "use the batch thing for objects and refs" and we'll
do two fsyncs, one for the object update, and one for refs. The common
case is that we'll have both in play.
So shouldn't this go to a higher level for both so we only create a "z"
.git/sync-it-now-please.txt or whatever once we do all pending updates
on the .git/ directory?
I can also imagine that we'd want that at an even higher level, e.g. for
"git pull" surely we'd want it not for refs or objects, but in
builtin/pull.c somewhere because we'll be updating the .git/index after
we do both refs and objects, and you'd want to fsync at the very end,
no?
None of the above should mean we can't pursue a more narrow approach for
now. I'm just:
1) Seeing if I understand what we're trying to do here, maybe not.
2) Encouraging you two to think about a holistic way to configure some
logical conclusion to this topic at large, so we won't end up with
core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,
core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)
I'll send another more generic follow-up E-Mail for #2.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 14:49 ` Ævar Arnfjörð Bjarmason
@ 2021-11-10 19:15 ` Neeraj Singh
2021-11-10 20:23 ` Ævar Arnfjörð Bjarmason
2021-11-11 12:06 ` Patrick Steinhardt
1 sibling, 1 reply; 34+ messages in thread
From: Neeraj Singh @ 2021-11-10 19:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Patrick Steinhardt, git, Jeff King, Johannes Schindelin,
Junio C Hamano, Eric Wong, Neeraj K. Singh, Linus Torvalds
On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Nov 10 2021, Patrick Steinhardt wrote:
>
> > + shutdown. This setting currently only controls loose refs in the object
> > + store, so updates to packed refs may not be equally durable. Takes the
> > + same parameters as `core.fsyncObjectFiles`.
> > +
>
> ...my understanding of it is basically a way of going back to what Linus
> pointed out way back in aafe9fbaf4f (Add config option to enable
> 'fsync()' of object files, 2008-06-18).
>
> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all
> and the directory entry, but on some FS's it would be sufficient to
> fsync() just y if they're created in that order. It'll imply an fsync of
> both x and y, is that accurate?
>
> If not you can probably discard the rest, but forging on:
>
> Why do we then need to fsync() a "z" file in get_object_directory()
> (i.e. .git/objects) then? That's a new "z", wasn't flushing "y" enough?
>
> Or if you've written .git/objects/x and .git/refs/y I can imagine
> wanting to create and sync a .git/z if the FS's semantics are to then
> flush all remaining updates from that tree up, but here it's
> .git/objects, not .git. That also seems to contract this above:
We're breaking through the abstraction provided by POSIX in 'batch' mode,
since the POSIX abstraction does not give us any option to be both safe
and reasonably fast on hardware that does something expensive upon fsync().
We need to ensure that 'x' and 'y' are handed off to the storage device via
a non-flushing pagecache cleaning call (e.g. sync_file_ranges or macOS fsync).
Then creating and flushing 'z' ensures that 'x' and 'y' will be visible after
successful completion. On FSes with a single journal that is flushed on fsync,
this should also ensure that any other files that have been cleaned out of the
pagecache and any other metadata updates are also persisted. The fsync provides
a barrier in addition to the durability.
> > ensure its data is persisted. After all refs have been written,
> > the directories which host refs are flushed.
>
> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for
> now), not .git/objects or .git?
>
> And again, forging on but more generally [continued below]...
>
> > + if (!strcmp(var, "core.fsyncreffiles")) {
>
> UX side: now we've got a core.fsyncRefFiles and
> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those
> work together like say "fsck.*" and "fetch.fsck.*" do, i.e. you'd be
> able to configure this once for objects and refs, or in two variables,
> one for objects, one for refs...
>
I agree with this feedback. We should have a core.fsync flag to control everything
that might be synced in the repo. However, we'd have to decide what we want
to do with packfiles and the index which are currently always fsynced.
> ...[continued from above]: Again, per my potentially wrong understanding
> of syncing a "x" and "y" via an fsync of a subsequent "z" that's
> adjacent on the FS to those two.
I suspect Patrick is concerned about the case where the worktree is on
a separate filesystem from the main repo, so there might be a motivation
to sync the worktree refs separately. Is that right, Patrick?
>
> Isn't this setting us up for a really bad interaction between this
> series and Neeraj's work? Well "bad" as in "bad for performance".
>
> I.e. you'll turn on "use the batch thing for objects and refs" and we'll
> do two fsyncs, one for the object update, and one for refs. The common
> case is that we'll have both in play.
>
> So shouldn't this go to a higher level for both so we only create a "z"
> .git/sync-it-now-please.txt or whatever once we do all pending updates
> on the .git/ directory?
>
> I can also imagine that we'd want that at an even higher level, e.g. for
> "git pull" surely we'd want it not for refs or objects, but in
> builtin/pull.c somewhere because we'll be updating the .git/index after
> we do both refs and objects, and you'd want to fsync at the very end,
> no?
>
> None of the above should mean we can't pursue a more narrow approach for
> now. I'm just:
>
> 1) Seeing if I understand what we're trying to do here, maybe not.
>
> 2) Encouraging you two to think about a holistic way to configure some
> logical conclusion to this topic at large, so we won't end up with
> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,
> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)
>
> I'll send another more generic follow-up E-Mail for #2.
In my view there are two separable concerns:
1) What durability do we want for the user's data when an entire 'git'
command completes? I.e. if I run 'git add <1000 files>; git commit' and the
system loses power after both commands return, should I see all of those files
in the committed state of the repo?
2) What internal consistency do we want in the git databases (ODB and REFS) if
we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes
before returning, can there be an invalid object or a torn ref state?
If were only concerned with (1), then doing a single fsync at the end of the high-level
git command would be sufficient. However, (2) requires more fsyncs to provide barriers
between different phases internal to the git commands. For instance, we need a barrier
between creating the ODB state for a commit (blobs, trees, commit obj) and the refs
pointing to it.
I am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds
each fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not
be user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)
would become apparent to the user.
The more optimal way to handle consistency and durability is Write-ahead-logging with log
replay on crash recovery. That approach can reduce the number of fsyncs in the active workload
to at most two (one to sync the log with a commit record and then one before truncating the
log after updating the main database). At that point, however, I think it would be best to
use an existing database engine with some modifications to create a good data layout for git.
Thanks,
Neeraj
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 19:15 ` Neeraj Singh
@ 2021-11-10 20:23 ` Ævar Arnfjörð Bjarmason
2021-11-11 0:03 ` Neeraj Singh
2021-11-11 12:14 ` Patrick Steinhardt
0 siblings, 2 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-10 20:23 UTC (permalink / raw)
To: Neeraj Singh
Cc: Patrick Steinhardt, git, Jeff King, Johannes Schindelin,
Junio C Hamano, Eric Wong, Neeraj K. Singh, Linus Torvalds
On Wed, Nov 10 2021, Neeraj Singh wrote:
> On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Nov 10 2021, Patrick Steinhardt wrote:
>>
>> > + shutdown. This setting currently only controls loose refs in the object
>> > + store, so updates to packed refs may not be equally durable. Takes the
>> > + same parameters as `core.fsyncObjectFiles`.
>> > +
>>
>> ...my understanding of it is basically a way of going back to what Linus
>> pointed out way back in aafe9fbaf4f (Add config option to enable
>> 'fsync()' of object files, 2008-06-18).
>>
>> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all
>> and the directory entry, but on some FS's it would be sufficient to
>> fsync() just y if they're created in that order. It'll imply an fsync of
>> both x and y, is that accurate?
>>
>> If not you can probably discard the rest, but forging on:
>>
>> Why do we then need to fsync() a "z" file in get_object_directory()
>> (i.e. .git/objects) then? That's a new "z", wasn't flushing "y" enough?
>>
>> Or if you've written .git/objects/x and .git/refs/y I can imagine
>> wanting to create and sync a .git/z if the FS's semantics are to then
>> flush all remaining updates from that tree up, but here it's
>> .git/objects, not .git. That also seems to contract this above:
>
> We're breaking through the abstraction provided by POSIX in 'batch' mode,
> since the POSIX abstraction does not give us any option to be both safe
> and reasonably fast on hardware that does something expensive upon fsync().
>
> We need to ensure that 'x' and 'y' are handed off to the storage device via
> a non-flushing pagecache cleaning call (e.g. sync_file_ranges or macOS fsync).
> Then creating and flushing 'z' ensures that 'x' and 'y' will be visible after
> successful completion. On FSes with a single journal that is flushed on fsync,
> this should also ensure that any other files that have been cleaned out of the
> pagecache and any other metadata updates are also persisted. The fsync provides
> a barrier in addition to the durability.
Yes. I understand that we're not doing POSIX fsyncing().
I'm asking about something else, i.e. with this not-like-POSIX-sync why
it is that when you have a directory:
A
and files:
A/{X,Y}
That you'd write those two, and then proceed to do the "batch flush" by
creating and fsync()-ing a:
B/Z
As opposed to either of:
A/Z
Or:
Z
I.e. why update .git/refs/* and create a flush file in .git/object/* and
not .git/refs/* or .git/*?
Maybe the answer is just that this is WIP code copied from your
.git/objects/ fsync() implementation, or maybe it's more subtle and I'm
missing something.
>> > ensure its data is persisted. After all refs have been written,
>> > the directories which host refs are flushed.
>>
>> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for
>> now), not .git/objects or .git?
>>
>> And again, forging on but more generally [continued below]...
>>
>> > + if (!strcmp(var, "core.fsyncreffiles")) {
>>
>> UX side: now we've got a core.fsyncRefFiles and
>> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those
>> work together like say "fsck.*" and "fetch.fsck.*" do, i.e. you'd be
>> able to configure this once for objects and refs, or in two variables,
>> one for objects, one for refs...
>>
>
> I agree with this feedback. We should have a core.fsync flag to control everything
> that might be synced in the repo. However, we'd have to decide what we want
> to do with packfiles and the index which are currently always fsynced.
*nod*. See this if you haven't yet:
https://lore.kernel.org/git/211110.86r1bogg27.gmgdl@evledraar.gmail.com/T/#u
>> ...[continued from above]: Again, per my potentially wrong understanding
>> of syncing a "x" and "y" via an fsync of a subsequent "z" that's
>> adjacent on the FS to those two.
>
> I suspect Patrick is concerned about the case where the worktree is on
> a separate filesystem from the main repo, so there might be a motivation
> to sync the worktree refs separately. Is that right, Patrick?
But he's syncing .git/objects, not .git/worktrees/<NAME>/refs/, no?
>>
>> Isn't this setting us up for a really bad interaction between this
>> series and Neeraj's work? Well "bad" as in "bad for performance".
>>
>> I.e. you'll turn on "use the batch thing for objects and refs" and we'll
>> do two fsyncs, one for the object update, and one for refs. The common
>> case is that we'll have both in play.
>>
>> So shouldn't this go to a higher level for both so we only create a "z"
>> .git/sync-it-now-please.txt or whatever once we do all pending updates
>> on the .git/ directory?
>>
>> I can also imagine that we'd want that at an even higher level, e.g. for
>> "git pull" surely we'd want it not for refs or objects, but in
>> builtin/pull.c somewhere because we'll be updating the .git/index after
>> we do both refs and objects, and you'd want to fsync at the very end,
>> no?
>>
>> None of the above should mean we can't pursue a more narrow approach for
>> now. I'm just:
>>
>> 1) Seeing if I understand what we're trying to do here, maybe not.
>>
>> 2) Encouraging you two to think about a holistic way to configure some
>> logical conclusion to this topic at large, so we won't end up with
>> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,
>> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)
>>
>> I'll send another more generic follow-up E-Mail for #2.
>
> In my view there are two separable concerns:
>
> 1) What durability do we want for the user's data when an entire 'git'
> command completes? I.e. if I run 'git add <1000 files>; git commit' and the
> system loses power after both commands return, should I see all of those files
> in the committed state of the repo?
>
> 2) What internal consistency do we want in the git databases (ODB and REFS) if
> we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes
> before returning, can there be an invalid object or a torn ref state?
>
> If were only concerned with (1), then doing a single fsync at the end of the high-level
> git command would be sufficient. However, (2) requires more fsyncs to provide barriers
> between different phases internal to the git commands. For instance, we need a barrier
> between creating the ODB state for a commit (blobs, trees, commit obj) and the refs
> pointing to it.
>
> I am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds
> each fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not
> be user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)
> would become apparent to the user.
>
> The more optimal way to handle consistency and durability is Write-ahead-logging with log
> replay on crash recovery. That approach can reduce the number of fsyncs in the active workload
> to at most two (one to sync the log with a commit record and then one before truncating the
> log after updating the main database). At that point, however, I think it would be best to
> use an existing database engine with some modifications to create a good data layout for git.
I think that git should safely store your data by default, we clearly
don't do that well enough in some cases, and should fix it.
But there's also cases where people would much rather have performance,
and I think we should support that. E.g. running git in CI, doing a
one-off import/fetch that you'll invoke "sync(1)" yourself after
etc. This is the direction Eric Wong's patches are going into.
I understand from his patches/comments that you're not correct that just
1-3 fsyncs are OK, i.e. for some use-cases 0 is OK, and any >0 is too
many, since it'll force a flush of the whole disk or something.
Even when git is is operating in a completely safe mode I think we'd
still have license to play it fast and loose with /some/ user data,
because users don't really care about all of their data in the .git/
directory.
I.e. you do care about the *.pack file, but an associated *.bitmap is a
derived file, so we probably don't need to fsync that too. Is it worth
worrying about? Probably not, but that's what I had in mind with the
above-linked proposed config schema.
Similarly for say writing 1k loose objects I think it would be
completely fine to end up with a corrupt object during a "git fetch", as
long as we also guarantee that we can gracefully recover from that
corruption.
I.e. 1) we didn't update the ref(s) involved 2) we know the FS has the
sorts of properties you're aiming for with "batch".
Now, as some patches I've had to object*.c recently show we don't handle
those cases gracefully either. I.e. if we find a short loose object
we'll panic, even if we'd be perfectly capable of getting it from
elsewhere, and nothing otherwise references it.
But if we fixed that and gc/fsck/fetch etc. learned to deal with that
content I don't see why wouldn't e.g. default to not fsyncing loose
objects when invoked from say "fetch" by default, depending on FS/OS
detection, and if we couldn't say it was safe defaulted to some "POSIX"
mode that would be pedantic but slow and safe.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 20:23 ` Ævar Arnfjörð Bjarmason
@ 2021-11-11 0:03 ` Neeraj Singh
2021-11-11 12:14 ` Patrick Steinhardt
1 sibling, 0 replies; 34+ messages in thread
From: Neeraj Singh @ 2021-11-11 0:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Patrick Steinhardt, git, Jeff King, Johannes Schindelin,
Junio C Hamano, Eric Wong, Neeraj K. Singh, Linus Torvalds
On Wed, Nov 10, 2021 at 09:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>
> Yes. I understand that we're not doing POSIX fsyncing().
>
> I'm asking about something else, i.e. with this not-like-POSIX-sync why
> it is that when you have a directory:
>
> A
>
> and files:
>
> A/{X,Y}
>
> That you'd write those two, and then proceed to do the "batch flush" by
> creating and fsync()-ing a:
>
> B/Z
>
> As opposed to either of:
>
> A/Z
>
> Or:
>
> Z
>
> I.e. why update .git/refs/* and create a flush file in .git/object/* and
> not .git/refs/* or .git/*?
>
> Maybe the answer is just that this is WIP code copied from your
> .git/objects/ fsync() implementation, or maybe it's more subtle and I'm
> missing something.
It looks I didn't really answer your actual question before. On the filesystems
which I'm familiar with at a code level (NTFS and ReFS on Windows), fsyncing a file
or dir anywhere on the filesystem should ensure that metadata operations completed
before the fsync starts are durable when the fsync returns. So which specific directory
we put the file in shouldn't matter as long as it's on the same filesystem as the other
files we're interested in.
>
> *nod*. See this if you haven't yet:
> https://lore.kernel.org/git/211110.86r1bogg27.gmgdl@evledraar.gmail.com/T/#u
I'll respond on that thread with my opinion. Thanks for reviewing this and pushing
for a good holistic approach.
Thanks,
Neeraj
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 20:23 ` Ævar Arnfjörð Bjarmason
2021-11-11 0:03 ` Neeraj Singh
@ 2021-11-11 12:14 ` Patrick Steinhardt
1 sibling, 0 replies; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-11 12:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Neeraj Singh, git, Jeff King, Johannes Schindelin,
Junio C Hamano, Eric Wong, Neeraj K. Singh, Linus Torvalds
[-- Attachment #1: Type: text/plain, Size: 5709 bytes --]
On Wed, Nov 10, 2021 at 09:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Nov 10 2021, Neeraj Singh wrote:
>
> > On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Wed, Nov 10 2021, Patrick Steinhardt wrote:
[snip]
> >> ...[continued from above]: Again, per my potentially wrong understanding
> >> of syncing a "x" and "y" via an fsync of a subsequent "z" that's
> >> adjacent on the FS to those two.
> >
> > I suspect Patrick is concerned about the case where the worktree is on
> > a separate filesystem from the main repo, so there might be a motivation
> > to sync the worktree refs separately. Is that right, Patrick?
>
> But he's syncing .git/objects, not .git/worktrees/<NAME>/refs/, no?
That'd be a bug then ;) My intent was to sync .git/refs and the
per-worktree refs.
[snip]
> > In my view there are two separable concerns:
> >
> > 1) What durability do we want for the user's data when an entire 'git'
> > command completes? I.e. if I run 'git add <1000 files>; git commit' and the
> > system loses power after both commands return, should I see all of those files
> > in the committed state of the repo?
> >
> > 2) What internal consistency do we want in the git databases (ODB and REFS) if
> > we crash midway through a 'git' command? I.e. if 'git add <1000 files>' crashes
> > before returning, can there be an invalid object or a torn ref state?
> >
> > If were only concerned with (1), then doing a single fsync at the end of the high-level
> > git command would be sufficient. However, (2) requires more fsyncs to provide barriers
> > between different phases internal to the git commands. For instance, we need a barrier
> > between creating the ODB state for a commit (blobs, trees, commit obj) and the refs
> > pointing to it.
> >
> > I am not concerned with a few additional fsyncs for (2). On recent mainstream filesystems/ssds
> > each fsync may cost tens of milliseconds, so the difference between 1 to 3 fsyncs would not
> > be user perceptible. However, going from a couple fsyncs to 1000 fsyncs (one per blob added)
> > would become apparent to the user.
> >
> > The more optimal way to handle consistency and durability is Write-ahead-logging with log
> > replay on crash recovery. That approach can reduce the number of fsyncs in the active workload
> > to at most two (one to sync the log with a commit record and then one before truncating the
> > log after updating the main database). At that point, however, I think it would be best to
> > use an existing database engine with some modifications to create a good data layout for git.
>
> I think that git should safely store your data by default, we clearly
> don't do that well enough in some cases, and should fix it.
>
> But there's also cases where people would much rather have performance,
> and I think we should support that. E.g. running git in CI, doing a
> one-off import/fetch that you'll invoke "sync(1)" yourself after
> etc. This is the direction Eric Wong's patches are going into.
>
> I understand from his patches/comments that you're not correct that just
> 1-3 fsyncs are OK, i.e. for some use-cases 0 is OK, and any >0 is too
> many, since it'll force a flush of the whole disk or something.
>
> Even when git is is operating in a completely safe mode I think we'd
> still have license to play it fast and loose with /some/ user data,
> because users don't really care about all of their data in the .git/
> directory.
I think we need to discern two important cases: the first case is us
losing data, and the second case is us leaving the repository in a
corrupt state. I'm okay-ish with the first case: if your machine crashes
it's not completely unexpected that losing data would be a natural
consequence (well, losing the data that's currently in transit at least).
But we should make sure that we're not leaving the repo in a corrupt
state under any circumstance, and that's exactly what can happen right
now because we don't flush refs to disk before doing the renames.
Assuming we've got semantics correct, we are thus forced to do a page
cache flush to make sure that data is on disk before doing a rename to
not corrupt the repo. But in the case where we're fine with losing some
data, we may skip doing the final fsync to also synchronize the rename
to disk.
Patrick
> I.e. you do care about the *.pack file, but an associated *.bitmap is a
> derived file, so we probably don't need to fsync that too. Is it worth
> worrying about? Probably not, but that's what I had in mind with the
> above-linked proposed config schema.
>
> Similarly for say writing 1k loose objects I think it would be
> completely fine to end up with a corrupt object during a "git fetch", as
> long as we also guarantee that we can gracefully recover from that
> corruption.
>
> I.e. 1) we didn't update the ref(s) involved 2) we know the FS has the
> sorts of properties you're aiming for with "batch".
>
> Now, as some patches I've had to object*.c recently show we don't handle
> those cases gracefully either. I.e. if we find a short loose object
> we'll panic, even if we'd be perfectly capable of getting it from
> elsewhere, and nothing otherwise references it.
>
> But if we fixed that and gc/fsck/fetch etc. learned to deal with that
> content I don't see why wouldn't e.g. default to not fsyncing loose
> objects when invoked from say "fetch" by default, depending on FS/OS
> detection, and if we couldn't say it was safe defaulted to some "POSIX"
> mode that would be pedantic but slow and safe.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 14:49 ` Ævar Arnfjörð Bjarmason
2021-11-10 19:15 ` Neeraj Singh
@ 2021-11-11 12:06 ` Patrick Steinhardt
1 sibling, 0 replies; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-11 12:06 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Johannes Schindelin, Junio C Hamano, Eric Wong,
Neeraj K. Singh, Linus Torvalds
[-- Attachment #1: Type: text/plain, Size: 7705 bytes --]
On Wed, Nov 10, 2021 at 03:49:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Nov 10 2021, Patrick Steinhardt wrote:
>
> > [...]
> > Fix this by introducing a new configuration "core.fsyncRefFiles". This
> > config matches behaviour of "core.fsyncObjectFiles" in that it provides
> > three different modes:
> >
> > - "off" disables calling fsync on ref files. This is the default
> > behaviour previous to this change and remains the default after
> > this change.
> >
> > - "on" enables calling fsync on ref files, where each reference is
> > flushed to disk before it is being committed. This is the safest
> > setting, but may incur significant performance overhead.
> >
> > - "batch" will flush the page cache of each file as it is written to
> > ensure its data is persisted. After all refs have been written,
> > the directories which host refs are flushed.
> >
> > With this change in place and when "core.fsyncRefFiles" is set to either
> > "on" or "batch", this kind of corruption shouldn't happen anymore.
> >
> > [1]: https://www.kernel.org/doc/Documentation/filesystems/ext4.txt
>
> With the understanding that my grokking of this approach is still
> somewhere between "uh, that works?" and "wow, voodoo FS magic!". ....
>
> I haven't looked at these changes in much daiter, or Neeraj's recent
> related changes but...
>
> > +core.fsyncRefFiles::
> > + A value indicating the level of effort Git will expend in trying to make
> > + refs added to the repo durable in the event of an unclean system
> > + shutdown. This setting currently only controls loose refs in the object
> > + store, so updates to packed refs may not be equally durable. Takes the
> > + same parameters as `core.fsyncObjectFiles`.
> > +
>
> ...my understanding of it is basically a way of going back to what Linus
> pointed out way back in aafe9fbaf4f (Add config option to enable
> 'fsync()' of object files, 2008-06-18).
>
> I.e. we've got files x and y. POSIX sayeth we'd need to fsync them all
> and the directory entry, but on some FS's it would be sufficient to
> fsync() just y if they're created in that order. It'll imply an fsync of
> both x and y, is that accurate?
>
> If not you can probably discard the rest, but forging on:
>
> Why do we then need to fsync() a "z" file in get_object_directory()
> (i.e. .git/objects) then? That's a new "z", wasn't flushing "y" enough?
>
> Or if you've written .git/objects/x and .git/refs/y I can imagine
> wanting to create and sync a .git/z if the FS's semantics are to then
> flush all remaining updates from that tree up, but here it's
> .git/objects, not .git. That also seems to contract this above:
>
> > ensure its data is persisted. After all refs have been written,
> > the directories which host refs are flushed.
>
> I.e. that would be .git/refs (let's ignore .git/HEAD and the like for
> now), not .git/objects or .git?
Yeah, .git/refs. Note that we need to flush refs in two locations though
given that there are common refs shared across worktrees, and then there
are worktree-specific refs. These may not even share the filesystem, so
there's not much we can do about this. We could play games with the
fsid, but I'm not sure it's worth it.
> And again, forging on but more generally [continued below]...
>
> > + if (!strcmp(var, "core.fsyncreffiles")) {
>
> UX side: now we've got a core.fsyncRefFiles and
> core.fsyncWhateverItWasCalled in Neeraj series. Let's make sure those
> work together like say "fsck.*" and "fetch.fsck.*" do, i.e. you'd be
> able to configure this once for objects and refs, or in two variables,
> one for objects, one for refs...
Honestly, I'd prefer to just have a global variable with per-subsystem
overrides so that you can say "Please batch-sync all files by default,
but I really don't care for files in the worktree". Kind of goes into
the same direction as your RFC.
[snip]
> > @@ -2665,7 +2719,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> > files_downcast(ref_store, REF_STORE_WRITE,
> > "ref_transaction_prepare");
> > size_t i;
> > - int ret = 0;
> > + int ret = 0, sync_flags = 0;
> > struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> > char *head_ref = NULL;
> > int head_type;
> > @@ -2777,8 +2831,14 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> > &update->new_oid, NULL,
> > NULL);
> > }
> > +
> > + sync_flags |= sync_loose_refs_flags(update->refname);
> > }
> >
> > + ret = sync_loose_refs(refs, sync_flags, err);
> > + if (ret)
> > + goto cleanup;
> > +
> > if (packed_transaction) {
> > if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> > ret = TRANSACTION_GENERIC_ERROR;
>
> ...[continued from above]: Again, per my potentially wrong understanding
> of syncing a "x" and "y" via an fsync of a subsequent "z" that's
> adjacent on the FS to those two.
>
> Isn't this setting us up for a really bad interaction between this
> series and Neeraj's work? Well "bad" as in "bad for performance".
>
> I.e. you'll turn on "use the batch thing for objects and refs" and we'll
> do two fsyncs, one for the object update, and one for refs. The common
> case is that we'll have both in play.
>
> So shouldn't this go to a higher level for both so we only create a "z"
> .git/sync-it-now-please.txt or whatever once we do all pending updates
> on the .git/ directory?
Good question. Taking a different stance would be to say "I don't ever
want to write a ref referring to an object which isn't yet guaranteed to
be persistent". In that case, writing and syncing objects first and then
updating refs would be the correct thing to do and wouldn't need
coordination on a higher level.
> I can also imagine that we'd want that at an even higher level, e.g. for
> "git pull" surely we'd want it not for refs or objects, but in
> builtin/pull.c somewhere because we'll be updating the .git/index after
> we do both refs and objects, and you'd want to fsync at the very end,
> no?
>
> None of the above should mean we can't pursue a more narrow approach for
> now. I'm just:
>
> 1) Seeing if I understand what we're trying to do here, maybe not.
Personally, my most important bullet point is that I want to get rid of
the corrupt refs we see in production every now and then. As Peff
pointed out in another email, it _might_ be sufficient to just flush out
the page cache for those without synchronizing any metadata at all. But
none of us seems to be sure that this really works the way we think it
does.
> 2) Encouraging you two to think about a holistic way to configure some
> logical conclusion to this topic at large, so we won't end up with
> core.fsyncConfigFiles, core.fsyncObjectFiles, core.fsyncIndexFile,
> core.fsyncRefFiles, core.fsyncTheCrapRebaseWritesOutFiles etc. :)
Yeah, I agree with you on this point. We definitely don't want to end up
with twenty different knobs to sync objects, refs, packed-refs, configs,
reflogs et al, and we're currently steering into that direction. Having
a central knob core.fsync which directs defaults for all subsystems
would be very much preferable if you ask me, where specific subsystems
can then be changed as the user sees fit.
What I think is important though is that we should have the right
defaults in place. It doesn't help to say "fsync is slow" as an excuse
to not do them at all by default, where the result is that users get a
corrupt repository.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 3/3] refs: add configuration to enable flushing of refs
2021-11-10 11:41 ` [PATCH v2 3/3] refs: add configuration to enable flushing of refs Patrick Steinhardt
2021-11-10 14:49 ` Ævar Arnfjörð Bjarmason
@ 2021-11-11 0:18 ` Neeraj Singh
1 sibling, 0 replies; 34+ messages in thread
From: Neeraj Singh @ 2021-11-11 0:18 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Eric Wong, Neeraj K. Singh
On Wed, Nov 10, 2021 at 12:41:03PM +0100, Patrick Steinhardt wrote:
>
> +static int sync_loose_ref(int fd)
> +{
> + switch (fsync_ref_files) {
> + case FSYNC_REF_FILES_OFF:
> + return 0;
> + case FSYNC_REF_FILES_ON:
> + return git_fsync(fd, FSYNC_HARDWARE_FLUSH);
> + case FSYNC_REF_FILES_BATCH:
> + return git_fsync(fd, FSYNC_WRITEOUT_ONLY);
> + default:
> + BUG("invalid fsync mode %d", fsync_ref_files);
> + }
> +}
> +
> +#define SYNC_LOOSE_REF_GITDIR (1 << 0)
> +#define SYNC_LOOSE_REF_COMMONDIR (1 << 1)
> +
> +static int sync_loose_refs_flags(const char *refname)
> +{
> + switch (ref_type(refname)) {
> + case REF_TYPE_PER_WORKTREE:
> + case REF_TYPE_PSEUDOREF:
> + return SYNC_LOOSE_REF_GITDIR;
> + case REF_TYPE_MAIN_PSEUDOREF:
> + case REF_TYPE_OTHER_PSEUDOREF:
> + case REF_TYPE_NORMAL:
> + return SYNC_LOOSE_REF_COMMONDIR;
> + default:
> + BUG("unknown ref type %d of ref %s",
> + ref_type(refname), refname);
> + }
> +}
> +
> +static int sync_loose_refs(struct files_ref_store *refs,
> + int flags,
> + struct strbuf *err)
> +{
> + if (fsync_ref_files != FSYNC_REF_FILES_BATCH)
> + return 0;
> +
> + if ((flags & SYNC_LOOSE_REF_GITDIR) &&
> + git_fsync_dir(refs->base.gitdir) < 0)
> + return -1;
> +
> + if ((flags & SYNC_LOOSE_REF_COMMONDIR) &&
> + git_fsync_dir(refs->gitcommondir) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
Now that I understand it better, I agree with Ævar's feedback. I think
only a single sync is needed to cover everything in .git/.
I'd also suggest renaming 'sync_loose_refs' to something which makes
it clearer that it's the batch-flush step. Maybe s/do_batch_fsync/do_batch_objects_fsync
and s/sync_loose_refs/do_batch_refs_fsync.
Thanks
Neeraj
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them
2021-11-10 11:40 ` [PATCH v2 0/3] " Patrick Steinhardt
` (2 preceding siblings ...)
2021-11-10 11:41 ` [PATCH v2 3/3] refs: add configuration to enable flushing of refs Patrick Steinhardt
@ 2021-11-10 14:44 ` Johannes Schindelin
2021-11-10 20:45 ` Jeff King
4 siblings, 0 replies; 34+ messages in thread
From: Johannes Schindelin @ 2021-11-10 14:44 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Wong, Neeraj K. Singh
Hi Patrick,
On Wed, 10 Nov 2021, Patrick Steinhardt wrote:
> This series is implements the same batching as Neeraj's series [1]. It
> introduces a new config switch "core.fsyncRefFiles" with the same three
> toggles "on", "off" and "batch". Given that it reuses funcitonality
> provided by the batched object flushing this series is based on "next".
Excellent, I am glad that my idea was helpful.
I read through the patches and found them easy to follow. Even 3/3, which
was a bit complex because of the need to support both transacted ref
updates as well as atomic renames/copies, is clear and well-written.
FWIW you could base your patches directly on `ns/batched-fsync`, I would
think.
Thank you,
Dscho
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them
2021-11-10 11:40 ` [PATCH v2 0/3] " Patrick Steinhardt
` (3 preceding siblings ...)
2021-11-10 14:44 ` [PATCH v2 0/3] refs: sync loose refs to disk before committing them Johannes Schindelin
@ 2021-11-10 20:45 ` Jeff King
2021-11-11 11:47 ` Patrick Steinhardt
4 siblings, 1 reply; 34+ messages in thread
From: Jeff King @ 2021-11-10 20:45 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Johannes Schindelin, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Wong, Neeraj K. Singh
On Wed, Nov 10, 2021 at 12:40:50PM +0100, Patrick Steinhardt wrote:
> Please note that I didn't yet add any performance numbers or tests.
> Performance tests didn't show any conclusive results on my machine given
> that I couldn't observe any noticeable impact at all, and I didn't write
> tests yet given that I first wanted to get some feedback on this series.
> If we agree that this is the correct way to go forward I'll of course
> put in some more time to add them.
Here's a mild adjustment of the quick test I showed earlier in [1]. It
uses your version for all cases, but swaps between the three config
options, and it uses --atomic to put everything into a single
transaction:
$ hyperfine -L v false,true,batch -p ./setup 'git -C dst.git config core.fsyncreffiles {v}; git.compile push --atomic dst.git refs/heads/*'
Benchmark 1: git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*
Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 7.8 ms, System: 3.9 ms]
Range (min … max): 10.1 ms … 11.0 ms 108 runs
Benchmark 2: git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*
Time (mean ± σ): 402.5 ms ± 6.2 ms [User: 13.9 ms, System: 10.7 ms]
Range (min … max): 387.3 ms … 408.9 ms 10 runs
Benchmark 3: git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*
Time (mean ± σ): 21.4 ms ± 0.6 ms [User: 8.0 ms, System: 5.2 ms]
Range (min … max): 20.7 ms … 24.9 ms 99 runs
Summary
'git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*' ran
2.05 ± 0.07 times faster than 'git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*'
38.51 ± 1.06 times faster than 'git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*'
So yes, the "batch" case is much improved. It's still more expensive
than skipping the fsync entirely, but it's within reason. The trick,
though is the --atomic. If I drop that flag, then "batch" takes as long
as "true". And of course that's the default.
I wonder if that means we'd want an extra mode: do the WRITEOUT_ONLY
flush, but _don't_ do a HARDWARE_FLUSH for each transaction. That
doesn't give you durability, but it (in theory) solves the out-of-order
journal problem without paying any performance cost.
I say "in theory" because I'm just assuming that the WRITEOUT thing
works as advertised. I don't have an easy way to test the outcome on a
power failure at the right moment here.
-Peff
[1] https://lore.kernel.org/git/YYTaiIlEKxHRVdCy@coredump.intra.peff.net/
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2 0/3] refs: sync loose refs to disk before committing them
2021-11-10 20:45 ` Jeff King
@ 2021-11-11 11:47 ` Patrick Steinhardt
0 siblings, 0 replies; 34+ messages in thread
From: Patrick Steinhardt @ 2021-11-11 11:47 UTC (permalink / raw)
To: Jeff King
Cc: git, Johannes Schindelin, Ævar Arnfjörð Bjarmason,
Junio C Hamano, Eric Wong, Neeraj K. Singh
[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]
On Wed, Nov 10, 2021 at 03:45:11PM -0500, Jeff King wrote:
> On Wed, Nov 10, 2021 at 12:40:50PM +0100, Patrick Steinhardt wrote:
>
> > Please note that I didn't yet add any performance numbers or tests.
> > Performance tests didn't show any conclusive results on my machine given
> > that I couldn't observe any noticeable impact at all, and I didn't write
> > tests yet given that I first wanted to get some feedback on this series.
> > If we agree that this is the correct way to go forward I'll of course
> > put in some more time to add them.
>
> Here's a mild adjustment of the quick test I showed earlier in [1]. It
> uses your version for all cases, but swaps between the three config
> options, and it uses --atomic to put everything into a single
> transaction:
>
> $ hyperfine -L v false,true,batch -p ./setup 'git -C dst.git config core.fsyncreffiles {v}; git.compile push --atomic dst.git refs/heads/*'
> Benchmark 1: git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*
> Time (mean ± σ): 10.5 ms ± 0.2 ms [User: 7.8 ms, System: 3.9 ms]
> Range (min … max): 10.1 ms … 11.0 ms 108 runs
>
> Benchmark 2: git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*
> Time (mean ± σ): 402.5 ms ± 6.2 ms [User: 13.9 ms, System: 10.7 ms]
> Range (min … max): 387.3 ms … 408.9 ms 10 runs
>
> Benchmark 3: git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*
> Time (mean ± σ): 21.4 ms ± 0.6 ms [User: 8.0 ms, System: 5.2 ms]
> Range (min … max): 20.7 ms … 24.9 ms 99 runs
>
> Summary
> 'git -C dst.git config core.fsyncreffiles false; git.compile push --atomic dst.git refs/heads/*' ran
> 2.05 ± 0.07 times faster than 'git -C dst.git config core.fsyncreffiles batch; git.compile push --atomic dst.git refs/heads/*'
> 38.51 ± 1.06 times faster than 'git -C dst.git config core.fsyncreffiles true; git.compile push --atomic dst.git refs/heads/*'
>
> So yes, the "batch" case is much improved. It's still more expensive
> than skipping the fsync entirely, but it's within reason. The trick,
> though is the --atomic. If I drop that flag, then "batch" takes as long
> as "true". And of course that's the default.
Great, thanks for doing these tests.
> I wonder if that means we'd want an extra mode: do the WRITEOUT_ONLY
> flush, but _don't_ do a HARDWARE_FLUSH for each transaction. That
> doesn't give you durability, but it (in theory) solves the out-of-order
> journal problem without paying any performance cost.
>
> I say "in theory" because I'm just assuming that the WRITEOUT thing
> works as advertised. I don't have an easy way to test the outcome on a
> power failure at the right moment here.
>
> -Peff
>
> [1] https://lore.kernel.org/git/YYTaiIlEKxHRVdCy@coredump.intra.peff.net/
Yeah, it's also one of the things I'm currently wondering about. I just
piggy-backed on the preexisting patch series which claim that it at
least ought to work on macOS and Windows, but left out of the equation
were Linux filesystems. I'm hoping that we'll get an answer to this
question via <211111.86pmr7pk9o.gmgdl@evledraar.gmail.com>, where Ævar
put Linus and Christopher Hellwig on Cc.
In any case, if the outcome of a WRITEOUT_ONLY flush would be that we
may not see all renames, but that every refs' contents would be
well-defined, then this is a tradeoff we'd probably want to make at
GitLab.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread