Re: [RFC PATCH 1/2] sha1-file: fsync() loose dir entry when core.fsyncObjectFiles

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 19, 2020, 11:38 UTC
Message-ID: <nycvar.QRO.7.76.6.2011191232490.56@tvgsbejvaqbjf.bet>
In-Reply-To: <87a6xii5gs.fsf@evledraar.gmail.com>

Hi Ævar,

On Tue, 22 Sep 2020, Ævar Arnfjörð Bjarmason wrote:

Show 43 quoted lines

> On Thu, Sep 17 2020, Johannes Sixt wrote:
>
> > Am 17.09.20 um 13:28 schrieb Ævar Arnfjörð Bjarmason:
> >> Change the behavior of core.fsyncObjectFiles to also sync the
> >> directory entry. I don't have a case where this broke, just going by
> >> paranoia and the fsync(2) manual page's guarantees about its behavior.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  sha1-file.c | 19 ++++++++++++++-----
> >>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/sha1-file.c b/sha1-file.c
> >> index dd65bd5c68..d286346921 100644
> >> --- a/sha1-file.c
> >> +++ b/sha1-file.c
> >> @@ -1784,10 +1784,14 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,
> >>  }
> >>
> >>  /* Finalize a file on disk, and close it. */
> >> -static void close_loose_object(int fd)
> >> +static void close_loose_object(int fd, const struct strbuf *dirname)
> >>  {
> >> -	if (fsync_object_files)
> >> +	int dirfd;
> >> +	if (fsync_object_files) {
> >>  		fsync_or_die(fd, "loose object file");
> >> +		dirfd = xopen(dirname->buf, O_RDONLY);
> >> +		fsync_or_die(dirfd, "loose object directory");
> >
> > Did you have the opportunity to verify that this works on Windows?
> > Opening a directory with open(2), I mean: It's disallowed according to
> > the docs:
> > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=vs-2019#return-value
>
> I did not, just did a quick hack for an RFC discussion (didn't even
> close() that fd), but if I pursue this I'll do it properly.
>
> Doing some research on it now reveals that we should probably have some
> Windows-specific code here, e.g. browsing GNUlib's source code reveals
> that it uses FlushFileBuffers(), and that code itself is taken from
> sqlite. SQLite also has special-case code for some Unix warts,
> e.g. OSX's and AIX's special fsync behaviors in its src/os_unix.c

If I understand correctly, the idea to `fsync()` directories is to ensure that metadata updates (such as renames) are flushed, too?

If so (and please note that my understanding of NTFS is not super deep in this regard), I think that we need not worry on Windows. I have come to believe that the `rename()` operations are flushed pretty much immediately, without any `FlushFileBuffers()` (or `_commit()`, as we actually do in `compat/mingw.h`, to convince yourself see https://github.com/git/git/blob/v2.29.2/compat/mingw.h#L135-L136).

Directories are not mentioned in `FlushFileBuffers()`'s documentation (https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-flushfilebuffers) nor in the documentation of `_commit()`: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/commit?view=msvc-160

Therefore, I believe that there is not even a Win32 equivalent of `fsync()`ing directories.

Ciao, Dscho

Show 8 quoted lines

>
> >> +	} if (close(fd) != 0) die_errno(_("error when closing loose object
> >> file")); }
> >
> > -- Hannes
>
>
>