From: Neeraj K. Singh via GitGitGadget <hidden> Date: 2021-08-25 01:51:37
Git for Windows has had fsyncing of object files enabled since "409cae91eb
(mingw: change core.fsyncObjectFiles = 1 by default, 2017-09-04)".
There have been requests to make core.fsyncObjectFiles the default
everywhere, but there are concerns about its performance cost (perf results
below). There's a long and gory thread here:
https://lore.kernel.org/git/87a7xcw8sa.fsf@linux-m68k.org/t/.
My change introduces the new 'core.fsyncobjectFiles = 2' setting, which
batches the data-integrity FLUSH command sent to the disk across multiple
loose object files added to the object database.
We take advantage of the bulk-checkin hooks already in the add command and
add some hooks to the update-index (which is used internally by stash).
Details are in the last patch of the series.
Here's a simple performance test script:
#!/bin/sh
git clone https://github.com/nodejs/node.git node-repo-cache
git clone node-repo-cache node-repo
cd node-repo
git --version
find . -name "*.c" -exec sh -c 'echo foo1 >> $1' -- {} \;
echo "----GIT stash fsync"
time git -c core.fsyncObjectFiles=true stash push
find . -name "*.c" -exec sh -c 'echo foo2 >> $1' -- {} \;
echo "----GIT stash fsync_defer"
time git -c core.fsyncObjectFiles=2 stash push
find . -name "*.c" -exec sh -c 'echo foo3 >> $1' -- {} \;
echo "----GIT stash no_fsync"
time git -c core.fsyncObjectFiles=false stash push
cd ..
rm -r -f node-repo
Hardware:
* Mac - Mac Mini 2018 running MacOS 11.5.1, APFS with a 1TB Apple NMVE SSD,
* Linux - Ubuntu 20.04 - ext4 running on a Hyper-V VM with a fixed VHDX
backed by a Samsung PM981.
* Win - Windows NTFS - Same Hyper-V host as Linux. Operation | Mac | Linux
| Windows
---------------- |---------|-------|---------- git fsync | 40.6 s | 7.8 s |
6.9s git fsync_defer | 6.5 s | 2.1 s | 3.8s git no_fsync | 1.7 s | 1.0 s |
2.6s
The windows version of git is slightly different:
https://github.com/git-for-windows/git/pull/3391. I also used a
Windows-specific test script.
I hope I'm CC'ing a reasonable set of people on this patch, based on the
last discussion.
Thanks, Neeraj Singh Windows Core File Systems.
Neeraj Singh (2):
object-file: use futimes rather than utime
core.fsyncobjectfiles: batch disk flushes
Documentation/config/core.txt | 17 ++++--
Makefile | 4 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 105 +++++++++++++++++++++++++++++++---
bulk-checkin.h | 4 +-
compat/mingw.c | 42 +++++++++-----
compat/mingw.h | 2 +
config.c | 4 +-
config.mak.uname | 2 +
configure.ac | 8 +++
git-compat-util.h | 7 +++
object-file.c | 23 ++------
wrapper.c | 36 ++++++++++++
write-or-die.c | 2 +-
15 files changed, 213 insertions(+), 49 deletions(-)
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v1
Pull-Request: https://github.com/git/git/pull/1076
--
gitgitgadget
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-25 01:51:39
From: Neeraj Singh <redacted>
Refactor the loose object file creation code and use the futimes(2) API
rather than utime. This should be slightly faster given that we already
have an FD to work with.
Signed-off-by: Neeraj Singh <redacted>
---
compat/mingw.c | 42 +++++++++++++++++++++++++++++-------------
compat/mingw.h | 2 ++
object-file.c | 17 ++++++++---------
3 files changed, 39 insertions(+), 22 deletions(-)
@@ -949,19 +949,40 @@ int mingw_fstat(int fd, struct stat *buf)}}-staticinlinevoidtime_t_to_filetime(time_tt,FILETIME*ft)+staticinlinevoidtimeval_to_filetime(conststructtimeval*t,FILETIME*ft){-longlongwinTime=t*10000000LL+116444736000000000LL;+longlongwinTime=t->tv_sec*10000000LL+t->tv_usec*10+116444736000000000LL;ft->dwLowDateTime=winTime;ft->dwHighDateTime=winTime>>32;}-intmingw_utime(constchar*file_name,conststructutimbuf*times)+intmingw_futimes(intfd,conststructtimevaltimes[2]){FILETIMEmft,aft;++if(times){+timeval_to_filetime(×[0],&aft);+timeval_to_filetime(×[1],&mft);+}else{+GetSystemTimeAsFileTime(&mft);+aft=mft;+}++if(!SetFileTime((HANDLE)_get_osfhandle(fd),NULL,&aft,&mft)){+errno=EINVAL;+return-1;+}++return0;+}++intmingw_utime(constchar*file_name,conststructutimbuf*times)+{intfh,rc;DWORDattrs;wchar_twfilename[MAX_PATH];+structtimevaltvs[2];+if(xutftowcs_path(wfilename,file_name)<0)return-1;
@@ -1860,12 +1860,13 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,}/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)+staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename){if(fsync_object_files)fsync_or_die(fd,"loose object file");if(close(fd)!=0)die_errno(_("error when closing loose object file"));+returnfinalize_object_file(tmpfile,filename);}/* Size of directory component, including the ending '/' */
@@ -1973,17 +1974,15 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));-close_loose_object(fd);-if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0)-warning_errno(_("failed utime() on %s"),tmp_file.buf);+structtimevaltvs[2]={0};+tvs[0].tv_sec=mtime;+tvs[1].tv_sec=mtime;+if(futimes(fd,tvs)<0)+warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnfinalize_object_file(tmp_file.buf,filename.buf);+returnclose_loose_object(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-25 01:51:40
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later name.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we will issue another fsync
internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the MacOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
MacOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
Signed-off-by: Neeraj Singh <redacted>
---
Documentation/config/core.txt | 17 ++++--
Makefile | 4 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 105 +++++++++++++++++++++++++++++++---
bulk-checkin.h | 4 +-
config.c | 4 +-
config.mak.uname | 2 +
configure.ac | 8 +++
git-compat-util.h | 7 +++
object-file.c | 12 +---
wrapper.c | 36 ++++++++++++
write-or-die.c | 2 +-
13 files changed, 177 insertions(+), 30 deletions(-)
@@ -548,12 +548,17 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A boolean value or the number '2', indicating the level of durability+ applied to object files.+++This setting controls how much effort Git makes to ensure that data added to+the object store are durable in the case of an unclean system shutdown. If+'false', Git allows data to remain in file system caches according to operating+system policy, whence they may be lost if the system loses power or crashes. A+value of 'true' instructs Git to force objects to stable storage immediately+when they are added to the object store. The number '2' is an experimental+value that also preserves durability but tries to perform hardware flushes in a+batch. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
@@ -55,13 +69,42 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)clear_exit:free(state->written);+old_plugged=state->plugged;memset(state,0,sizeof(*state));+state->plugged=old_plugged;strbuf_release(&packname);/* Make objects we just wrote available to ourselves */reprepare_packed_git(the_repository);}+staticvoiddo_sync_and_rename(structbulk_rename_state*state,structlock_file*lock_file)+{+if(state->nr_renames){+inti;++/*+*Issueafullhardwareflushagainstthelockfiletoensure+*thatallobjectsaredurablebeforeanyrenamesoccur.+*Thecodeinfsync_and_close_loose_object_bulk_checkinhas+*alreadyensuredthatwriteouthasoccurred,butithasnot+*flushedanywritebackcacheinthestoragehardware.+*/+fsync_or_die(get_lock_file_fd(lock_file),get_lock_file_path(lock_file));++for(i=0;i<state->nr_renames;i++){+if(finalize_object_file(state->renames[i].src,state->renames[i].dst))+die_errno(_("could not rename '%s'"),state->renames[i].src);++free(state->renames[i].src);+free(state->renames[i].dst);+}++free(state->renames);+memset(state,0,sizeof(*state));+}+}+staticintalready_written(structbulk_checkin_state*state,structobject_id*oid){inti;
@@ -256,25 +299,69 @@ static int deflate_to_pack(struct bulk_checkin_state *state,return0;}+staticvoidadd_rename_bulk_checkin(structbulk_rename_state*state,+constchar*src,constchar*dst)+{+structobject_rename*rename;++ALLOC_GROW(state->renames,state->nr_renames+1,state->alloc_renames);++rename=&state->renames[state->nr_renames++];+rename->src=xstrdup(src);+rename->dst=xstrdup(dst);+}++intfsync_and_close_loose_object_bulk_checkin(intfd,constchar*tmpfile,+constchar*filename)+{+if(fsync_object_files){+/*+*Ifwehaveapluggedbulkcheckin,weissueacallthat+*cleansthefilesystempagecachebutavoidsahardwareflush+*command.Lateronwewillissueasinglehardwareflush+*beforerenamingfilesaspartofdo_sync_and_rename.+*/+if(bulk_checkin_state.plugged&&+fsync_object_files==2&&+git_fsync(fd,FSYNC_WRITEOUT_ONLY)>=0){+add_rename_bulk_checkin(&bulk_rename_state,tmpfile,filename);+if(close(fd))+die_errno(_("error when closing loose object file"));++return0;++}else{+fsync_or_die(fd,"loose object file");+}+}++if(close(fd))+die_errno(_("error when closing loose object file"));++returnfinalize_object_file(tmpfile,filename);+}+intindex_bulk_checkin(structobject_id*oid,intfd,size_tsize,enumobject_typetype,constchar*path,unsignedflags){-intstatus=deflate_to_pack(&state,oid,fd,size,type,+intstatus=deflate_to_pack(&bulk_checkin_state,oid,fd,size,type,path,flags);-if(!state.plugged)-finish_bulk_checkin(&state);+if(!bulk_checkin_state.plugged)+finish_bulk_checkin(&bulk_checkin_state);returnstatus;}voidplug_bulk_checkin(void){-state.plugged=1;+bulk_checkin_state.plugged=1;}-voidunplug_bulk_checkin(void)+voidunplug_bulk_checkin(structlock_file*lock_file){-state.plugged=0;-if(state.f)-finish_bulk_checkin(&state);+bulk_checkin_state.plugged=0;+if(bulk_checkin_state.f)+finish_bulk_checkin(&bulk_checkin_state);++do_sync_and_rename(&bulk_rename_state,lock_file);}
@@ -1859,16 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-returnfinalize_object_file(tmpfile,filename);-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1982,7 +1972,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnclose_loose_object(fd,tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-25 05:38:43
On Wed, Aug 25, 2021 at 01:51:32AM +0000, Neeraj Singh via GitGitGadget wrote:
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
Another interesting way to flush on linux would be the syncfs call,
which syncs all files on a file system. Once you write more than
handful or two of files that tends to win out over a batch of fsync
calls.
From: Johannes Schindelin <hidden> Date: 2021-08-25 13:51:13
Hi Neeraj,
Thank you so much for this patch series! Overall, I am very happy with the
direction this is going.
I will offer a couple of suggestions below, inlined.
On Wed, 25 Aug 2021, Neeraj Singh via GitGitGadget wrote:
From: Neeraj Singh <redacted>
Refactor the loose object file creation code and use the futimes(2) API
rather than utime. This should be slightly faster given that we already
have an FD to work with.
If I were you, I would spell out "file descriptor" here.
@@ -949,19 +949,40 @@ int mingw_fstat(int fd, struct stat *buf)}}-staticinlinevoidtime_t_to_filetime(time_tt,FILETIME*ft)+staticinlinevoidtimeval_to_filetime(conststructtimeval*t,FILETIME*ft){-longlongwinTime=t*10000000LL+116444736000000000LL;+longlongwinTime=t->tv_sec*10000000LL+t->tv_usec*10+116444736000000000LL;
Technically, this is a change in behavior, right? We did not use to use
nanosecond precision. But I don't think that we actually make use of this
in this patch.
At first, I wondered whether it would make sense to pass the access time
and the modified time separately, as pointers. I don't think that we pass
around arrays as function parameters in Git anywhere else.
But then I realized that `futimes()` is available in this precise form on
Linux and on the BSDs. Therefore, it is not up to us to decide the
function's signature.
However, now that I looked at the manual page, I noticed that this
function is not part of any POSIX standard.
Which makes me think that we will have to do a bit more than just define
it on Windows: we will have to introduce a `Makefile` knob (just like you
did with `HAVE_SYNC_FILE_RANGE` in patch 2/2) and set that specifically
for Linux and the BSDs, and use `futimes()` only if it is available
(otherwise fall back to `utime()`).
Then, as a separate patch, we should introduce this Windows-specific shim
and declare that it is available via `config.mak.uname`.
I am a _huge_ fan of patches that are so clear and obvious that bugs have
a hard time creeping in without being spotted immediately. And I think
that this organization would help achieve this goal.
Please lose the space between the function name and the opening
parenthesis. I know, the preimage of this diff has it, but that was an
oversight and definitely disagrees with our current coding style.
quoted hunk
+{
int fh, rc;
DWORD attrs;
wchar_t wfilename[MAX_PATH];
+ struct timeval tvs[2];
+
if (xutftowcs_path(wfilename, file_name) < 0)
return -1;
It is too bad that we have to copy around those values just to convert
them, but I cannot think of any better way, either. And it's not like
we're in a hot loop: this code will be dominated by I/O anyways.
@@ -1860,12 +1860,13 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,}/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)+staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename){if(fsync_object_files)fsync_or_die(fd,"loose object file");if(close(fd)!=0)die_errno(_("error when closing loose object file"));+returnfinalize_object_file(tmpfile,filename);
While this is a clear change of behavior, this function has only one
caller, and that caller is adjusted accordingly.
Could you add this clarification of context to the commit message? I know
it will help me in the future, when I have to get up to speed again by
reading the commit history.
Thank you,
Johannes
quoted hunk
}
/* Size of directory component, including the ending '/' */
@@ -1973,17 +1974,15 @@ static int write_loose_object(const struct object_id *oid, char *hdr, die(_("confused by unstable object source data for %s"), oid_to_hex(oid));- close_loose_object(fd);- if (mtime) {- struct utimbuf utb;- utb.actime = mtime;- utb.modtime = mtime;- if (utime(tmp_file.buf, &utb) < 0)- warning_errno(_("failed utime() on %s"), tmp_file.buf);+ struct timeval tvs[2] = {0};+ tvs[0].tv_sec = mtime;+ tvs[1].tv_sec = mtime;+ if (futimes(fd, tvs) < 0)+ warning_errno(_("failed futimes() on %s"), tmp_file.buf); }- return finalize_object_file(tmp_file.buf, filename.buf);+ return close_loose_object(fd, tmp_file.buf, filename.buf); } static int freshen_loose_object(const struct object_id *oid)--
On Wed, Aug 25 2021, Neeraj Singh via GitGitGadget wrote:
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later name.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we will issue another fsync
internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the MacOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
MacOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
Thanks for working on this, good to see fsck issues picked up after some
on-list pause.
@@ -548,12 +548,17 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A boolean value or the number '2', indicating the level of durability+ applied to object files.+++This setting controls how much effort Git makes to ensure that data added to+the object store are durable in the case of an unclean system shutdown. If+'false', Git allows data to remain in file system caches according to operating+system policy, whence they may be lost if the system loses power or crashes. A+value of 'true' instructs Git to force objects to stable storage immediately+when they are added to the object store. The number '2' is an experimental+value that also preserves durability but tries to perform hardware flushes in a+batch.
Some feedback/thoughts:
0) Let's not expose "2" to users, but give it some friendly config name
and just translate this to the enum internally.
1) Your commit message says "When updating the index and/or refs[...]"
but we're changing core.fsyncObjectFiles here, I assume that's
summarizing existing behavior then
2) You say "when adding many [loose] objects to a repo[...]", and the
test-case is "git stash push", but for e.g. accepting pushes we have
transfer.unpackLimit.
It would be interesting to see if/how this impacts performance there,
and also if not that should at least be called out in
documentation. I.e. users might want to set this differently on servers
v.s. checkouts.
But also, is this sort of thing something we could mitigate even more in
commands like "git stash push" by just writing a pack instead of N loose
objects?
I don't think such questions should preclude changing the fsync
approach, or offering more options, but they should inform our
longer-term goals.
3) Re some of the musings about fsync() recently in
https://lore.kernel.org/git/877dhs20x3.fsf@evledraar.gmail.com/; is this
method of doing not-quite-an-fsync guaranteed by some OS's / POSIX etc,
or is it more like the initial approach before core.fsyncObjectFiles,
i.e. the happy-go-lucky approach described in the "[...]that orders data
writes properly[...]" documentation you're removing.
4) While that documentation written by Linus long ago is rather
flippant, I think just removing it and not replacing it with some
discussion about how this is a trade-off v.s. real-world filesystem
semantics isn't a good change.
5) On a similar thought as transfer.unpackLimit in #2, I wonder if this
fsync() setting shouldn't really be something we should be splitting
up. I.e. maybe handle batch loose object writes one way, ref updates
another way etc. I think moving core.fsync* to a setting like what we
have for fsck.* and <cmd>.fsck.* is probably a better thing to do in the
longer term.
I.e. being able to do things like:
fsync.objectFiles = none
fsync.refFiles = cache # or "hardware"
receive.fsync.objectFiles = hardware
receive.fsync.refFiles = hardware
Or whatever, i.e. we're using one hammer for all of these now, but I
suspect most users who care about fsync care about /some/ fsync, not
everything.
6) Inline comments below.
In a crash of git itself it seems we're going to leave some litter
behind in the object dir now, and "git gc" won't know how to clean it
up. I think this is going to want to just use the tmp-objdir.[ch] API,
which might or might not need to be extended for loose objects / some
oddities of this use-case.
Also, if you have a pair of things like this the string-list API is much
more pleasing to use than coming up with your own encapsulation.
All boilerplate duplicating things you'd get with a string-list for free...
+ /*
+ * 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 renaming files as part of do_sync_and_rename.
+ */
So this is the sort of thing I meant by extending Linus's docs, I know
some FS's work this way, but not all do.
Also there's no guarantee in git that your .git is on one FS, so I think
even for the FS's you have in mind this might not be an absolute
guarantee...
On Tue, Aug 24, 2021 at 6:51 PM Neeraj K. Singh via GitGitGadget
[off-list ref] wrote:
Hardware:
* Mac - Mac Mini 2018 running MacOS 11.5.1, APFS with a 1TB Apple NMVE SSD,
* Linux - Ubuntu 20.04 - ext4 running on a Hyper-V VM with a fixed VHDX
backed by a Samsung PM981.
* Win - Windows NTFS - Same Hyper-V host as Linux. Operation | Mac | Linux
| Windows
---------------- |---------|-------|---------- git fsync | 40.6 s | 7.8 s |
6.9s git fsync_defer | 6.5 s | 2.1 s | 3.8s git no_fsync | 1.7 s | 1.0 s |
2.6s
On Tue, Aug 24, 2021 at 10:38 PM Christoph Hellwig [off-list ref] wrote:
On Wed, Aug 25, 2021 at 01:51:32AM +0000, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
Another interesting way to flush on linux would be the syncfs call,
which syncs all files on a file system. Once you write more than
handful or two of files that tends to win out over a batch of fsync
calls.
I'd expect syncfs to suffer from the noisy-neighbor problem that Linus
alluded to on the big
thread you kicked off. The equivalent call on Windows currently
requires administrative
privileges (I'm really not sure exactly why, perhaps we should change that).
If someone adds a more targeted bulk sync interface to the Linux
kernel, I'm sure Git could be
changed to use it. Maybe an fcntl(2) interface that initiates
writeback and registers completion with an
eventfd.
From: Johannes Schindelin <hidden> Date: 2021-08-25 18:52:42
Hi Neeraj,
continuing my review here, inlined.
On Wed, 25 Aug 2021, Neeraj Singh via GitGitGadget wrote:
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
It makes sense, but I would recommend using a more easily explained value
than `2`. Maybe `delayed`? Or `bulk` or `batched`?
The way this would be implemented would look somewhat like the
implementation for `core.abbrev`, which also accepts a string ("auto") or
a Boolean (or even an integral number), see
https://github.com/git/git/blob/v2.33.0/config.c#L1367-L1381:
if (!strcmp(var, "core.abbrev")) {
if (!value)
return config_error_nonbool(var);
if (!strcasecmp(value, "auto"))
default_abbrev = -1;
else if (!git_parse_maybe_bool_text(value))
default_abbrev = the_hash_algo->hexsz;
else {
int abbrev = git_config_int(var, value);
if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)
return error(_("abbrev length out of range: %d"), abbrev);
default_abbrev = abbrev;
}
return 0;
}
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later name.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we will issue another fsync
internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the MacOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
MacOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
You included a very nice table with performance numbers in the cover
letter. Maybe include that here, in the commit message?
@@ -548,12 +548,17 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A boolean value or the number '2', indicating the level of durability+ applied to object files.+++This setting controls how much effort Git makes to ensure that data added to+the object store are durable in the case of an unclean system shutdown. If
In addition to the content, I also like a lot that this tempers down the
language to be a lot more agreeable to read.
quoted hunk
+'false', Git allows data to remain in file system caches according to operating
+system policy, whence they may be lost if the system loses power or crashes. A
+value of 'true' instructs Git to force objects to stable storage immediately
+when they are added to the object store. The number '2' is an experimental
+value that also preserves durability but tries to perform hardware flushes in a
+batch.
core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
This change to `cmd_update_index()`, would it make sense to separate it
out into its own commit? I think it would, as it is a slight change of
behavior of the `--stdin` mode, no?
While it definitely looks better after this patch, having the new code
_and_ the rename in the same set of changes makes it a bit harder to
review and to spot bugs.
Could I ask you to split this rename out into its own, preparatory patch
("preparatory" meaning that it should be ordered before the patch that
adds support for the new fsync mode)?
Since this variable is designed to hold the value of the `plugged` field
of the `bulk_checkin_state`, which is declared as `unsigned plugged:1;`,
we probably want a `:1` here, too.
Also: is it really "old", rather than "orig"? I would have expected the
name `orig_plugged` or `save_plugged`.
Unfortunately, I lack the context to understand the purpose of this. Is
the idea that `plugged` gives an indication whether we're still within
that batch that should be fsync'ed all at once?
I only see one caller where this would make a difference, and that caller
is `deflate_to_pack()`. Maybe we should just start that function with
`unsigned save_plugged:1 = state->plugged;` and restore it after the
`while (1)` loop?
strbuf_release(&packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git(the_repository);
}
+static void do_sync_and_rename(struct bulk_rename_state *state, struct lock_file *lock_file)
+{
+ if (state->nr_renames) {
+ int i;
+
+ /*
+ * Issue a full hardware flush against the lock file to ensure
+ * that all objects are durable before any renames occur.
+ * The code in fsync_and_close_loose_object_bulk_checkin has
+ * already ensured that writeout has occurred, but it has not
+ * flushed any writeback cache in the storage hardware.
+ */
+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
+
+ for (i = 0; i < state->nr_renames; i++) {
+ if (finalize_object_file(state->renames[i].src, state->renames[i].dst))
+ die_errno(_("could not rename '%s'"), state->renames[i].src);
+
+ free(state->renames[i].src);
+ free(state->renames[i].dst);
+ }
+
+ free(state->renames);
+ memset(state, 0, sizeof(*state));
Hmm. There is a lot of `memset()`ing going on, and I am not quite sure
that I like what I am seeing. It does not help that there are now two very
easily-confused structs: `bulk_rename_state` and `bulk_checkin_state`.
Which made me worried at first that we might be resetting the `renames`
field inadvertently in `finish_bulk_checkin()`.
Maybe we can do this instead?
FREE_AND_NULL(state->renames);
state->nr_renames = state->alloc_renames = 0;
quoted hunk
+ }
+}
+
static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
{
int i;
@@ -256,25 +299,69 @@ static int deflate_to_pack(struct bulk_checkin_state *state, return 0; }+static void add_rename_bulk_checkin(struct bulk_rename_state *state,+ const char *src, const char *dst)+{+ struct object_rename *rename;++ ALLOC_GROW(state->renames, state->nr_renames + 1, state->alloc_renames);++ rename = &state->renames[state->nr_renames++];+ rename->src = xstrdup(src);+ rename->dst = xstrdup(dst);+}++int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,+ const char *filename)+{+ if (fsync_object_files) {+ /*+ * 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 renaming files as part of do_sync_and_rename.+ */+ if (bulk_checkin_state.plugged &&+ fsync_object_files == 2 &&+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {+ add_rename_bulk_checkin(&bulk_rename_state, tmpfile, filename);+ if (close(fd))+ die_errno(_("error when closing loose object file"));++ return 0;++ } else {+ fsync_or_die(fd, "loose object file");+ }+ }++ if (close(fd))+ die_errno(_("error when closing loose object file"));++ return finalize_object_file(tmpfile, filename);+}+ int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags) {- int status = deflate_to_pack(&state, oid, fd, size, type,+ int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type, path, flags);- if (!state.plugged)- finish_bulk_checkin(&state);+ if (!bulk_checkin_state.plugged)+ finish_bulk_checkin(&bulk_checkin_state); return status; } void plug_bulk_checkin(void) {- state.plugged = 1;+ bulk_checkin_state.plugged = 1; }-void unplug_bulk_checkin(void)+void unplug_bulk_checkin(struct lock_file *lock_file) {- state.plugged = 0;- if (state.f)- finish_bulk_checkin(&state);+ bulk_checkin_state.plugged = 0;+ if (bulk_checkin_state.f)+ finish_bulk_checkin(&bulk_checkin_state);++ do_sync_and_rename(&bulk_rename_state, lock_file); }
@@ -1859,16 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-returnfinalize_object_file(tmpfile,filename);-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1982,7 +1972,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnclose_loose_object(fd,tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
@@ -538,6 +538,42 @@ int xmkstemp_mode(char *filename_template, int mode)returnfd;}+intgit_fsync(intfd,enumfsync_actionaction)+{+if(action==FSYNC_WRITEOUT_ONLY){+#ifdef __APPLE__+/*+*onMacOSX,fsyncjustcausesfilesystemcachewritebackbutdoesnot+*flushhardwarecaches.+*/+returnfsync(fd);+#endif++#ifdef HAVE_SYNC_FILE_RANGE+/*+*Onlinux2.6.17andabove,sync_file_rangeisthewaytoissue+*awritebackwithoutahardwareflush.Anoffsetof0andsizeof0+*indicateswriteoutoftheentirefileandthewaitflagsensurethatall+*dirtydataiswrittentothedisk(potentiallyinadisk-sidecache)+*beforewecontinue.+*/++returnsync_file_range(fd,0,0,SYNC_FILE_RANGE_WAIT_BEFORE|+SYNC_FILE_RANGE_WRITE|+SYNC_FILE_RANGE_WAIT_AFTER);+#endif++errno=ENOSYS;+return-1;+}
Hmm. I wonder whether we can do this more consistently with how Git
usually does platform-specific things.
In the 3rd patch, the one where you implemented Windows-specific support,
in the Git for Windows PR at
https://github.com/git-for-windows/git/pull/3391, you introduce a
`mingw_fsync_no_flush()` function and define `fsync_no_flush` to expand to
that function name.
This is very similar to how Git does things. Take for example the
`offset_1st_component` macro:
https://github.com/git/git/blob/v2.33.0/git-compat-util.h#L386-L392
Unless defined in a platform-specific manner, it is defined in
`git-compat-util.h`:
#ifndef offset_1st_component
static inline int git_offset_1st_component(const char *path)
{
return is_dir_sep(path[0]);
}
#define offset_1st_component git_offset_1st_component
#endif
And on Windows, it is defined as following
(https://github.com/git/git/blob/v2.33.0/compat/win32/path-utils.h#L34-L35),
before the lines quoted above:
int win32_offset_1st_component(const char *path);
#define offset_1st_component win32_offset_1st_component
We could do the exact same thing here. Define a platform-specific
`mingw_fsync_no_flush()` in `compat/mingw.h` and define the macro
`fsync_no_flush` to point to it. In `git-compat-util.h`, in the
`__APPLE__`-specific part, implement it via `fsync()`. And later, in the
platform-independent part, _iff_ the macro has not yet been defined,
implement an inline function that does that `HAVE_SYNC_FILE_RANGE` dance
and falls back to `ENOSYS`.
That would contain the platform-specific `#ifdef` blocks to
`git-compat-util.h`, which is exactly where we want them.
Same thing here. We would probably want something like `fsync_with_flush`
here.
It is my hope that you find my comments and suggestions helpful.
Thank you,
Johannes
quoted hunk
+}
+
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
int err;
@@ -949,19 +949,40 @@ int mingw_fstat(int fd, struct stat *buf)}}-staticinlinevoidtime_t_to_filetime(time_tt,FILETIME*ft)+staticinlinevoidtimeval_to_filetime(conststructtimeval*t,FILETIME*ft){-longlongwinTime=t*10000000LL+116444736000000000LL;+longlongwinTime=t->tv_sec*10000000LL+t->tv_usec*10+116444736000000000LL;
Technically, this is a change in behavior, right? We did not use to use
nanosecond precision. But I don't think that we actually make use of this
in this patch.
At first, I wondered whether it would make sense to pass the access time
and the modified time separately, as pointers. I don't think that we pass
around arrays as function parameters in Git anywhere else.
But then I realized that `futimes()` is available in this precise form on
Linux and on the BSDs. Therefore, it is not up to us to decide the
function's signature.
However, now that I looked at the manual page, I noticed that this
function is not part of any POSIX standard.
Which makes me think that we will have to do a bit more than just define
it on Windows: we will have to introduce a `Makefile` knob (just like you
did with `HAVE_SYNC_FILE_RANGE` in patch 2/2) and set that specifically
for Linux and the BSDs, and use `futimes()` only if it is available
(otherwise fall back to `utime()`).
Then, as a separate patch, we should introduce this Windows-specific shim
and declare that it is available via `config.mak.uname`.
Thanks for taking another look at the man pages. I looked again too and saw
that futimens is part of POSIX.1-2008:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html.
If I switch to futimens and implement the Windows shim at the same
time, is that sufficient to
address your feedback? I'd rather not ifdef this one since the
codeflow is quite different
depending on the presence of the API.
Please lose the space between the function name and the opening
parenthesis. I know, the preimage of this diff has it, but that was an
oversight and definitely disagrees with our current coding style.
Will do.
quoted
+{
int fh, rc;
DWORD attrs;
wchar_t wfilename[MAX_PATH];
+ struct timeval tvs[2];
+
if (xutftowcs_path(wfilename, file_name) < 0)
return -1;
It is too bad that we have to copy around those values just to convert
them, but I cannot think of any better way, either. And it's not like
we're in a hot loop: this code will be dominated by I/O anyways.
Yeah, the cost of this is approximately 3-4 cycles (load-to-use
latency), so no one will notice relative to the system call overhead.
@@ -1860,12 +1860,13 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,}/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)+staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename){if(fsync_object_files)fsync_or_die(fd,"loose object file");if(close(fd)!=0)die_errno(_("error when closing loose object file"));+returnfinalize_object_file(tmpfile,filename);
While this is a clear change of behavior, this function has only one
caller, and that caller is adjusted accordingly.
Could you add this clarification of context to the commit message? I know
it will help me in the future, when I have to get up to speed again by
reading the commit history.
On Wed, Aug 25, 2021 at 9:31 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Wed, Aug 25 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later name.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we will issue another fsync
internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the MacOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
MacOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
Thanks for working on this, good to see fsck issues picked up after some
on-list pause.
@@ -548,12 +548,17 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A boolean value or the number '2', indicating the level of durability+ applied to object files.+++This setting controls how much effort Git makes to ensure that data added to+the object store are durable in the case of an unclean system shutdown. If+'false', Git allows data to remain in file system caches according to operating+system policy, whence they may be lost if the system loses power or crashes. A+value of 'true' instructs Git to force objects to stable storage immediately+when they are added to the object store. The number '2' is an experimental+value that also preserves durability but tries to perform hardware flushes in a+batch.
Some feedback/thoughts:
0) Let's not expose "2" to users, but give it some friendly config name
and just translate this to the enum internally.
Agreed. I'll follow suggestions from here and elsewhere to make this a
human-readable
string. Is "core.fsyncObjectFiles=batch" acceptable?
1) Your commit message says "When updating the index and/or refs[...]"
but we're changing core.fsyncObjectFiles here, I assume that's
summarizing existing behavior then
That's what I intended. I'll make the patch description more explicit
about that.
In general, this patch is only concerned with loose object files. It's
my assumption
that other parts of the system (like the refs db) need to perform their own data
consistency and must have their own durability.
I do think that long-term, the Git community should think about having
a general transaction
mechanism with redo logging to have a consistent method for achieving
durability.
2) You say "when adding many [loose] objects to a repo[...]", and the
test-case is "git stash push", but for e.g. accepting pushes we have
transfer.unpackLimit.
It would be interesting to see if/how this impacts performance there,
and also if not that should at least be called out in
documentation. I.e. users might want to set this differently on servers
v.s. checkouts.
But also, is this sort of thing something we could mitigate even more in
commands like "git stash push" by just writing a pack instead of N loose
objects?
I don't think such questions should preclude changing the fsync
approach, or offering more options, but they should inform our
longer-term goals.
Dealing only/mostly in packfiles would be a great approach. I'd hope that
this fsyncing work would mostly be superseded if such a change is rolled out.
I just read about the geometric repacking stuff, and it looks reminiscent of the
LSM-tree approach to databases.
3) Re some of the musings about fsync() recently in
https://lore.kernel.org/git/877dhs20x3.fsf@evledraar.gmail.com/; is this
method of doing not-quite-an-fsync guaranteed by some OS's / POSIX etc,
or is it more like the initial approach before core.fsyncObjectFiles,
i.e. the happy-go-lucky approach described in the "[...]that orders data
writes properly[...]" documentation you're removing.
I am confident about the validity of the batched approach on Windows when
running on NTFS and ReFS, given my background as a Windows FS developer.
We are unlikely to change any mainstream data consistency behavior of
our filesystems
to be weaker, given the type of errors such changes would cause.
In Windows, we call the FS requirements to support this change
"external metadata consistency",
which states that all metadata operations that could have led to a
state later FSYNCed must be
visible after the fsync.
macOS's fsync documentation indicates that they are likely to
implement the required guarantees.
They specifically say that fsync triggers writeback or data and
metadata, but does not issue a hardware
flush. Please see the doc at
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/fsync.2.html.
It is also notable that Apple SSDs have particularly bad performance
for flush operations (we noticed this
when booting Windows through BootCamp as well).
Unfortunately my perusal of the man pages and documentation I could find doesn't
give me this level of confidence on typical Linux filesystems. For
instance, the notion of having to
fsync the parent directory in order to render an inode's link findable
eliminates a lot of the
advantage of this change, though we could batch those and would have
to do at most 256.
This thread is somewhat instructive, but inconclusive:
https://lwn.net/ml/linux-fsdevel/1552418820-18102-1-git-send-email-jaya@cs.utexas.edu/.
One conclusion from reviewing that thread is that as of then,
sync_file_ranges isn't actually enough
to make a hard guarantee about writeout occurring. See
https://lore.kernel.org/linux-fsdevel/20190319204330.GY26298@dastard/.
My hope is that the Linux FS developers have rectified that shortcoming by now.
4) While that documentation written by Linus long ago is rather
flippant, I think just removing it and not replacing it with some
discussion about how this is a trade-off v.s. real-world filesystem
semantics isn't a good change.
I don't think the replaced documentation applies anymore to an ext4 or xfs
system with delayed allocation. Those filesystems effectively have
data=writeback
semantics because they don't need data ordering to avoid exposing unwritten
data, and so don't write the data at any particular syscall boundary
or with any particular
ordering.
I think my updated version of the documentation for "= false" is
accurate and more helpful
from a user perspective ("up to OS policy when your data becomes durable in
the event of an unclean shutdown"). "= true" also has a reasonable
description, though I
might add some verbiage indicating that this setting could be costly.
I'll take a crack at improving the batched mode documentation.
5) On a similar thought as transfer.unpackLimit in #2, I wonder if this
fsync() setting shouldn't really be something we should be splitting
up. I.e. maybe handle batch loose object writes one way, ref updates
another way etc. I think moving core.fsync* to a setting like what we
have for fsck.* and <cmd>.fsck.* is probably a better thing to do in the
longer term.
I.e. being able to do things like:
fsync.objectFiles = none
fsync.refFiles = cache # or "hardware"
receive.fsync.objectFiles = hardware
receive.fsync.refFiles = hardware
Or whatever, i.e. we're using one hammer for all of these now, but I
suspect most users who care about fsync care about /some/ fsync, not
everything.
I disagree. I believe Git should offer a consolidated config setting
with two overall goals:
1) A consistent high-integrity setting across the entire git
index/object/ref state, primarily
for people using a repo for active development of changes. This should
roughly guarantee
that when a git command that adds data to the repo completes, the data
is durable within git,
including the refs needed to find it.
2) A lower-integrity setting useful for build/CI, maintainers who are
applying lots of patches, etc,
where it is expected that the data is available elsewhere and can be
recovered easily.
In a crash of git itself it seems we're going to leave some litter
behind in the object dir now, and "git gc" won't know how to clean it
up. I think this is going to want to just use the tmp-objdir.[ch] API,
which might or might not need to be extended for loose objects / some
oddities of this use-case.
It appears that "git prune" would take care of these files.
Also, if you have a pair of things like this the string-list API is much
more pleasing to use than coming up with your own encapsulation.
So with this and other use of the "state" variable is this part of
bulk-checkin going to become thread-unsafe, was that already the case?
Yes, this code was already thread-unsafe if we're in the "bulk checkin
plugged" mode and
that hasn't changed. Is it worth fixing this right now? Is there a
preexisting example of code
that uses thread-local-storage inside a library function and then
merges the state later? Bonus
points for only doing the thread-local stuff if alternate threads are
actually active.
All boilerplate duplicating things you'd get with a string-list for free...
Will fix.Thanks.
quoted
+ /*
+ * 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 renaming files as part of do_sync_and_rename.
+ */
So this is the sort of thing I meant by extending Linus's docs, I know
some FS's work this way, but not all do.
Also there's no guarantee in git that your .git is on one FS, so I think
even for the FS's you have in mind this might not be an absolute
guarantee...
This is unfortunately an issue that isn't resolvable within Git. I think there's
value in supporting batched mode for the common systems and filesystems
that do support the guarantee. I'd like to be able to set batched mode as the
default on Windows eventually. It also looks like it can be default on macOS.
I think linux might be able to get to the desired semantics for default distro
filesystems with minimal changes. Perhaps this new mode in Git would provide
them with a motivation to do so.
On Wed, Aug 25, 2021 at 11:52 AM Johannes Schindelin
[off-list ref] wrote:
On Wed, 25 Aug 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
MacOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = 2' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
It makes sense, but I would recommend using a more easily explained value
than `2`. Maybe `delayed`? Or `bulk` or `batched`?
The way this would be implemented would look somewhat like the
implementation for `core.abbrev`, which also accepts a string ("auto") or
a Boolean (or even an integral number), see
https://github.com/git/git/blob/v2.33.0/config.c#L1367-L1381:
if (!strcmp(var, "core.abbrev")) {
if (!value)
return config_error_nonbool(var);
if (!strcasecmp(value, "auto"))
default_abbrev = -1;
else if (!git_parse_maybe_bool_text(value))
default_abbrev = the_hash_algo->hexsz;
else {
int abbrev = git_config_int(var, value);
if (abbrev < minimum_abbrev || abbrev > the_hash_algo->hexsz)
return error(_("abbrev length out of range: %d"), abbrev);
default_abbrev = abbrev;
}
return 0;
}
Thanks for the code example. I'll follow something similar. I'll prefer the name
"batch" for the new fsync mode.
quoted
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later name.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we will issue another fsync
internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the MacOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
MacOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
You included a very nice table with performance numbers in the cover
letter. Maybe include that here, in the commit message?
@@ -548,12 +548,17 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A boolean value or the number '2', indicating the level of durability+ applied to object files.+++This setting controls how much effort Git makes to ensure that data added to+the object store are durable in the case of an unclean system shutdown. If
In addition to the content, I also like a lot that this tempers down the
language to be a lot more agreeable to read.
quoted
+'false', Git allows data to remain in file system caches according to operating
+system policy, whence they may be lost if the system loses power or crashes. A
+value of 'true' instructs Git to force objects to stable storage immediately
+when they are added to the object store. The number '2' is an experimental
+value that also preserves durability but tries to perform hardware flushes in a
+batch.
I'll be revising this text a little bit to respond to avarab's
feedback. I'm guessing
that this will need a few more rounds of tweaking.
quoted
core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
This change to `cmd_update_index()`, would it make sense to separate it
out into its own commit? I think it would, as it is a slight change of
behavior of the `--stdin` mode, no?
Will do.
This makes me think that there is some risk here if someone launches an
update-index and then attempts to use the index to find a newly-added
OID without
completing the update-index invocation. It's possible to construct
such a scenario
if someone's using the "--verbose" scenario to find out about actions
taken by the
subprocess. Do you think I should add a flag to conditionally enable
bulk-checkin to
avoid regression for this (I expect unlikely) case?
While it definitely looks better after this patch, having the new code
_and_ the rename in the same set of changes makes it a bit harder to
review and to spot bugs.
Could I ask you to split this rename out into its own, preparatory patch
("preparatory" meaning that it should be ordered before the patch that
adds support for the new fsync mode)?
Will do. I'm going to change a few things as I'll mention below.
Since this variable is designed to hold the value of the `plugged` field
of the `bulk_checkin_state`, which is declared as `unsigned plugged:1;`,
we probably want a `:1` here, too.
Also: is it really "old", rather than "orig"? I would have expected the
name `orig_plugged` or `save_plugged`.
Unfortunately, I lack the context to understand the purpose of this. Is
the idea that `plugged` gives an indication whether we're still within
that batch that should be fsync'ed all at once?
I only see one caller where this would make a difference, and that caller
is `deflate_to_pack()`. Maybe we should just start that function with
`unsigned save_plugged:1 = state->plugged;` and restore it after the
`while (1)` loop?
The problem being solved here is that in some (rare) circumstances it looks like
we could unplug the state before an explicit unplug call. That would
be fatal for the
bulk-fsync code, and is probably suboptimal for the bulk checkin code.
I'm going to separate
this out the boolean to a different variable in my preparatory patch
so that this fragility goes away.
quoted
strbuf_release(&packname);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git(the_repository);
}
+static void do_sync_and_rename(struct bulk_rename_state *state, struct lock_file *lock_file)
+{
+ if (state->nr_renames) {
+ int i;
+
+ /*
+ * Issue a full hardware flush against the lock file to ensure
+ * that all objects are durable before any renames occur.
+ * The code in fsync_and_close_loose_object_bulk_checkin has
+ * already ensured that writeout has occurred, but it has not
+ * flushed any writeback cache in the storage hardware.
+ */
+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
+
+ for (i = 0; i < state->nr_renames; i++) {
+ if (finalize_object_file(state->renames[i].src, state->renames[i].dst))
+ die_errno(_("could not rename '%s'"), state->renames[i].src);
+
+ free(state->renames[i].src);
+ free(state->renames[i].dst);
+ }
+
+ free(state->renames);
+ memset(state, 0, sizeof(*state));
Hmm. There is a lot of `memset()`ing going on, and I am not quite sure
that I like what I am seeing. It does not help that there are now two very
easily-confused structs: `bulk_rename_state` and `bulk_checkin_state`.
Which made me worried at first that we might be resetting the `renames`
field inadvertently in `finish_bulk_checkin()`.
Yeah, the problem I was trying to avoid was an issue with resetting the rename
state during the "too big pack" case. What do you think about a
"bulk_pack_state" and
a "bulk_fsync_state" variable? One side advantage of the
"s/state/bulk_rename_state" change
is that the variable name is no longer ambiguous in the debugger
across different object files.
Maybe we can do this instead?
FREE_AND_NULL(state->renames);
state->nr_renames = state->alloc_renames = 0;
I wrote it that way originally, but saw an error with coccinelle and
then decided to follow
the convention from elsewhere in the file. That's when I noticed the
nasty "early unplug" case,
so the Github actions certainly saved me from an unfortunate bug.
Thanks for setting them up!
I'll go toward your suggestion.
quoted
+ }
+}
+
static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
{
int i;
@@ -256,25 +299,69 @@ static int deflate_to_pack(struct bulk_checkin_state *state, return 0; }+static void add_rename_bulk_checkin(struct bulk_rename_state *state,+ const char *src, const char *dst)+{+ struct object_rename *rename;++ ALLOC_GROW(state->renames, state->nr_renames + 1, state->alloc_renames);++ rename = &state->renames[state->nr_renames++];+ rename->src = xstrdup(src);+ rename->dst = xstrdup(dst);+}++int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,+ const char *filename)+{+ if (fsync_object_files) {+ /*+ * 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 renaming files as part of do_sync_and_rename.+ */+ if (bulk_checkin_state.plugged &&+ fsync_object_files == 2 &&+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {+ add_rename_bulk_checkin(&bulk_rename_state, tmpfile, filename);+ if (close(fd))+ die_errno(_("error when closing loose object file"));++ return 0;++ } else {+ fsync_or_die(fd, "loose object file");+ }+ }++ if (close(fd))+ die_errno(_("error when closing loose object file"));++ return finalize_object_file(tmpfile, filename);+}+ int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags) {- int status = deflate_to_pack(&state, oid, fd, size, type,+ int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type, path, flags);- if (!state.plugged)- finish_bulk_checkin(&state);+ if (!bulk_checkin_state.plugged)+ finish_bulk_checkin(&bulk_checkin_state); return status; } void plug_bulk_checkin(void) {- state.plugged = 1;+ bulk_checkin_state.plugged = 1; }-void unplug_bulk_checkin(void)+void unplug_bulk_checkin(struct lock_file *lock_file) {- state.plugged = 0;- if (state.f)- finish_bulk_checkin(&state);+ bulk_checkin_state.plugged = 0;+ if (bulk_checkin_state.f)+ finish_bulk_checkin(&bulk_checkin_state);++ do_sync_and_rename(&bulk_rename_state, lock_file); }
@@ -1859,16 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-returnfinalize_object_file(tmpfile,filename);-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1982,7 +1972,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnclose_loose_object(fd,tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
@@ -538,6 +538,42 @@ int xmkstemp_mode(char *filename_template, int mode)returnfd;}+intgit_fsync(intfd,enumfsync_actionaction)+{+if(action==FSYNC_WRITEOUT_ONLY){+#ifdef __APPLE__+/*+*onMacOSX,fsyncjustcausesfilesystemcachewritebackbutdoesnot+*flushhardwarecaches.+*/+returnfsync(fd);+#endif++#ifdef HAVE_SYNC_FILE_RANGE+/*+*Onlinux2.6.17andabove,sync_file_rangeisthewaytoissue+*awritebackwithoutahardwareflush.Anoffsetof0andsizeof0+*indicateswriteoutoftheentirefileandthewaitflagsensurethatall+*dirtydataiswrittentothedisk(potentiallyinadisk-sidecache)+*beforewecontinue.+*/++returnsync_file_range(fd,0,0,SYNC_FILE_RANGE_WAIT_BEFORE|+SYNC_FILE_RANGE_WRITE|+SYNC_FILE_RANGE_WAIT_AFTER);+#endif++errno=ENOSYS;+return-1;+}
Hmm. I wonder whether we can do this more consistently with how Git
usually does platform-specific things.
In the 3rd patch, the one where you implemented Windows-specific support,
in the Git for Windows PR at
https://github.com/git-for-windows/git/pull/3391, you introduce a
`mingw_fsync_no_flush()` function and define `fsync_no_flush` to expand to
that function name.
This is very similar to how Git does things. Take for example the
`offset_1st_component` macro:
https://github.com/git/git/blob/v2.33.0/git-compat-util.h#L386-L392
Unless defined in a platform-specific manner, it is defined in
`git-compat-util.h`:
#ifndef offset_1st_component
static inline int git_offset_1st_component(const char *path)
{
return is_dir_sep(path[0]);
}
#define offset_1st_component git_offset_1st_component
#endif
And on Windows, it is defined as following
(https://github.com/git/git/blob/v2.33.0/compat/win32/path-utils.h#L34-L35),
before the lines quoted above:
int win32_offset_1st_component(const char *path);
#define offset_1st_component win32_offset_1st_component
We could do the exact same thing here. Define a platform-specific
`mingw_fsync_no_flush()` in `compat/mingw.h` and define the macro
`fsync_no_flush` to point to it. In `git-compat-util.h`, in the
`__APPLE__`-specific part, implement it via `fsync()`. And later, in the
platform-independent part, _iff_ the macro has not yet been defined,
implement an inline function that does that `HAVE_SYNC_FILE_RANGE` dance
and falls back to `ENOSYS`.
That would contain the platform-specific `#ifdef` blocks to
`git-compat-util.h`, which is exactly where we want them.
Same thing here. We would probably want something like `fsync_with_flush`
here.
I thought about doing it that way originally. But there's the
unfortunate fact that
I'd have to alias fsync_no_flush to fsync on macOS and then fsync to
fnctl(F_FULLFSYNC),
I felt that it would be clearer to someone reviewing this
functionality if we provide a
very explicit git_fsync API with a well-named flag and document the
OS-specific craziness
in the C file rather than through a layer of macros in the header file.
Given that, are you okay with keeping this code layout in the C file,
potentially with
more local modifications?
It is my hope that you find my comments and suggestions helpful.
Thank you,
Johannes
Very much so! Again thanks for the review.
-Neeraj
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-26 05:50:29
On Wed, Aug 25, 2021 at 05:49:45PM -0700, Neeraj Singh wrote:
Unfortunately my perusal of the man pages and documentation I could find doesn't
give me this level of confidence on typical Linux filesystems. For
instance, the notion of having to
fsync the parent directory in order to render an inode's link findable
eliminates a lot of the
advantage of this change, though we could batch those and would have
to do at most 256.
This thread is somewhat instructive, but inconclusive:
https://lwn.net/ml/linux-fsdevel/1552418820-18102-1-git-send-email-jaya@cs.utexas.edu/.
fsync/fdatasync only guarantees consistency for the file handle they
are called on. The first linked document mentioned an implementation
artifact that file systems with metadata logging tend to force their
log out until the last modified transaction and thus force out metadata
changes done earlier. This won't help with actual data writes at all,
as for them the fact of writing back data will often generate new
metadata changes., and in general is not a property to rely on if you
care about data integrity. It is nice to optimize the order of the
fsync calls for metadata only workloads, as then often the later fsync
calls on earlier modified file handles will be no-ops.
One conclusion from reviewing that thread is that as of then,
sync_file_ranges isn't actually enough
to make a hard guarantee about writeout occurring. See
https://lore.kernel.org/linux-fsdevel/20190319204330.GY26298@dastard/.
My hope is that the Linux FS developers have rectified that shortcoming by now.
I'm not sure what shortcoming you mean. sync_file_ranges is a system
call that only causes data writeback. It never performs metadata write
back and thus is not an integrity operation at all. That is also very
clearly documented in the man page.
I think my updated version of the documentation for "= false" is
accurate and more helpful
from a user perspective ("up to OS policy when your data becomes durable in
the event of an unclean shutdown"). "= true" also has a reasonable
description, though I
might add some verbiage indicating that this setting could be costly.
Your version is much better. In fact it almost still too nice as in
general it will not be durable and you do end up with a corrupted
repository in that case. Note that even for bad old ext3 that was
usually the case.
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-26 05:54:43
On Wed, Aug 25, 2021 at 10:40:53AM -0700, Neeraj Singh wrote:
I'd expect syncfs to suffer from the noisy-neighbor problem that Linus
alluded to on the big
thread you kicked off.
It does. That being said I suspect in most developer workstation
use cases it will still be a win. Maybe I'll look into implemeting
it after your series lands.
If someone adds a more targeted bulk sync interface to the Linux
kernel, I'm sure Git could be
changed to use it. Maybe an fcntl(2) interface that initiates
writeback and registers completion with an
eventfd.
That is in general very hard to do with how the VM-level writeback
occurs. In the file system itself it could work much better, e.g.
for XFS we write the log up to a specific sequence number and could
notify when doing that.
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-26 05:57:28
On Wed, Aug 25, 2021 at 06:11:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
3) Re some of the musings about fsync() recently in
https://lore.kernel.org/git/877dhs20x3.fsf@evledraar.gmail.com/; is this
method of doing not-quite-an-fsync guaranteed by some OS's / POSIX etc,
or is it more like the initial approach before core.fsyncObjectFiles,
i.e. the happy-go-lucky approach described in the "[...]that orders data
writes properly[...]" documentation you're removing.
Except for the now removed ext3 filesystem in Linux that basically turned
every fsync into syncfs, that is a file system-wide sync I've never
heard about such behavior for data writeback. Many file systems will
sometimes or always behave like that for metadata writeback, but there
is no guarantees you could rely on for that.
From: Neeraj K. Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:42
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
* Introduce a separate preparatory patch to the bulk-checkin infrastructure
to separate the 'plugged' variable and rename the 'state' variable, as
suggested by dscho.
* Add performance numbers to the commit message of the main bulk fsync
patch, as suggested by dscho.
* Add a comment about the non-thread-safety of the bulk-checkin
infrastructure, as suggested by avarab.
* Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested
by dscho and avarab and others.
* Add more details to Documentation/config/core.txt about the various
settings and their intended effects, as suggested by avarab.
* Switch to the string-list API to hold the rename state, as suggested by
avarab.
* Create a separate update-index patch to use bulk-checkin as suggested by
dscho.
* Add Windows support in the upstream git. This is done in a way that
should not conflict with git-for-windows.
* Add new performance tests that shows the delta based on fsync mode.
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
Neeraj Singh (6):
object-file: use futimens rather than utime
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
core.fsyncobjectfiles: add windows support for batch mode
update-index: use the bulk-checkin infrastructure
core.fsyncobjectfiles: performance tests for add and stash
Documentation/config/core.txt | 26 ++++++--
Makefile | 6 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 92 +++++++++++++++++++++++++----
bulk-checkin.h | 4 +-
cache.h | 8 ++-
compat/mingw.c | 53 +++++++++++------
compat/mingw.h | 5 ++
compat/win32/flush.c | 29 +++++++++
config.c | 8 ++-
config.mak.uname | 4 ++
configure.ac | 8 +++
contrib/buildsystems/CMakeLists.txt | 3 +-
environment.c | 2 +-
git-compat-util.h | 7 +++
object-file.c | 23 ++------
t/perf/lib-unique-files.sh | 32 ++++++++++
t/perf/p3700-add.sh | 43 ++++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++++
wrapper.c | 40 +++++++++++++
write-or-die.c | 2 +-
22 files changed, 389 insertions(+), 58 deletions(-)
create mode 100644 compat/win32/flush.c
create mode 100644 t/perf/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v2
Pull-Request: https://github.com/git/git/pull/1076
Range-diff vs v1:
1: 2c1ddef6057 ! 1: fc3d5a7b635 object-file: use futimes rather than utime
@@ Metadata
Author: Neeraj Singh [off-list ref]
## Commit message ##
- object-file: use futimes rather than utime
+ object-file: use futimens rather than utime
- Refactor the loose object file creation code and use the futimes(2) API
- rather than utime. This should be slightly faster given that we already
- have an FD to work with.
+ Make close_loose_object do all of the steps for syncing and correctly
+ naming a new loose object so that it can be reimplemented in the
+ upcoming bulk-fsync mode.
+
+ Use futimens, which is available in POSIX.1-2008 to update the file
+ timestamps. This should be slightly faster than utime, since we have
+ a file descriptor already available. This change allows us to update
+ the time before closing, renaming, and potentially fsyincing the file
+ being refreshed. This code is currently only invoked by git-pack-objects
+ via force_object_loose.
+
+ Implement a futimens shim for the Windows port of Git.
Signed-off-by: Neeraj Singh [off-list ref]
## compat/mingw.c ##
+@@ compat/mingw.c: int mingw_chmod(const char *filename, int mode)
+ * The unit of FILETIME is 100-nanoseconds since January 1, 1601, UTC.
+ * Returns the 100-nanoseconds ("hekto nanoseconds") since the epoch.
+ */
++
++#define UNIX_EPOCH_FILETIME 116444736000000000LL
++
+ static inline long long filetime_to_hnsec(const FILETIME *ft)
+ {
+ long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
+ /* Windows to Unix Epoch conversion */
+- return winTime - 116444736000000000LL;
++ return winTime - UNIX_EPOCH_FILETIME;
+ }
+
+ static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
+@@ compat/mingw.c: static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts)
+ ts->tv_nsec = (hnsec % 10000000) * 100;
+ }
+
++static inline void timespec_to_filetime(const struct timespec *t, FILETIME *ft)
++{
++ long long winTime = t->tv_sec * 10000000LL + t->tv_nsec / 100 + UNIX_EPOCH_FILETIME;
++ ft->dwLowDateTime = winTime;
++ ft->dwHighDateTime = winTime >> 32;
++}
++
+ /**
+ * Verifies that safe_create_leading_directories() would succeed.
+ */
@@ compat/mingw.c: int mingw_fstat(int fd, struct stat *buf)
}
}
-static inline void time_t_to_filetime(time_t t, FILETIME *ft)
-+static inline void timeval_to_filetime(const struct timeval *t, FILETIME *ft)
++int mingw_futimens(int fd, const struct timespec times[2])
{
- long long winTime = t * 10000000LL + 116444736000000000LL;
-+ long long winTime = t->tv_sec * 10000000LL + t->tv_usec * 10 + 116444736000000000LL;
- ft->dwLowDateTime = winTime;
- ft->dwHighDateTime = winTime >> 32;
- }
-
--int mingw_utime (const char *file_name, const struct utimbuf *times)
-+int mingw_futimes(int fd, const struct timeval times[2])
- {
- FILETIME mft, aft;
+- ft->dwLowDateTime = winTime;
+- ft->dwHighDateTime = winTime >> 32;
++ FILETIME mft, aft;
+
+ if (times) {
-+ timeval_to_filetime(×[0], &aft);
-+ timeval_to_filetime(×[1], &mft);
++ timespec_to_filetime(×[0], &aft);
++ timespec_to_filetime(×[1], &mft);
+ } else {
+ GetSystemTimeAsFileTime(&mft);
+ aft = mft;
@@ compat/mingw.c: int mingw_fstat(int fd, struct stat *buf)
+ }
+
+ return 0;
-+}
-+
-+int mingw_utime (const char *file_name, const struct utimbuf *times)
-+{
+ }
+
+-int mingw_utime (const char *file_name, const struct utimbuf *times)
++int mingw_utime(const char *file_name, const struct utimbuf *times)
+ {
+- FILETIME mft, aft;
int fh, rc;
DWORD attrs;
wchar_t wfilename[MAX_PATH];
-+ struct timeval tvs[2];
++ struct timespec ts[2];
+
if (xutftowcs_path(wfilename, file_name) < 0)
return -1;
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
- } else {
- GetSystemTimeAsFileTime(&mft);
- aft = mft;
-+ memset(tvs, 0, sizeof(tvs));
-+ tvs[0].tv_sec = times->actime;
-+ tvs[1].tv_sec = times->modtime;
++ memset(ts, 0, sizeof(ts));
++ ts[0].tv_sec = times->actime;
++ ts[1].tv_sec = times->modtime;
}
- if (!SetFileTime((HANDLE)_get_osfhandle(fh), NULL, &aft, &mft)) {
- errno = EINVAL;
@@ compat/mingw.c: int mingw_utime (const char *file_name, const struct utimbuf *ti
- } else
- rc = 0;
+
-+ rc = mingw_futimes(fh, times ? tvs : NULL);
++ rc = mingw_futimens(fh, times ? ts : NULL);
close(fh);
revert_attrs:
@@ compat/mingw.h: int mingw_fstat(int fd, struct stat *buf);
int mingw_utime(const char *file_name, const struct utimbuf *times);
#define utime mingw_utime
-+int mingw_futimes(int fd, const struct timeval times[2]);
-+#define futimes mingw_futimes
++int mingw_futimens(int fd, const struct timespec times[2]);
++#define futimens mingw_futimens
size_t mingw_strftime(char *s, size_t max,
const char *format, const struct tm *tm);
#define strftime mingw_strftime
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *
- utb.modtime = mtime;
- if (utime(tmp_file.buf, &utb) < 0)
- warning_errno(_("failed utime() on %s"), tmp_file.buf);
-+ struct timeval tvs[2] = {0};
-+ tvs[0].tv_sec = mtime;
-+ tvs[1].tv_sec = mtime;
-+ if (futimes(fd, tvs) < 0)
++ struct timespec ts[2] = {0};
++ ts[0].tv_sec = mtime;
++ ts[1].tv_sec = mtime;
++ if (futimens(fd, ts) < 0)
+ warning_errno(_("failed futimes() on %s"), tmp_file.buf);
}
-: ----------- > 2: 49f72800bfb bulk-checkin: rename 'state' variable and separate 'plugged' boolean
2: d1e68d4a2af ! 3: 2c1c907b12a core.fsyncobjectfiles: batch disk flushes
@@ Metadata
Author: Neeraj Singh [off-list ref]
## Commit message ##
- core.fsyncobjectfiles: batch disk flushes
+ core.fsyncobjectfiles: batched disk flushes
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
- MacOS, and Linux each offer mechanisms to write data from the filesystem
+ macOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
- This patch introduces a new 'core.fsyncObjectFiles = 2' option that
+ This patch introduces a new 'core.fsyncObjectFiles = batch' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
@@ Commit message
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
- later name.
+ later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
- 3. When updating the index and/or refs, we will issue another fsync
- internal to that operation.
+ 3. When updating the index and/or refs, we assume that Git will issue
+ another fsync internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
@@ Commit message
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
- This change also updates the MacOS code to trigger a real hardware flush
+ This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
- MacOS there was no guarantee of durability since a simple fsync(2) call
+ macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
+ _Performance numbers_:
+
+ Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
+ Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
+ Windows - Same host as Linux, a preview version of Windows 11.
+ This number is from a patch later in the series.
+
+ Adding 500 files to the repo with 'git add' Times reported in seconds.
+
+ core.fsyncObjectFiles | Linux | Mac | Windows
+ ----------------------|-------|-------|--------
+ false | 0.06 | 0.35 | 0.61
+ true | 1.88 | 11.18 | 2.47
+ batch | 0.15 | 0.41 | 1.53
+
Signed-off-by: Neeraj Singh [off-list ref]
## Documentation/config/core.txt ##
@@ Documentation/config/core.txt: core.whitespace::
-data writes properly, but can be useful for filesystems that do not use
-journalling (traditional UNIX filesystems) or that only journal metadata
-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
-+ A boolean value or the number '2', indicating the level of durability
-+ applied to object files.
++ A value indicating the level of effort Git will expend in
++ trying to make objects added to the repo durable in the event
++ of an unclean system shutdown. This setting currently only
++ controls the object store, so updates to any refs or the
++ index may not be equally durable.
++
-+This setting controls how much effort Git makes to ensure that data added to
-+the object store are durable in the case of an unclean system shutdown. If
-+'false', Git allows data to remain in file system caches according to operating
-+system policy, whence they may be lost if the system loses power or crashes. A
-+value of 'true' instructs Git to force objects to stable storage immediately
-+when they are added to the object store. The number '2' is an experimental
-+value that also preserves durability but tries to perform hardware flushes in a
-+batch.
++* `false` allows data to remain in file system caches according to
++ operating system policy, whence it may be lost if the system loses power
++ or crashes.
++* `true` triggers a data integrity flush for each object added to the
++ object store. This is the safest setting that is likely to ensure durability
++ across all operating systems and file systems that honor the 'fsync' system
++ call. However, this setting comes with a significant performance cost on
++ common hardware.
++* `batch` enables an experimental mode that uses interfaces available in some
++ operating systems to write object data with a minimal set of FLUSH CACHE
++ (or equivalent) commands sent to the storage controller. If the operating
++ system interfaces are not available, this mode behaves the same as `true`.
++ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
++ filesystems and on Windows for repos stored on NTFS or ReFS.
core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
## Makefile ##
+@@ Makefile: all::
+ #
+ # Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.
+ #
++# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.
++#
+ # Define NEEDS_LIBRT if your platform requires linking with librt (glibc version
+ # before 2.17) for clock_gettime and CLOCK_MONOTONIC.
+ #
@@ Makefile: ifdef HAVE_CLOCK_MONOTONIC
BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
endif
@@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
- ## builtin/update-index.c ##
-@@
- */
- #define USE_THE_INDEX_COMPATIBILITY_MACROS
- #include "cache.h"
-+#include "bulk-checkin.h"
- #include "config.h"
- #include "lockfile.h"
- #include "quote.h"
-@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- struct strbuf unquoted = STRBUF_INIT;
-
- setup_work_tree();
-+ plug_bulk_checkin();
- while (getline_fn(&buf, stdin) != EOF) {
- char *p;
- if (!nul_term_line && buf.buf[0] == '"') {
-@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- chmod_path(set_executable_bit, p);
- free(p);
- }
-+ unplug_bulk_checkin(&lock_file);
- strbuf_release(&unquoted);
- strbuf_release(&buf);
- }
-
## bulk-checkin.c ##
@@
*/
@@ bulk-checkin.c
#include "repository.h"
#include "csum-file.h"
#include "pack.h"
-@@
+ #include "strbuf.h"
++#include "string-list.h"
#include "packfile.h"
#include "object-store.h"
-+struct object_rename {
-+ char *src;
-+ char *dst;
-+};
-+
-+static struct bulk_rename_state {
-+ struct object_rename *renames;
-+ uint32_t alloc_renames;
-+ uint32_t nr_renames;
-+} bulk_rename_state;
-+
- static struct bulk_checkin_state {
- unsigned plugged:1;
+ static int bulk_checkin_plugged;
-@@ bulk-checkin.c: static struct bulk_checkin_state {
- struct pack_idx_entry **written;
- uint32_t alloc_written;
- uint32_t nr_written;
--} state;
++static struct string_list bulk_fsync_state = STRING_LIST_INIT_DUP;
+
-+} bulk_checkin_state;
-
- static void finish_bulk_checkin(struct bulk_checkin_state *state)
- {
- struct object_id oid;
- struct strbuf packname = STRBUF_INIT;
- int i;
-+ unsigned old_plugged;
-
- if (!state->f)
- return;
-@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state)
-
- clear_exit:
- free(state->written);
-+ old_plugged = state->plugged;
- memset(state, 0, sizeof(*state));
-+ state->plugged = old_plugged;
-
- strbuf_release(&packname);
- /* Make objects we just wrote available to ourselves */
+ static struct bulk_checkin_state {
+ char *pack_tmp_name;
+ struct hashfile *f;
+@@ bulk-checkin.c: clear_exit:
reprepare_packed_git(the_repository);
}
-+static void do_sync_and_rename(struct bulk_rename_state *state, struct lock_file *lock_file)
++static void do_sync_and_rename(struct string_list *fsync_state, struct lock_file *lock_file)
+{
-+ if (state->nr_renames) {
-+ int i;
++ if (fsync_state->nr) {
++ struct string_list_item *rename;
+
+ /*
+ * Issue a full hardware flush against the lock file to ensure
@@ bulk-checkin.c: static void finish_bulk_checkin(struct bulk_checkin_state *state
+ */
+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
+
-+ for (i = 0; i < state->nr_renames; i++) {
-+ if (finalize_object_file(state->renames[i].src, state->renames[i].dst))
-+ die_errno(_("could not rename '%s'"), state->renames[i].src);
++ for_each_string_list_item(rename, fsync_state) {
++ const char *src = rename->string;
++ const char *dst = rename->util;
+
-+ free(state->renames[i].src);
-+ free(state->renames[i].dst);
++ if (finalize_object_file(src, dst))
++ die_errno(_("could not rename '%s' to '%s'"), src, dst);
+ }
+
-+ free(state->renames);
-+ memset(state, 0, sizeof(*state));
++ string_list_clear(fsync_state, 1);
+ }
+}
+
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
return 0;
}
-+static void add_rename_bulk_checkin(struct bulk_rename_state *state,
++static void add_rename_bulk_checkin(struct string_list *fsync_state,
+ const char *src, const char *dst)
+{
-+ struct object_rename *rename;
-+
-+ ALLOC_GROW(state->renames, state->nr_renames + 1, state->alloc_renames);
-+
-+ rename = &state->renames[state->nr_renames++];
-+ rename->src = xstrdup(src);
-+ rename->dst = xstrdup(dst);
++ string_list_insert(fsync_state, src)->util = xstrdup(dst);
+}
+
+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
+ const char *filename)
+{
-+ if (fsync_object_files) {
++ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
+ /*
+ * 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 renaming files as part of do_sync_and_rename.
+ */
-+ if (bulk_checkin_state.plugged &&
-+ fsync_object_files == 2 &&
++ if (bulk_checkin_plugged &&
++ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
-+ add_rename_bulk_checkin(&bulk_rename_state, tmpfile, filename);
++ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
+ if (close(fd))
+ die_errno(_("error when closing loose object file"));
+
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type,
const char *path, unsigned flags)
- {
-- int status = deflate_to_pack(&state, oid, fd, size, type,
-+ int status = deflate_to_pack(&bulk_checkin_state, oid, fd, size, type,
- path, flags);
-- if (!state.plugged)
-- finish_bulk_checkin(&state);
-+ if (!bulk_checkin_state.plugged)
-+ finish_bulk_checkin(&bulk_checkin_state);
- return status;
- }
-
- void plug_bulk_checkin(void)
- {
-- state.plugged = 1;
-+ bulk_checkin_state.plugged = 1;
+@@ bulk-checkin.c: void plug_bulk_checkin(void)
+ bulk_checkin_plugged = 1;
}
-void unplug_bulk_checkin(void)
+void unplug_bulk_checkin(struct lock_file *lock_file)
{
-- state.plugged = 0;
-- if (state.f)
-- finish_bulk_checkin(&state);
-+ bulk_checkin_state.plugged = 0;
-+ if (bulk_checkin_state.f)
-+ finish_bulk_checkin(&bulk_checkin_state);
+ assert(bulk_checkin_plugged);
+ bulk_checkin_plugged = 0;
+ if (bulk_checkin_state.f)
+ finish_bulk_checkin(&bulk_checkin_state);
+
-+ do_sync_and_rename(&bulk_rename_state, lock_file);
++ do_sync_and_rename(&bulk_fsync_state, lock_file);
}
## bulk-checkin.h ##
@@ bulk-checkin.h
#endif
+ ## cache.h ##
+@@ cache.h: void reset_shared_repository(void);
+ extern int read_replace_refs;
+ extern char *git_replace_ref_base;
+
+-extern int fsync_object_files;
++enum FSYNC_OBJECT_FILES_MODE {
++ FSYNC_OBJECT_FILES_OFF,
++ FSYNC_OBJECT_FILES_ON,
++ FSYNC_OBJECT_FILES_BATCH
++};
++
++extern enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
+ extern int core_preload_index;
+ extern int precomposed_unicode;
+ extern int protect_hfs;
+
## config.c ##
@@ config.c: static int git_default_core_config(const char *var, const char *value, void *cb)
}
if (!strcmp(var, "core.fsyncobjectfiles")) {
- fsync_object_files = git_config_bool(var, value);
-+ int is_bool;
-+
-+ fsync_object_files = git_config_bool_or_int(var, value, &is_bool);
++ if (!value)
++ return config_error_nonbool(var);
++ if (!strcasecmp(value, "batch"))
++ fsync_object_files = FSYNC_OBJECT_FILES_BATCH;
++ else
++ fsync_object_files = git_config_bool(var, value)
++ ? FSYNC_OBJECT_FILES_ON : FSYNC_OBJECT_FILES_OFF;
return 0;
}
@@ configure.ac: AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
# Define NO_SETITIMER if you don't have setitimer.
GIT_CHECK_FUNC(setitimer,
+ ## environment.c ##
+@@ environment.c: const char *git_hooks_path;
+ int zlib_compression_level = Z_BEST_SPEED;
+ int core_compression_level;
+ int pack_compression_level = Z_DEFAULT_COMPRESSION;
+-int fsync_object_files;
++enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
+ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
+ size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
+ size_t delta_base_cache_limit = 96 * 1024 * 1024;
+
## git-compat-util.h ##
@@ git-compat-util.h: __attribute__((format (printf, 1, 2))) NORETURN
void BUG(const char *fmt, ...);
-: ----------- > 4: 546ad9c82e8 core.fsyncobjectfiles: add windows support for batch mode
-: ----------- > 5: d8843185fe4 update-index: use the bulk-checkin infrastructure
-: ----------- > 6: 73b5d41be94 core.fsyncobjectfiles: performance tests for add and stash
--
gitgitgadget
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:43
From: Neeraj Singh <redacted>
Make close_loose_object do all of the steps for syncing and correctly
naming a new loose object so that it can be reimplemented in the
upcoming bulk-fsync mode.
Use futimens, which is available in POSIX.1-2008 to update the file
timestamps. This should be slightly faster than utime, since we have
a file descriptor already available. This change allows us to update
the time before closing, renaming, and potentially fsyincing the file
being refreshed. This code is currently only invoked by git-pack-objects
via force_object_loose.
Implement a futimens shim for the Windows port of Git.
Signed-off-by: Neeraj Singh <redacted>
---
compat/mingw.c | 53 ++++++++++++++++++++++++++++++++++----------------
compat/mingw.h | 2 ++
object-file.c | 17 ++++++++--------
3 files changed, 46 insertions(+), 26 deletions(-)
@@ -734,11 +734,14 @@ int mingw_chmod(const char *filename, int mode)*TheunitofFILETIMEis100-nanosecondssinceJanuary1,1601,UTC.*Returnsthe100-nanoseconds("hekto nanoseconds")sincetheepoch.*/++#define UNIX_EPOCH_FILETIME 116444736000000000LL+staticinlinelonglongfiletime_to_hnsec(constFILETIME*ft){longlongwinTime=((longlong)ft->dwHighDateTime<<32)+ft->dwLowDateTime;/* Windows to Unix Epoch conversion */-returnwinTime-116444736000000000LL;+returnwinTime-UNIX_EPOCH_FILETIME;}staticinlinevoidfiletime_to_timespec(constFILETIME*ft,structtimespec*ts)
@@ -949,19 +959,33 @@ int mingw_fstat(int fd, struct stat *buf)}}-staticinlinevoidtime_t_to_filetime(time_tt,FILETIME*ft)+intmingw_futimens(intfd,conststructtimespectimes[2]){-longlongwinTime=t*10000000LL+116444736000000000LL;-ft->dwLowDateTime=winTime;-ft->dwHighDateTime=winTime>>32;+FILETIMEmft,aft;++if(times){+timespec_to_filetime(×[0],&aft);+timespec_to_filetime(×[1],&mft);+}else{+GetSystemTimeAsFileTime(&mft);+aft=mft;+}++if(!SetFileTime((HANDLE)_get_osfhandle(fd),NULL,&aft,&mft)){+errno=EINVAL;+return-1;+}++return0;}-intmingw_utime(constchar*file_name,conststructutimbuf*times)+intmingw_utime(constchar*file_name,conststructutimbuf*times){-FILETIMEmft,aft;intfh,rc;DWORDattrs;wchar_twfilename[MAX_PATH];+structtimespects[2];+if(xutftowcs_path(wfilename,file_name)<0)return-1;
@@ -1860,12 +1860,13 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,}/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)+staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename){if(fsync_object_files)fsync_or_die(fd,"loose object file");if(close(fd)!=0)die_errno(_("error when closing loose object file"));+returnfinalize_object_file(tmpfile,filename);}/* Size of directory component, including the ending '/' */
@@ -1973,17 +1974,15 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));-close_loose_object(fd);-if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0)-warning_errno(_("failed utime() on %s"),tmp_file.buf);+structtimespects[2]={0};+ts[0].tv_sec=mtime;+ts[1].tv_sec=mtime;+if(futimens(fd,ts)<0)+warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnfinalize_object_file(tmp_file.buf,filename.buf);+returnclose_loose_object(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:46
From: Neeraj Singh <redacted>
Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.
* Rename 'state' variable to 'bulk_checkin_state', since we will later
be adding 'bulk_fsync_state'. This also makes the variable easier to
find in the debugger, since the name is more unique.
* Move the 'plugged' data member of 'bulk_checkin_state' into a separate
static variable. Doing this avoids resetting the variable in
finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we
seem to unintentionally disable the plugging functionality the first
time a new packfile must be created due to packfile size limits. While
disabling the plugging state only results in suboptimal behavior for
the current code, it would be fatal for the bulk-fsync functionality
later in this patch series.
Signed-off-by: Neeraj Singh <redacted>
---
bulk-checkin.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:47
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
macOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = batch' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Signed-off-by: Neeraj Singh <redacted>
---
Documentation/config/core.txt | 26 ++++++++++---
Makefile | 6 +++
builtin/add.c | 3 +-
bulk-checkin.c | 70 ++++++++++++++++++++++++++++++++++-
bulk-checkin.h | 4 +-
cache.h | 8 +++-
config.c | 8 +++-
config.mak.uname | 2 +
configure.ac | 8 ++++
environment.c | 2 +-
git-compat-util.h | 7 ++++
object-file.c | 12 +-----
wrapper.c | 36 ++++++++++++++++++
write-or-die.c | 2 +-
14 files changed, 170 insertions(+), 24 deletions(-)
@@ -548,12 +548,26 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A value indicating the level of effort Git will expend in+ trying to make objects added to the repo durable in the event+ of an unclean system shutdown. This setting currently only+ controls the object store, so updates to any refs or the+ index may not be equally durable.+++* `false` allows data to remain in file system caches according to+ operating system policy, whence it may be lost if the system loses power+ or crashes.+* `true` triggers a data integrity flush for each object added to the+ object store. This is the safest setting that is likely to ensure durability+ across all operating systems and file systems that honor the 'fsync' system+ call. However, this setting comes with a significant performance cost on+ common hardware.+* `batch` enables an experimental mode that uses interfaces available in some+ operating systems to write object data with a minimal set of FLUSH CACHE+ (or equivalent) commands sent to the storage controller. If the operating+ system interfaces are not available, this mode behaves the same as `true`.+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS+ filesystems and on Windows for repos stored on NTFS or ReFS. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
@@ -406,6 +406,8 @@ all::## Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.#+# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.+## Define NEEDS_LIBRT if your platform requires linking with librt (glibc version# before 2.17) for clock_gettime and CLOCK_MONOTONIC.#
@@ -62,6 +66,32 @@ clear_exit:reprepare_packed_git(the_repository);}+staticvoiddo_sync_and_rename(structstring_list*fsync_state,structlock_file*lock_file)+{+if(fsync_state->nr){+structstring_list_item*rename;++/*+*Issueafullhardwareflushagainstthelockfiletoensure+*thatallobjectsaredurablebeforeanyrenamesoccur.+*Thecodeinfsync_and_close_loose_object_bulk_checkinhas+*alreadyensuredthatwriteouthasoccurred,butithasnot+*flushedanywritebackcacheinthestoragehardware.+*/+fsync_or_die(get_lock_file_fd(lock_file),get_lock_file_path(lock_file));++for_each_string_list_item(rename,fsync_state){+constchar*src=rename->string;+constchar*dst=rename->util;++if(finalize_object_file(src,dst))+die_errno(_("could not rename '%s' to '%s'"),src,dst);+}++string_list_clear(fsync_state,1);+}+}+staticintalready_written(structbulk_checkin_state*state,structobject_id*oid){inti;
@@ -256,6 +286,42 @@ static int deflate_to_pack(struct bulk_checkin_state *state,return0;}+staticvoidadd_rename_bulk_checkin(structstring_list*fsync_state,+constchar*src,constchar*dst)+{+string_list_insert(fsync_state,src)->util=xstrdup(dst);+}++intfsync_and_close_loose_object_bulk_checkin(intfd,constchar*tmpfile,+constchar*filename)+{+if(fsync_object_files!=FSYNC_OBJECT_FILES_OFF){+/*+*Ifwehaveapluggedbulkcheckin,weissueacallthat+*cleansthefilesystempagecachebutavoidsahardwareflush+*command.Lateronwewillissueasinglehardwareflush+*beforerenamingfilesaspartofdo_sync_and_rename.+*/+if(bulk_checkin_plugged&&+fsync_object_files==FSYNC_OBJECT_FILES_BATCH&&+git_fsync(fd,FSYNC_WRITEOUT_ONLY)>=0){+add_rename_bulk_checkin(&bulk_fsync_state,tmpfile,filename);+if(close(fd))+die_errno(_("error when closing loose object file"));++return0;++}else{+fsync_or_die(fd,"loose object file");+}+}++if(close(fd))+die_errno(_("error when closing loose object file"));++returnfinalize_object_file(tmpfile,filename);+}+intindex_bulk_checkin(structobject_id*oid,intfd,size_tsize,enumobject_typetype,constchar*path,unsignedflags)
@@ -1859,16 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticintclose_loose_object(intfd,constchar*tmpfile,constchar*filename)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-returnfinalize_object_file(tmpfile,filename);-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1982,7 +1972,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed futimes() on %s"),tmp_file.buf);}-returnclose_loose_object(fd,tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,filename.buf);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:48
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to snoop the output of --verbose to
find out when update-index has actually processed a given path.
Additionally the index is locked for the duration of the update.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/update-index.c | 3 +++
1 file changed, 3 insertions(+)
@@ -0,0 +1,32 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the current directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++foriin$(test_seq$dirs)+do+localdir=$basedir/dir$i++mkdir-p"$dir">/dev/null+forjin$(test_seq$files)+do+test_create_unique_files_counter__=$((test_create_unique_files_counter__+1))+echo"$test_create_unique_files_base__.$test_create_unique_files_counter__">"$dir/file$j.txt"+done+done+}
@@ -0,0 +1,43 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of add"++../perf-lib.sh++.$TEST_DIRECTORY/perf/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"$GIT_PERF_REPEAT_COUNT"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++test_perf"add $total_files files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$maddfiles+"+done++test_done
@@ -0,0 +1,46 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of stash"++../perf-lib.sh++.$TEST_DIRECTORY/perf/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"$GIT_PERF_REPEAT_COUNT"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++# We only stash files in the 'files' subdirectory since+# the perf test infrastructure creates files in the+# current working directory that need to be preserved+test_perf"stash 500 files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$mstashpush-u--files+"+done++test_done
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-08-27 23:49:52
From: Neeraj Singh <redacted>
This commit adds a win32 implementation for fsync_no_flush that is
called git_fsync. The 'NtFlushBuffersFileEx' function being called is
available since Windows 8. If the function is not available, we
return -1 and Git falls back to doing a full fsync.
The operating system is told to flush data only without a hardware
flush primitive. A later full fsync will cause the metadata log
to be flushed and then the disk cache to be flushed on NTFS and
ReFS. Other filesystems will treat this as a full flush operation.
I added a new file here for this system call so as not to conflict with
downstream changes in the git-for-windows repository related to fscache.
Signed-off-by: Neeraj Singh <redacted>
---
compat/mingw.h | 3 +++
compat/win32/flush.c | 29 +++++++++++++++++++++++++++++
config.mak.uname | 2 ++
contrib/buildsystems/CMakeLists.txt | 3 ++-
wrapper.c | 4 ++++
5 files changed, 40 insertions(+), 1 deletion(-)
create mode 100644 compat/win32/flush.c
On Wed, Aug 25, 2021 at 10:50 PM Christoph Hellwig [off-list ref] wrote:
On Wed, Aug 25, 2021 at 05:49:45PM -0700, Neeraj Singh wrote:
quoted
One conclusion from reviewing that thread is that as of then,
sync_file_ranges isn't actually enough
to make a hard guarantee about writeout occurring. See
https://lore.kernel.org/linux-fsdevel/20190319204330.GY26298@dastard/.
My hope is that the Linux FS developers have rectified that shortcoming by now.
I'm not sure what shortcoming you mean. sync_file_ranges is a system
call that only causes data writeback. It never performs metadata write
back and thus is not an integrity operation at all. That is also very
clearly documented in the man page.
You're right. On re-read of the man page, sync_file_range is listed as
an "extremely dangerous"
system call. The opportunity in the linux kernel is to offer an
alternative set of flags or separate
API that allows for an application like Git to separate a metadata
writeback request from the disk flush.
Separately, I'm hoping I can push from the Windows filesystem side to
get a barrier primitive put into
the NVME standard so that we can offer more useful behavior to
applications rather than these painful
hardware flushes.
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-28 06:57:06
On Fri, Aug 27, 2021 at 05:20:44PM -0700, Neeraj Singh wrote:
You're right. On re-read of the man page, sync_file_range is listed as
an "extremely dangerous"
system call. The opportunity in the linux kernel is to offer an
alternative set of flags or separate
API that allows for an application like Git to separate a metadata
writeback request from the disk flush.
How do you want to do that? I metadata writeback without a cache flush
is worse than useless, in fact it is generally actively harmful.
To take XFS as an example: fsync and fdatasync do the following thing:
1) writeback all dirty data for file to the data device
2) flush the write cache of the data device to ensure they are really
on disk before writing back the metadata referring to them
3) write out the log up until the log sequence that contained the last
modifications to the file
4) flush the cache for the log device.
If the data device and the log device are the same (they usually are
for common setups) and the log device support the FUA bit that writes
through the cache, the log writes use that bit and this step can
be skipped.
So in general there are very few metadata writes, and it is absolutely
essential to flush the cache before that, because otherwise your metadata
could point to data that might not actually have made it to disk.
The best way to optimize such a workload is by first batching all the
data writeout for multiple fils in step one, then only doing one cache
flush and one log force (as we call it) to cover all the files. syncfs
will do that, but without a good way to pick individual files.
Separately, I'm hoping I can push from the Windows filesystem side to
get a barrier primitive put into
the NVME standard so that we can offer more useful behavior to
applications rather than these painful
hardware flushes.
I'm not sure what you mean with barriers, but if you mean the concept
of implying a global ordering on I/Os as we did in Linux back in the
bad old days the barrier bio flag, or badly reinvented by this paper:
https://www.usenix.org/conference/fast18/presentation/won
they might help a little bit with single threaded operations, but will
heavily degrade I/O performance for multithreaded workloads. As an
active member of (but not speaking for) the NVMe technical working group
with a bit of knowledge of SSD internals I also doubt it will be very
well received there.
On Fri, Aug 27, 2021 at 11:57 PM Christoph Hellwig [off-list ref] wrote:
On Fri, Aug 27, 2021 at 05:20:44PM -0700, Neeraj Singh wrote:
quoted
You're right. On re-read of the man page, sync_file_range is listed as
an "extremely dangerous"
system call. The opportunity in the linux kernel is to offer an
alternative set of flags or separate
API that allows for an application like Git to separate a metadata
writeback request from the disk flush.
How do you want to do that? I metadata writeback without a cache flush
is worse than useless, in fact it is generally actively harmful.
To take XFS as an example: fsync and fdatasync do the following thing:
1) writeback all dirty data for file to the data device
2) flush the write cache of the data device to ensure they are really
on disk before writing back the metadata referring to them
3) write out the log up until the log sequence that contained the last
modifications to the file
4) flush the cache for the log device.
If the data device and the log device are the same (they usually are
for common setups) and the log device support the FUA bit that writes
through the cache, the log writes use that bit and this step can
be skipped.
So in general there are very few metadata writes, and it is absolutely
essential to flush the cache before that, because otherwise your metadata
could point to data that might not actually have made it to disk.
The best way to optimize such a workload is by first batching all the
data writeout for multiple fils in step one, then only doing one cache
flush and one log force (as we call it) to cover all the files. syncfs
will do that, but without a good way to pick individual files.
Yes, I think we want to do step (1) of your sequence for all of the files, then
issue steps (2-4) for all files as a group. Of course, if the log
fills up then we
can flush the intermediate steps. The unfortunate thing is that
there's no Linux interface
to do step (1) and to also ensure that the relevant data is in the log
stream or is
otherwise available to be part of the durable metadata.
It seems to me that XFS would be compatible with this sequence if the
appropriate
kernel API exists.
quoted
Separately, I'm hoping I can push from the Windows filesystem side to
get a barrier primitive put into
the NVME standard so that we can offer more useful behavior to
applications rather than these painful
hardware flushes.
I'm not sure what you mean with barriers, but if you mean the concept
of implying a global ordering on I/Os as we did in Linux back in the
bad old days the barrier bio flag, or badly reinvented by this paper:
https://www.usenix.org/conference/fast18/presentation/won
they might help a little bit with single threaded operations, but will
heavily degrade I/O performance for multithreaded workloads. As an
active member of (but not speaking for) the NVMe technical working group
with a bit of knowledge of SSD internals I also doubt it will be very
well received there.
I looked at that paper and definitely agree with you about the questionable
implementation strategy they picked. I don't (yet) have detailed knowledge of
SSD internals, but it's surprising to me that there is little value to
barrier semantics
within the drive as opposed to a full durability sync. At least for
Windows, we have
a database (the Registry) for which any single-threaded latency
improvement would
be welcome.
From: Christoph Hellwig <hch@lst.de> Date: 2021-09-01 05:09:40
On Tue, Aug 31, 2021 at 12:59:14PM -0700, Neeraj Singh wrote:
quoted
So in general there are very few metadata writes, and it is absolutely
essential to flush the cache before that, because otherwise your metadata
could point to data that might not actually have made it to disk.
The best way to optimize such a workload is by first batching all the
data writeout for multiple fils in step one, then only doing one cache
flush and one log force (as we call it) to cover all the files. syncfs
will do that, but without a good way to pick individual files.
Yes, I think we want to do step (1) of your sequence for all of the files, then
issue steps (2-4) for all files as a group. Of course, if the log
fills up then we
can flush the intermediate steps. The unfortunate thing is that
there's no Linux interface
to do step (1) and to also ensure that the relevant data is in the log
stream or is
otherwise available to be part of the durable metadata.
There is also no interface to do 2-4 separately, mostly because they
are so hard to separate. The only API I could envision is one that takes
an array of file descriptors and has the semantics of doing a fsync/
fdatasync for all of them, allowing the implementation to optimize
the order. It would be implementable, but not quite as efficient
as syncfs. I'm also pretty sure we've seen a few attempts at it in
the past that ran into various issues and didn't really make it far.
I looked at that paper and definitely agree with you about the questionable
implementation strategy they picked. I don't (yet) have detailed knowledge of
SSD internals, but it's surprising to me that there is little value to
barrier semantics
within the drive as opposed to a full durability sync. At least for
Windows, we have
a database (the Registry) for which any single-threaded latency
improvement would
be welcome.
The major issue with barrier like in the paper above or as historic
Linux 2.6 kernels had it is that it enforces a global ordering. For
software-only implementation like the Linux one this was already bad
enough, but a hardware/firmware implementation in nvme would mean you'd
have to add global serialize to a storage interface standard and its
implementations, while these are very much about offering parallelisms.
In fact we'd also have very similar issues with the modern Linux block
layer, which has applied some similar ideas.
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget
[off-list ref] wrote:
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
* Introduce a separate preparatory patch to the bulk-checkin infrastructure
to separate the 'plugged' variable and rename the 'state' variable, as
suggested by dscho.
* Add performance numbers to the commit message of the main bulk fsync
patch, as suggested by dscho.
* Add a comment about the non-thread-safety of the bulk-checkin
infrastructure, as suggested by avarab.
* Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested
by dscho and avarab and others.
* Add more details to Documentation/config/core.txt about the various
settings and their intended effects, as suggested by avarab.
* Switch to the string-list API to hold the rename state, as suggested by
avarab.
* Create a separate update-index patch to use bulk-checkin as suggested by
dscho.
* Add Windows support in the upstream git. This is done in a way that
should not conflict with git-for-windows.
* Add new performance tests that shows the delta based on fsync mode.
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
Neeraj Singh (6):
object-file: use futimens rather than utime
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
core.fsyncobjectfiles: add windows support for batch mode
update-index: use the bulk-checkin infrastructure
core.fsyncobjectfiles: performance tests for add and stash
Documentation/config/core.txt | 26 ++++++--
Makefile | 6 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 92 +++++++++++++++++++++++++----
bulk-checkin.h | 4 +-
cache.h | 8 ++-
compat/mingw.c | 53 +++++++++++------
compat/mingw.h | 5 ++
compat/win32/flush.c | 29 +++++++++
config.c | 8 ++-
config.mak.uname | 4 ++
configure.ac | 8 +++
contrib/buildsystems/CMakeLists.txt | 3 +-
environment.c | 2 +-
git-compat-util.h | 7 +++
object-file.c | 23 ++------
t/perf/lib-unique-files.sh | 32 ++++++++++
t/perf/p3700-add.sh | 43 ++++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++++
wrapper.c | 40 +++++++++++++
write-or-die.c | 2 +-
22 files changed, 389 insertions(+), 58 deletions(-)
create mode 100644 compat/win32/flush.c
create mode 100644 t/perf/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v2
Pull-Request: https://github.com/git/git/pull/1076
Hello everyone,
I'd like to bump this review up in people's inboxes since Patch V2
hasn't gotten any traction in over a week.
Thanks in advance for taking a look,
- Neeraj Singh
Windows Core Filesystems Team
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget
[off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
* Introduce a separate preparatory patch to the bulk-checkin infrastructure
to separate the 'plugged' variable and rename the 'state' variable, as
suggested by dscho.
* Add performance numbers to the commit message of the main bulk fsync
patch, as suggested by dscho.
* Add a comment about the non-thread-safety of the bulk-checkin
infrastructure, as suggested by avarab.
* Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested
by dscho and avarab and others.
* Add more details to Documentation/config/core.txt about the various
settings and their intended effects, as suggested by avarab.
* Switch to the string-list API to hold the rename state, as suggested by
avarab.
* Create a separate update-index patch to use bulk-checkin as suggested by
dscho.
* Add Windows support in the upstream git. This is done in a way that
should not conflict with git-for-windows.
* Add new performance tests that shows the delta based on fsync mode.
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
Neeraj Singh (6):
object-file: use futimens rather than utime
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
core.fsyncobjectfiles: add windows support for batch mode
update-index: use the bulk-checkin infrastructure
core.fsyncobjectfiles: performance tests for add and stash
Documentation/config/core.txt | 26 ++++++--
Makefile | 6 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 92 +++++++++++++++++++++++++----
bulk-checkin.h | 4 +-
cache.h | 8 ++-
compat/mingw.c | 53 +++++++++++------
compat/mingw.h | 5 ++
compat/win32/flush.c | 29 +++++++++
config.c | 8 ++-
config.mak.uname | 4 ++
configure.ac | 8 +++
contrib/buildsystems/CMakeLists.txt | 3 +-
environment.c | 2 +-
git-compat-util.h | 7 +++
object-file.c | 23 ++------
t/perf/lib-unique-files.sh | 32 ++++++++++
t/perf/p3700-add.sh | 43 ++++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++++
wrapper.c | 40 +++++++++++++
write-or-die.c | 2 +-
22 files changed, 389 insertions(+), 58 deletions(-)
create mode 100644 compat/win32/flush.c
create mode 100644 t/perf/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v2
Pull-Request: https://github.com/git/git/pull/1076
Hello everyone,
I'd like to bump this review up in people's inboxes since Patch V2
hasn't gotten any traction in over a week.
Thanks in advance for taking a look,
- Neeraj Singh
Windows Core Filesystems Team
From: Randall S. Becker <hidden> Date: 2021-09-07 19:54:18
On September 7, 2021 3:44 PM, Neeraj Singh wrote:
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
While POSIX.1-2008, this function is not available on every single POSIX-compliant platform. Please make sure that the code will not cause a breakage on some platforms - the ones I maintain, in particular. Neither futimes nor futimens is available on either NonStop ia64 or x86. The platform only has utime, so this needs to be wrapped with an option in config.mak.uname.
Thanks,
Randall
On Tue, Sep 7, 2021 at 12:54 PM Randall S. Becker
[off-list ref] wrote:
On September 7, 2021 3:44 PM, Neeraj Singh wrote:
quoted
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
While POSIX.1-2008, this function is not available on every single POSIX-compliant platform. Please make sure that the code will not cause a breakage on some platforms - the ones I maintain, in particular. Neither futimes nor futimens is available on either NonStop ia64 or x86. The platform only has utime, so this needs to be wrapped with an option in config.mak.uname.
Thanks,
Randall
Ugh. Fair enough. How do other contributors feel about me moving back
to utime, but instead just doing the utime over in
builtins/pack-objects.c? The idea would be to eliminate the mtime
logic entirely from write_loose_object and just do it at the top-level
in loosen_unused_packed_objects.
Thanks,
Neeraj
On Tue, Sep 7, 2021 at 12:44 PM Neeraj Singh [off-list ref] wrote:
Hello everyone,
I'd like to bump this review up in people's inboxes since Patch V2
hasn't gotten any traction in over a week.
Thanks in advance for taking a look,
- Neeraj Singh
Windows Core Filesystems Team
BTW, I updated the github PR to enable batch mode everywhere, and all
the tests passed, which is good news to me.
Thanks,
Neeraj
On Tue, Sep 7, 2021 at 12:54 PM Randall S. Becker
[off-list ref] wrote:
quoted
On September 7, 2021 3:44 PM, Neeraj Singh wrote:
quoted
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
While POSIX.1-2008, this function is not available on every single
POSIX-compliant platform. Please make sure that the code will not
cause a breakage on some platforms - the ones I maintain, in
particular. Neither futimes nor futimens is available on either
NonStop ia64 or x86. The platform only has utime, so this needs to
be wrapped with an option in config.mak.uname.
Thanks,
Randall
Ugh. Fair enough. How do other contributors feel about me moving back
to utime, but instead just doing the utime over in
builtins/pack-objects.c? The idea would be to eliminate the mtime
logic entirely from write_loose_object and just do it at the top-level
in loosen_unused_packed_objects.
Aside from where it lives, can't we just have a wrapper that takes both
the filename & fd, and then on some platforms will need to dispatch to a
slower filename-only version, but can hopefully use the new fd-accepting
function?
From: Randall S. Becker <hidden> Date: 2021-09-08 14:04:35
On September 7, 2021 9:23 PM, Ævar Arnfjörð Bjarmason wrote:
Subject: Re: [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles
On Tue, Sep 07 2021, Neeraj Singh wrote:
quoted
On Tue, Sep 7, 2021 at 12:54 PM Randall S. Becker
[off-list ref] wrote:
quoted
On September 7, 2021 3:44 PM, Neeraj Singh wrote:
quoted
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the
previous feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
While POSIX.1-2008, this function is not available on every single
POSIX-compliant platform. Please make sure that the code will not
cause a breakage on some platforms - the ones I maintain, in
particular. Neither futimes nor futimens is available on either
NonStop ia64 or x86. The platform only has utime, so this needs to be
wrapped with an option in config.mak.uname.
Thanks,
Randall
Ugh. Fair enough. How do other contributors feel about me moving back
to utime, but instead just doing the utime over in
builtins/pack-objects.c? The idea would be to eliminate the mtime
logic entirely from write_loose_object and just do it at the top-level
in loosen_unused_packed_objects.
Aside from where it lives, can't we just have a wrapper that takes both the filename & fd, and then on some platforms will need to
dispatch to a slower filename-only version, but can hopefully use the new fd-accepting function?
I'm not really enamoured with this direction at all. It means that any platform would have to potentially skip a version of git (resulting from the broken build from a wrapper that is not compilable) after the patches were applied, unless the patches for all of those platforms are included. Even adding a Makefile option would be similar. This should be an "Enable if supported" feature, not a default-or-broken, feature. At best, I'd have to monitor for the time where the patch is applied and hope I can figure out the wrapper changes (around my $DAYJOB) in time to make the same release. This seems a bit counter to a "keeping things compatible" philosophy. Maybe there's something I'm missing here.
-Randall
On Tue, Sep 7, 2021 at 6:23 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Tue, Sep 07 2021, Neeraj Singh wrote:
quoted
On Tue, Sep 7, 2021 at 12:54 PM Randall S. Becker
[off-list ref] wrote:
quoted
On September 7, 2021 3:44 PM, Neeraj Singh wrote:
quoted
On Fri, Aug 27, 2021 at 4:49 PM Neeraj K. Singh via GitGitGadget [off-list ref] wrote:
quoted
Thanks to everyone for review so far! I've responded to the previous
feedback and changed the patch series a bit.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
While POSIX.1-2008, this function is not available on every single
POSIX-compliant platform. Please make sure that the code will not
cause a breakage on some platforms - the ones I maintain, in
particular. Neither futimes nor futimens is available on either
NonStop ia64 or x86. The platform only has utime, so this needs to
be wrapped with an option in config.mak.uname.
Thanks,
Randall
Ugh. Fair enough. How do other contributors feel about me moving back
to utime, but instead just doing the utime over in
builtins/pack-objects.c? The idea would be to eliminate the mtime
logic entirely from write_loose_object and just do it at the top-level
in loosen_unused_packed_objects.
Aside from where it lives, can't we just have a wrapper that takes both
the filename & fd, and then on some platforms will need to dispatch to a
slower filename-only version, but can hopefully use the new fd-accepting
function?
I had some concerns around using utime() while a file descriptor is open.
There's some risk of sharing violation on Windows (doesn't matter since we'd
be using futimens), but I was also concerned that there might be some OSes that
update the mtime on close(fd), thus overwriting the effects of utime.
Maybe that's an unwarranted concern, but it's part of why I didn't want to have
different call sequences on different OSes.
I'd be happy to implement your suggestion though and see what happens. But I
also feel that this time update thing is pretty ancillary to the real
goal of my change.
I'm only doing it because it's in the same area. The effects of
getting mtime wrong
would be pretty subtle -- I think we'd just not be deleting some
unpacked unreachable
objects as soon as expected. Do you have a strong objection to
lifting the time update
logic out?
From: Neeraj K. Singh via GitGitGadget <hidden> Date: 2021-09-14 03:38:49
Thanks to everyone for review so far!
Changes since v2:
* Removed an unused Makefile define (FSYNC_DOESNT_FLUSH) that slipped in
from an intermediate change.
* Drop the futimens part of the patch and return to just calling utime, now
within the new bulk_checkin code. The utime to futimens change seemed to
be problematic for some platforms (thanks Randall Becker), and is really
orthogonal to the rest of the patch series.
* (Optional commit) Enable batch mode by default so that we can shake loose
any issues relating to deferring the renames until the
unplug_bulk_checkin.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
* Introduce a separate preparatory patch to the bulk-checkin infrastructure
to separate the 'plugged' variable and rename the 'state' variable, as
suggested by dscho.
* Add performance numbers to the commit message of the main bulk fsync
patch, as suggested by dscho.
* Add a comment about the non-thread-safety of the bulk-checkin
infrastructure, as suggested by avarab.
* Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested
by dscho and avarab and others.
* Add more details to Documentation/config/core.txt about the various
settings and their intended effects, as suggested by avarab.
* Switch to the string-list API to hold the rename state, as suggested by
avarab.
* Create a separate update-index patch to use bulk-checkin as suggested by
dscho.
* Add Windows support in the upstream git. This is done in a way that
should not conflict with git-for-windows.
* Add new performance tests that shows the delta based on fsync mode.
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
Neeraj Singh (6):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
core.fsyncobjectfiles: add windows support for batch mode
update-index: use the bulk-checkin infrastructure
core.fsyncobjectfiles: performance tests for add and stash
core.fsyncobjectfiles: enable batch mode for testing
Documentation/config/core.txt | 26 +++++--
Makefile | 6 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 103 +++++++++++++++++++++++++---
bulk-checkin.h | 5 +-
cache.h | 8 ++-
compat/mingw.h | 3 +
compat/win32/flush.c | 29 ++++++++
config.c | 8 ++-
config.mak.uname | 3 +
configure.ac | 8 +++
contrib/buildsystems/CMakeLists.txt | 3 +-
environment.c | 2 +-
git-compat-util.h | 7 ++
object-file.c | 22 +-----
t/perf/lib-unique-files.sh | 32 +++++++++
t/perf/p3700-add.sh | 43 ++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++
wrapper.c | 40 +++++++++++
write-or-die.c | 2 +-
21 files changed, 358 insertions(+), 44 deletions(-)
create mode 100644 compat/win32/flush.c
create mode 100644 t/perf/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v3
Pull-Request: https://github.com/git/git/pull/1076
Range-diff vs v2:
1: fc3d5a7b635 < -: ----------- object-file: use futimens rather than utime
2: 49f72800bfb = 1: d5893e28df1 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
3: 2c1c907b12a ! 2: f8b5b709e9e core.fsyncobjectfiles: batched disk flushes
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
+}
+
+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
-+ const char *filename)
++ const char *filename, time_t mtime)
+{
++ int do_finalize = 1;
++ int ret = 0;
++
+ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
+ /*
+ * If we have a plugged bulk checkin, we issue a call that
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
+ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
+ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
-+ if (close(fd))
-+ die_errno(_("error when closing loose object file"));
-+
-+ return 0;
++ do_finalize = 0;
+
+ } else {
+ fsync_or_die(fd, "loose object file");
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
+ if (close(fd))
+ die_errno(_("error when closing loose object file"));
+
-+ return finalize_object_file(tmpfile, filename);
++ if (mtime) {
++ struct utimbuf utb;
++ utb.actime = mtime;
++ utb.modtime = mtime;
++ if (utime(tmpfile, &utb) < 0)
++ warning_errno(_("failed utime() on %s"), tmpfile);
++ }
++
++ if (do_finalize)
++ ret = finalize_object_file(tmpfile, filename);
++
++ return ret;
+}
+
int index_bulk_checkin(struct object_id *oid,
@@ bulk-checkin.h
#include "cache.h"
-+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile, const char *filename);
++int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
++ const char *filename, time_t mtime);
+
int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type,
@@ config.mak.uname: ifeq ($(uname_S),Linux)
HAVE_GETDELIM = YesPlease
SANE_TEXT_GREP=-a
FREAD_READS_DIRECTORIES = UnfortunatelyYes
-@@ config.mak.uname: ifeq ($(uname_S),Darwin)
- COMPAT_OBJS += compat/precompose_utf8.o
- BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
- BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
-+ BASIC_CFLAGS += -DFSYNC_DOESNT_FLUSH=1
- HAVE_BSD_SYSCTL = YesPlease
- FREAD_READS_DIRECTORIES = UnfortunatelyYes
- HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
## configure.ac ##
@@ configure.ac: AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
@@ object-file.c: int hash_object_file(const struct git_hash_algo *algo, const void
}
-/* Finalize a file on disk, and close it. */
--static int close_loose_object(int fd, const char *tmpfile, const char *filename)
+-static void close_loose_object(int fd)
-{
- if (fsync_object_files)
- fsync_or_die(fd, "loose object file");
- if (close(fd) != 0)
- die_errno(_("error when closing loose object file"));
-- return finalize_object_file(tmpfile, filename);
-}
-
/* Size of directory component, including the ending '/' */
static inline int directory_size(const char *filename)
{
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
- warning_errno(_("failed futimes() on %s"), tmp_file.buf);
- }
+ die(_("confused by unstable object source data for %s"),
+ oid_to_hex(oid));
-- return close_loose_object(fd, tmp_file.buf, filename.buf);
-+ return fsync_and_close_loose_object_bulk_checkin(fd, tmp_file.buf, filename.buf);
+- close_loose_object(fd);
+-
+- if (mtime) {
+- struct utimbuf utb;
+- utb.actime = mtime;
+- utb.modtime = mtime;
+- if (utime(tmp_file.buf, &utb) < 0)
+- warning_errno(_("failed utime() on %s"), tmp_file.buf);
+- }
+-
+- return finalize_object_file(tmp_file.buf, filename.buf);
++ return fsync_and_close_loose_object_bulk_checkin(fd, tmp_file.buf,
++ filename.buf, mtime);
}
static int freshen_loose_object(const struct object_id *oid)
4: 546ad9c82e8 = 3: 815a862e229 core.fsyncobjectfiles: add windows support for batch mode
5: d8843185fe4 = 4: 6b576038986 update-index: use the bulk-checkin infrastructure
6: 73b5d41be94 = 5: b7ca3ba9302 core.fsyncobjectfiles: performance tests for add and stash
-: ----------- > 6: 55a40fc8fd5 core.fsyncobjectfiles: enable batch mode for testing
--
gitgitgadget
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-14 03:39:03
From: Neeraj Singh <redacted>
Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.
* Rename 'state' variable to 'bulk_checkin_state', since we will later
be adding 'bulk_fsync_state'. This also makes the variable easier to
find in the debugger, since the name is more unique.
* Move the 'plugged' data member of 'bulk_checkin_state' into a separate
static variable. Doing this avoids resetting the variable in
finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we
seem to unintentionally disable the plugging functionality the first
time a new packfile must be created due to packfile size limits. While
disabling the plugging state only results in suboptimal behavior for
the current code, it would be fatal for the bulk-fsync functionality
later in this patch series.
Signed-off-by: Neeraj Singh <redacted>
---
bulk-checkin.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-14 03:39:03
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
macOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = batch' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Signed-off-by: Neeraj Singh <redacted>
---
Documentation/config/core.txt | 26 ++++++++---
Makefile | 6 +++
builtin/add.c | 3 +-
bulk-checkin.c | 81 ++++++++++++++++++++++++++++++++++-
bulk-checkin.h | 5 ++-
cache.h | 8 +++-
config.c | 8 +++-
config.mak.uname | 1 +
configure.ac | 8 ++++
environment.c | 2 +-
git-compat-util.h | 7 +++
object-file.c | 22 +---------
wrapper.c | 36 ++++++++++++++++
write-or-die.c | 2 +-
14 files changed, 182 insertions(+), 33 deletions(-)
@@ -548,12 +548,26 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A value indicating the level of effort Git will expend in+ trying to make objects added to the repo durable in the event+ of an unclean system shutdown. This setting currently only+ controls the object store, so updates to any refs or the+ index may not be equally durable.+++* `false` allows data to remain in file system caches according to+ operating system policy, whence it may be lost if the system loses power+ or crashes.+* `true` triggers a data integrity flush for each object added to the+ object store. This is the safest setting that is likely to ensure durability+ across all operating systems and file systems that honor the 'fsync' system+ call. However, this setting comes with a significant performance cost on+ common hardware.+* `batch` enables an experimental mode that uses interfaces available in some+ operating systems to write object data with a minimal set of FLUSH CACHE+ (or equivalent) commands sent to the storage controller. If the operating+ system interfaces are not available, this mode behaves the same as `true`.+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS+ filesystems and on Windows for repos stored on NTFS or ReFS. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
@@ -406,6 +406,8 @@ all::## Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.#+# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.+## Define NEEDS_LIBRT if your platform requires linking with librt (glibc version# before 2.17) for clock_gettime and CLOCK_MONOTONIC.#
@@ -62,6 +66,32 @@ clear_exit:reprepare_packed_git(the_repository);}+staticvoiddo_sync_and_rename(structstring_list*fsync_state,structlock_file*lock_file)+{+if(fsync_state->nr){+structstring_list_item*rename;++/*+*Issueafullhardwareflushagainstthelockfiletoensure+*thatallobjectsaredurablebeforeanyrenamesoccur.+*Thecodeinfsync_and_close_loose_object_bulk_checkinhas+*alreadyensuredthatwriteouthasoccurred,butithasnot+*flushedanywritebackcacheinthestoragehardware.+*/+fsync_or_die(get_lock_file_fd(lock_file),get_lock_file_path(lock_file));++for_each_string_list_item(rename,fsync_state){+constchar*src=rename->string;+constchar*dst=rename->util;++if(finalize_object_file(src,dst))+die_errno(_("could not rename '%s' to '%s'"),src,dst);+}++string_list_clear(fsync_state,1);+}+}+staticintalready_written(structbulk_checkin_state*state,structobject_id*oid){inti;
@@ -256,6 +286,53 @@ static int deflate_to_pack(struct bulk_checkin_state *state,return0;}+staticvoidadd_rename_bulk_checkin(structstring_list*fsync_state,+constchar*src,constchar*dst)+{+string_list_insert(fsync_state,src)->util=xstrdup(dst);+}++intfsync_and_close_loose_object_bulk_checkin(intfd,constchar*tmpfile,+constchar*filename,time_tmtime)+{+intdo_finalize=1;+intret=0;++if(fsync_object_files!=FSYNC_OBJECT_FILES_OFF){+/*+*Ifwehaveapluggedbulkcheckin,weissueacallthat+*cleansthefilesystempagecachebutavoidsahardwareflush+*command.Lateronwewillissueasinglehardwareflush+*beforerenamingfilesaspartofdo_sync_and_rename.+*/+if(bulk_checkin_plugged&&+fsync_object_files==FSYNC_OBJECT_FILES_BATCH&&+git_fsync(fd,FSYNC_WRITEOUT_ONLY)>=0){+add_rename_bulk_checkin(&bulk_fsync_state,tmpfile,filename);+do_finalize=0;++}else{+fsync_or_die(fd,"loose object file");+}+}++if(close(fd))+die_errno(_("error when closing loose object file"));++if(mtime){+structutimbufutb;+utb.actime=mtime;+utb.modtime=mtime;+if(utime(tmpfile,&utb)<0)+warning_errno(_("failed utime() on %s"),tmpfile);+}++if(do_finalize)+ret=finalize_object_file(tmpfile,filename);++returnret;+}+intindex_bulk_checkin(structobject_id*oid,intfd,size_tsize,enumobject_typetype,constchar*path,unsignedflags)
@@ -1859,15 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1973,17 +1964,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));-close_loose_object(fd);--if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0)-warning_errno(_("failed utime() on %s"),tmp_file.buf);-}--returnfinalize_object_file(tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,+filename.buf,mtime);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-14 03:39:06
From: Neeraj Singh <redacted>
This commit adds a win32 implementation for fsync_no_flush that is
called git_fsync. The 'NtFlushBuffersFileEx' function being called is
available since Windows 8. If the function is not available, we
return -1 and Git falls back to doing a full fsync.
The operating system is told to flush data only without a hardware
flush primitive. A later full fsync will cause the metadata log
to be flushed and then the disk cache to be flushed on NTFS and
ReFS. Other filesystems will treat this as a full flush operation.
I added a new file here for this system call so as not to conflict with
downstream changes in the git-for-windows repository related to fscache.
Signed-off-by: Neeraj Singh <redacted>
---
compat/mingw.h | 3 +++
compat/win32/flush.c | 29 +++++++++++++++++++++++++++++
config.mak.uname | 2 ++
contrib/buildsystems/CMakeLists.txt | 3 ++-
wrapper.c | 4 ++++
5 files changed, 40 insertions(+), 1 deletion(-)
create mode 100644 compat/win32/flush.c
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-14 03:39:10
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to snoop the output of --verbose to
find out when update-index has actually processed a given path.
Additionally the index is locked for the duration of the update.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/update-index.c | 3 +++
1 file changed, 3 insertions(+)
@@ -0,0 +1,32 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the current directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++foriin$(test_seq$dirs)+do+localdir=$basedir/dir$i++mkdir-p"$dir">/dev/null+forjin$(test_seq$files)+do+test_create_unique_files_counter__=$((test_create_unique_files_counter__+1))+echo"$test_create_unique_files_base__.$test_create_unique_files_counter__">"$dir/file$j.txt"+done+done+}
@@ -0,0 +1,43 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of add"++../perf-lib.sh++.$TEST_DIRECTORY/perf/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"$GIT_PERF_REPEAT_COUNT"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++test_perf"add $total_files files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$maddfiles+"+done++test_done
@@ -0,0 +1,46 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of stash"++../perf-lib.sh++.$TEST_DIRECTORY/perf/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"$GIT_PERF_REPEAT_COUNT"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++# We only stash files in the 'files' subdirectory since+# the perf test infrastructure creates files in the+# current working directory that need to be preserved+test_perf"stash 500 files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$mstashpush-u--files+"+done++test_done
From: Christoph Hellwig <hch@lst.de> Date: 2021-09-14 05:49:37
On Tue, Sep 14, 2021 at 03:38:39AM +0000, Neeraj K. Singh via GitGitGadget wrote:
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
If this series lands I can give the syncfs variant a spin. It might not
be the best option for gt hosting services, but I think it will be very
helpful for typical developer workstations.
On 14/09/21 10.38, Neeraj Singh via GitGitGadget wrote:
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Interesting here the performance.
You said that core.fsyncObjectFiles=batch performed 2.5x slower than
core.fsyncObjectFile=false on Linux and Windows, why?
--
An old man doll... just what I always wanted! - Clara
On Tue, Sep 14, 2021 at 3:39 AM Bagas Sanjaya [off-list ref] wrote:
On 14/09/21 10.38, Neeraj Singh via GitGitGadget wrote:
quoted
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Interesting here the performance.
You said that core.fsyncObjectFiles=batch performed 2.5x slower than
core.fsyncObjectFile=false on Linux and Windows, why?
The goal of batch mode is to minimize the number of disk cache flush operations.
We still have to issue writes to the disk (and wait for them to
complete) in batch mode,
and on my test system those writes have to cross the VM boundary. The
Mac is running
macOS natively, so performance of the writes is probably a little better.
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:20:17
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null++foriin$(test_seq$dirs)+do+localdir=$basedir/dir$i++mkdir-p"$dir">/dev/null+forjin$(test_seq$files)+do+test_create_unique_files_counter__=$((test_create_unique_files_counter__+1))+echo"$test_create_unique_files_base__.$test_create_unique_files_counter__">"$dir/file$j.txt"+done+done+}
@@ -7,6 +7,8 @@ test_description='Test of git add, including the -- option.' ../test-lib.sh+.$TEST_DIRECTORY/lib-unique-files.sh+# Test the file mode "$1" of the file "$2" in the index. test_mode_in_index(){case"$(gitls-files-s"$2")"in
@@ -33,6 +35,15 @@ test_expect_success \'Test that "git add -- -q" works'\'touch -- -q && git add -- -q'+test_expect_success'git add: core.fsyncobjectfiles=batch'"+test_create_unique_files24fsync-files&&+git-ccore.fsyncobjectfiles=batchadd--./fsync-files/&&+rm-ffsynced_files&&+gitls-files--stagefsync-files/>fsynced_files&&+test_line_count=8fsynced_files&&+catfsynced_files|awk'{print \$2}'|xargs-n1gitcat-file-e+"+ test_expect_success\'git add: Test that executable bit is not used if core.filemode=0'\'gitconfigcore.filemode0&&
@@ -1293,6 +1294,19 @@ test_expect_success 'stash handles skip-worktree entries nicely' 'gitrev-parse--verifyrefs/stash:A.t'+test_expect_success'stash with core.fsyncobjectfiles=batch'"+test_create_unique_files24fsync-files&&+git-ccore.fsyncobjectfiles=batchstashpush-u--./fsync-files/&&+rm-ffsynced_files&&++# The files were untracked, so use the third parent,+# which contains the untracked files+gitls-tree-rstash^3--./fsync-files/>fsynced_files&&+test_line_count=8fsynced_files&&+catfsynced_files|awk'{print \$3}'|xargs-n1gitcat-file-e+"++ test_expect_success'stash -c stash.useBuiltin=false warning ''expected="stash.useBuiltin support has been removed"&&
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:20:18
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
macOS, and Linux each offer mechanisms to write data from the filesystem
page cache without initiating a hardware flush.
This patch introduces a new 'core.fsyncObjectFiles = batch' option that
takes advantage of the bulk-checkin infrastructure to batch up hardware
flushes.
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Signed-off-by: Neeraj Singh <redacted>
---
Documentation/config/core.txt | 26 ++++++++---
Makefile | 6 +++
builtin/add.c | 3 +-
bulk-checkin.c | 81 ++++++++++++++++++++++++++++++++++-
bulk-checkin.h | 5 ++-
cache.h | 8 +++-
config.c | 7 ++-
config.mak.uname | 1 +
configure.ac | 8 ++++
environment.c | 2 +-
git-compat-util.h | 7 +++
object-file.c | 22 +---------
wrapper.c | 44 +++++++++++++++++++
write-or-die.c | 2 +-
14 files changed, 189 insertions(+), 33 deletions(-)
@@ -548,12 +548,26 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A value indicating the level of effort Git will expend in+ trying to make objects added to the repo durable in the event+ of an unclean system shutdown. This setting currently only+ controls the object store, so updates to any refs or the+ index may not be equally durable.+++* `false` allows data to remain in file system caches according to+ operating system policy, whence it may be lost if the system loses power+ or crashes.+* `true` triggers a data integrity flush for each object added to the+ object store. This is the safest setting that is likely to ensure durability+ across all operating systems and file systems that honor the 'fsync' system+ call. However, this setting comes with a significant performance cost on+ common hardware.+* `batch` enables an experimental mode that uses interfaces available in some+ operating systems to write object data with a minimal set of FLUSH CACHE+ (or equivalent) commands sent to the storage controller. If the operating+ system interfaces are not available, this mode behaves the same as `true`.+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS+ filesystems and on Windows for repos stored on NTFS or ReFS. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
@@ -406,6 +406,8 @@ all::## Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.#+# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.+## Define NEEDS_LIBRT if your platform requires linking with librt (glibc version# before 2.17) for clock_gettime and CLOCK_MONOTONIC.#
@@ -62,6 +66,32 @@ clear_exit:reprepare_packed_git(the_repository);}+staticvoiddo_sync_and_rename(structstring_list*fsync_state,structlock_file*lock_file)+{+if(fsync_state->nr){+structstring_list_item*rename;++/*+*Issueafullhardwareflushagainstthelockfiletoensure+*thatallobjectsaredurablebeforeanyrenamesoccur.+*Thecodeinfsync_and_close_loose_object_bulk_checkinhas+*alreadyensuredthatwriteouthasoccurred,butithasnot+*flushedanywritebackcacheinthestoragehardware.+*/+fsync_or_die(get_lock_file_fd(lock_file),get_lock_file_path(lock_file));++for_each_string_list_item(rename,fsync_state){+constchar*src=rename->string;+constchar*dst=rename->util;++if(finalize_object_file(src,dst))+die_errno(_("could not rename '%s' to '%s'"),src,dst);+}++string_list_clear(fsync_state,1);+}+}+staticintalready_written(structbulk_checkin_state*state,structobject_id*oid){inti;
@@ -256,6 +286,53 @@ static int deflate_to_pack(struct bulk_checkin_state *state,return0;}+staticvoidadd_rename_bulk_checkin(structstring_list*fsync_state,+constchar*src,constchar*dst)+{+string_list_insert(fsync_state,src)->util=xstrdup(dst);+}++intfsync_and_close_loose_object_bulk_checkin(intfd,constchar*tmpfile,+constchar*filename,time_tmtime)+{+intdo_finalize=1;+intret=0;++if(fsync_object_files!=FSYNC_OBJECT_FILES_OFF){+/*+*Ifwehaveapluggedbulkcheckin,weissueacallthat+*cleansthefilesystempagecachebutavoidsahardwareflush+*command.Lateronwewillissueasinglehardwareflush+*beforerenamingfilesaspartofdo_sync_and_rename.+*/+if(bulk_checkin_plugged&&+fsync_object_files==FSYNC_OBJECT_FILES_BATCH&&+git_fsync(fd,FSYNC_WRITEOUT_ONLY)>=0){+add_rename_bulk_checkin(&bulk_fsync_state,tmpfile,filename);+do_finalize=0;++}else{+fsync_or_die(fd,"loose object file");+}+}++if(close(fd))+die_errno(_("error when closing loose object file"));++if(mtime){+structutimbufutb;+utb.actime=mtime;+utb.modtime=mtime;+if(utime(tmpfile,&utb)<0)+warning_errno(_("failed utime() on %s"),tmpfile);+}++if(do_finalize)+ret=finalize_object_file(tmpfile,filename);++returnret;+}+intindex_bulk_checkin(structobject_id*oid,intfd,size_tsize,enumobject_typetype,constchar*path,unsignedflags)
@@ -1859,15 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1973,17 +1964,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));-close_loose_object(fd);--if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0)-warning_errno(_("failed utime() on %s"),tmp_file.buf);-}--returnfinalize_object_file(tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,+filename.buf,mtime);}staticintfreshen_loose_object(conststructobject_id*oid)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:20:23
From: Neeraj Singh <redacted>
Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.
* Rename 'state' variable to 'bulk_checkin_state', since we will later
be adding 'bulk_fsync_state'. This also makes the variable easier to
find in the debugger, since the name is more unique.
* Move the 'plugged' data member of 'bulk_checkin_state' into a separate
static variable. Doing this avoids resetting the variable in
finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we
seem to unintentionally disable the plugging functionality the first
time a new packfile must be created due to packfile size limits. While
disabling the plugging state only results in suboptimal behavior for
the current code, it would be fatal for the bulk-fsync functionality
later in this patch series.
Signed-off-by: Neeraj Singh <redacted>
---
bulk-checkin.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:20:26
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to snoop the output of --verbose to
find out when update-index has actually processed a given path.
Additionally the index is locked for the duration of the update.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/update-index.c | 3 +++
1 file changed, 3 insertions(+)
From: Neeraj K. Singh via GitGitGadget <hidden> Date: 2021-09-21 02:21:58
Thanks to everyone for review so far! Changes since v3:
* Fix core.fsyncobjectfiles option parsing as suggested by Junio: We now
accept no value to mean "true" and we require 'batch' to be lowercase.
* Leave the default fsync mode as 'false'. Git for windows can change its
default when this series makes it over to that fork.
* Use a switch statement in git_fsync, as suggested by Junio.
* Add regression test cases for core.fsyncobjectfiles=batch. This should
keep the batch functionality basically working in upstream git even if
few users adopt batch mode initially. I expect git-for-windows will
provide a good baking area for the new mode.
Changes since v2:
* Removed an unused Makefile define (FSYNC_DOESNT_FLUSH) that slipped in
from an intermediate change.
* Drop the futimens part of the patch and return to just calling utime, now
within the new bulk_checkin code. The utime to futimens change seemed to
be problematic for some platforms (thanks Randall Becker), and is really
orthogonal to the rest of the patch series.
* (Optional commit) Enable batch mode by default so that we can shake loose
any issues relating to deferring the renames until the
unplug_bulk_checkin.
Changes since v1:
* Switch from futimes(2) to futimens(2), which is in POSIX.1-2008. Contrary
to dscho's suggestion, I'm still implementing the Windows version in the
same patch and I'm not doing autoconf detection since this is a POSIX
function.
* Introduce a separate preparatory patch to the bulk-checkin infrastructure
to separate the 'plugged' variable and rename the 'state' variable, as
suggested by dscho.
* Add performance numbers to the commit message of the main bulk fsync
patch, as suggested by dscho.
* Add a comment about the non-thread-safety of the bulk-checkin
infrastructure, as suggested by avarab.
* Rename the experimental mode to core.fsyncobjectfiles=batch, as suggested
by dscho and avarab and others.
* Add more details to Documentation/config/core.txt about the various
settings and their intended effects, as suggested by avarab.
* Switch to the string-list API to hold the rename state, as suggested by
avarab.
* Create a separate update-index patch to use bulk-checkin as suggested by
dscho.
* Add Windows support in the upstream git. This is done in a way that
should not conflict with git-for-windows.
* Add new performance tests that shows the delta based on fsync mode.
NOTE: Based on Christoph Hellwig's comments, the 'batch' mode is not correct
on Linux, since sync_file_range does not provide data integrity guarantees.
There is currently no kernel interface suitable to achieve disk flush
batching as is, but he suggested that he might implement a 'syncfs' variant
on top of this patchset. This code is still useful on macOS and Windows, and
the config documentation makes that clear.
Neeraj Singh (6):
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
core.fsyncobjectfiles: add windows support for batch mode
update-index: use the bulk-checkin infrastructure
core.fsyncobjectfiles: tests for batch mode
core.fsyncobjectfiles: performance tests for add and stash
Documentation/config/core.txt | 26 +++++--
Makefile | 6 ++
builtin/add.c | 3 +-
builtin/update-index.c | 3 +
bulk-checkin.c | 103 +++++++++++++++++++++++++---
bulk-checkin.h | 5 +-
cache.h | 8 ++-
compat/mingw.h | 3 +
compat/win32/flush.c | 29 ++++++++
config.c | 7 +-
config.mak.uname | 3 +
configure.ac | 8 +++
contrib/buildsystems/CMakeLists.txt | 3 +-
environment.c | 2 +-
git-compat-util.h | 7 ++
object-file.c | 22 +-----
t/lib-unique-files.sh | 34 +++++++++
t/perf/p3700-add.sh | 43 ++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++
t/t3700-add.sh | 11 +++
t/t3903-stash.sh | 14 ++++
wrapper.c | 48 +++++++++++++
write-or-die.c | 2 +-
23 files changed, 392 insertions(+), 44 deletions(-)
create mode 100644 compat/win32/flush.c
create mode 100644 t/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v4
Pull-Request: https://github.com/git/git/pull/1076
Range-diff vs v3:
1: d5893e28df1 = 1: d5893e28df1 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
2: f8b5b709e9e ! 2: 12cad737635 core.fsyncobjectfiles: batched disk flushes
@@ config.c: static int git_default_core_config(const char *var, const char *value,
if (!strcmp(var, "core.fsyncobjectfiles")) {
- fsync_object_files = git_config_bool(var, value);
-+ if (!value)
-+ return config_error_nonbool(var);
-+ if (!strcasecmp(value, "batch"))
++ if (value && !strcmp(value, "batch"))
+ fsync_object_files = FSYNC_OBJECT_FILES_BATCH;
++ else if (git_config_bool(var, value))
++ fsync_object_files = FSYNC_OBJECT_FILES_ON;
+ else
-+ fsync_object_files = git_config_bool(var, value)
-+ ? FSYNC_OBJECT_FILES_ON : FSYNC_OBJECT_FILES_OFF;
++ fsync_object_files = FSYNC_OBJECT_FILES_OFF;
return 0;
}
@@ wrapper.c: int xmkstemp_mode(char *filename_template, int mode)
+int git_fsync(int fd, enum fsync_action action)
+{
-+ if (action == FSYNC_WRITEOUT_ONLY) {
++ switch (action) {
++ case FSYNC_WRITEOUT_ONLY:
++
+#ifdef __APPLE__
+ /*
-+ * on Mac OS X, fsync just causes filesystem cache writeback but does not
++ * on macOS, fsync just causes filesystem cache writeback but does not
+ * flush hardware caches.
+ */
+ return fsync(fd);
@@ wrapper.c: int xmkstemp_mode(char *filename_template, int mode)
+
+ errno = ENOSYS;
+ return -1;
-+ }
++
++ case FSYNC_HARDWARE_FLUSH:
+
+#ifdef __APPLE__
-+ return fcntl(fd, F_FULLFSYNC);
++ return fcntl(fd, F_FULLFSYNC);
+#else
-+ return fsync(fd);
++ return fsync(fd);
+#endif
++
++ default:
++ BUG("unexpected git_fsync(%d) call", action);
++ }
++
+}
+
static int warn_if_unremovable(const char *op, const char *file, int rc)
3: 815a862e229 ! 3: a5b3e21b762 core.fsyncobjectfiles: add windows support for batch mode
@@ wrapper.c: int git_fsync(int fd, enum fsync_action action)
+
errno = ENOSYS;
return -1;
- }
+
4: 6b576038986 = 4: f7f756f3932 update-index: use the bulk-checkin infrastructure
-: ----------- > 5: afb0028e796 core.fsyncobjectfiles: tests for batch mode
5: b7ca3ba9302 ! 6: 3e6b80b5fa2 core.fsyncobjectfiles: performance tests for add and stash
@@ Commit message
Signed-off-by: Neeraj Singh [off-list ref]
- ## t/perf/lib-unique-files.sh (new) ##
-@@
-+# Helper to create files with unique contents
-+
-+test_create_unique_files_base__=$(date -u)
-+test_create_unique_files_counter__=0
-+
-+# Create multiple files with unique contents. Takes the number of
-+# directories, the number of files in each directory, and the base
-+# directory.
-+#
-+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files
-+# each in the current directory, all
-+# with unique contents.
-+
-+test_create_unique_files() {
-+ test "$#" -ne 3 && BUG "3 param"
-+
-+ local dirs=$1
-+ local files=$2
-+ local basedir=$3
-+
-+ for i in $(test_seq $dirs)
-+ do
-+ local dir=$basedir/dir$i
-+
-+ mkdir -p "$dir" > /dev/null
-+ for j in $(test_seq $files)
-+ do
-+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
-+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
-+ done
-+ done
-+}
-
## t/perf/p3700-add.sh (new) ##
@@
+#!/bin/sh
@@ t/perf/p3700-add.sh (new)
+
+. ./perf-lib.sh
+
-+. $TEST_DIRECTORY/perf/lib-unique-files.sh
++. $TEST_DIRECTORY/lib-unique-files.sh
+
+test_perf_default_repo
+test_checkout_worktree
@@ t/perf/p3700-add.sh (new)
+# We need to create the files each time we run the perf test, but
+# we do not want to measure the cost of creating the files, so run
+# the tet once.
-+if test "$GIT_PERF_REPEAT_COUNT" -ne 1
++if test "${GIT_PERF_REPEAT_COUNT-1}" -ne 1
+then
+ echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
+ GIT_PERF_REPEAT_COUNT=1
@@ t/perf/p3900-stash.sh (new)
+
+. ./perf-lib.sh
+
-+. $TEST_DIRECTORY/perf/lib-unique-files.sh
++. $TEST_DIRECTORY/lib-unique-files.sh
+
+test_perf_default_repo
+test_checkout_worktree
@@ t/perf/p3900-stash.sh (new)
+# We need to create the files each time we run the perf test, but
+# we do not want to measure the cost of creating the files, so run
+# the tet once.
-+if test "$GIT_PERF_REPEAT_COUNT" -ne 1
++if test "${GIT_PERF_REPEAT_COUNT-1}" -ne 1
+then
+ echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
+ GIT_PERF_REPEAT_COUNT=1
6: 55a40fc8fd5 < -: ----------- core.fsyncobjectfiles: enable batch mode for testing
--
gitgitgadget
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:21:59
From: Neeraj Singh <redacted>
This commit adds a win32 implementation for fsync_no_flush that is
called git_fsync. The 'NtFlushBuffersFileEx' function being called is
available since Windows 8. If the function is not available, we
return -1 and Git falls back to doing a full fsync.
The operating system is told to flush data only without a hardware
flush primitive. A later full fsync will cause the metadata log
to be flushed and then the disk cache to be flushed on NTFS and
ReFS. Other filesystems will treat this as a full flush operation.
I added a new file here for this system call so as not to conflict with
downstream changes in the git-for-windows repository related to fscache.
Signed-off-by: Neeraj Singh <redacted>
---
compat/mingw.h | 3 +++
compat/win32/flush.c | 29 +++++++++++++++++++++++++++++
config.mak.uname | 2 ++
contrib/buildsystems/CMakeLists.txt | 3 ++-
wrapper.c | 4 ++++
5 files changed, 40 insertions(+), 1 deletion(-)
create mode 100644 compat/win32/flush.c
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-21 02:21:59
From: Neeraj Singh <redacted>
Add a basic performance test for "git add" and "git stash" of a lot of
new objects with various fsync settings.
Signed-off-by: Neeraj Singh <redacted>
---
t/perf/p3700-add.sh | 43 ++++++++++++++++++++++++++++++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
@@ -0,0 +1,43 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of add"++../perf-lib.sh++.$TEST_DIRECTORY/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"${GIT_PERF_REPEAT_COUNT-1}"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++test_perf"add $total_files files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$maddfiles+"+done++test_done
@@ -0,0 +1,46 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of stash"++../perf-lib.sh++.$TEST_DIRECTORY/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"${GIT_PERF_REPEAT_COUNT-1}"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++# We only stash files in the 'files' subdirectory since+# the perf test infrastructure creates files in the+# current working directory that need to be preserved+test_perf"stash 500 files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$mstashpush-u--files+"+done++test_done
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
Perhaps note too that:
4. For loose objects, refs etc. we may or may not create directories,
and most certainly will be updating metadata on the immediate
directory containing the file, but none of that's fsync()'d.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
There's no discussion of whether this is or isn't known to also work
some Linux FS's, and for these OS's where this does work is this only
for the object files themselves, or does metadata also "ride along"?
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Per my https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com
and 6/6 in this series we've got perf tests for add/stash, but it would
be really interesting to see how this is impacted by
transfer.unpackLimit in cases where we may be writing packs or loose
objects.
[...]
core.fsyncObjectFiles::
- This boolean will enable 'fsync()' when writing object files.
-+
-This is a total waste of time and effort on a filesystem that orders
-data writes properly, but can be useful for filesystems that do not use
-journalling (traditional UNIX filesystems) or that only journal metadata
-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
+ A value indicating the level of effort Git will expend in
+ trying to make objects added to the repo durable in the event
+ of an unclean system shutdown. This setting currently only
+ controls the object store, so updates to any refs or the
+ index may not be equally durable.
All these mentions of "object" should really clarify that it's "loose
objects", i.e. we always fsync pack files.
+* `false` allows data to remain in file system caches according to
+ operating system policy, whence it may be lost if the system loses power
+ or crashes.
As noted in point #4 of
https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com/ while
this direction is overall an improvement over the previously flippant
docs, they at least alluded to the context that the assumption behind
"false" is that you don't really care about loose objects, you care
about loose objects *and* the ref update or whatever.
As I think (this is from memory) we've covered already this may have
been all based on some old ext3 assumption, but it's probably worth
summarizing that here, i.e. if you've got an FS with global ordered
operations you can probably skip this, but probably not etc.
+* `true` triggers a data integrity flush for each object added to the
+ object store. This is the safest setting that is likely to ensure durability
+ across all operating systems and file systems that honor the 'fsync' system
+ call. However, this setting comes with a significant performance cost on
+ common hardware.
This is really overpromising things by omitting the fact that eve if
we're getting this feature you've hacked up right, we're still not
fsyncing dir entries etc (also noted above).
So something that describes the narrow scope here, along with "loose
objects" etc....
+* `batch` enables an experimental mode that uses interfaces available in some
+ operating systems to write object data with a minimal set of FLUSH CACHE
+ (or equivalent) commands sent to the storage controller. If the operating
+ system interfaces are not available, this mode behaves the same as `true`.
+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
+ filesystems and on Windows for repos stored on NTFS or ReFS.
Again, even if it's called "core.fsyncObjectFiles" if we're going to say
"safe" we really need to say safe in what sense. Having written and
fsync()'d the file is helping nobody if the metadata never arrives....
I think less indentation here would be nice:
if (!fsync_state->nr)
return;
/* rest of unindented body */
Or better yet do this check in unplug_bulk_checkin(), then here:
fsync_or_die();
for_each_string_list_item() { ...}
string_list_clear(....);
quoted hunk
+ struct string_list_item *rename;
+
+ /*
+ * Issue a full hardware flush against the lock file to ensure
+ * that all objects are durable before any renames occur.
+ * The code in fsync_and_close_loose_object_bulk_checkin has
+ * already ensured that writeout has occurred, but it has not
+ * flushed any writeback cache in the storage hardware.
+ */
+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
+
+ for_each_string_list_item(rename, fsync_state) {
+ const char *src = rename->string;
+ const char *dst = rename->util;
+
+ if (finalize_object_file(src, dst))
+ die_errno(_("could not rename '%s' to '%s'"), src, dst);
+ }
+
+ string_list_clear(fsync_state, 1);
+ }
+}
+
static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
{
int i;
Just has one caller, why not just inline the string_list_insert()
call...
+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
+ const char *filename, time_t mtime)
+{
+ int do_finalize = 1;
+ int ret = 0;
+
+ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
Let's do postive enum comparisons, and with switch() statements, so the
compiler helps us to see if we've covered them all.
+ /*
+ * 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 renaming files as part of do_sync_and_rename.
+ */
+ if (bulk_checkin_plugged &&
+ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
+ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
+ do_finalize = 0;
+
+ } else {
+ fsync_or_die(fd, "loose object file");
+ }
+ }
So nothing ever explicitly checks FSYNC_OBJECT_FILES_ON...?
Since the point of this setting is safety, let's explicitly check
true/false here, use git_config_maybe_bool(), and perhaps issue a
warning on unknown values, but maybe that would get too verbose...
If we have a future "supersafe" mode, it'll get mapped to "false" on
older versions of git, probably not a good idea...
@@ -1859,15 +1859,6 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,return0;}-/* Finalize a file on disk, and close it. */-staticvoidclose_loose_object(intfd)-{-if(fsync_object_files)-fsync_or_die(fd,"loose object file");-if(close(fd)!=0)-die_errno(_("error when closing loose object file"));-}-/* Size of directory component, including the ending '/' */staticinlineintdirectory_size(constchar*filename){
@@ -1973,17 +1964,8 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));-close_loose_object(fd);--if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0)-warning_errno(_("failed utime() on %s"),tmp_file.buf);-}--returnfinalize_object_file(tmp_file.buf,filename.buf);+returnfsync_and_close_loose_object_bulk_checkin(fd,tmp_file.buf,+filename.buf,mtime);}staticintfreshen_loose_object(conststructobject_id*oid)
See just an informative link to the API docs, or is the comemnt on the
memset() in particular. This comment seems like it's just doing a
Google/Bing search for you, so maybe better without it?
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to snoop the output of --verbose to
find out when update-index has actually processed a given path.
Additionally the index is locked for the duration of the update.
Would you really need to sniff the verbose output? If I'm streaming data
to update-index now it looks like I could assume before that
update-index would have done the work if I managed to fflush() to it,
since it's processing a line at a time and doing the work in that
line-at-a-time loop.
I.e. you could print lines to it, and then do concurrent object lookups
knowing the data was written already...
I think this is probably fine, but that case seems way likelier than
someone sniffing back the verbose output, presumably for the "add" in
update_one(), but that's called in the getline_fn() loop...
All of this makes me wonder why this isn't using tmp-objdir.c, i.e. we
could have our cake and eat it too by writing the "real" objects, and
then just renaming them between directories instead. But perhaps the
answer has something to do with the metadata issues I raised.
And well, tmp-objdir.c isn't going to help someone in practice that's
relying on this "update-index --stdin" behavior, as they won't know
where we staged the temporary files...
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted hunk
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null
Why the >/dev/null? It's not a "-rfv", and any errors would go to
stderr.
+ mkdir -p "$dir" > /dev/null
Ditto.
+ for j in $(test_seq $files)
+ do
+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
Would be much more readable if we these variables were shorter.
But actually, why are we trying to create files as a function of "date
-u" at all? This is all in the trash directory, which is rm -rf'd beween
runs, why aren't names created with test_seq or whatever OK? I.e. just
1.txt, 2.txt....
+test_expect_success 'stash with core.fsyncobjectfiles=batch' "
+ test_create_unique_files 2 4 fsync-files &&
+ git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
+ rm -f fsynced_files &&
+
+ # The files were untracked, so use the third parent,
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
We really prefer our tests to create the same data each time if
possible, but as noted with the "date -u" comment above you're
explicitly bypassing that, but I still can't see why...
On Tue, Sep 21, 2021 at 4:41 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
Perhaps note too that:
4. For loose objects, refs etc. we may or may not create directories,
and most certainly will be updating metadata on the immediate
directory containing the file, but none of that's fsync()'d.
quoted
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
There's no discussion of whether this is or isn't known to also work
some Linux FS's, and for these OS's where this does work is this only
for the object files themselves, or does metadata also "ride along"?
I unfortunately can't examine Linux kernel source code and the details
of metadata
consistency behavior across files is not something that anyone in that
group wants
to pin down. As far as I can tell, the only thing that's really
guaranteed is fsyncing
every single file you write down and its parent directory if you're
creating a new file
(which we always are). As came up in conversation with Christoph
Hellwig elsewhere
on thread, Linux doesn't have any set of syscalls to make batch mode
safe. It does look
like XFS would be safe if sync_file_ranges actually promised to wait
for all pagecache
writeback definitively, since it would do a "log force" to push all
the dirty metadata to
disk when we do our final fsync.
I really didn't want to say something definitive about what Linux can
or will do, since I'm
not in a position to really know or influence them. Christoph did say
that he would be
interested in contributing a variant to this patch that would be
definitively safe on filesystems
that honor syncfs.
quoted
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Per my https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com
and 6/6 in this series we've got perf tests for add/stash, but it would
be really interesting to see how this is impacted by
transfer.unpackLimit in cases where we may be writing packs or loose
objects.
I'm having trouble understanding how unpackLimit is related to 'git stash'
or 'git add'. From code inspection, it doesn't look like we're using
those settings
for adding objects except from across a transport.
Are you proposing that we have a similar setting for adding objects
via 'add' using
a packfile? I think that would be a good goal, but it might be a bit
tricky since we've
likely done a lot of the work to buffer the input objects in order to
compute their OIDs,
before we know how many objects there are to add. If the policy were
to "always add to
a packfile", it would be easier.
quoted
[...]
core.fsyncObjectFiles::
- This boolean will enable 'fsync()' when writing object files.
-+
-This is a total waste of time and effort on a filesystem that orders
-data writes properly, but can be useful for filesystems that do not use
-journalling (traditional UNIX filesystems) or that only journal metadata
-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
+ A value indicating the level of effort Git will expend in
+ trying to make objects added to the repo durable in the event
+ of an unclean system shutdown. This setting currently only
+ controls the object store, so updates to any refs or the
+ index may not be equally durable.
All these mentions of "object" should really clarify that it's "loose
objects", i.e. we always fsync pack files.
quoted
+* `false` allows data to remain in file system caches according to
+ operating system policy, whence it may be lost if the system loses power
+ or crashes.
As noted in point #4 of
https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com/ while
this direction is overall an improvement over the previously flippant
docs, they at least alluded to the context that the assumption behind
"false" is that you don't really care about loose objects, you care
about loose objects *and* the ref update or whatever.
As I think (this is from memory) we've covered already this may have
been all based on some old ext3 assumption, but it's probably worth
summarizing that here, i.e. if you've got an FS with global ordered
operations you can probably skip this, but probably not etc.
quoted
+* `true` triggers a data integrity flush for each object added to the
+ object store. This is the safest setting that is likely to ensure durability
+ across all operating systems and file systems that honor the 'fsync' system
+ call. However, this setting comes with a significant performance cost on
+ common hardware.
This is really overpromising things by omitting the fact that eve if
we're getting this feature you've hacked up right, we're still not
fsyncing dir entries etc (also noted above).
So something that describes the narrow scope here, along with "loose
objects" etc....
quoted
+* `batch` enables an experimental mode that uses interfaces available in some
+ operating systems to write object data with a minimal set of FLUSH CACHE
+ (or equivalent) commands sent to the storage controller. If the operating
+ system interfaces are not available, this mode behaves the same as `true`.
+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
+ filesystems and on Windows for repos stored on NTFS or ReFS.
Again, even if it's called "core.fsyncObjectFiles" if we're going to say
"safe" we really need to say safe in what sense. Having written and
fsync()'d the file is helping nobody if the metadata never arrives....
My concern with your feedback here is that this is user-facing documentation.
I'd assume that people who are not intimately familiar with both their
filesystem
and Git's internals would just be completely mystified by a long commentary on
the specifics in the Config documentation. I think over time Git should focus on
making this setting really guarantee durability in a meaningful way
across the entire
repository.
I think less indentation here would be nice:
if (!fsync_state->nr)
return;
/* rest of unindented body */
Will fix.
Or better yet do this check in unplug_bulk_checkin(), then here:
fsync_or_die();
for_each_string_list_item() { ...}
string_list_clear(....);
I'd prefer to put it in the callee for reasons of
separation-of-concerns. I don't want
to have the caller and callee partially implement the contract. The
compiler should
do a good enough job, since it's only one caller and will probably get
totally inilined.
quoted
+ struct string_list_item *rename;
+
+ /*
+ * Issue a full hardware flush against the lock file to ensure
+ * that all objects are durable before any renames occur.
+ * The code in fsync_and_close_loose_object_bulk_checkin has
+ * already ensured that writeout has occurred, but it has not
+ * flushed any writeback cache in the storage hardware.
+ */
+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
+
+ for_each_string_list_item(rename, fsync_state) {
+ const char *src = rename->string;
+ const char *dst = rename->util;
+
+ if (finalize_object_file(src, dst))
+ die_errno(_("could not rename '%s' to '%s'"), src, dst);
+ }
+
+ string_list_clear(fsync_state, 1);
+ }
+}
+
static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
{
int i;
Just has one caller, why not just inline the string_list_insert()
call...
I thought about doing that before. I'll do it.
quoted
+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
+ const char *filename, time_t mtime)
+{
+ int do_finalize = 1;
+ int ret = 0;
+
+ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
Let's do postive enum comparisons, and with switch() statements, so the
compiler helps us to see if we've covered them all.
Ok, will switch to switch.
quoted
+ /*
+ * 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 renaming files as part of do_sync_and_rename.
+ */
+ if (bulk_checkin_plugged &&
+ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
+ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
+ do_finalize = 0;
+
+ } else {
+ fsync_or_die(fd, "loose object file");
+ }
+ }
So nothing ever explicitly checks FSYNC_OBJECT_FILES_ON...?
Yeah, I did it this way to avoid any code duplication, but I can change to
a switch if it doesn't require too much repetition.
Since the point of this setting is safety, let's explicitly check
true/false here, use git_config_maybe_bool(), and perhaps issue a
warning on unknown values, but maybe that would get too verbose...
If we have a future "supersafe" mode, it'll get mapped to "false" on
older versions of git, probably not a good idea...
I took Junio's suggestion verbatim. I'll try a warning if the value
exists, and is not 'batch' or <maybe bool>.
Thanks for looking at my changes so thoroughly!
-Neeraj
See just an informative link to the API docs, or is the comemnt on the
memset() in particular. This comment seems like it's just doing a
Google/Bing search for you, so maybe better without it?
Will remove. Just wanted to make sure everyone knows taht this is
documented somewhere :).
On Tue, Sep 21, 2021 at 4:53 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to snoop the output of --verbose to
find out when update-index has actually processed a given path.
Additionally the index is locked for the duration of the update.
Would you really need to sniff the verbose output? If I'm streaming data
to update-index now it looks like I could assume before that
update-index would have done the work if I managed to fflush() to it,
since it's processing a line at a time and doing the work in that
line-at-a-time loop.
I.e. you could print lines to it, and then do concurrent object lookups
knowing the data was written already...
I think this is probably fine, but that case seems way likelier than
someone sniffing back the verbose output, presumably for the "add" in
update_one(), but that's called in the getline_fn() loop...
Does fflush really guarantee that the reader has picked up the input from
a pipe across all environments? Even if a reader picks up the input, does
that mean that the reader is done processing it?
Do you think I really need to revise this comment? Maybe leave a terser,
'this usage is thought to be unlikely'?
All of this makes me wonder why this isn't using tmp-objdir.c, i.e. we
could have our cake and eat it too by writing the "real" objects, and
then just renaming them between directories instead. But perhaps the
answer has something to do with the metadata issues I raised.
And well, tmp-objdir.c isn't going to help someone in practice that's
relying on this "update-index --stdin" behavior, as they won't know
where we staged the temporary files...
One motivation of the current design behind renaming the files is that
some networked filesystems don't seem to like cross-directory renames
much. It also so happens that ReFS on Windows also prefers renames to
stay within the directory. Actually any filesystem would likely be
slightly faster,
since fewer objects are being modified (one dir versus two).
On Tue, Sep 21, 2021 at 4:58 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null
Why the >/dev/null? It's not a "-rfv", and any errors would go to
stderr.
Will fix. Clearly I don't know UNIX very well.
quoted
+ mkdir -p "$dir" > /dev/null
Ditto.
Will fix.
quoted
+ for j in $(test_seq $files)
+ do
+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
Would be much more readable if we these variables were shorter.
But actually, why are we trying to create files as a function of "date
-u" at all? This is all in the trash directory, which is rm -rf'd beween
runs, why aren't names created with test_seq or whatever OK? I.e. just
1.txt, 2.txt....
The uniqueness is in the contents of the file. I wanted to make sure that
we are really creating new objects and not reusing old ones. Is the scope
of the "trash repo" small enough that I can be guaranteed that a new one
is created before my test since the last time I tried adding something to
the ODB?
quoted
+test_expect_success 'stash with core.fsyncobjectfiles=batch' "
+ test_create_unique_files 2 4 fsync-files &&
+ git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
+ rm -f fsynced_files &&
+
+ # The files were untracked, so use the third parent,
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
We really prefer our tests to create the same data each time if
possible, but as noted with the "date -u" comment above you're
explicitly bypassing that, but I still can't see why...
I'm trying to make sure we get new object contents. Is there a better
way to achieve what I want without the risk of finding that the contents
are already in the database from a previous test run?
Thanks again for the thorough review,
-Neeraj
On Tue, Sep 21, 2021 at 4:58 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null
Why the >/dev/null? It's not a "-rfv", and any errors would go to
stderr.
Will fix. Clearly I don't know UNIX very well.
quoted
quoted
+ mkdir -p "$dir" > /dev/null
Ditto.
Will fix.
quoted
quoted
+ for j in $(test_seq $files)
+ do
+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
Would be much more readable if we these variables were shorter.
But actually, why are we trying to create files as a function of "date
-u" at all? This is all in the trash directory, which is rm -rf'd beween
runs, why aren't names created with test_seq or whatever OK? I.e. just
1.txt, 2.txt....
The uniqueness is in the contents of the file. I wanted to make sure that
we are really creating new objects and not reusing old ones. Is the scope
of the "trash repo" small enough that I can be guaranteed that a new one
is created before my test since the last time I tried adding something to
the ODB?
quoted
quoted
+test_expect_success 'stash with core.fsyncobjectfiles=batch' "
+ test_create_unique_files 2 4 fsync-files &&
+ git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
+ rm -f fsynced_files &&
+
+ # The files were untracked, so use the third parent,
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
We really prefer our tests to create the same data each time if
possible, but as noted with the "date -u" comment above you're
explicitly bypassing that, but I still can't see why...
I'm trying to make sure we get new object contents. Is there a better
way to achieve what I want without the risk of finding that the contents
are already in the database from a previous test run?
You can just do something like:
test_expect_success 'setup data' '
test_commit A &&
test_commit B
'
Which will create files A.t, B.t etc, or create them via:
obj=$(echo foo | git hash-object -w --stdin)
etc.
I.e. the uniqueness you're doing here seems to assume that tests are
re-using the same object store across runs, but we create a new trash
directory for each one, if you run the test with "-d" you can see it
being left behind for inspection. This is already ensured for the test.
The only potential caveat I can imagine is that some filesystem like say
btrfs-like that does some COW or object de-duplication would behave
differently, but other than that...
On Tue, Sep 21, 2021 at 4:41 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
When the new mode is enabled we do the following for new objects:
1. Create a tmp_obj_XXXX file and write the object data to it.
2. Issue a pagecache writeback request and wait for it to complete.
3. Record the tmp name and the final name in the bulk-checkin state for
later rename.
At the end of the entire transaction we:
1. Issue a fsync against the lock file to flush the hardware writeback
cache, which should by now have processed the tmp file writes.
2. Rename all of the temp files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation.
Perhaps note too that:
4. For loose objects, refs etc. we may or may not create directories,
and most certainly will be updating metadata on the immediate
directory containing the file, but none of that's fsync()'d.
quoted
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
There's no discussion of whether this is or isn't known to also work
some Linux FS's, and for these OS's where this does work is this only
for the object files themselves, or does metadata also "ride along"?
I unfortunately can't examine Linux kernel source code and the details
of metadata
consistency behavior across files is not something that anyone in that
group wants
to pin down. As far as I can tell, the only thing that's really
guaranteed is fsyncing
every single file you write down and its parent directory if you're
creating a new file
(which we always are). As came up in conversation with Christoph
Hellwig elsewhere
on thread, Linux doesn't have any set of syscalls to make batch mode
safe. It does look
like XFS would be safe if sync_file_ranges actually promised to wait
for all pagecache
writeback definitively, since it would do a "log force" to push all
the dirty metadata to
disk when we do our final fsync.
I really didn't want to say something definitive about what Linux can
or will do, since I'm
not in a position to really know or influence them. Christoph did say
that he would be
interested in contributing a variant to this patch that would be
definitively safe on filesystems
that honor syncfs.
*nod*, it's fine if it's omitted. Just wondering if we knew but weren't
saying etc.
quoted
quoted
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Per my https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com
and 6/6 in this series we've got perf tests for add/stash, but it would
be really interesting to see how this is impacted by
transfer.unpackLimit in cases where we may be writing packs or loose
objects.
I'm having trouble understanding how unpackLimit is related to 'git stash'
or 'git add'. From code inspection, it doesn't look like we're using
those settings
for adding objects except from across a transport.
Are you proposing that we have a similar setting for adding objects
via 'add' using
a packfile? I think that would be a good goal, but it might be a bit
tricky since we've
likely done a lot of the work to buffer the input objects in order to
compute their OIDs,
before we know how many objects there are to add. If the policy were
to "always add to
a packfile", it would be easier.
No, just that in the documentation that we should be explaining to the
reader that this mode that optimizes for loose object writing benefits
particular commands, but e.g. on the server-side that we'll probably
never write 500 objects, but stream them to one pack.
Which might also inform next steps for the commands this does help with,
i.e. can we make more things stream to packs? I think having this mode
is at worst a good transitory thing to have, but perhaps longer term
we'll want to simply write fewer individual loose objects.
In any case, pushing to a server with this configured and scaling that
by transfer.unpackLimit should nicely demonstrate the pack v.s. loose
object scenario at different fsck-settings.
quoted
quoted
[...]
core.fsyncObjectFiles::
- This boolean will enable 'fsync()' when writing object files.
-+
-This is a total waste of time and effort on a filesystem that orders
-data writes properly, but can be useful for filesystems that do not use
-journalling (traditional UNIX filesystems) or that only journal metadata
-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").
+ A value indicating the level of effort Git will expend in
+ trying to make objects added to the repo durable in the event
+ of an unclean system shutdown. This setting currently only
+ controls the object store, so updates to any refs or the
+ index may not be equally durable.
All these mentions of "object" should really clarify that it's "loose
objects", i.e. we always fsync pack files.
quoted
+* `false` allows data to remain in file system caches according to
+ operating system policy, whence it may be lost if the system loses power
+ or crashes.
As noted in point #4 of
https://lore.kernel.org/git/87mtp5cwpn.fsf@evledraar.gmail.com/ while
this direction is overall an improvement over the previously flippant
docs, they at least alluded to the context that the assumption behind
"false" is that you don't really care about loose objects, you care
about loose objects *and* the ref update or whatever.
As I think (this is from memory) we've covered already this may have
been all based on some old ext3 assumption, but it's probably worth
summarizing that here, i.e. if you've got an FS with global ordered
operations you can probably skip this, but probably not etc.
quoted
+* `true` triggers a data integrity flush for each object added to the
+ object store. This is the safest setting that is likely to ensure durability
+ across all operating systems and file systems that honor the 'fsync' system
+ call. However, this setting comes with a significant performance cost on
+ common hardware.
This is really overpromising things by omitting the fact that eve if
we're getting this feature you've hacked up right, we're still not
fsyncing dir entries etc (also noted above).
So something that describes the narrow scope here, along with "loose
objects" etc....
quoted
+* `batch` enables an experimental mode that uses interfaces available in some
+ operating systems to write object data with a minimal set of FLUSH CACHE
+ (or equivalent) commands sent to the storage controller. If the operating
+ system interfaces are not available, this mode behaves the same as `true`.
+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
+ filesystems and on Windows for repos stored on NTFS or ReFS.
Again, even if it's called "core.fsyncObjectFiles" if we're going to say
"safe" we really need to say safe in what sense. Having written and
fsync()'d the file is helping nobody if the metadata never arrives....
My concern with your feedback here is that this is user-facing documentation.
I'd assume that people who are not intimately familiar with both their
filesystem
and Git's internals would just be completely mystified by a long commentary on
the specifics in the Config documentation. I think over time Git should focus on
making this setting really guarantee durability in a meaningful way
across the entire
repository.
Yeah, this setting though is probably going to be tweaked only by fairly
expert-level users of git.
I think it's fine if it just explicitly punts and says something like
'this is what it does, this may or may not work on your FS' etc., my
main issue with the current docs is that they give off this vibe of
knowing a lot more than they're telling you.
I think less indentation here would be nice:
if (!fsync_state->nr)
return;
/* rest of unindented body */
Will fix.
quoted
Or better yet do this check in unplug_bulk_checkin(), then here:
fsync_or_die();
for_each_string_list_item() { ...}
string_list_clear(....);
I'd prefer to put it in the callee for reasons of
separation-of-concerns. I don't want
to have the caller and callee partially implement the contract. The
compiler should
do a good enough job, since it's only one caller and will probably get
totally inilined.
*nod*
For what it's worth I meant the "inlined" just in terms of avoiding the
indirection for human readers, it won't matter to the machine,
especially since this is all I/O bound...
On Tue, Sep 21, 2021 at 7:02 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Tue, Sep 21 2021, Neeraj Singh wrote:
quoted
On Tue, Sep 21, 2021 at 4:58 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null
Why the >/dev/null? It's not a "-rfv", and any errors would go to
stderr.
Will fix. Clearly I don't know UNIX very well.
quoted
quoted
+ mkdir -p "$dir" > /dev/null
Ditto.
Will fix.
quoted
quoted
+ for j in $(test_seq $files)
+ do
+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
Would be much more readable if we these variables were shorter.
But actually, why are we trying to create files as a function of "date
-u" at all? This is all in the trash directory, which is rm -rf'd beween
runs, why aren't names created with test_seq or whatever OK? I.e. just
1.txt, 2.txt....
The uniqueness is in the contents of the file. I wanted to make sure that
we are really creating new objects and not reusing old ones. Is the scope
of the "trash repo" small enough that I can be guaranteed that a new one
is created before my test since the last time I tried adding something to
the ODB?
quoted
quoted
+test_expect_success 'stash with core.fsyncobjectfiles=batch' "
+ test_create_unique_files 2 4 fsync-files &&
+ git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
+ rm -f fsynced_files &&
+
+ # The files were untracked, so use the third parent,
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
We really prefer our tests to create the same data each time if
possible, but as noted with the "date -u" comment above you're
explicitly bypassing that, but I still can't see why...
I'm trying to make sure we get new object contents. Is there a better
way to achieve what I want without the risk of finding that the contents
are already in the database from a previous test run?
You can just do something like:
test_expect_success 'setup data' '
test_commit A &&
test_commit B
'
Which will create files A.t, B.t etc, or create them via:
obj=$(echo foo | git hash-object -w --stdin)
etc.
I.e. the uniqueness you're doing here seems to assume that tests are
re-using the same object store across runs, but we create a new trash
directory for each one, if you run the test with "-d" you can see it
being left behind for inspection. This is already ensured for the test.
The only potential caveat I can imagine is that some filesystem like say
btrfs-like that does some COW or object de-duplication would behave
differently, but other than that...
It looks like the same repo is reused for each test_expect_success
line in the top-level t*.sh script.
So for test_create_unique_files to be maximally useful, it should have
some state that is different for
each invocation. How about I use the test_tick mechanism to produce
this uniqueness? It wouldn't
be globally unique like the date method, but it should be good enough
if the repo is recycled every time
test-lib is reinitialized.
I'm changing lib-unique-files to use test_tick and to be a little more
readable as you suggested. Please
let me know if you have any other suggestions.
Since the point of this setting is safety, let's explicitly check
true/false here, use git_config_maybe_bool(), and perhaps issue a
warning on unknown values, but maybe that would get too verbose...
If we have a future "supersafe" mode, it'll get mapped to "false" on
older versions of git, probably not a good idea...
I took Junio's suggestion verbatim. I'll try a warning if the value
exists, and is not 'batch' or <maybe bool>.
An update on this. I tested out some values:
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=batch add ./
fsync_object_files: 2
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=0 add ./
fsync_object_files: 0
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=1 add ./
fsync_object_files: 1
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=2 add ./
fsync_object_files: 1
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=barf add ./
fatal: bad boolean config value 'barf' for 'core.fsyncobjectfiles'
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=true add ./
fsync_object_files: 1
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=false add ./
fsync_object_files: 0
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=t add ./
fatal: bad boolean config value 't' for 'core.fsyncobjectfiles'
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=y add ./
fatal: bad boolean config value 'y' for 'core.fsyncobjectfiles'
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=yes add ./
fsync_object_files: 1
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=no add ./
fsync_object_files: 0
nksingh@neerajsi-x1:~/src/git$ ./git -c core.fsyncobjectfiles=nope add ./
fatal: bad boolean config value 'nope' for 'core.fsyncobjectfiles'
So I think the code already works like you are suggesting (thanks Junio!).
On Tue, Sep 21, 2021 at 7:02 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Tue, Sep 21 2021, Neeraj Singh wrote:
quoted
On Tue, Sep 21, 2021 at 4:58 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:
quoted
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for 'git add'
and 'git stash'. These tests ensure that the added
data winds up in the object database.
I verified the tests by introducing an incorrect rename
in do_sync_and_rename.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 34 ++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 11 +++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
3 files changed, 59 insertions(+)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,34 @@+# Helper to create files with unique contents++test_create_unique_files_base__=$(date-u)+test_create_unique_files_counter__=0++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files+# each in the specified directory, all+# with unique contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3++rm-rf$basedir>/dev/null
Why the >/dev/null? It's not a "-rfv", and any errors would go to
stderr.
Will fix. Clearly I don't know UNIX very well.
quoted
quoted
+ mkdir -p "$dir" > /dev/null
Ditto.
Will fix.
quoted
quoted
+ for j in $(test_seq $files)
+ do
+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
Would be much more readable if we these variables were shorter.
But actually, why are we trying to create files as a function of "date
-u" at all? This is all in the trash directory, which is rm -rf'd beween
runs, why aren't names created with test_seq or whatever OK? I.e. just
1.txt, 2.txt....
The uniqueness is in the contents of the file. I wanted to make sure that
we are really creating new objects and not reusing old ones. Is the scope
of the "trash repo" small enough that I can be guaranteed that a new one
is created before my test since the last time I tried adding something to
the ODB?
quoted
quoted
+test_expect_success 'stash with core.fsyncobjectfiles=batch' "
+ test_create_unique_files 2 4 fsync-files &&
+ git -c core.fsyncobjectfiles=batch stash push -u -- ./fsync-files/ &&
+ rm -f fsynced_files &&
+
+ # The files were untracked, so use the third parent,
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
We really prefer our tests to create the same data each time if
possible, but as noted with the "date -u" comment above you're
explicitly bypassing that, but I still can't see why...
I'm trying to make sure we get new object contents. Is there a better
way to achieve what I want without the risk of finding that the contents
are already in the database from a previous test run?
You can just do something like:
test_expect_success 'setup data' '
test_commit A &&
test_commit B
'
Which will create files A.t, B.t etc, or create them via:
obj=$(echo foo | git hash-object -w --stdin)
etc.
I.e. the uniqueness you're doing here seems to assume that tests are
re-using the same object store across runs, but we create a new trash
directory for each one, if you run the test with "-d" you can see it
being left behind for inspection. This is already ensured for the test.
The only potential caveat I can imagine is that some filesystem like say
btrfs-like that does some COW or object de-duplication would behave
differently, but other than that...
It looks like the same repo is reused for each test_expect_success
line in the top-level t*.sh script.
So for test_create_unique_files to be maximally useful, it should have
some state that is different for
each invocation. How about I use the test_tick mechanism to produce
this uniqueness? It wouldn't
be globally unique like the date method, but it should be good enough
if the repo is recycled every time
test-lib is reinitialized.
I'm changing lib-unique-files to use test_tick and to be a little more
readable as you suggested. Please
let me know if you have any other suggestions.
Ah, sorry, I thought you meant you wanted uniqueness within the test
file, but no, by default we'll create *one* repo for you, and each
test_expect_success reuses that.
Generally tests that want that do one of (in each test_expect_success):
# I'm making my own repo
git init new-repo 1 &&
(
cd new-repo-1 &&
[...]
)
# Or, in the first one
<setup the repo data>
# Then, in a second one
git clone . new-repo-1
I.e. just using "git clone" to ferry the data around, or cp -R if you'd
like to retain the exact file layout etc.
On Tue, Sep 21, 2021 at 6:27 PM Neeraj Singh [off-list ref] wrote:
On Tue, Sep 21, 2021 at 4:53 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
All of this makes me wonder why this isn't using tmp-objdir.c, i.e. we
could have our cake and eat it too by writing the "real" objects, and
then just renaming them between directories instead. But perhaps the
answer has something to do with the metadata issues I raised.
And well, tmp-objdir.c isn't going to help someone in practice that's
relying on this "update-index --stdin" behavior, as they won't know
where we staged the temporary files...
One motivation of the current design behind renaming the files is that
some networked filesystems don't seem to like cross-directory renames
much. It also so happens that ReFS on Windows also prefers renames to
stay within the directory. Actually any filesystem would likely be
slightly faster,
since fewer objects are being modified (one dir versus two).
Whelp, as part of v5 I tried to make unpack-objects.c use the batch fsync
mode and now I see a strong reason to take your tmp-objdir suggestion. As
part of OBJ_REF_DELTA unpacking, we need access to the object while
we're in the plugged state. I didn't notice this at first, but got
lucky that I tested
that case first and hit an error.
V5 will create a tmp-objdir and add a new interface to install it as the primary
objdir.
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:18
From: Neeraj Singh <redacted>
If a temporary ODB is active, as determined by GIT_QUARANTINE_PATH
being set, create object files with their final names. This avoids
an extra rename beyond what is needed to merge the temporary ODB in
tmp_objdir_migrate.
Creating an object file with the expected final name should be okay
since the git process writing to the temporary object store is the
only writer, and it only invokes write_loose_object/create_object_file
after checking that the object doesn't exist.
Signed-off-by: Neeraj Singh <redacted>
---
environment.c | 4 ++++
object-file.c | 51 ++++++++++++++++++++++++++++++++++----------------
object-store.h | 6 ++++++
repository.c | 2 ++
repository.h | 1 +
5 files changed, 48 insertions(+), 16 deletions(-)
@@ -1878,21 +1883,37 @@ static inline int directory_size(const char *filename)}/*-*Thiscreatesatemporaryfileinthesamedirectoryasthefinal-*'filename'+*Thiscreatesalooseobjectfileforthespecifiedobjectid.+*Ifwe'reworkinginatemporaryobjectdirectory,thefileis+*createdwithitsfinalfilename,otherwiseitiscreatedwith+*atemporarynameandrenamedbyfinalize_object_file.+*Ifnorenameisrequired,anemptystringisreturnedintmp.**Wewanttoavoidcross-directoryfilenamerenames,becausethose*canhaveproblemsonvariousfilesystems(FAT,NFS,Coda).*/-staticintcreate_tmpfile(structstrbuf*tmp,constchar*filename)+staticintcreate_objfile(conststructobject_id*oid,structstrbuf*tmp,+structstrbuf*filename){-intfd,dirlen=directory_size(filename);+intfd,dirlen,is_retrying=0;+constchar*object_name;+staticconstintobject_mode=0444;+loose_object_path(the_repository,filename,oid);+dirlen=directory_size(filename->buf);++retry_create:strbuf_reset(tmp);-strbuf_add(tmp,filename,dirlen);-strbuf_addstr(tmp,"tmp_obj_XXXXXX");-fd=git_mkstemp_mode(tmp->buf,0444);-if(fd<0&&dirlen&&errno==ENOENT){+if(!the_repository->objects->odb->is_temp){+strbuf_add(tmp,filename->buf,dirlen);+object_name="tmp_obj_XXXXXX";+strbuf_addstr(tmp,object_name);+fd=git_mkstemp_mode(tmp->buf,object_mode);+}else{+fd=open(filename->buf,O_CREAT|O_EXCL|O_RDWR,object_mode);+}++if(fd<0&&dirlen&&errno==ENOENT&&!is_retrying){/**Makesurethedirectoryexists;notethatthecontents*ofthebufferareundefinedaftermkstempreturnsan
@@ -1900,15 +1921,15 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)*scratch.*/strbuf_reset(tmp);-strbuf_add(tmp,filename,dirlen-1);+strbuf_add(tmp,filename->buf,dirlen-1);if(mkdir(tmp->buf,0777)&&errno!=EEXIST)return-1;if(adjust_shared_perm(tmp->buf))return-1;/* Try again */-strbuf_addstr(tmp,"/tmp_obj_XXXXXX");-fd=git_mkstemp_mode(tmp->buf,0444);+is_retrying=1;+gotoretry_create;}returnfd;}
@@ -1925,14 +1946,12 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;-loose_object_path(the_repository,&filename,oid);--fd=create_tmpfile(&tmp_file,filename.buf);+fd=create_objfile(oid,&tmp_file,&filename);if(fd<0){if(errno==EACCES)returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+returnerror_errno(_("unable to create object file"));}/* Set it up */
From: Neeraj K. Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:20
Thanks to everyone for review so far! Changes since v4, all in response to
review feedback from Ævar Arnfjörð Bjarmason:
* Update core.fsyncobjectfiles documentation to specify 'loose' objects and
to add a statement about not fsyncing parent directories.
* I still don't want to make any promises on behalf of the Linux FS developers
in the documentation. However, according to [v4.1] and my understanding
of how XFS journals are documented to work, it looks like recent versions
of Linux running on XFS should be as safe as Windows or macOS in 'batch'
mode. I don't know about ext4, since it's not clear to me when metadata
updates are made visible to the journal.
* Rewrite the core batched fsync change to use the tmp-objdir lib. As Ævar
pointed out, this lets us access the added loose objects immediately,
rather than only after unplugging the bulk checkin. This is a hard
requirement in unpack-objects for resolving OBJ_REF_DELTA packed objects.
* As a preparatory patch, the object-file code now doesn't do a rename if it's in a
tmp objdir (as determined by the quarantine environment variable).
* I added support to the tmp-objdir lib to replace the 'main' writable odb.
* Instead of using a lockfile for the final full fsync, we now use a new dummy
temp file. Doing that makes the below unpack-objects change easier.
* Add bulk-checkin support to unpack-objects, which is used in fetch and
push. In addition to making those operations faster, it allows us to
directly compare performance of packfiles against loose objects. Please
see [v4.2] for a measurement of 'git push' to a local upstream with
different numbers of unique new files.
* Rename FSYNC_OBJECT_FILES_MODE to fsync_object_files_mode.
* Remove comment with link to NtFlushBuffersFileEx documentation.
* Make t/lib-unique-files.sh a bit cleaner. We are still creating unique
contents, but now this uses test_tick, so it should be deterministic from
run to run.
* Ensure there are tests for all of the modified commands. Make the
unpack-objects tests validate that the unpacked objects are really
available in the ODB.
References for v4: [v4.1]
https://lore.kernel.org/linux-fsdevel/20190419072938.31320-1-amir73il@gmail.com/#t
[v4.2]
https://docs.google.com/spreadsheets/d/1uxMBkEXFFnQ1Y3lXKqcKpw6Mq44BzhpCAcPex14T-QQ/edit#gid=1898936117
Changes since v3:
* Fix core.fsyncobjectfiles option parsing as suggested by Junio: We now
accept no value to mean "true" and we require 'batch' to be lowercase.
* Leave the default fsync mode as 'false'. Git for windows can change its
default when this series makes it over to that fork.
* Use a switch statement in git_fsync, as suggested by Junio.
* Add regression test cases for core.fsyncobjectfiles=batch. This should
keep the batch functionality basically working in upstream git even if
few users adopt batch mode initially. I expect git-for-windows will
provide a good baking area for the new mode.
Neeraj Singh (7):
object-file.c: do not rename in a temp odb
bulk-checkin: rename 'state' variable and separate 'plugged' boolean
core.fsyncobjectfiles: batched disk flushes
update-index: use the bulk-checkin infrastructure
unpack-objects: use the bulk-checkin infrastructure
core.fsyncobjectfiles: tests for batch mode
core.fsyncobjectfiles: performance tests for add and stash
Documentation/config/core.txt | 29 +++++++--
Makefile | 6 ++
builtin/add.c | 1 +
builtin/unpack-objects.c | 3 +
builtin/update-index.c | 6 ++
bulk-checkin.c | 92 +++++++++++++++++++++++---
bulk-checkin.h | 2 +
cache.h | 8 ++-
config.c | 7 +-
config.mak.uname | 1 +
configure.ac | 8 +++
environment.c | 6 +-
git-compat-util.h | 7 ++
object-file.c | 118 +++++++++++++++++++++++++++++-----
object-store.h | 22 +++++++
object.c | 2 +-
repository.c | 2 +
repository.h | 1 +
t/lib-unique-files.sh | 36 +++++++++++
t/perf/p3700-add.sh | 43 +++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++
t/t3700-add.sh | 20 ++++++
t/t3903-stash.sh | 14 ++++
t/t5300-pack-object.sh | 30 +++++----
tmp-objdir.c | 20 +++++-
tmp-objdir.h | 6 ++
wrapper.c | 44 +++++++++++++
write-or-die.c | 2 +-
28 files changed, 532 insertions(+), 50 deletions(-)
create mode 100644 t/lib-unique-files.sh
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1076%2Fneerajsi-msft%2Fneerajsi%2Fbulk-fsync-object-files-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1076/neerajsi-msft/neerajsi/bulk-fsync-object-files-v5
Pull-Request: https://github.com/git/git/pull/1076
Range-diff vs v4:
-: ----------- > 1: 95315f35a28 object-file.c: do not rename in a temp odb
1: d5893e28df1 = 2: df6fab94d67 bulk-checkin: rename 'state' variable and separate 'plugged' boolean
2: 12cad737635 ! 3: fe19cdfc930 core.fsyncobjectfiles: batched disk flushes
@@ Commit message
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
- macOS, and Linux each offer mechanisms to write data from the filesystem
- page cache without initiating a hardware flush.
+ and macOS offer mechanisms to write data from the filesystem page cache
+ without initiating a hardware flush. Linux has the sync_file_range API,
+ which issues a pagecache writeback request reliably after version 5.2.
This patch introduces a new 'core.fsyncObjectFiles = batch' option that
- takes advantage of the bulk-checkin infrastructure to batch up hardware
- flushes.
+ batches up hardware flushes. It hooks into the bulk-checkin plugging and
+ unplugging functionality and takes advantage of tmp-objdir.
- When the new mode is enabled we do the following for new objects:
-
- 1. Create a tmp_obj_XXXX file and write the object data to it.
+ When the new mode is enabled we do the following for each new object:
+ 1. Create the object in a tmp-objdir.
2. Issue a pagecache writeback request and wait for it to complete.
- 3. Record the tmp name and the final name in the bulk-checkin state for
- later rename.
- At the end of the entire transaction we:
- 1. Issue a fsync against the lock file to flush the hardware writeback
- cache, which should by now have processed the tmp file writes.
- 2. Rename all of the temp files to their final names.
+ At the end of the entire transaction when unplugging bulk checkin we:
+ 1. Issue an fsync against a dummy file to flush the hardware writeback
+ cache, which should by now have processed the tmp-objdir writes.
+ 2. Rename all of the tmp-objdir files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
- another fsync internal to that operation.
+ another fsync internal to that operation. This is not the case today,
+ but may be a good extension to those components.
On a filesystem with a singular journal that is updated during name
- operations (e.g. create, link, rename, etc), such as NTFS and HFS+, we
+ operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
@@ Documentation/config/core.txt: core.whitespace::
+ A value indicating the level of effort Git will expend in
+ trying to make objects added to the repo durable in the event
+ of an unclean system shutdown. This setting currently only
-+ controls the object store, so updates to any refs or the
-+ index may not be equally durable.
++ controls loose objects in the object store, so updates to any
++ refs or the index may not be equally durable.
++
+* `false` allows data to remain in file system caches according to
+ operating system policy, whence it may be lost if the system loses power
+ or crashes.
-+* `true` triggers a data integrity flush for each object added to the
++* `true` triggers a data integrity flush for each loose object added to the
+ object store. This is the safest setting that is likely to ensure durability
+ across all operating systems and file systems that honor the 'fsync' system
+ call. However, this setting comes with a significant performance cost on
-+ common hardware.
++ common hardware. Git does not currently fsync parent directories for
++ newly-added files, so some filesystems may still allow data to be lost on
++ system crash.
+* `batch` enables an experimental mode that uses interfaces available in some
-+ operating systems to write object data with a minimal set of FLUSH CACHE
-+ (or equivalent) commands sent to the storage controller. If the operating
-+ system interfaces are not available, this mode behaves the same as `true`.
-+ This mode is expected to be safe on macOS for repos stored on HFS+ or APFS
-+ filesystems and on Windows for repos stored on NTFS or ReFS.
++ operating systems to write loose object data with a minimal set of FLUSH
++ CACHE (or equivalent) commands sent to the storage controller. If the
++ operating system interfaces are not available, this mode behaves the same as
++ `true`. This mode is expected to be as safe as `true` on macOS for repos
++ stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or
++ ReFS.
core.preloadIndex::
Enable parallel index preload for operations like 'git diff'
@@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix)
if (chmod_arg && pathspec.nr)
exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
-- unplug_bulk_checkin();
+
-+ unplug_bulk_checkin(&lock_file);
+ unplug_bulk_checkin();
finish:
- if (write_locked_index(&the_index, &lock_file,
## bulk-checkin.c ##
@@
@@ bulk-checkin.c
#include "pack.h"
#include "strbuf.h"
+#include "string-list.h"
++#include "tmp-objdir.h"
#include "packfile.h"
#include "object-store.h"
static int bulk_checkin_plugged;
-
-+static struct string_list bulk_fsync_state = STRING_LIST_INIT_DUP;
++static int needs_batch_fsync;
+
++static struct tmp_objdir *bulk_fsync_objdir;
+
static struct bulk_checkin_state {
char *pack_tmp_name;
- struct hashfile *f;
@@ bulk-checkin.c: clear_exit:
reprepare_packed_git(the_repository);
}
-+static void do_sync_and_rename(struct string_list *fsync_state, struct lock_file *lock_file)
++/*
++ * Cleanup after batch-mode fsync_object_files.
++ */
++static void do_batch_fsync(void)
+{
-+ if (fsync_state->nr) {
-+ struct string_list_item *rename;
-+
-+ /*
-+ * Issue a full hardware flush against the lock file to ensure
-+ * that all objects are durable before any renames occur.
-+ * The code in fsync_and_close_loose_object_bulk_checkin has
-+ * already ensured that writeout has occurred, but it has not
-+ * flushed any writeback cache in the storage hardware.
-+ */
-+ fsync_or_die(get_lock_file_fd(lock_file), get_lock_file_path(lock_file));
-+
-+ for_each_string_list_item(rename, fsync_state) {
-+ const char *src = rename->string;
-+ const char *dst = rename->util;
-+
-+ if (finalize_object_file(src, dst))
-+ die_errno(_("could not rename '%s' to '%s'"), src, dst);
-+ }
-+
-+ string_list_clear(fsync_state, 1);
++ /*
++ * 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);
+ }
++
++ if (bulk_fsync_objdir)
++ tmp_objdir_migrate(bulk_fsync_objdir);
+}
+
static int already_written(struct bulk_checkin_state *state, struct object_id *oid)
@@ bulk-checkin.c: static int deflate_to_pack(struct bulk_checkin_state *state,
return 0;
}
-+static void add_rename_bulk_checkin(struct string_list *fsync_state,
-+ const char *src, const char *dst)
++void fsync_loose_object_bulk_checkin(int fd)
+{
-+ string_list_insert(fsync_state, src)->util = xstrdup(dst);
-+}
-+
-+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
-+ const char *filename, time_t mtime)
-+{
-+ int do_finalize = 1;
-+ int ret = 0;
-+
-+ if (fsync_object_files != FSYNC_OBJECT_FILES_OFF) {
-+ /*
-+ * 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 renaming files as part of do_sync_and_rename.
-+ */
-+ if (bulk_checkin_plugged &&
-+ fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
-+ git_fsync(fd, FSYNC_WRITEOUT_ONLY) >= 0) {
-+ add_rename_bulk_checkin(&bulk_fsync_state, tmpfile, filename);
-+ do_finalize = 0;
-+
-+ } else {
-+ fsync_or_die(fd, "loose object file");
-+ }
-+ }
-+
-+ if (close(fd))
-+ die_errno(_("error when closing loose object file"));
-+
-+ if (mtime) {
-+ struct utimbuf utb;
-+ utb.actime = mtime;
-+ utb.modtime = mtime;
-+ if (utime(tmpfile, &utb) < 0)
-+ warning_errno(_("failed utime() on %s"), tmpfile);
++ assert(fsync_object_files == FSYNC_OBJECT_FILES_BATCH);
++
++ /*
++ * 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(the_repository->objects->odb->is_temp);
++ if (!needs_batch_fsync)
++ needs_batch_fsync = 1;
++ } else {
++ fsync_or_die(fd, "loose object file");
+ }
-+
-+ if (do_finalize)
-+ ret = finalize_object_file(tmpfile, filename);
-+
-+ return ret;
+}
+
int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type,
const char *path, unsigned flags)
-@@ bulk-checkin.c: void plug_bulk_checkin(void)
+@@ bulk-checkin.c: int index_bulk_checkin(struct object_id *oid,
+ void plug_bulk_checkin(void)
+ {
+ assert(!bulk_checkin_plugged);
++
++ /*
++ * Create a temporary object directory if the current
++ * object directory is not already temporary.
++ */
++ if (fsync_object_files == FSYNC_OBJECT_FILES_BATCH &&
++ !the_repository->objects->odb->is_temp) {
++ bulk_fsync_objdir = tmp_objdir_create();
++ if (!bulk_fsync_objdir)
++ die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch"));
++
++ tmp_objdir_replace_main_odb(bulk_fsync_objdir);
++ }
++
bulk_checkin_plugged = 1;
}
--void unplug_bulk_checkin(void)
-+void unplug_bulk_checkin(struct lock_file *lock_file)
- {
- assert(bulk_checkin_plugged);
+@@ bulk-checkin.c: void unplug_bulk_checkin(void)
bulk_checkin_plugged = 0;
if (bulk_checkin_state.f)
finish_bulk_checkin(&bulk_checkin_state);
+
-+ do_sync_and_rename(&bulk_fsync_state, lock_file);
++ do_batch_fsync();
}
## bulk-checkin.h ##
@@ bulk-checkin.h
#include "cache.h"
-+int fsync_and_close_loose_object_bulk_checkin(int fd, const char *tmpfile,
-+ const char *filename, time_t mtime);
++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);
-
- void plug_bulk_checkin(void);
--void unplug_bulk_checkin(void);
-+void unplug_bulk_checkin(struct lock_file *);
-
- #endif
## cache.h ##
@@ cache.h: void reset_shared_repository(void);
@@ cache.h: void reset_shared_repository(void);
extern char *git_replace_ref_base;
-extern int fsync_object_files;
-+enum FSYNC_OBJECT_FILES_MODE {
++enum fsync_object_files_mode {
+ FSYNC_OBJECT_FILES_OFF,
+ FSYNC_OBJECT_FILES_ON,
+ FSYNC_OBJECT_FILES_BATCH
+};
+
-+extern enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
++extern enum fsync_object_files_mode fsync_object_files;
extern int core_preload_index;
extern int precomposed_unicode;
extern int protect_hfs;
@@ environment.c: const char *git_hooks_path;
int core_compression_level;
int pack_compression_level = Z_DEFAULT_COMPRESSION;
-int fsync_object_files;
-+enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
++enum fsync_object_files_mode fsync_object_files;
size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
size_t delta_base_cache_limit = 96 * 1024 * 1024;
@@ git-compat-util.h: __attribute__((format (printf, 1, 2))) NORETURN
* Returns 0 on success, which includes trying to unlink an object that does
## object-file.c ##
-@@ object-file.c: int hash_object_file(const struct git_hash_algo *algo, const void *buf,
- return 0;
+@@ object-file.c: void add_to_alternates_memory(const char *reference)
+ '\n', NULL, 0);
}
--/* Finalize a file on disk, and close it. */
--static void close_loose_object(int fd)
--{
++struct object_directory *set_temporary_main_odb(const char *dir)
++{
++ struct object_directory *main_odb, *new_odb, *old_next;
++
++ /*
++ * Make sure alternates are initialized, or else our entry may be
++ * overwritten when they are.
++ */
++ prepare_alt_odb(the_repository);
++
++ /* Copy the existing object directory and make it an alternate. */
++ main_odb = the_repository->objects->odb;
++ new_odb = xmalloc(sizeof(*new_odb));
++ *new_odb = *main_odb;
++ *the_repository->objects->odb_tail = new_odb;
++ the_repository->objects->odb_tail = &(new_odb->next);
++ new_odb->next = NULL;
++
++ /*
++ * Reinitialize the main odb with the specified path, being careful
++ * to keep the next pointer value.
++ */
++ old_next = main_odb->next;
++ memset(main_odb, 0, sizeof(*main_odb));
++ main_odb->next = old_next;
++ main_odb->is_temp = 1;
++ main_odb->path = xstrdup(dir);
++ return new_odb;
++}
++
++void restore_main_odb(struct object_directory *odb)
++{
++ struct object_directory **prev, *main_odb;
++
++ /* Unlink the saved previous main ODB from the list. */
++ prev = &the_repository->objects->odb->next;
++ assert(*prev);
++ while (*prev != odb) {
++ prev = &(*prev)->next;
++ }
++ *prev = odb->next;
++ if (*prev == NULL)
++ the_repository->objects->odb_tail = prev;
++
++ /*
++ * Restore the data from the old main odb, being careful to
++ * keep the next pointer value
++ */
++ main_odb = the_repository->objects->odb;
++ SWAP(*main_odb, *odb);
++ main_odb->next = odb->next;
++ free_object_directory(odb);
++}
++
+ /*
+ * Compute the exact path an alternate is at and returns it. In case of
+ * error NULL is returned and the human readable error is added to `err`
+@@ object-file.c: 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)
+ {
- if (fsync_object_files)
-- fsync_or_die(fd, "loose object file");
-- if (close(fd) != 0)
-- die_errno(_("error when closing loose object file"));
--}
--
- /* Size of directory component, including the ending '/' */
- static inline int directory_size(const char *filename)
++ switch (fsync_object_files) {
++ case FSYNC_OBJECT_FILES_OFF:
++ break;
++ case FSYNC_OBJECT_FILES_ON:
+ fsync_or_die(fd, "loose object file");
++ break;
++ case FSYNC_OBJECT_FILES_BATCH:
++ fsync_loose_object_bulk_checkin(fd);
++ break;
++ default:
++ BUG("Invalid fsync_object_files mode.");
++ }
++
+ if (close(fd) != 0)
+ die_errno(_("error when closing loose object file"));
+ }
+
+ ## object-store.h ##
+@@ object-store.h: void add_to_alternates_file(const char *dir);
+ */
+ void add_to_alternates_memory(const char *dir);
+
++/*
++ * Replace the current main object directory with the specified temporary
++ * object directory. We make a copy of the former main object directory,
++ * add it as an in-memory alternate, and return the copy so that it can
++ * be restored via restore_main_odb.
++ */
++struct object_directory *set_temporary_main_odb(const char *dir);
++
++/*
++ * Restore a previous ODB replaced by set_temporary_main_odb.
++ */
++void restore_main_odb(struct object_directory *odb);
++
+ /*
+ * Populate and return the loose object cache array corresponding to the
+ * given object ID.
+@@ object-store.h: struct oidtree *odb_loose_cache(struct object_directory *odb,
+ /* Empty the loose object cache for the specified object directory. */
+ void odb_clear_loose_cache(struct object_directory *odb);
+
++/* Clear and free the specified object directory */
++void free_object_directory(struct object_directory *odb);
++
+ struct packed_git {
+ struct hashmap_entry packmap_ent;
+ struct packed_git *next;
+
+ ## object.c ##
+@@ object.c: struct raw_object_store *raw_object_store_new(void)
+ return o;
+ }
+
+-static void free_object_directory(struct object_directory *odb)
++void free_object_directory(struct object_directory *odb)
{
-@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
- die(_("confused by unstable object source data for %s"),
- oid_to_hex(oid));
+ free(odb->path);
+ odb_clear_loose_cache(odb);
+
+ ## tmp-objdir.c ##
+@@
+ struct tmp_objdir {
+ struct strbuf path;
+ struct strvec env;
++ struct object_directory *prev_main_odb;
+ };
-- close_loose_object(fd);
--
-- if (mtime) {
-- struct utimbuf utb;
-- utb.actime = mtime;
-- utb.modtime = mtime;
-- if (utime(tmp_file.buf, &utb) < 0)
-- warning_errno(_("failed utime() on %s"), tmp_file.buf);
-- }
--
-- return finalize_object_file(tmp_file.buf, filename.buf);
-+ return fsync_and_close_loose_object_bulk_checkin(fd, tmp_file.buf,
-+ filename.buf, mtime);
+ /*
+@@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
+ * freeing memory; it may cause a deadlock if the signal
+ * arrived while libc's allocator lock is held.
+ */
+- if (!on_signal)
++ if (!on_signal) {
++ if (t->prev_main_odb)
++ restore_main_odb(t->prev_main_odb);
+ tmp_objdir_free(t);
++ }
++
+ return err;
}
- static int freshen_loose_object(const struct object_id *oid)
+@@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(void)
+ t = xmalloc(sizeof(*t));
+ strbuf_init(&t->path, 0);
+ strvec_init(&t->env);
++ t->prev_main_odb = NULL;
+
+ strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
+
+@@ tmp-objdir.c: int tmp_objdir_migrate(struct tmp_objdir *t)
+ if (!t)
+ return 0;
+
++ if (t->prev_main_odb) {
++ restore_main_odb(t->prev_main_odb);
++ t->prev_main_odb = NULL;
++ }
++
+ strbuf_addbuf(&src, &t->path);
+ strbuf_addstr(&dst, get_object_directory());
+
+@@ tmp-objdir.c: void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)
+ {
+ add_to_alternates_memory(t->path.buf);
+ }
++
++void tmp_objdir_replace_main_odb(struct tmp_objdir *t)
++{
++ if (t->prev_main_odb)
++ BUG("the main object database is already replaced");
++ t->prev_main_odb = set_temporary_main_odb(t->path.buf);
++}
+
+ ## tmp-objdir.h ##
+@@ tmp-objdir.h: int tmp_objdir_destroy(struct tmp_objdir *);
+ */
+ void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
+
++/*
++ * Replaces the main object store in the current process with the temporary
++ * object directory and makes the former main object store an alternate.
++ */
++void tmp_objdir_replace_main_odb(struct tmp_objdir *);
++
+ #endif /* TMP_OBJDIR_H */
## wrapper.c ##
@@ wrapper.c: int xmkstemp_mode(char *filename_template, int mode)
3: a5b3e21b762 < -: ----------- core.fsyncobjectfiles: add windows support for batch mode
4: f7f756f3932 ! 4: 485b4a767df update-index: use the bulk-checkin infrastructure
@@ Commit message
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
- expecting to see objects would have to snoop the output of --verbose to
- find out when update-index has actually processed a given path.
- Additionally the index is locked for the duration of the update.
+ expecting to see objects would have to synchronize with the update-index
+ process after passing it a file path.
Signed-off-by: Neeraj Singh [off-list ref]
@@ builtin/update-index.c
#include "lockfile.h"
#include "quote.h"
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- struct strbuf unquoted = STRBUF_INIT;
- setup_work_tree();
-+ plug_bulk_checkin();
- while (getline_fn(&buf, stdin) != EOF) {
- char *p;
- if (!nul_term_line && buf.buf[0] == '"') {
+ the_index.updated_skipworktree = 1;
+
++ /* we might be adding many objects to the object database */
++ plug_bulk_checkin();
++
+ /*
+ * Custom copy of parse_options() because we want to handle
+ * filename arguments as they come.
@@ builtin/update-index.c: int cmd_update_index(int argc, const char **argv, const char *prefix)
- chmod_path(set_executable_bit, p);
- free(p);
- }
-+ unplug_bulk_checkin(&lock_file);
- strbuf_release(&unquoted);
strbuf_release(&buf);
}
+
++ /* by now we must have added all of the new objects */
++ unplug_bulk_checkin();
+ if (split_index > 0) {
+ if (git_config_get_split_index() == 0)
+ warning(_("core.splitIndex is set to false; "
-: ----------- > 5: 889e7668760 unpack-objects: use the bulk-checkin infrastructure
5: afb0028e796 ! 6: 0f2e3b25759 core.fsyncobjectfiles: tests for batch mode
@@ Metadata
## Commit message ##
core.fsyncobjectfiles: tests for batch mode
- Add test cases to exercise batch mode for 'git add'
- and 'git stash'. These tests ensure that the added
- data winds up in the object database.
+ Add test cases to exercise batch mode for:
+ * 'git add'
+ * 'git stash'
+ * 'git update-index'
+ * 'git unpack-objects'
- I verified the tests by introducing an incorrect rename
- in do_sync_and_rename.
+ These tests ensure that the added data winds up in the object database.
+
+ In this change we introduce a new test helper lib-unique-files.sh. The
+ goal of this library is to create a tree of files that have different
+ oids from any other files that may have been created in the current test
+ repo. This helps us avoid missing validation of an object being added due
+ to it already being in the repo.
Signed-off-by: Neeraj Singh [off-list ref]
@@ t/lib-unique-files.sh (new)
@@
+# Helper to create files with unique contents
+
-+test_create_unique_files_base__=$(date -u)
-+test_create_unique_files_counter__=0
+
+# Create multiple files with unique contents. Takes the number of
+# directories, the number of files in each directory, and the base
+# directory.
+#
-+# test_create_unique_files 2 3 . -- Creates 2 directories with 3 files
-+# each in the specified directory, all
-+# with unique contents.
++# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files
++# each in my_dir, all with unique
++# contents.
+
+test_create_unique_files() {
+ test "$#" -ne 3 && BUG "3 param"
@@ t/lib-unique-files.sh (new)
+ local dirs=$1
+ local files=$2
+ local basedir=$3
++ local counter=0
++ test_tick
++ local basedata=$test_tick
++
+
-+ rm -rf $basedir >/dev/null
++ rm -rf $basedir
+
+ for i in $(test_seq $dirs)
+ do
+ local dir=$basedir/dir$i
+
-+ mkdir -p "$dir" > /dev/null
++ mkdir -p "$dir"
+ for j in $(test_seq $files)
+ do
-+ test_create_unique_files_counter__=$((test_create_unique_files_counter__ + 1))
-+ echo "$test_create_unique_files_base__.$test_create_unique_files_counter__" >"$dir/file$j.txt"
++ counter=$((counter + 1))
++ echo "$basedata.$counter" >"$dir/file$j.txt"
+ done
+ done
+}
@@ t/t3700-add.sh: test_expect_success \
+ rm -f fsynced_files &&
+ git ls-files --stage fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
-+ cat fsynced_files | awk '{print \$2}' | xargs -n1 git cat-file -e
++ awk -- '{print \$2}' fsynced_files | xargs -n1 git cat-file -e
++"
++
++test_expect_success 'git update-index: core.fsyncobjectfiles=batch' "
++ test_create_unique_files 2 4 fsync-files2 &&
++ find fsync-files2 ! -type d -print | xargs git -c core.fsyncobjectfiles=batch update-index --add -- &&
++ rm -f fsynced_files2 &&
++ git ls-files --stage fsync-files2/ > fsynced_files2 &&
++ test_line_count = 8 fsynced_files2 &&
++ awk -- '{print \$2}' fsynced_files2 | xargs -n1 git cat-file -e
+"
+
test_expect_success \
@@ t/t3903-stash.sh: test_expect_success 'stash handles skip-worktree entries nicel
+ # which contains the untracked files
+ git ls-tree -r stash^3 -- ./fsync-files/ > fsynced_files &&
+ test_line_count = 8 fsynced_files &&
-+ cat fsynced_files | awk '{print \$3}' | xargs -n1 git cat-file -e
++ awk -- '{print \$3}' fsynced_files | xargs -n1 git cat-file -e
+"
+
+
test_expect_success 'stash -c stash.useBuiltin=false warning ' '
expected="stash.useBuiltin support has been removed" &&
+
+ ## t/t5300-pack-object.sh ##
+@@ t/t5300-pack-object.sh: test_expect_success 'pack-objects with bogus arguments' '
+
+ check_unpack () {
+ test_when_finished "rm -rf git2" &&
+- git init --bare git2 &&
+- git -C git2 unpack-objects -n <"$1".pack &&
+- git -C git2 unpack-objects <"$1".pack &&
+- (cd .git && find objects -type f -print) |
+- while read path
+- do
+- cmp git2/$path .git/$path || {
+- echo $path differs.
+- return 1
+- }
+- done
++ git $2 init --bare git2 &&
++ (
++ git $2 -C git2 unpack-objects -n <"$1".pack &&
++ git $2 -C git2 unpack-objects <"$1".pack &&
++ git $2 -C git2 cat-file --batch-check="%(objectname)"
++ ) <obj-list >current &&
++ cmp obj-list current
+ }
+
+ test_expect_success 'unpack without delta' '
+ check_unpack test-1-${packname_1}
+ '
+
++test_expect_success 'unpack without delta (core.fsyncobjectfiles=batch)' '
++ check_unpack test-1-${packname_1} "-c core.fsyncobjectfiles=batch"
++'
++
+ test_expect_success 'pack with REF_DELTA' '
+ packname_2=$(git pack-objects --progress test-2 <obj-list 2>stderr) &&
+ check_deltas stderr -gt 0
+@@ t/t5300-pack-object.sh: test_expect_success 'unpack with REF_DELTA' '
+ check_unpack test-2-${packname_2}
+ '
+
++test_expect_success 'unpack with REF_DELTA (core.fsyncobjectfiles=batch)' '
++ check_unpack test-2-${packname_2} "-c core.fsyncobjectfiles=batch"
++'
++
+ test_expect_success 'pack with OFS_DELTA' '
+ packname_3=$(git pack-objects --progress --delta-base-offset test-3 \
+ <obj-list 2>stderr) &&
+@@ t/t5300-pack-object.sh: test_expect_success 'unpack with OFS_DELTA' '
+ check_unpack test-3-${packname_3}
+ '
+
++test_expect_success 'unpack with OFS_DELTA (core.fsyncobjectfiles=batch)' '
++ check_unpack test-3-${packname_3} "-c core.fsyncobjectfiles=batch"
++'
++
+ test_expect_success 'compare delta flavors' '
+ perl -e '\''
+ defined($_ = -s $_) or die for @ARGV;
6: 3e6b80b5fa2 = 7: 6543564376a core.fsyncobjectfiles: performance tests for add and stash
--
gitgitgadget
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:21
From: Neeraj Singh <redacted>
Preparation for adding bulk-fsync to the bulk-checkin.c infrastructure.
* Rename 'state' variable to 'bulk_checkin_state', since we will later
be adding 'bulk_fsync_state'. This also makes the variable easier to
find in the debugger, since the name is more unique.
* Move the 'plugged' data member of 'bulk_checkin_state' into a separate
static variable. Doing this avoids resetting the variable in
finish_bulk_checkin when zeroing the 'bulk_checkin_state'. As-is, we
seem to unintentionally disable the plugging functionality the first
time a new packfile must be created due to packfile size limits. While
disabling the plugging state only results in suboptimal behavior for
the current code, it would be fatal for the bulk-fsync functionality
later in this patch series.
Signed-off-by: Neeraj Singh <redacted>
---
bulk-checkin.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:28
From: Neeraj Singh <redacted>
When adding many objects to a repo with core.fsyncObjectFiles set to
true, the cost of fsync'ing each object file can become prohibitive.
One major source of the cost of fsync is the implied flush of the
hardware writeback cache within the disk drive. Fortunately, Windows,
and macOS offer mechanisms to write data from the filesystem page cache
without initiating a hardware flush. Linux has the sync_file_range API,
which issues a pagecache writeback request reliably after version 5.2.
This patch introduces a new 'core.fsyncObjectFiles = batch' option that
batches up hardware flushes. It hooks into the bulk-checkin plugging and
unplugging functionality and takes advantage of tmp-objdir.
When the new mode is enabled we do the following for each new object:
1. Create the object in a tmp-objdir.
2. Issue a pagecache writeback request and wait for it to complete.
At the end of the entire transaction when unplugging bulk checkin we:
1. Issue an fsync against a dummy file to flush the hardware writeback
cache, which should by now have processed the tmp-objdir writes.
2. Rename all of the tmp-objdir files to their final names.
3. When updating the index and/or refs, we assume that Git will issue
another fsync internal to that operation. This is not the case today,
but may be a good extension to those components.
On a filesystem with a singular journal that is updated during name
operations (e.g. create, link, rename, etc), such as NTFS, HFS+, or XFS we
would expect the fsync to trigger a journal writeout so that this
sequence is enough to ensure that the user's data is durable by the time
the git command returns.
This change also updates the macOS code to trigger a real hardware flush
via fnctl(fd, F_FULLFSYNC) when fsync_or_die is called. Previously, on
macOS there was no guarantee of durability since a simple fsync(2) call
does not flush any hardware caches.
_Performance numbers_:
Linux - Hyper-V VM running Kernel 5.11 (Ubuntu 20.04) on a fast SSD.
Mac - macOS 11.5.1 running on a Mac mini on a 1TB Apple SSD.
Windows - Same host as Linux, a preview version of Windows 11.
This number is from a patch later in the series.
Adding 500 files to the repo with 'git add' Times reported in seconds.
core.fsyncObjectFiles | Linux | Mac | Windows
----------------------|-------|-------|--------
false | 0.06 | 0.35 | 0.61
true | 1.88 | 11.18 | 2.47
batch | 0.15 | 0.41 | 1.53
Signed-off-by: Neeraj Singh <redacted>
---
Documentation/config/core.txt | 29 ++++++++++++---
Makefile | 6 +++
builtin/add.c | 1 +
bulk-checkin.c | 70 +++++++++++++++++++++++++++++++++++
bulk-checkin.h | 2 +
cache.h | 8 +++-
config.c | 7 +++-
config.mak.uname | 1 +
configure.ac | 8 ++++
environment.c | 2 +-
git-compat-util.h | 7 ++++
object-file.c | 67 ++++++++++++++++++++++++++++++++-
object-store.h | 16 ++++++++
object.c | 2 +-
tmp-objdir.c | 20 +++++++++-
tmp-objdir.h | 6 +++
wrapper.c | 44 ++++++++++++++++++++++
write-or-die.c | 2 +-
18 files changed, 285 insertions(+), 13 deletions(-)
@@ -548,12 +548,29 @@ core.whitespace:: errors. The default tab width is 8. Allowed values are 1 to 63. core.fsyncObjectFiles::- This boolean will enable 'fsync()' when writing object files.-+-This is a total waste of time and effort on a filesystem that orders-data writes properly, but can be useful for filesystems that do not use-journalling (traditional UNIX filesystems) or that only journal metadata-and not file contents (OS X's HFS+, or Linux ext3 with "data=writeback").+ A value indicating the level of effort Git will expend in+ trying to make objects added to the repo durable in the event+ of an unclean system shutdown. This setting currently only+ controls loose objects in the object store, so updates to any+ refs or the index may not be equally durable.+++* `false` allows data to remain in file system caches according to+ operating system policy, whence it may be lost if the system loses power+ or crashes.+* `true` triggers a data integrity flush for each loose object added to the+ object store. This is the safest setting that is likely to ensure durability+ across all operating systems and file systems that honor the 'fsync' system+ call. However, this setting comes with a significant performance cost on+ common hardware. Git does not currently fsync parent directories for+ newly-added files, so some filesystems may still allow data to be lost on+ system crash.+* `batch` enables an experimental mode that uses interfaces available in some+ operating systems to write loose object data with a minimal set of FLUSH+ CACHE (or equivalent) commands sent to the storage controller. If the+ operating system interfaces are not available, this mode behaves the same as+ `true`. This mode is expected to be as safe as `true` on macOS for repos+ stored on HFS+ or APFS filesystems and on Windows for repos stored on NTFS or+ ReFS. core.preloadIndex:: Enable parallel index preload for operations like 'git diff'
@@ -406,6 +406,8 @@ all::## Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.#+# Define HAVE_SYNC_FILE_RANGE if your platform has sync_file_range.+## Define NEEDS_LIBRT if your platform requires linking with librt (glibc version# before 2.17) for clock_gettime and CLOCK_MONOTONIC.#
@@ -270,6 +324,20 @@ int index_bulk_checkin(struct object_id *oid,voidplug_bulk_checkin(void){assert(!bulk_checkin_plugged);++/*+*Createatemporaryobjectdirectoryifthecurrent+*objectdirectoryisnotalreadytemporary.+*/+if(fsync_object_files==FSYNC_OBJECT_FILES_BATCH&&+!the_repository->objects->odb->is_temp){+bulk_fsync_objdir=tmp_objdir_create();+if(!bulk_fsync_objdir)+die(_("Could not create temporary object directory for core.fsyncobjectfiles=batch"));++tmp_objdir_replace_main_odb(bulk_fsync_objdir);+}+bulk_checkin_plugged=1;}
@@ -750,6 +750,60 @@ void add_to_alternates_memory(const char *reference)'\n',NULL,0);}+structobject_directory*set_temporary_main_odb(constchar*dir)+{+structobject_directory*main_odb,*new_odb,*old_next;++/*+*Makesurealternatesareinitialized,orelseourentrymaybe+*overwrittenwhentheyare.+*/+prepare_alt_odb(the_repository);++/* Copy the existing object directory and make it an alternate. */+main_odb=the_repository->objects->odb;+new_odb=xmalloc(sizeof(*new_odb));+*new_odb=*main_odb;+*the_repository->objects->odb_tail=new_odb;+the_repository->objects->odb_tail=&(new_odb->next);+new_odb->next=NULL;++/*+*Reinitializethemainodbwiththespecifiedpath,beingcareful+*tokeepthenextpointervalue.+*/+old_next=main_odb->next;+memset(main_odb,0,sizeof(*main_odb));+main_odb->next=old_next;+main_odb->is_temp=1;+main_odb->path=xstrdup(dir);+returnnew_odb;+}++voidrestore_main_odb(structobject_directory*odb)+{+structobject_directory**prev,*main_odb;++/* Unlink the saved previous main ODB from the list. */+prev=&the_repository->objects->odb->next;+assert(*prev);+while(*prev!=odb){+prev=&(*prev)->next;+}+*prev=odb->next;+if(*prev==NULL)+the_repository->objects->odb_tail=prev;++/*+*Restorethedatafromtheoldmainodb,beingcarefulto+*keepthenextpointervalue+*/+main_odb=the_repository->objects->odb;+SWAP(*main_odb,*odb);+main_odb->next=odb->next;+free_object_directory(odb);+}+/**Computetheexactpathanalternateisatandreturnsit.Incaseof*errorNULLisreturnedandthehumanreadableerrorisaddedto`err`
@@ -1867,8 +1921,19 @@ int hash_object_file(const struct git_hash_algo *algo, const void *buf,/* Finalize a file on disk, and close it. */staticvoidclose_loose_object(intfd){-if(fsync_object_files)+switch(fsync_object_files){+caseFSYNC_OBJECT_FILES_OFF:+break;+caseFSYNC_OBJECT_FILES_ON:fsync_or_die(fd,"loose object file");+break;+caseFSYNC_OBJECT_FILES_BATCH:+fsync_loose_object_bulk_checkin(fd);+break;+default:+BUG("Invalid fsync_object_files mode.");+}+if(close(fd)!=0)die_errno(_("error when closing loose object file"));}
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:33
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to synchronize with the update-index
process after passing it a file path.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/update-index.c | 6 ++++++
1 file changed, 6 insertions(+)
@@ -1088,6 +1089,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)the_index.updated_skipworktree=1;+/* we might be adding many objects to the object database */+plug_bulk_checkin();+/**Customcopyofparse_options()becausewewanttohandle*filenameargumentsastheycome.
@@ -1168,6 +1172,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)strbuf_release(&buf);}+/* by now we must have added all of the new objects */+unplug_bulk_checkin();if(split_index>0){if(git_config_get_split_index()==0)warning(_("core.splitIndex is set to false; "
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:52
From: Neeraj Singh <redacted>
The unpack-objects functionality is used by fetch, push, and fast-import
to turn the transfered data into object database entries when there are
fewer objects than the 'unpacklimit' setting.
By enabling bulk-checkin when unpacking objects, we can take advantage
of batched fsyncs.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/unpack-objects.c | 3 +++
1 file changed, 3 insertions(+)
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:53
From: Neeraj Singh <redacted>
Add test cases to exercise batch mode for:
* 'git add'
* 'git stash'
* 'git update-index'
* 'git unpack-objects'
These tests ensure that the added data winds up in the object database.
In this change we introduce a new test helper lib-unique-files.sh. The
goal of this library is to create a tree of files that have different
oids from any other files that may have been created in the current test
repo. This helps us avoid missing validation of an object being added due
to it already being in the repo.
Signed-off-by: Neeraj Singh <redacted>
---
t/lib-unique-files.sh | 36 ++++++++++++++++++++++++++++++++++++
t/t3700-add.sh | 20 ++++++++++++++++++++
t/t3903-stash.sh | 14 ++++++++++++++
t/t5300-pack-object.sh | 30 +++++++++++++++++++-----------
4 files changed, 89 insertions(+), 11 deletions(-)
create mode 100644 t/lib-unique-files.sh
@@ -0,0 +1,36 @@+# Helper to create files with unique contents+++# Create multiple files with unique contents. Takes the number of+# directories, the number of files in each directory, and the base+# directory.+#+# test_create_unique_files 2 3 my_dir -- Creates 2 directories with 3 files+# each in my_dir, all with unique+# contents.++test_create_unique_files(){+test"$#"-ne3&&BUG"3 param"++localdirs=$1+localfiles=$2+localbasedir=$3+localcounter=0+test_tick+localbasedata=$test_tick+++rm-rf$basedir++foriin$(test_seq$dirs)+do+localdir=$basedir/dir$i++mkdir-p"$dir"+forjin$(test_seq$files)+do+counter=$((counter+1))+echo"$basedata.$counter">"$dir/file$j.txt"+done+done+}
@@ -7,6 +7,8 @@ test_description='Test of git add, including the -- option.' ../test-lib.sh+.$TEST_DIRECTORY/lib-unique-files.sh+# Test the file mode "$1" of the file "$2" in the index. test_mode_in_index(){case"$(gitls-files-s"$2")"in
@@ -33,6 +35,24 @@ test_expect_success \'Test that "git add -- -q" works'\'touch -- -q && git add -- -q'+test_expect_success'git add: core.fsyncobjectfiles=batch'"+test_create_unique_files24fsync-files&&+git-ccore.fsyncobjectfiles=batchadd--./fsync-files/&&+rm-ffsynced_files&&+gitls-files--stagefsync-files/>fsynced_files&&+test_line_count=8fsynced_files&&+awk--'{print \$2}'fsynced_files|xargs-n1gitcat-file-e+"++test_expect_success'git update-index: core.fsyncobjectfiles=batch'"+test_create_unique_files24fsync-files2&&+findfsync-files2!-typed-print|xargsgit-ccore.fsyncobjectfiles=batchupdate-index--add--&&+rm-ffsynced_files2&&+gitls-files--stagefsync-files2/>fsynced_files2&&+test_line_count=8fsynced_files2&&+awk--'{print \$2}'fsynced_files2|xargs-n1gitcat-file-e+"+ test_expect_success\'git add: Test that executable bit is not used if core.filemode=0'\'gitconfigcore.filemode0&&
@@ -1293,6 +1294,19 @@ test_expect_success 'stash handles skip-worktree entries nicely' 'gitrev-parse--verifyrefs/stash:A.t'+test_expect_success'stash with core.fsyncobjectfiles=batch'"+test_create_unique_files24fsync-files&&+git-ccore.fsyncobjectfiles=batchstashpush-u--./fsync-files/&&+rm-ffsynced_files&&++# The files were untracked, so use the third parent,+# which contains the untracked files+gitls-tree-rstash^3--./fsync-files/>fsynced_files&&+test_line_count=8fsynced_files&&+awk--'{print \$3}'fsynced_files|xargs-n1gitcat-file-e+"++ test_expect_success'stash -c stash.useBuiltin=false warning ''expected="stash.useBuiltin support has been removed"&&
From: Neeraj Singh via GitGitGadget <hidden> Date: 2021-09-24 20:12:54
From: Neeraj Singh <redacted>
Add a basic performance test for "git add" and "git stash" of a lot of
new objects with various fsync settings.
Signed-off-by: Neeraj Singh <redacted>
---
t/perf/p3700-add.sh | 43 ++++++++++++++++++++++++++++++++++++++++
t/perf/p3900-stash.sh | 46 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
create mode 100755 t/perf/p3700-add.sh
create mode 100755 t/perf/p3900-stash.sh
@@ -0,0 +1,43 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of add"++../perf-lib.sh++.$TEST_DIRECTORY/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"${GIT_PERF_REPEAT_COUNT-1}"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++test_perf"add $total_files files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$maddfiles+"+done++test_done
@@ -0,0 +1,46 @@+#!/bin/sh+#+# This test measures the performance of adding new files to the object database+# and index. The test was originally added to measure the effect of the+# core.fsyncObjectFiles=batch mode, which is why we are testing different values+# of that setting explicitly and creating a lot of unique objects.++test_description="Tests performance of stash"++../perf-lib.sh++.$TEST_DIRECTORY/lib-unique-files.sh++test_perf_default_repo+test_checkout_worktree++dir_count=10+files_per_dir=50+total_files=$((dir_count*files_per_dir))++# We need to create the files each time we run the perf test, but+# we do not want to measure the cost of creating the files, so run+# the tet once.+iftest"${GIT_PERF_REPEAT_COUNT-1}"-ne1+then+echo"warning: Setting GIT_PERF_REPEAT_COUNT=1">&2+GIT_PERF_REPEAT_COUNT=1+fi++forminfalsetruebatch+do+test_expect_success"create the files for core.fsyncObjectFiles=$m"'+gitreset--hard&&+# create files across directories+test_create_unique_files$dir_count$files_per_dirfiles+'++# We only stash files in the 'files' subdirectory since+# the perf test infrastructure creates files in the+# current working directory that need to be preserved+test_perf"stash 500 files (core.fsyncObjectFiles=$m)""+git-ccore.fsyncobjectfiles=$mstashpush-u--files+"+done++test_done
On Fri, Sep 24, 2021 at 1:12 PM Neeraj Singh via GitGitGadget
[off-list ref] wrote:
From: Neeraj Singh <redacted>
The update-index functionality is used internally by 'git stash push' to
setup the internal stashed commit.
This change enables bulk-checkin for update-index infrastructure to
speed up adding new objects to the object database by leveraging the
pack functionality and the new bulk-fsync functionality. This mode
is enabled when passing paths to update-index via the --stdin flag,
as is done by 'git stash'.
This part of the description is now inaccurate. All modes of update-index are
now enlightened to use bulk_checkin. I'll just remove the sentence that
scopes the change to --stdin on reroll.
There is some risk with this change, since under batch fsync, the object
files will not be available until the update-index is entirely complete.
This usage is unlikely, since any tool invoking update-index and
expecting to see objects would have to synchronize with the update-index
process after passing it a file path.
Signed-off-by: Neeraj Singh <redacted>
---
builtin/update-index.c | 6 ++++++
1 file changed, 6 insertions(+)