"Neeraj Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> +* `batch` enables a mode that uses writeout-only flushes to stage multiple
> + updates in the disk writeback cache and then does a single full fsync of
> + a dummy file to trigger the disk cache flush at the end of the operation.
It is unfortunate that we have a rather independent "unplug" that is
not tied to the "this is the last operation in the batch"---if there
were we didn't have to invent a dummy but a single full sync on the
real file who happened to be the last one in the batch would be
sufficient. It would not matter, if the batch is any meaningful
size, hopefully.
> +/*
> + * Cleanup after batch-mode fsync_object_files.
> + */
> +static void do_batch_fsync(void)
> +{
> + /*
> + * Issue a full hardware flush against a temporary file to ensure
> + * that all objects are durable before any renames occur. The code in
> + * fsync_loose_object_bulk_checkin has already issued a writeout
> + * request, but it has not flushed any writeback cache in the storage
> + * 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);
> + needs_batch_fsync = 0;
> + }
> +
> + if (bulk_fsync_objdir) {
> + tmp_objdir_migrate(bulk_fsync_objdir);
> + bulk_fsync_objdir = NULL;
The struct obtained from tmp_objdir_create() is consumed by
tmp_objdir_migrate() so the only clean-up left for the caller to do
is to clear it to NULL. OK.
> + }
This initially made me wonder why we need two independent flags.
After applying this patch but not any later steps, upon plugging, we
create the tentative object directory, and any loose object will be
created there, but because nobody calls the writeout-only variant
via fsync_loose_object_bulk_checkin() yet, needs_batch_fsync may not
be turned on. But even in that case, any new loose objects are in
the tentative object directory and need to be migrated to the real
place.
And we may not cover all the existing code paths at the end of the
series, or any new code paths right away after they get introduced,
to be aware of the fsync_loose_object_bulk_checkin() when they
create a loose object file, so it is most likely that these two if
statements will be with us forever.
OK.
> @@ -274,6 +311,24 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
> return 0;
> }
>
> +void fsync_loose_object_bulk_checkin(int fd)
> +{
> + /*
> + * If we have a plugged bulk checkin, we issue a call that
> + * cleans the filesystem page cache but avoids a hardware flush
> + * command. Later on we will issue a single hardware flush
> + * before as part of do_batch_fsync.
> + */
> + if (bulk_checkin_plugged &&
> + git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
> + assert(bulk_fsync_objdir);
> + if (!needs_batch_fsync)
> + needs_batch_fsync = 1;
Except for when we unplug, do we ever flip needs_batch_fsync bit
off, once it is set? If the answer is no, wouldn't it be clearer to
unconditionally set it, instead of "set it only for the first time"?
> + } else {
> + fsync_or_die(fd, "loose object file");
> + }
> +}
> +
> int index_bulk_checkin(struct object_id *oid,
> int fd, size_t size, enum object_type type,
> const char *path, unsigned flags)
> @@ -288,6 +343,19 @@ int index_bulk_checkin(struct object_id *oid,
> void plug_bulk_checkin(void)
> {
> assert(!bulk_checkin_plugged);
> +
> + /*
> + * A temporary object directory is used to hold the files
> + * while they are not fsynced.
> + */
> + if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT)) {
> + bulk_fsync_objdir = tmp_objdir_create("bulk-fsync");
> + if (!bulk_fsync_objdir)
> + die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch"));
> +
> + tmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);
> + }
> +
> bulk_checkin_plugged = 1;
> }
>
> @@ -297,4 +365,6 @@ void unplug_bulk_checkin(void)
> bulk_checkin_plugged = 0;
> if (bulk_checkin_state.f)
> finish_bulk_checkin(&bulk_checkin_state);
> +
> + do_batch_fsync();
> }
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index b26f3dc3b74..08f292379b6 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -6,6 +6,8 @@
>
> #include "cache.h"
>
> +void fsync_loose_object_bulk_checkin(int fd);
> +
> int index_bulk_checkin(struct object_id *oid,
> int fd, size_t size, enum object_type type,
> const char *path, unsigned flags);
> diff --git a/cache.h b/cache.h
> index 3160bc1e489..d1ae51388c9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1040,7 +1040,8 @@ extern int use_fsync;
>
> enum fsync_method {
> FSYNC_METHOD_FSYNC,
> - FSYNC_METHOD_WRITEOUT_ONLY
> + FSYNC_METHOD_WRITEOUT_ONLY,
> + FSYNC_METHOD_BATCH
> };
Style.
These days we allow trailing comma to enum definitions. Perhaps
give a trailing comma after _BATCH so that the next update patch
will become less noisy?
Thanks.
