Re: [PATCH v3 2/6] core.fsyncobjectfiles: batched disk flushes
From: Junio C Hamano <hidden>
Date: 2021-09-14 19:34:21
"Neeraj Singh via GitGitGadget" [off-list ref] writes:
quoted hunk
diff --git a/config.c b/config.c index cb4a8058bff..9fe3602e1c4 100644 --- a/config.c +++ b/config.c@@ -1509,7 +1509,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.fsyncobjectfiles")) { - fsync_object_files = git_config_bool(var, value); + if (!value) + return config_error_nonbool(var); + if (!strcasecmp(value, "batch")) + fsync_object_files = FSYNC_OBJECT_FILES_BATCH; + else + fsync_object_files = git_config_bool(var, value) + ? FSYNC_OBJECT_FILES_ON : FSYNC_OBJECT_FILES_OFF; return 0;
The original code used to allow the short-and-sweet valueless true [core] fsyncobjectfiles but it no longer does by calling it a nonbool error. This breaks existing users' repositories that have been happily working, doesn't it? Perhaps if (value && !strcmp(value, "batch")) fsync_object_files = FSYNC_OBJECT_FILES_BATCH; else if (git_config_bool(var, value)) fsync_object_files = FSYNC_OBJECT_FILES_ON; else fsync_object_files = FSYNC_OBJECT_FILES_OFF;
quoted hunk
-/* Finalize a file on disk, and close it. */ -static void close_loose_object(int fd) -{ - if (fsync_object_files) - fsync_or_die(fd, "loose object file"); - if (close(fd) != 0) - die_errno(_("error when closing loose object file")); -} - /* Size of directory component, including the ending '/' */ static inline int directory_size(const char *filename) {@@ -1973,17 +1964,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr, die(_("confused by unstable object source data for %s"), oid_to_hex(oid)); - close_loose_object(fd); - - if (mtime) { - struct utimbuf utb; - utb.actime = mtime; - utb.modtime = mtime; - if (utime(tmp_file.buf, &utb) < 0) - warning_errno(_("failed utime() on %s"), tmp_file.buf); - } - - return finalize_object_file(tmp_file.buf, filename.buf); + return fsync_and_close_loose_object_bulk_checkin(fd, tmp_file.buf, + filename.buf, mtime); }
This block of code looked familiar and I was about to complain "why add it in one step and remove it in another?" But it is a different instance from the one that was added in one of the previous patches ;-).
quoted hunk
+int git_fsync(int fd, enum fsync_action action) +{ + if (action == FSYNC_WRITEOUT_ONLY) { +#ifdef __APPLE__ + /* + * on Mac OS X, fsync just causes filesystem cache writeback but does not + * flush hardware caches. + */ + return fsync(fd); +#endif + +#ifdef HAVE_SYNC_FILE_RANGE + /* + * On linux 2.6.17 and above, sync_file_range is the way to issue + * a writeback without a hardware flush. An offset of 0 and size of 0 + * indicates writeout of the entire file and the wait flags ensure that all + * dirty data is written to the disk (potentially in a disk-side cache) + * before we continue. + */ + + return sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WAIT_BEFORE | + SYNC_FILE_RANGE_WRITE | + SYNC_FILE_RANGE_WAIT_AFTER); +#endif + + errno = ENOSYS; + return -1; + }
This allows the caller that can take advantage of writeout-only mode to naturally fall back on the full sync per each file if we cannot do a writeout-only sync. OK.
quoted hunk
+#ifdef __APPLE__ + return fcntl(fd, F_FULLFSYNC); +#else + return fsync(fd); +#endif +}
If we are introducing "enum fsync_action", we should have some way
to make it clear that we are covering all the possible values of
"action".
Switching on action, i.e.
switch (action) {
case FSYNC_WRITEOUT_ONLY:
...
break;
case FSYNC_HARDWARE_FLUSH:
...
break;
default:
BUG("unexpected git_fsync(%d) call", action);
}
would be one way to do so.
Thanks.