Subject: Re: [PATCH v5 04/14] core.fsyncmethod: batched disk flushes for loose-objects

From: Junio C Hamano <[email-masked]>
Date: Thu, 31 Mar 2022 11:05:34 -0700

Neeraj Singh <[email-masked]> writes:

> 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.

The "except" part admits that it does not render well, no?

What happens if you modify the second and subsequent paragraph after
the "+" continuation in the way suggested?  Does it make it worse,
or does it make it worse?

> 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.

OK.

>> > +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.

Yup.