{"thread":{"id":"13725","subject":"reducing prune sync()s","startedAt":"2008-05-29T20:57:43Z","lastAt":"2008-06-02T22:23:40Z","messageCount":15,"participants":["Frank Ch. Eigler","Linus Torvalds","David Dillow","Florian Weimer","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"78085","messageId":"20080529205743.GC17123@redhat.com","threadId":"13725","inReplyTo":null,"subject":"reducing prune sync()s","fromName":"Frank Ch. Eigler","fromEmail":"fche@redhat.com","sentAt":"2008-05-29T20:57:43Z","receivedAt":"2008-05-29T20:57:43Z","isPatch":false,"sender":{"key":"fche@redhat.com","avatar":"https://gravatar.com/avatar/c42923a074e7dc44da251150f545ae29e0143c6c0cf25b0f6193288cdc166a9e?d=mp&s=160"},"body":"Hi -\n\nIn at least builtin-prune-packed.c, builtin-prune.c, and\nguilt-repack.sh, there is an unconditional sync call. On a machine\nwith lots of dirty disk data, writing it to some slow device, this\nsync can take a long time.\n\nWould there be interest in making this sync disableable with\ngit-config? Or perhaps having the blanket sync be replaced a\nlist of fsync()s for only the relevant git repository files?\n\n- FChE\n"},{"id":"78100","messageId":"alpine.LFD.1.10.0805291656260.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"20080529205743.GC17123@redhat.com","subject":"Re: reducing prune sync()s","fromName":"Linus Torvalds","fromEmail":"torvalds@linuxfoundation.org","sentAt":"2008-05-30T00:27:35Z","receivedAt":"2008-05-30T00:27:35Z","isPatch":false,"sender":{"key":"torvalds@linuxfoundation.org","avatar":null},"body":"\n\nOn Thu, 29 May 2008, Frank Ch. Eigler wrote:\n> \n> Would there be interest in making this sync disableable with\n> git-config?\n\nNo, that's just too scary. But..\n\n>\t Or perhaps having the blanket sync be replaced a\n> list of fsync()s for only the relevant git repository files?\n\nThat would be much better. The code was ported from shell script, and \nthere is no fsync() in shell, but the rule should basically be that you \ncan remove all the objects that correspond to a pack-file after you have \nmade sure that the pack-file (and it's index - we can re-generate the pack \nindex, but realistically speaking it's *much* better to not have to) is \nstable on disk.\n\nSoemthing like this *may* work. THIS IS TOTALLY UNTESTED. And when I say \n\"TOTALLY UNTESTED\", I mean it. Zero testing. None. Nada. Zilch. Testing is \nfor people who are actually interested in the feature (hint, hint).\n\nBut if somebody tests this and actually does an strace and sees that yes, \nit does the fsync() on each pack-file it cares about, then I'll happily \nsign off on this patch. It's fairly obvious.\n\nWhat it does is to make \"has_sha1_pack()\" return the actual packed_git \npointer it found that contains the SHA1 in question, instead of just an \ninteger boolean. That changes no users, since everybody tests it for just \ntruthiness value anyway.\n\nIt then introduces a \"sha1_is_stably_packed()\" function that not just \nlooks up the pack entry, but also does an fsync() on it if it isn't \nalready marked stable. Fairly trivial, as I said. But this is important \ncode, so it really does need actual real-life testing. Trivial code often \nhas trivial bugs.\n\n\t\tLinus\n\n---\n builtin-prune-packed.c | 23 +++++++++++++++++++++--\n cache.h | 6 ++++--\n sha1_file.c | 9 ++++++---\n 3 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-prune-packed.c b/builtin-prune-packed.c\nindex 23faf31..b64a5d4 100644\n--- a/builtin-prune-packed.c\n+++ b/builtin-prune-packed.c\n@@ -10,6 +10,26 @@ static const char prune_packed_usage[] =\n \n static struct progress *progress;\n \n+static int sha1_is_stably_packed(const unsigned char *sha1)\n+{\n+\tstruct packed_git *pack;\n+\n+\tpack = has_sha1_pack(sha1, NULL);\n+\tif (!pack)\n+\t\treturn 0;\n+\tif (!pack->pack_stable) {\n+\t\tint fd = pack->pack_fd;\n+\t\tif (fd < 0) {\n+\t\t\tif (open_packed_git(pack) < 0)\n+\t\t\t\treturn 0;\n+\t\t\tfd = pack->pack_fd;\n+\t\t}\n+\t\tfsync(fd);\n+\t\tpack->pack_stable = 1;\n+\t}\n+\treturn 1;\n+}\n+\n static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)\n {\n \tstruct dirent *de;\n@@ -23,7 +43,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)\n \t\tmemcpy(hex+2, de->d_name, 38);\n \t\tif (get_sha1_hex(hex, sha1))\n \t\t\tcontinue;\n-\t\tif (!has_sha1_pack(sha1, NULL))\n+\t\tif (!sha1_is_stably_packed(sha1))\n \t\t\tcontinue;\n \t\tmemcpy(pathname + len, de->d_name, 38);\n \t\tif (opts & DRY_RUN)\n@@ -85,7 +105,6 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n \t\t/* Handle arguments here .. */\n \t\tusage(prune_packed_usage);\n \t}\n-\tsync();\n \tprune_packed_objects(opts);\n \treturn 0;\n }\ndiff --git a/cache.h b/cache.h\nindex eab1a17..3bb85eb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -540,7 +540,7 @@ extern int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,\n extern int write_sha1_to_fd(int fd, const unsigned char *sha1);\n extern int move_temp_to_file(const char *tmpfile, const char *filename);\n \n-extern int has_sha1_pack(const unsigned char *sha1, const char **ignore);\n+extern struct packed_git *has_sha1_pack(const unsigned char *sha1, const char **ignore);\n extern int has_sha1_file(const unsigned char *sha1);\n \n extern int has_pack_file(const unsigned char *sha1);\n@@ -646,7 +646,8 @@ extern struct packed_git {\n \tint index_version;\n \ttime_t mtime;\n \tint pack_fd;\n-\tint pack_local;\n+\tunsigned int pack_local:1,\n+\t\t pack_stable:1;\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n \tchar pack_name[FLEX_ARRAY]; /* more */\n@@ -708,6 +709,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \n extern void pack_report(void);\n extern int open_pack_index(struct packed_git *);\n+extern int open_packed_git(struct packed_git *p);\n extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9679040..52e9824 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -703,7 +703,7 @@ static int open_packed_git_1(struct packed_git *p)\n \treturn 0;\n }\n \n-static int open_packed_git(struct packed_git *p)\n+int open_packed_git(struct packed_git *p)\n {\n \tif (!open_packed_git_1(p))\n \t\treturn 0;\n@@ -824,6 +824,7 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \tp->windows = NULL;\n \tp->pack_fd = -1;\n \tp->pack_local = local;\n+\tp->pack_stable = 0;\n \tp->mtime = st.st_mtime;\n \tif (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))\n \t\thashclr(p->sha1);\n@@ -2381,10 +2382,12 @@ int has_pack_file(const unsigned char *sha1)\n \treturn 1;\n }\n \n-int has_sha1_pack(const unsigned char *sha1, const char **ignore_packed)\n+struct packed_git *has_sha1_pack(const unsigned char *sha1, const char **ignore_packed)\n {\n \tstruct pack_entry e;\n-\treturn find_pack_entry(sha1, &e, ignore_packed);\n+\tif (find_pack_entry(sha1, &e, ignore_packed))\n+\t\treturn e.p;\n+\treturn NULL;\n }\n \n int has_sha1_file(const unsigned char *sha1)\n"},{"id":"78102","messageId":"alpine.LFD.1.10.0805291727490.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291656260.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"Linus Torvalds","fromEmail":"torvalds@linuxfoundation.org","sentAt":"2008-05-30T00:32:30Z","receivedAt":"2008-05-30T00:32:30Z","isPatch":false,"sender":{"key":"torvalds@linuxfoundation.org","avatar":null},"body":"\n\nOn Thu, 29 May 2008, Linus Torvalds wrote:\n> On Thu, 29 May 2008, Frank Ch. Eigler wrote:\n> > \n> >\t Or perhaps having the blanket sync be replaced a\n> > list of fsync()s for only the relevant git repository files?\n> \n> That would be much better.\n\nSide note: a lot of systems make \"fsync()\" pretty expensive too. It's one \nof my main disagreements with most log-based filesystems - fsync() can in \ntheory be fast, but almost always implies flushing the whole log, even if \n99.9% of that log is totally unrelated to the actual file you want to \nfsync().\n\nSo fsync() isn't always all that much better than sync(). It *should* be, \nbut reality sometimes bites. So testing should include at least some level \nof \"yes, it actually improves things at least on xyz\"...\n\n\t\t\tLinus\n"},{"id":"78106","messageId":"20080530015043.GB4032@redhat.com","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291727490.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"Frank Ch. Eigler","fromEmail":"fche@redhat.com","sentAt":"2008-05-30T01:50:43Z","receivedAt":"2008-05-30T01:50:43Z","isPatch":false,"sender":{"key":"fche@redhat.com","avatar":"https://gravatar.com/avatar/c42923a074e7dc44da251150f545ae29e0143c6c0cf25b0f6193288cdc166a9e?d=mp&s=160"},"body":"Hi --\n\nOn Thu, May 29, 2008 at 05:32:30PM -0700, Linus Torvalds wrote:\n\n> [...] Side note: a lot of systems make \"fsync()\" pretty expensive\n> too. It's one of my main disagreements with most log-based\n> filesystems - fsync() can in theory be fast, but almost always\n> implies flushing the whole log, even if 99.9% of that log is totally\n> unrelated to the actual file you want to fsync(). [...]\n\nThe scenario where I ran into this was different: a usb-drive\nfilesystem was busy running receiving backups, collecting, oh, several\nGB of dirty data, while git was working on a normal local disk on a\nseparate filesystem. The system-wide git sync unnecessarily blocked\non the usb filesystem too.\n\n- FChE\n"},{"id":"78105","messageId":"1212112295.3094.3.camel@obelisk.thedillows.org","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291656260.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"David Dillow","fromEmail":"dave@thedillows.org","sentAt":"2008-05-30T01:51:35Z","receivedAt":"2008-05-30T01:51:35Z","isPatch":false,"sender":{"key":"dave@thedillows.org","avatar":null},"body":"\nOn Thu, 2008-05-29 at 17:27 -0700, Linus Torvalds wrote:\n> That would be much better. The code was ported from shell script, and \n> there is no fsync() in shell, but the rule should basically be that you \n> can remove all the objects that correspond to a pack-file after you have \n> made sure that the pack-file (and it's index - we can re-generate the pack \n> index, but realistically speaking it's *much* better to not have to) is \n> stable on disk.\n\nEven if the data is stable on disk, don't we also need to ensure the\npack's connectivity to the namespace is also stable? Without an fsync()\nof the directory that contains it, could it go away?\n\nOf course, this is me recollecting a several-year-old exchange on LKML,\nso I don't know if it is still needed or not, or on systems other than\nLinux.\n\nDave\n"},{"id":"78107","messageId":"alpine.LFD.1.10.0805291905360.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"1212112295.3094.3.camel@obelisk.thedillows.org","subject":"Re: reducing prune sync()s","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-30T02:17:06Z","receivedAt":"2008-05-30T02:17:06Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 29 May 2008, David Dillow wrote:\n> \n> On Thu, 2008-05-29 at 17:27 -0700, Linus Torvalds wrote:\n> > That would be much better. The code was ported from shell script, and \n> > there is no fsync() in shell, but the rule should basically be that you \n> > can remove all the objects that correspond to a pack-file after you have \n> > made sure that the pack-file (and it's index - we can re-generate the pack \n> > index, but realistically speaking it's *much* better to not have to) is \n> > stable on disk.\n> \n> Even if the data is stable on disk, don't we also need to ensure the\n> pack's connectivity to the namespace is also stable? Without an fsync()\n> of the directory that contains it, could it go away?\n\nIn theory, yes. That said, it would always be in lost+found, so the data \nwouldn't ever get really lost. In that sense it is no different from a lot \nof other theoretical git corruption issues - git in general does *not* \nguarantee that the repository will not need to be \"fixed up\", it just \nmakes a strong case for\n\n - git will always at least *see* the corruption\n\n (ie it is by design is very hard to corrupt a git repo silently and \n subtly!)\n\n - git makes it very hard to lose data\n\n Old data is not overwritten, but that doesn't mean that you may not \n have to _look_ for it!\n\nAn example of the latter is how a crash in the middle of \"git commit\" may \nactually cause partial *new* objects to be on disk (the objects themselves \nare not fsync'ed when written!) and may end up with the ref being updated \nbut some object it points to was never written (again, a \"git commit\" does \nnot wait until things are stable!), and the index file may be totally \ncorrupt (again, no fsync anywhere!).\n\nSo if you have a system crash at a really bad time, you may have a git \nrepository that needs manual intervention to actually be *usable*. I hope \nnobody ever believed anything else. That manual intervention may be things \nlike:\n\n - throw away a corrupt .git/index file and re-create it (git read-tree)\n\n - possibly have to recover refs manually (\"git fsck\" + look at dangling \n commits)\n\n - actually throw away broken commits, and re-create them (ie basically \n doing a \"git reset \" plus re-committing the working \n tree or perhaps re-doing a whole \"git am\" series or something)\n\n - and yes, possibly recover lost inodes from /lost+found\n\nNow, quite frankly, all of these are extremely rare. In most cases they \nwill never happen at all, simply because the filesystem itself is more or \nless transactional. \n\n\t\tLinus\n"},{"id":"78108","messageId":"alpine.LFD.1.10.0805291923030.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291905360.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-30T02:30:59Z","receivedAt":"2008-05-30T02:30:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 29 May 2008, Linus Torvalds wrote:\n> \n> So if you have a system crash at a really bad time, you may have a git \n> repository that needs manual intervention to actually be *usable*. I hope \n> nobody ever believed anything else. That manual intervention may be things \n> like:\n> ...\n> - actually throw away broken commits, and re-create them (ie basically \n> doing a \"git reset \" plus re-committing the working \n> tree or perhaps re-doing a whole \"git am\" series or something)\n\nThe important part here is that it's only the *new* state that can be this \nkind of \"broken commits\". In other words, you'd never have to re-do actual \n*old* commits, just the commits you were doing as things crashed - the \ncommits that you were in the middle of doing, and still have the data for.\n\nExample from my case: I may have series of 250+ commits that I create with \n\"git am\" when I sync up with Andrew, and I very much want the speed of \nbeing able to create all that new commit data without ever even causing a \n_single_ synchronous disk write.\n\nSo if the machine were to crash in the middle of the series, I might lose \nall of that data, but I still have my mailbox, so I'd just need to reset \nto the point before I even started the \"git am\", and re-do the whole \nseries. My actual *base* repository objects would never get corrupted.\n\n[ And one final notice: I don't know about others, but I've actually had \n more corruption from disks going bad etc that from system crashes per \n se. And when *that* happens, old data is obviously as easily gone as new \n data is. So absolutely _nothing_ replaces backups. It doesn't matter if \n you do a \"fsync()\" after every single byte write - a disk crash can and \n will corrupt things that were \"stable\". So even \"stable storage\" is \n very much unstable in the end. ]\n\n\t\t\tLinus\n"},{"id":"78150","messageId":"20080530152527.GF4032@redhat.com","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291656260.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"Frank Ch. Eigler","fromEmail":"fche@redhat.com","sentAt":"2008-05-30T15:25:27Z","receivedAt":"2008-05-30T15:25:27Z","isPatch":false,"sender":{"key":"fche@redhat.com","avatar":"https://gravatar.com/avatar/c42923a074e7dc44da251150f545ae29e0143c6c0cf25b0f6193288cdc166a9e?d=mp&s=160"},"body":"Hi -\n\nOn Thu, May 29, 2008 at 05:27:35PM -0700, Linus Torvalds wrote:\n> [...]\n> >\t Or perhaps having the blanket sync be replaced a\n> > list of fsync()s for only the relevant git repository files?\n> [...]\n> Soemthing like this *may* work. THIS IS TOTALLY UNTESTED. And when I say \n> \"TOTALLY UNTESTED\", I mean it. Zero testing. None. Nada. Zilch. Testing is \n> for people who are actually interested in the feature (hint, hint).\n\nThe patch does add an fsync or two into the mix, a \"git gc\" or \n\"git repack -a\" still goes through the \"git-repack\" shell script, which\nstill did its \"sync\". How about this patch, which adds a \"git-fsync\"\nbuiltin for the shell scripts? Added to yours, it replaces all the\nsyncs with fsync's, and tests fine in the same environment originally\nreported. (Lots of dirty data for another filesystem does not block\nthe fsyncs.)\n\ndiff --git a/.gitignore b/.gitignore\nindex 4ff2fec..708c5ac 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -47,6 +47,7 @@ git-for-each-ref\n git-format-patch\n git-fsck\n git-fsck-objects\n+git-fsync\n git-gc\n git-get-tar-commit-id\n git-grep\ndiff --git a/Makefile b/Makefile\nindex 865e2bf..2148196 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -500,6 +500,7 @@ BUILTIN_OBJS += builtin-fetch.o\n BUILTIN_OBJS += builtin-fmt-merge-msg.o\n BUILTIN_OBJS += builtin-for-each-ref.o\n BUILTIN_OBJS += builtin-fsck.o\n+BUILTIN_OBJS += builtin-fsync.o\n BUILTIN_OBJS += builtin-gc.o\n BUILTIN_OBJS += builtin-grep.o\n BUILTIN_OBJS += builtin-init-db.o\ndiff --git a/builtin-fsync.c b/builtin-fsync.c\nnew file mode 100644\nindex 0000000..8c4c7f7\n--- /dev/null\n+++ b/builtin-fsync.c\n@@ -0,0 +1,31 @@\n+/*\n+ * Copyright (c) 2008 Frank Ch. Eigler\n+ */\n+#include \"cache.h\"\n+#include \"commit.h\"\n+#include \"tar.h\"\n+#include \"builtin.h\"\n+#include \"quote.h\"\n+\n+static const char fsync_usage[] =\n+\"git-fsync ...\\n\";\n+\n+int cmd_fsync(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tfor (i=1; i \n> On Thu, May 29, 2008 at 05:27:35PM -0700, Linus Torvalds wrote:\n> > [...]\n> > >\t Or perhaps having the blanket sync be replaced a\n> > > list of fsync()s for only the relevant git repository files?\n> > [...]\n> > Soemthing like this *may* work. THIS IS TOTALLY UNTESTED. And when I say \n> > \"TOTALLY UNTESTED\", I mean it. Zero testing. None. Nada. Zilch. Testing is \n> > for people who are actually interested in the feature (hint, hint).\n> \n> The patch does add an fsync or two into the mix, a \"git gc\" or \n> \"git repack -a\" still goes through the \"git-repack\" shell script, which\n> still did its \"sync\".\n\nYes.\n\nBut I actually think there is a simpler and more straightforward approach.\n\nInstead of being careful when removing objects (whether old packs or loose \nobjects that are made redundant by a new pack), the simpler approach is to \njust always fsync() the new pack when creating it.\n\nI was always very careful to *not* make git depend on any serialized IO, \nbut the reason for that was literally the fact that I wanted to make sure \nthat I could batch up things efficiently, and do any serialization (if I \nwanted to) later. So it was literally always about the whole \"apply \nseveral hundred patches in one go\" kind of thing.\n\nAnd the thing is, the repacking phase *is* the \"serialize things later (if \nyou want)\" thing, so doing things synchronously at that point is actually \nperfectly fine.\n\nAnd every single \"let's remove objects\" operation is literally always \nabout the fact that we have a new better pack-file, making old objects \nredundant, so if we just create those new pack-files stably on disk, then \nany subsequent action pretty much by definition doesn't need any sync. \nBecause we know that the only thing we can really care about *is* stable.\n\nSo this is a conceptually much more direct approach. Creating pack-files \nreally is the special occasion, since it's (a) literally the event that \ncauses other objects to potentially be stale (b) fairly rare and (c) not \nnormally limited by disk-IO anyway (ie a \"git fetch\" will create a new \npack-file, but it's normally limited by the network overhead or the cost \nof creating the pack-file, not by adding a fsync() to make sure that the \nend result is stable).\n\nSo I'll follow up with a two-patch series (the first to create pack-files \nand their indexes stably on disk, the second to just remove the now \nunnecessary 'sync()' calls). I'll give it *some* basic testing first, \nthough.\n\n\t\tLinus\n"},{"id":"78155","messageId":"alpine.LFD.1.10.0805300905080.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805300844310.3141@woody.linux-foundation.org","subject":"[PATCH 1/2] Make pack creation always fsync() the result","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-30T16:08:11Z","receivedAt":"2008-05-30T16:08:11Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nFrom: Linus Torvalds \nDate: Fri, 30 May 2008 08:42:16 -0700\n\nThis means that we can depend on packs always being stable on disk,\nsimplifying a lot of the object serialization worries. And unlike loose\nobjects, serializing pack creation IO isn't going to be a performance\nkiller.\n\nSigned-off-by: Linus Torvalds \n---\n\nOk, so this is pretty straightforward. I haven't given it a *lot* of \ntesting, but while it can certainly also have bugs, it's not even trying \nto be \"clever\" like my previous attempt. \n\nI was always a bit leery about doing a 'fsync()' on a read-only file \ndescriptor, and in general about doing an fsync() on a file that was \ncreated by something else. \n\n builtin-pack-objects.c | 4 +++-\n cache.h | 1 +\n csum-file.c | 7 +++++--\n csum-file.h | 6 +++++-\n fast-import.c | 2 +-\n index-pack.c | 1 +\n pack-write.c | 2 +-\n write_or_die.c | 7 +++++++\n 8 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 70d2f5d..4c2e0cd 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -515,10 +515,12 @@ static void write_pack_file(void)\n \t\t * If so, rewrite it like in fast-import\n \t\t */\n \t\tif (pack_to_stdout || nr_written == nr_remaining) {\n-\t\t\tsha1close(f, sha1, 1);\n+\t\t\tunsigned flags = pack_to_stdout ? CSUM_CLOSE : CSUM_FSYNC;\n+\t\t\tsha1close(f, sha1, flags);\n \t\t} else {\n \t\t\tint fd = sha1close(f, NULL, 0);\n \t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written);\n+\t\t\tfsync_or_die(fd, pack_tmp_name);\n \t\t\tclose(fd);\n \t\t}\n \ndiff --git a/cache.h b/cache.h\nindex eab1a17..092a997 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -761,6 +761,7 @@ extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n extern void write_or_die(int fd, const void *buf, size_t count);\n extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n+extern void fsync_or_die(int fd, const char *);\n \n /* pager.c */\n extern void setup_pager(void);\ndiff --git a/csum-file.c b/csum-file.c\nindex 9728a99..ace64f1 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -32,21 +32,24 @@ static void sha1flush(struct sha1file *f, unsigned int count)\n \t}\n }\n \n-int sha1close(struct sha1file *f, unsigned char *result, int final)\n+int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n {\n \tint fd;\n \tunsigned offset = f->offset;\n+\n \tif (offset) {\n \t\tSHA1_Update(&f->ctx, f->buffer, offset);\n \t\tsha1flush(f, offset);\n \t\tf->offset = 0;\n \t}\n-\tif (final) {\n+\tif (flags & (CSUM_CLOSE | CSUM_FSYNC)) {\n \t\t/* write checksum and close fd */\n \t\tSHA1_Final(f->buffer, &f->ctx);\n \t\tif (result)\n \t\t\thashcpy(result, f->buffer);\n \t\tsha1flush(f, 20);\n+\t\tif (flags & CSUM_FSYNC)\n+\t\t\tfsync_or_die(f->fd, f->name);\n \t\tif (close(f->fd))\n \t\t\tdie(\"%s: sha1 file error on close (%s)\",\n \t\t\t f->name, strerror(errno));\ndiff --git a/csum-file.h b/csum-file.h\nindex 1af7656..72c9487 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -16,9 +16,13 @@ struct sha1file {\n \tunsigned char buffer[8192];\n };\n \n+/* sha1close flags */\n+#define CSUM_CLOSE\t1\n+#define CSUM_FSYNC\t2\n+\n extern struct sha1file *sha1fd(int fd, const char *name);\n extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);\n-extern int sha1close(struct sha1file *, unsigned char *, int);\n+extern int sha1close(struct sha1file *, unsigned char *, unsigned int);\n extern int sha1write(struct sha1file *, void *, unsigned int);\n extern void crc32_begin(struct sha1file *);\n extern uint32_t crc32_end(struct sha1file *);\ndiff --git a/fast-import.c b/fast-import.c\nindex 93119bb..e72b286 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -890,7 +890,7 @@ static char *create_index(void)\n \t\tSHA1_Update(&ctx, (*c)->sha1, 20);\n \t}\n \tsha1write(f, pack_data->sha1, sizeof(pack_data->sha1));\n-\tsha1close(f, NULL, 1);\n+\tsha1close(f, NULL, CSUM_FSYNC);\n \tfree(idx);\n \tSHA1_Final(pack_data->sha1, &ctx);\n \treturn tmpfile;\ndiff --git a/index-pack.c b/index-pack.c\nindex aaba944..5ac91ba 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -694,6 +694,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \tif (!from_stdin) {\n \t\tclose(input_fd);\n \t} else {\n+\t\tfsync_or_die(output_fd, curr_pack_name);\n \t\terr = close(output_fd);\n \t\tif (err)\n \t\t\tdie(\"error while closing pack file: %s\", strerror(errno));\ndiff --git a/pack-write.c b/pack-write.c\nindex c66c8af..f52cabe 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -139,7 +139,7 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects,\n \t}\n \n \tsha1write(f, sha1, 20);\n-\tsha1close(f, NULL, 1);\n+\tsha1close(f, NULL, CSUM_FSYNC);\n \tSHA1_Final(sha1, &ctx);\n \treturn index_name;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 32f9914..630be4c 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -78,6 +78,13 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \treturn total;\n }\n \n+void fsync_or_die(int fd, const char *msg)\n+{\n+\tif (fsync(fd) < 0) {\n+\t\tdie(\"%s: fsync error (%s)\", msg, strerror(errno));\n+\t}\n+}\n+\n void write_or_die(int fd, const void *buf, size_t count)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-- \n1.5.6.rc0.48.g5eea\n"},{"id":"78156","messageId":"alpine.LFD.1.10.0805300908200.3141@woody.linux-foundation.org","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805300905080.3141@woody.linux-foundation.org","subject":"[PATCH 2/2] Remove now unnecessary 'sync()' calls","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-30T16:11:55Z","receivedAt":"2008-05-30T16:11:55Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Fri, 30 May 2008 08:54:46 -0700\n\nSince the pack-files are now always created stably on disk, there is no\nneed to sync() before pruning lose objects or old stale pack-files.\n\nSigned-off-by: Linus Torvalds \n---\n\nThis literally just removes the syncs. The only thing they wanted to \nprotect were the pack-files, that are now created stably.\n\nYes, you can screw this up by doing direct filesystem operations on the \npack-files (ie rsync/http walkers etc), but let's face it - those \noperations are pretty much fundamentally more problematic than anything we \ncan do anyway, so I canno bring myself to care.\n\nAlso, maybe I missed some case where we should fsync. I think this is all \ngood, but having other people look at and think about this would be better \nstill.\n\n builtin-prune-packed.c | 1 -\n builtin-prune.c | 1 -\n git-repack.sh | 1 -\n 3 files changed, 0 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-prune-packed.c b/builtin-prune-packed.c\nindex 23faf31..241afbb 100644\n--- a/builtin-prune-packed.c\n+++ b/builtin-prune-packed.c\n@@ -85,7 +85,6 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n \t\t/* Handle arguments here .. */\n \t\tusage(prune_packed_usage);\n \t}\n-\tsync();\n \tprune_packed_objects(opts);\n \treturn 0;\n }\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex 25f9304..bd3d2f6 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -156,7 +156,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tmark_reachable_objects(&revs, 1);\n \tprune_object_dir(get_object_directory());\n \n-\tsync();\n \tprune_packed_objects(show_only);\n \tremove_temporary_files();\n \treturn 0;\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 10f735c..072d1b4 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -125,7 +125,6 @@ then\n \t# We know $existing are all redundant.\n \tif [ -n \"$existing\" ]\n \tthen\n-\t\tsync\n \t\t( cd \"$PACKDIR\" &&\n \t\t for e in $existing\n \t\t do\n-- \n1.5.6.rc0.48.g5eea\n"},{"id":"78162","messageId":"87iqwvo8sp.fsf@mid.deneb.enyo.de","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805291727490.3141@woody.linux-foundation.org","subject":"Re: reducing prune sync()s","fromName":"Florian Weimer","fromEmail":"fw@deneb.enyo.de","sentAt":"2008-05-30T20:07:18Z","receivedAt":"2008-05-30T20:07:18Z","isPatch":false,"sender":{"key":"fw@deneb.enyo.de","avatar":null},"body":"* Linus Torvalds:\n\n> Side note: a lot of systems make \"fsync()\" pretty expensive too. It's one \n> of my main disagreements with most log-based filesystems - fsync() can in \n> theory be fast, but almost always implies flushing the whole log, even if \n> 99.9% of that log is totally unrelated to the actual file you want to \n> fsync().\n\nAnd flushing the whole log might be less expensive than several partial\nflushes with ordering constraints. If Linux ever gets support for\npartial log flushes, I suppose you could restore the previous\nperformance by using sync_file_range() with approriate flags (to get the\ndata in flight to disk), followed by a second round of calls to to\nfsync() (to actually wait for I/O completion).\n\n> So fsync() isn't always all that much better than sync().\n\nsync() is potentially a no-op, particularly if some of the targeted\nfiles are still open.\n"},{"id":"78165","messageId":"alpine.LFD.1.10.0805301620040.23581@xanadu.home","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805300905080.3141@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] Make pack creation always fsync() the result","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-05-30T20:27:01Z","receivedAt":"2008-05-30T20:27:01Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 30 May 2008, Linus Torvalds wrote:\n\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 70d2f5d..4c2e0cd 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -515,10 +515,12 @@ static void write_pack_file(void)\n> \t\t * If so, rewrite it like in fast-import\n> \t\t */\n> \t\tif (pack_to_stdout || nr_written == nr_remaining) {\n> -\t\t\tsha1close(f, sha1, 1);\n> +\t\t\tunsigned flags = pack_to_stdout ? CSUM_CLOSE : CSUM_FSYNC;\n> +\t\t\tsha1close(f, sha1, flags);\n> \t\t} else {\n\nMicro nit: wouldn't it look more obvious if it was written as:\n\n\tif (pack_to_stdout) {\n\t\tsha1close(f, sha1, CSUM_CLOSE);\n\t} else if (nr_written == nr_remaining) {\n\t\tsha1close(f, sha1, CSUM_FSYNC);\n\t} else {\n\t\t...\n\nOtherwise looks sane to me.\n\n\nNicolas\n"},{"id":"78228","messageId":"20080531141927.GC32168@redhat.com","threadId":"13725","inReplyTo":"alpine.LFD.1.10.0805300905080.3141@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] Make pack creation always fsync() the result","fromName":"Frank Ch. Eigler","fromEmail":"fche@redhat.com","sentAt":"2008-05-31T14:19:27Z","receivedAt":"2008-05-31T14:19:27Z","isPatch":true,"sender":{"key":"fche@redhat.com","avatar":"https://gravatar.com/avatar/c42923a074e7dc44da251150f545ae29e0143c6c0cf25b0f6193288cdc166a9e?d=mp&s=160"},"body":"Hi -\n\nOn Fri, May 30, 2008 at 09:08:11AM -0700, Linus Torvalds wrote:\n\n> [fsync on pack creation]\n> This means that we can depend on packs always being stable on disk,\n> simplifying a lot of the object serialization worries. And unlike loose\n> objects, serializing pack creation IO isn't going to be a performance\n> killer. [...]\n\nIf you stabilize the outputs of the pack procedure rather than its\ninputs, this makes me wonder if ordinary unpacked git objects would\nalso need some sort of fsync treatment.\n\n- FChE\n"},{"id":"78426","messageId":"alpine.LFD.1.10.0806021522120.3473@woody.linux-foundation.org","threadId":"13725","inReplyTo":"20080531141927.GC32168@redhat.com","subject":"Re: [PATCH 1/2] Make pack creation always fsync() the result","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-02T22:23:40Z","receivedAt":"2008-06-02T22:23:40Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 31 May 2008, Frank Ch. Eigler wrote:\n> \n> If you stabilize the outputs of the pack procedure rather than its\n> inputs, this makes me wonder if ordinary unpacked git objects would\n> also need some sort of fsync treatment.\n\nNo, see the earlier discussion about the difference between \"old\" and \n\"new\" objects.\n\nPack-files can contain old objects that were _previously_ stable, so we \nneed to make sure that they are at least as stable as the objects they \nreplace. In contrast, new loose objects never replace old data, so they \ncan always be re-created by just re-doing the git operation.\n\n\t\tLinus\n"}]}