| Message ID | 3ac9d4d7abd224a4c0991f1036f2d95eedb9ceac.1636544377.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: sync loose refs to disk before committing them | expand |
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().
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;
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(-)