On Wed, Mar 30, 2022 at 10:37 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> > index 9da3e5d88f6..3c90ba0b395 100644
> > --- a/Documentation/config/core.txt
> > +++ b/Documentation/config/core.txt
> > @@ -596,6 +596,14 @@ core.fsyncMethod::
> > * `writeout-only` issues pagecache writeback requests, but depending on the
> > filesystem and storage hardware, data added to the repository may not be
> > durable in the event of a system crash. This is the default mode on macOS.
> > +* `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.
> > ++
> > + Currently `batch` mode only applies to loose-object files. Other repository
> > + data is made durable as if `fsync` was specified. This mode is expected to
> > + be as safe as `fsync` on macOS for repos stored on HFS+ or APFS filesystems
> > + and on Windows for repos stored on NTFS or ReFS filesystems.
>
> Does this format correctly? I had an impression that the second and
> subsequent paragraphs, connected with a line with a single "+" on
> it, has to be flushed left without indentation.
>
Here's the man format (Not sure how this will render going through gmail):
• 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.
Currently `batch` mode only applies to loose-object
files. Other repository
data is made durable as if `fsync` was specified.
This mode is expected to
be as safe as `fsync` on macOS for repos stored on
HFS+ or APFS filesystems
and on Windows for repos stored on NTFS or ReFS filesystems.
To describe the above if it doesn't render correctly, we have a
bulleted list where the batch after the * is bolded. Other instances
of single backtick quoted text just appears as plaintext. The
descriptive "Currently `batch` mode..." paragraph is under the bullet
point and well-indented.
In HTML the output looks good as well, except that the descriptive
paragraph is in monospace for some reason.
> > diff --git a/bulk-checkin.c b/bulk-checkin.c
> > index 8b0fd5c7723..9799d247cad 100644
> > --- a/bulk-checkin.c
> > +++ b/bulk-checkin.c
> > @@ -3,15 +3,20 @@
> > */
> > #include "cache.h"
> > #include "bulk-checkin.h"
> > +#include "lockfile.h"
> > #include "repository.h"
> > #include "csum-file.h"
> > #include "pack.h"
> > #include "strbuf.h"
> > +#include "string-list.h"
> > +#include "tmp-objdir.h"
> > #include "packfile.h"
> > #include "object-store.h"
> >
> > static int odb_transaction_nesting;
> >
> > +static struct tmp_objdir *bulk_fsync_objdir;
>
> I wonder if this should be added to the bulk_checkin_state structure
> as a new member, especially if we fix the erroneous call to
> finish_bulk_checkin() as a preliminary fix-up of a bug that existed
> even before this series.
>
It seems like the only thing tying this to the bulk_checkin_state
(which I've renamed in my local changes to bulk_checkin_packfile) is
that they're both generally written when a transaction is active.
Keeping fsync separate from packfile should help the reader see that
the two sets of functions access only their respective global state.
If we add another optimization strategy (e.g. appendable pack files),
it would get its own separate state and functions that are independent
of the large-blob packfile and loose-object fsync optimizations.
> > +/*
> > + * Cleanup after batch-mode fsync_object_files.
> > + */
> > +static void do_batch_fsync(void)
> > +{
> > + struct strbuf temp_path = STRBUF_INIT;
> > + struct tempfile *temp;
> > +
> > + if (!bulk_fsync_objdir)
> > + return;
> > +
> > + /*
> > + * 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 or any filesystem logs. This fsync call acts as a barrier
> > + * to ensure that the data in each new object file is durable before
> > + * the final name is visible.
> > + */
> > + 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);
> > +
> > + /*
> > + * Make the object files visible in the primary ODB after their data is
> > + * fully durable.
> > + */
> > + tmp_objdir_migrate(bulk_fsync_objdir);
> > + bulk_fsync_objdir = NULL;
> > +}
>
> OK.
>
> > +void prepare_loose_object_bulk_checkin(void)
> > +{
> > + /*
> > + * We lazily create the temporary object directory
> > + * the first time an object might be added, since
> > + * callers may not know whether any objects will be
> > + * added at the time they call begin_odb_transaction.
> > + */
> > + if (!odb_transaction_nesting || bulk_fsync_objdir)
> > + return;
> > +
> > + bulk_fsync_objdir = tmp_objdir_create("bulk-fsync");
> > + if (bulk_fsync_objdir)
> > + tmp_objdir_replace_primary_odb(bulk_fsync_objdir, 0);
> > +}
>
> OK. If we got a failure from tmp_objdir_create(), then we don't
> swap and end up creating a new loose object file in the primary
> object store. I wonder if we at least want to note that fact for
> later use at "unplug" time. We may create a few loose objects in
> the primary object store without any fsync, then a later call may
> successfully create a temporary object directory and we'd create
> more loose objects in the temporary one, which are flushed with the
> "create a dummy and fsync" trick and migrated, but do we need to do
> something to the ones we created in the primary object store before
> all that happens?
>
> > +void fsync_loose_object_bulk_checkin(int fd, const char *filename)
> > +{
> > + /*
> > + * If we have an active ODB transaction, 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_fsync_objdir ||
> > + git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
> > + fsync_or_die(fd, filename);
> > + }
> > +}
>
> Ah, if we have successfully created the temporary directory, we
> don't do full fsync but just writeout-only one, so there is no need
> for the worry I mentioned in the previous paragraph. OK.
>
There is the possibility that we might create the objdir when calling
prepare_loose_object_bulk_checkin but somehow fail to write the object
and yet still make it to end_odb_transaction. In that case, we'd
issue an extra dummy fsync. I don't think this case is worth extra
code to track, since it's a single fsync in a weird failure case.
> > @@ -301,4 +370,6 @@ void end_odb_transaction(void)
> >
> > if (bulk_checkin_state.f)
> > finish_bulk_checkin(&bulk_checkin_state);
> > +
> > + do_batch_fsync();
> > }
>
> OK.
>
> > @@ -1961,6 +1963,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
> > static struct strbuf tmp_file = STRBUF_INIT;
> > static struct strbuf filename = STRBUF_INIT;
> >
> > + if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
> > + prepare_loose_object_bulk_checkin();
> > +
> > loose_object_path(the_repository, &filename, oid);
> >
> > fd = create_tmpfile(&tmp_file, filename.buf);
>
> The necessary change to the "workhorse" code path is surprisingly
> small, which is pleasing to see.
>
> Thanks.
Thanks for looking at this.
