{"thread":{"id":"39395","subject":"Troubleshoot clone issue to NFS.","startedAt":"2015-05-21T13:13:32Z","lastAt":"2015-06-16T20:50:06Z","messageCount":32,"participants":["steve.norman@thomsonreuters.com","Christian Couder","Duy Nguyen","Jeff King","Junio C Hamano","Michael Haggerty","Shawn Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"261784","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C5519758FC3@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":null,"subject":"Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-05-21T13:13:32Z","receivedAt":"2015-05-21T13:13:32Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"In setting up some new git servers I was trying to test the performance of some NFS mounted volumes and when compared to local disk (although this is a vitualized server so not truly local) cloning to NFS was taking a long time.\n\nHere are some timings:\n\n~ $ time bin/git clone https://github.com/git/git test\nCloning into 'test'...\nremote: Counting objects: 185964, done.\nremote: Compressing objects: 100% (276/276), done.\nremote: Total 185964 (delta 203), reused 32 (delta 32), pack-reused 185656\nReceiving objects: 100% (185964/185964), 61.42 MiB | 26.16 MiB/s, done.\nResolving deltas: 100% (135454/135454), done.\nChecking connectivity... done.\n\nreal    0m8.156s\nuser    0m10.569s\nsys     0m3.857s\n\n~ $ time bin/git clone https://github.com/git/git /sami/test\nCloning into '/sami/test'...\nremote: Counting objects: 185964, done.\nremote: Compressing objects: 100% (276/276), done.\nremote: Total 185964 (delta 203), reused 32 (delta 32), pack-reused 185656\nReceiving objects: 100% (185964/185964), 61.42 MiB | 10.15 MiB/s, done.\nResolving deltas: 100% (135454/135454), done.\nChecking connectivity... done.\nChecking out files: 100% (2795/2795), done.\n\nreal    0m58.685s\nuser    0m12.153s\nsys     0m7.619s\n\nSo cloning to NFS is 50 seconds slower on average (I've run this a lot over the last few days and it does appear to be consistent and not a network / github connectivity issue).  Tests creating files on the NFS with dd didn't show that much difference to the local disk (and were sometimes quicker).\n\nVolume mount differences are:\n\n/dev/mapper/rootvg-homelv on /home type ext4 (rw,nodev)\nnfsserver:/vol/sami/repos on /sami type nfs (rw,bg,nfsvers=3,tcp,hard,nointr,timeo=600,rsize=32768,wsize=32768,addr=10.1.1.1)\n\nAnd there doesn't appear to be any issue with NFS retransmissions:\n\n/sami $ nfsstat\nClient rpc stats:\ncalls      retrans    authrefrsh\n11719847   0          11720190\n\nThis morning I did some more digging as when I tried this on a newly build server the NFS times were slower than local disk, but only buy around 6-10 seconds.  The new server had git 1.7.1  installed on it compared to 2.4.0 on the machine I've been testing on.  So I build a number of versions of git to test each one to try and find the point where it changed:\n\nv1.8.0          11.363 s\nv1.8.3          13.597 s\nv1.8.4          13.958 s\nv1.8.4.1                14.563 s\nv1.8.4.2                1m 0s\nv1.8.4.3                1m 9s\nv1.8.4.5                1m 1s\nv1.8.5          1m 0s\nv1.8.5.6                1m 0s\nv1.9.0          1m 38\n\nv2.4.0          58s\nv2.4.1          58s\n\nSo there appears to be a change in 1.8.4.2 that made this issue appear for me.  Looking at the release notes the only thing that I can see that might be related could be:\n\n* When an object is not found after checking the packfiles and then\n   loose object directory, read_sha1_file() re-checks the packfiles to\n   prevent racing with a concurrent repacker; teach the same logic to\n   has_sha1_file().\n\nSo the questions are:\n\n1) Should I expect a clone to NFS to be that much slower?\n2) Is there anything I could do to speed it up (I've tried --bare as that is what the repositories will be when stored on NFS and there wasn't really a difference).\n3) What else can I use in git to compare the performance on local to NFS to see if it is just clones that are affected?\n4) I assume I could bisect between 1.8.4.1 and 1.8.4.2 to find exactly where things get worse for me?\n\nThanks for any help,\n\nSteve\n\n________________________________\n\nThis e-mail is for the sole use of the intended recipient and contains information that may be privileged and/or confidential. If you are not an intended recipient, please notify the sender by return e-mail and delete this e-mail and any attachments. Certain required legal entity disclosures can be accessed on our website.<http://thomsonreuters.com/prof_disclosures/>\n"},{"id":"261787","messageId":"CAP8UFD2iOUH-dJAUJvoQTS_BxJU-9ZrmjaBHutUPVc_=_u71GQ@mail.gmail.com","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C5519758FC3@C111KXTEMBX51.ERF.thomson.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-21T14:00:47Z","receivedAt":"2015-05-21T14:00:47Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, May 21, 2015 at 3:13 PM,  <steve.norman@thomsonreuters.com> wrote:\n\n[...]\n\n> So the questions are:\n>\n> 1) Should I expect a clone to NFS to be that much slower?\n> 2) Is there anything I could do to speed it up (I've tried --bare as that is what the repositories will be when stored on NFS and there wasn't really a difference).\n> 3) What else can I use in git to compare the performance on local to NFS to see if it is just clones that are affected?\n> 4) I assume I could bisect between 1.8.4.1 and 1.8.4.2 to find exactly where things get worse for me?\n\nYeah, it would be very nice if you could bisect further.\nYou can probably do it automatically using a script such as the one\nJunio used in this email:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/45195/\n\nThanks,\nChristian.\n"},{"id":"261790","messageId":"CACsJy8AMhEKe-eM7jvYcEx+7ZmfvdD+p1s4VYHjKuAwZsDWc-w@mail.gmail.com","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C5519758FC3@C111KXTEMBX51.ERF.thomson.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-21T14:31:41Z","receivedAt":"2015-05-21T14:31:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 21, 2015 at 8:13 PM,  <steve.norman@thomsonreuters.com> wrote:\n\n> So there appears to be a change in 1.8.4.2 that made this issue appear for me.  Looking at the release notes the only thing that I can see that might be related could be:\n>\n> * When an object is not found after checking the packfiles and then\n>    loose object directory, read_sha1_file() re-checks the packfiles to\n>    prevent racing with a concurrent repacker; teach the same logic to\n>    has_sha1_file().\n\nThat would be commit 45e8a74 (has_sha1_file: re-check pack directory\nbefore giving up - 2013-08-30). Maybe you can try the version without\nthat commit to confirm. In case an object is not found pack directory\nis re-read again, which might cause some increased load on nfs.\nhas_sha1_file() not finding the object should not happen often.. Maybe\nyou could do an strace (following forks) on the \"git clone\" process to\nsee if readdir() is really called a lot?\n-- \nDuy\n"},{"id":"261791","messageId":"CACsJy8Cs6GcRQ-kgnSqwxP4MPHfds9qiir1_O1hc5cZ+0QP-EA@mail.gmail.com","threadId":"39395","inReplyTo":"CACsJy8AMhEKe-eM7jvYcEx+7ZmfvdD+p1s4VYHjKuAwZsDWc-w@mail.gmail.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-21T14:38:30Z","receivedAt":"2015-05-21T14:38:30Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 21, 2015 at 9:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> In case an object is not found pack directory\n> is re-read again, which might cause some increased load on nfs.\n> has_sha1_file() not finding the object should not happen often..\n\nThat last statement is probably very wrong, but I have no time to test\nthis now. In index-pack, there is a has_sha1_file() for file collision\ntest. That call on a fresh clone would fail for _every_ object in the\n(new) pack and the cost of reprepare pack files could be sky high...\n-- \nDuy\n"},{"id":"261794","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C5519759641@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":"CACsJy8Cs6GcRQ-kgnSqwxP4MPHfds9qiir1_O1hc5cZ+0QP-EA@mail.gmail.com","subject":"RE: Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-05-21T15:53:57Z","receivedAt":"2015-05-21T15:53:57Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"On Thu, May 21, 2015a at 9:31 PM, Duy Nguyen [mailto:pclouds@gmail.com], did scribble:\n> > In case an object is not found pack directory is re-read again, which\n> > might cause some increased load on nfs.\n> > has_sha1_file() not finding the object should not happen often..\n> \n> That last statement is probably very wrong, but I have no time to test this\n> now. In index-pack, there is a has_sha1_file() for file collision test. That call\n> on a fresh clone would fail for _every_ object in the\n> (new) pack and the cost of reprepare pack files could be sky high...\n\nConfirmed with bisect that it is that commit:\n\n~/git $ git bisect bad\n45e8a7487339c0f0ea28244ef06851308d07387c is the first bad commit\ncommit 45e8a7487339c0f0ea28244ef06851308d07387c\nAuthor: Jeff King <peff@peff.net>\nDate:   Fri Aug 30 15:14:13 2013 -0400\n\nI have an strace for that build but it is 153 megabytes so I probably shouldn't attach, but the call summary is below:\n\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 93.71   39.670084         103    386835      2171 futex\n  3.16    1.338572           7    190550       181 open\n  1.56    0.658786          18     37450         3 read\n  0.62    0.262740           2    141390           pread\n  0.41    0.171526           5     37313         9 write\n  0.18    0.076166           0    188859    188835 access\n  0.11    0.047941           0    374712           getdents\n  0.11    0.045174          11      4067      3910 lstat\n  0.06    0.023630           0    190425           close\n  0.04    0.017995           6      2975         1 fstat\n  0.02    0.007668        1917         4           wait4\n  0.01    0.004150           1      5065           madvise\n  0.01    0.003548           0     16090         8 recvfrom\n  0.00    0.001872           0      8048           select\n  0.00    0.001870          11       173         1 mkdir\n  0.00    0.000872           0      8055           poll\n  0.00    0.000262          22        12        12 readlink\n  0.00    0.000185           0      1217      1146 stat\n  0.00    0.000158           0       457           mprotect\n  0.00    0.000074           0       298           mmap\n  0.00    0.000069           1       109         8 rt_sigreturn\n  0.00    0.000047           0       159           brk\n  0.00    0.000021           1        17           getcwd\n  0.00    0.000000           0        42         3 lseek\n  0.00    0.000000           0        92           munmap\n  0.00    0.000000           0        35           rt_sigaction\n  0.00    0.000000           0         9           rt_sigprocmask\n  0.00    0.000000           0         8         3 ioctl\n  0.00    0.000000           0        11           pipe\n  0.00    0.000000           0         3           dup\n  0.00    0.000000           0         8           dup2\n  0.00    0.000000           0         6           setitimer\n  0.00    0.000000           0        11         1 socket\n  0.00    0.000000           0         8         7 connect\n  0.00    0.000000           0         8           sendto\n  0.00    0.000000           0         2           recvmsg\n  0.00    0.000000           0         1           bind\n  0.00    0.000000           0         1           getsockname\n  0.00    0.000000           0         3         1 getpeername\n  0.00    0.000000           0         2           setsockopt\n  0.00    0.000000           0         2           getsockopt\n  0.00    0.000000           0         8           clone\n  0.00    0.000000           0         5           execve\n  0.00    0.000000           0         3           uname\n  0.00    0.000000           0       100           fcntl\n  0.00    0.000000           0         2           fsync\n  0.00    0.000000           0        13           chdir\n  0.00    0.000000           0        14           rename\n  0.00    0.000000           0         2           link\n  0.00    0.000000           0         5           unlink\n  0.00    0.000000           0         2           symlink\n  0.00    0.000000           0         9           chmod\n  0.00    0.000000           0         6           getrlimit\n  0.00    0.000000           0         2           sysinfo\n  0.00    0.000000           0         8           getuid\n  0.00    0.000000           0         1           statfs\n  0.00    0.000000           0         5           arch_prctl\n  0.00    0.000000           0         1           gettid\n  0.00    0.000000           0         5           set_tid_address\n  0.00    0.000000           0        13           set_robust_list\n------ ----------- ----------- --------- --------- ----------------\n100.00   42.333410               1594736    196300 total\n\nIs there anything else I can provide or test?\n\nThanks for the help,\n\nSteve\n\n"},{"id":"261866","messageId":"CACsJy8BULBJ=cL1+4TFX_7tYSCFL3MNEz1Ay0YGqx8W_8=nwAg@mail.gmail.com","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C5519759641@C111KXTEMBX51.ERF.thomson.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-22T00:16:54Z","receivedAt":"2015-05-22T00:16:54Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 21, 2015 at 10:53 PM,  <steve.norman@thomsonreuters.com> wrote:\n> On Thu, May 21, 2015a at 9:31 PM, Duy Nguyen [mailto:pclouds@gmail.com], did scribble:\n>> > In case an object is not found pack directory is re-read again, which\n>> > might cause some increased load on nfs.\n>> > has_sha1_file() not finding the object should not happen often..\n>>\n>> That last statement is probably very wrong, but I have no time to test this\n>> now. In index-pack, there is a has_sha1_file() for file collision test. That call\n>> on a fresh clone would fail for _every_ object in the\n>> (new) pack and the cost of reprepare pack files could be sky high...\n>\n> Confirmed with bisect that it is that commit:\n>\n> ~/git $ git bisect bad\n> 45e8a7487339c0f0ea28244ef06851308d07387c is the first bad commit\n> commit 45e8a7487339c0f0ea28244ef06851308d07387c\n> Author: Jeff King <peff@peff.net>\n> Date:   Fri Aug 30 15:14:13 2013 -0400\n>\n> I have an strace for that build but it is 153 megabytes so I probably shouldn't attach, but the call summary is below:\n>\n> % time     seconds  usecs/call     calls    errors syscall\n> ------ ----------- ----------- --------- --------- ----------------\n>  93.71   39.670084         103    386835      2171 futex\n>   3.16    1.338572           7    190550       181 open\n>   1.56    0.658786          18     37450         3 read\n>   0.62    0.262740           2    141390           pread\n>   0.41    0.171526           5     37313         9 write\n>   0.18    0.076166           0    188859    188835 access\n>   0.11    0.047941           0    374712           getdents\n>   0.11    0.045174          11      4067      3910 lstat\n>   0.06    0.023630           0    190425           close\n>   0.04    0.017995           6      2975         1 fstat\n>   0.02    0.007668        1917         4           wait4\n>   0.01    0.004150           1      5065           madvise\n>   0.01    0.003548           0     16090         8 recvfrom\n>   0.00    0.001872           0      8048           select\n>   0.00    0.001870          11       173         1 mkdir\n>   0.00    0.000872           0      8055           poll\n>   0.00    0.000262          22        12        12 readlink\n>   0.00    0.000185           0      1217      1146 stat\n>   0.00    0.000158           0       457           mprotect\n>   0.00    0.000074           0       298           mmap\n>   0.00    0.000069           1       109         8 rt_sigreturn\n>   0.00    0.000047           0       159           brk\n>   0.00    0.000021           1        17           getcwd\n>   0.00    0.000000           0        42         3 lseek\n>   0.00    0.000000           0        92           munmap\n>   0.00    0.000000           0        35           rt_sigaction\n>   0.00    0.000000           0         9           rt_sigprocmask\n>   0.00    0.000000           0         8         3 ioctl\n>   0.00    0.000000           0        11           pipe\n>   0.00    0.000000           0         3           dup\n>   0.00    0.000000           0         8           dup2\n>   0.00    0.000000           0         6           setitimer\n>   0.00    0.000000           0        11         1 socket\n>   0.00    0.000000           0         8         7 connect\n>   0.00    0.000000           0         8           sendto\n>   0.00    0.000000           0         2           recvmsg\n>   0.00    0.000000           0         1           bind\n>   0.00    0.000000           0         1           getsockname\n>   0.00    0.000000           0         3         1 getpeername\n>   0.00    0.000000           0         2           setsockopt\n>   0.00    0.000000           0         2           getsockopt\n>   0.00    0.000000           0         8           clone\n>   0.00    0.000000           0         5           execve\n>   0.00    0.000000           0         3           uname\n>   0.00    0.000000           0       100           fcntl\n>   0.00    0.000000           0         2           fsync\n>   0.00    0.000000           0        13           chdir\n>   0.00    0.000000           0        14           rename\n>   0.00    0.000000           0         2           link\n>   0.00    0.000000           0         5           unlink\n>   0.00    0.000000           0         2           symlink\n>   0.00    0.000000           0         9           chmod\n>   0.00    0.000000           0         6           getrlimit\n>   0.00    0.000000           0         2           sysinfo\n>   0.00    0.000000           0         8           getuid\n>   0.00    0.000000           0         1           statfs\n>   0.00    0.000000           0         5           arch_prctl\n>   0.00    0.000000           0         1           gettid\n>   0.00    0.000000           0         5           set_tid_address\n>   0.00    0.000000           0        13           set_robust_list\n> ------ ----------- ----------- --------- --------- ----------------\n> 100.00   42.333410               1594736    196300 total\n>\n> Is there anything else I can provide or test?\n\nIn builtin/index-pack.c, replace the line \"collision_test_needed =\nhas_sha1_file(sha1);\" with \"collision_test_needed = 0;\". Security is\ncompromised but for this test it should be ok. Then clone again. I\nhope the new number gets down close to v1.8.4.1.\n-- \nDuy\n"},{"id":"261889","messageId":"20150522071224.GA10734@peff.net","threadId":"39395","inReplyTo":"CACsJy8BULBJ=cL1+4TFX_7tYSCFL3MNEz1Ay0YGqx8W_8=nwAg@mail.gmail.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T07:12:24Z","receivedAt":"2015-05-22T07:12:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2015 at 07:16:54AM +0700, Duy Nguyen wrote:\n\n> > Is there anything else I can provide or test?\n> \n> In builtin/index-pack.c, replace the line \"collision_test_needed =\n> has_sha1_file(sha1);\" with \"collision_test_needed = 0;\". Security is\n> compromised but for this test it should be ok. Then clone again. I\n> hope the new number gets down close to v1.8.4.1.\n\nYeah, I think that is a good starting point. I timed a local clone\nbefore and after 45e8a74; there is a small difference on my system\n(about 5%), but it goes away with your suggestion.\n\nThe problem with has_sha1_file() prior to v1.8.4.2 is that it is racy\nwith respect to simultaneous operations; we might claim we do not have\nan object, when in fact we do. As you noted, usually has_sha1_file()\nreturns true (i.e., we look up objects that we expect to have), and the\nperformance impact is minimal.\n\nBut for code paths where _not_ having the object is normal, the impact\nis much greater. So I think there are two possibilities for improving\nthis:\n\n  1. Find places where we expect the object will not exist (like the\n     collision_test check you pointed out) and use a\n     \"has_sha1_file_fast\" that accepts that it may very occasionally\n     erroneously return false. In this case it would mean potentially\n     skipping a collision check, but I think that is OK. That could have\n     security implications, but only if an attacker:\n\n       a. has broken sha1 to generate a colliding object\n\n       b. can manipulate the victim into repacking in a loop\n\n       c. can manipulate the victim into fetching (or receiving a push)\n          simultaneously with (b)\n\n     at which point they can try to race the repack procedure to add\n     their colliding object to the repository. It seems rather unlikely\n     (especially part a).\n\n  2. Make reprepare_packed_git() cheaper in the common case that nothing\n     has changed. It would probably be enough to stat(\"objects/pack\").\n     We know that packfiles themselves do not change; we may only add or\n     delete them. And since the hierarchy of objects/pack is flat, we\n     know that the mtime on that directory will change if any packs are\n     added or removed.\n\n     Of course, we are still doing an extra stat() for each has_sha1_file\n     call. Whether that helps for the NFS case depends on whether stat()\n     is significantly cheaper than opendir/readdir/closedir. On my local\n     disk, the hacky patch below did seem to give me back the 5% lost by\n     45e8a74 (I did it directly on master, since that old commit does\n     not have the stat_validity infrastructure).\n\n---\ndiff --git a/cache.h b/cache.h\nindex c97d807..17c840c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1661,6 +1661,7 @@ int sane_execvp(const char *file, char *const argv[]);\n  */\n struct stat_validity {\n \tstruct stat_data *sd;\n+\tunsigned mode;\n };\n \n void stat_validity_clear(struct stat_validity *sv);\ndiff --git a/read-cache.c b/read-cache.c\nindex 36ff89f..1d8702f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2281,6 +2281,7 @@ void stat_validity_clear(struct stat_validity *sv)\n {\n \tfree(sv->sd);\n \tsv->sd = NULL;\n+\tsv->mode = 0;\n }\n \n int stat_validity_check(struct stat_validity *sv, const char *path)\n@@ -2291,18 +2292,19 @@ int stat_validity_check(struct stat_validity *sv, const char *path)\n \t\treturn sv->sd == NULL;\n \tif (!sv->sd)\n \t\treturn 0;\n-\treturn S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);\n+\treturn sv->mode == st.st_mode && !match_stat_data(sv->sd, &st);\n }\n \n void stat_validity_update(struct stat_validity *sv, int fd)\n {\n \tstruct stat st;\n \n-\tif (fstat(fd, &st) < 0 || !S_ISREG(st.st_mode))\n+\tif (fstat(fd, &st) < 0)\n \t\tstat_validity_clear(sv);\n \telse {\n \t\tif (!sv->sd)\n \t\t\tsv->sd = xcalloc(1, sizeof(struct stat_data));\n+\t\tsv->mode = st.st_mode;\n \t\tfill_stat_data(sv->sd, &st);\n \t}\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ccc6dac..abe1be9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1225,7 +1225,8 @@ static void report_pack_garbage(struct string_list *list)\n \treport_helper(list, seen_bits, first, list->nr);\n }\n \n-static void prepare_packed_git_one(char *objdir, int local)\n+static void prepare_packed_git_one(char *objdir, int local,\n+\t\t\t\t   struct stat_validity *validity)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tsize_t dirnamelen;\n@@ -1235,6 +1236,12 @@ static void prepare_packed_git_one(char *objdir, int local)\n \n \tstrbuf_addstr(&path, objdir);\n \tstrbuf_addstr(&path, \"/pack\");\n+\n+\tif (stat_validity_check(validity, path.buf)) {\n+\t\tstrbuf_release(&path);\n+\t\treturn;\n+\t}\n+\n \tdir = opendir(path.buf);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n@@ -1243,6 +1250,10 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tstrbuf_release(&path);\n \t\treturn;\n \t}\n+\n+\t/* XXX dirfd is a BSD-ism that only made it into POSIX.1-2008 */\n+\tstat_validity_update(validity, dirfd(dir));\n+\n \tstrbuf_addch(&path, '/');\n \tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n@@ -1348,15 +1359,17 @@ static void rearrange_packed_git(void)\n static int prepare_packed_git_run_once = 0;\n void prepare_packed_git(void)\n {\n+\tstatic struct stat_validity validity;\n \tstruct alternate_object_database *alt;\n \n \tif (prepare_packed_git_run_once)\n \t\treturn;\n-\tprepare_packed_git_one(get_object_directory(), 1);\n+\tprepare_packed_git_one(get_object_directory(), 1, &validity);\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\talt->name[-1] = 0;\n-\t\tprepare_packed_git_one(alt->base, 0);\n+\t\t/* XXX should carry pack validity struct for each alternate */\n+\t\tprepare_packed_git_one(alt->base, 0, NULL);\n \t\talt->name[-1] = '/';\n \t}\n \trearrange_packed_git();\n"},{"id":"261891","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C551975ADA4@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":"20150522071224.GA10734@peff.net","subject":"RE: Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-05-22T08:35:52Z","receivedAt":"2015-05-22T08:35:52Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"On Friday, May 22, 2015 @ 8:12 AM Jeff King did scribble:\n\n> > In builtin/index-pack.c, replace the line \"collision_test_needed =\n> > has_sha1_file(sha1);\" with \"collision_test_needed = 0;\". Security is\n> > compromised but for this test it should be ok. Then clone again. I\n> > hope the new number gets down close to v1.8.4.1.\n> \n> Yeah, I think that is a good starting point. I timed a local clone\n> before and after 45e8a74; there is a small difference on my system\n> (about 5%), but it goes away with your suggestion.\n\nTested this change on a couple of versions, first of all on the revision\nwhere things go wrong for me:\n\n~ $ bin/git --version\ngit version 1.8.4.1.g45e8a74.dirty\n\n~ $ time bin/git clone https://github.com/git/git test\n<snip>\n\nreal    0m7.105s\nuser    0m9.566s\nsys     0m0.989s\n\n~ $ time bin/git clone https://github.com/git/git /sami/test\n<snip>\n\nreal    0m14.411s\nuser    0m9.703s\nsys     0m1.374s\n\nThis is more in line with what I see normally.   Also tested on master:\n\n~ $ bin/git --version\ngit version 2.4.1.217.g6c1249c.dirty\n\n~ $ time bin/git clone https://github.com/git/git test\n<snip>\n\nreal    0m5.946s\nuser    0m9.111s\nsys     0m1.332s\n\n\n~ $ time bin/git clone https://github.com/git/git /sami/test\n<snip>\n\nreal    0m12.344s\nuser    0m9.187s\nsys     0m1.579s\n\nSo similar on the latest as well.\n\n> The problem with has_sha1_file() prior to v1.8.4.2 is that it is racy\n> with respect to simultaneous operations; we might claim we do not have\n> an object, when in fact we do. As you noted, usually has_sha1_file()\n> returns true (i.e., we look up objects that we expect to have), and the\n> performance impact is minimal.\n> \n> But for code paths where _not_ having the object is normal, the impact\n> is much greater. So I think there are two possibilities for improving\n> this:\n> \n>   1. Find places where we expect the object will not exist (like the\n>      collision_test check you pointed out) and use a\n>      \"has_sha1_file_fast\" that accepts that it may very occasionally\n>      erroneously return false. In this case it would mean potentially\n>      skipping a collision check, but I think that is OK. That could have\n>      security implications, but only if an attacker:\n> \n>        a. has broken sha1 to generate a colliding object\n> \n>        b. can manipulate the victim into repacking in a loop\n> \n>        c. can manipulate the victim into fetching (or receiving a push)\n>           simultaneously with (b)\n> \n>      at which point they can try to race the repack procedure to add\n>      their colliding object to the repository. It seems rather unlikely\n>      (especially part a).\n> \n>   2. Make reprepare_packed_git() cheaper in the common case that nothing\n>      has changed. It would probably be enough to stat(\"objects/pack\").\n>      We know that packfiles themselves do not change; we may only add or\n>      delete them. And since the hierarchy of objects/pack is flat, we\n>      know that the mtime on that directory will change if any packs are\n>      added or removed.\n> \n>      Of course, we are still doing an extra stat() for each has_sha1_file\n>      call. Whether that helps for the NFS case depends on whether stat()\n>      is significantly cheaper than opendir/readdir/closedir. On my local\n>      disk, the hacky patch below did seem to give me back the 5% lost by\n>      45e8a74 (I did it directly on master, since that old commit does\n>      not have the stat_validity infrastructure).\n\nAlso tested master with the patch provided:\n\n~ $ bin/git --version\ngit version 2.4.1.217.g6c1249c.dirty\n\n~ $ time git clone https://github.com/git/git test\n\nreal    0m8.263s\nuser    0m10.550s\nsys     0m3.763s\n\n~ $ time git clone https://github.com/git/git /sami/test\n\nreal    1m3.286s\nuser    0m12.149s\nsys     0m9.192s\n\nSo the patch isn't reducing the time taken when cloning to NAS.\n\nHere are the top calls from strace\n\n% time     seconds  usecs/call     calls    errors syscall\n------ ----------- ----------- --------- --------- ----------------\n 90.68   19.946171          46    437398     45436 futex\n  4.63    1.017637          22     46141         5 read\n  2.37    0.521161           4    141429           pread\n  0.73    0.161442           3     47130         9 write\n  0.42    0.093146           0    188645    188621 access\n  0.38    0.083033          26      3209       181 open\n  0.32    0.069587           0    188613      1146 stat\n  0.23    0.050855          12      4082      3925 lstat\n  0.11    0.023317           8      2979         1 fstat\n  0.04    0.009134           0     35696         3 recvfrom\n  0.03    0.007666        1917         4           wait4\n  0.02    0.004478           1      3923           madvise\n  0.01    0.002291           0     17858           poll\n  0.01    0.002155           0     17851           select\n\nThanks for looking into this.\n\nSteve\n"},{"id":"261894","messageId":"CACsJy8DMqj8sCowZ5f-QZMgrAMFjGOaWfYjZWHxs+jvBsDpp5w@mail.gmail.com","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C551975ADA4@C111KXTEMBX51.ERF.thomson.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-22T10:05:36Z","receivedAt":"2015-05-22T10:05:36Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 22, 2015 at 3:35 PM,  <steve.norman@thomsonreuters.com> wrote:\n> Tested this change on a couple of versions, first of all on the revision\n> where things go wrong for me:\n>\n> ...\n>\n> ~ $ time git clone https://github.com/git/git test\n>\n> real    0m8.263s\n> user    0m10.550s\n> sys     0m3.763s\n>\n> ~ $ time git clone https://github.com/git/git /sami/test\n>\n> real    1m3.286s\n> user    0m12.149s\n> sys     0m9.192s\n>\n> So the patch isn't reducing the time taken when cloning to NAS.\n\nStrange. Maybe there is something else... Anyway some numbers from me.\nThis is nfs3 hosted by Raspberry Pi, accessed over wireless. I just\nrun index-pack on git.git pack instead of full clone.\n\n - v1.8.4.1 34s\n - v1.8.4.2 519s (oh.. wireless..)\n - v1.8.4.2 without has_sha1_file() in index-pack.c 33s\n - v1.8.4.2 + Jeff's mtime patch 36s\n-- \nDuy\n"},{"id":"261912","messageId":"xmqqsiaofzk0.fsf@gitster.dls.corp.google.com","threadId":"39395","inReplyTo":"CACsJy8DMqj8sCowZ5f-QZMgrAMFjGOaWfYjZWHxs+jvBsDpp5w@mail.gmail.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-22T14:37:51Z","receivedAt":"2015-05-22T14:37:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Strange. Maybe there is something else... Anyway some numbers from me.\n> This is nfs3 hosted by Raspberry Pi, accessed over wireless. I just\n> run index-pack on git.git pack instead of full clone.\n\nSo the checkout codepath to touch working tree is one difference?\nAre there other differences?\n\n>\n>  - v1.8.4.1 34s\n>  - v1.8.4.2 519s (oh.. wireless..)\n>  - v1.8.4.2 without has_sha1_file() in index-pack.c 33s\n>  - v1.8.4.2 + Jeff's mtime patch 36s\n"},{"id":"261917","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C551975B851@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":"CACsJy8DMqj8sCowZ5f-QZMgrAMFjGOaWfYjZWHxs+jvBsDpp5w@mail.gmail.com","subject":"RE: Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-05-22T15:02:45Z","receivedAt":"2015-05-22T15:02:45Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"On Friday, May 22, 2015 @ 11:06 AM Duy Nguyen did write:\n\n> Strange. Maybe there is something else... Anyway some numbers from me.\n> This is nfs3 hosted by Raspberry Pi, accessed over wireless. I just\n> run index-pack on git.git pack instead of full clone.\n> \n>  - v1.8.4.1 34s\n>  - v1.8.4.2 519s (oh.. wireless..)\n>  - v1.8.4.2 without has_sha1_file() in index-pack.c 33s\n>  - v1.8.4.2 + Jeff's mtime patch 36s\n\nJust ran the test again and it was 12 seconds.   Too many versions of git available on the \nmachine and I think I might have run the wrong one:\n\n~ $ time bin/git clone https://github.com/git/git test\nCloning into 'test'...\nremote: Counting objects: 186015, done.\nremote: Compressing objects: 100% (8/8), done.\nremote: Total 186015 (delta 8), reused 7 (delta 7), pack-reused 186000\nReceiving objects: 100% (186015/186015), 61.33 MiB | 30.35 MiB/s, done.\nResolving deltas: 100% (135512/135512), done.\nChecking connectivity... done.\n\nreal    0m6.931s\nuser    0m10.011s\nsys     0m2.685s\n\n~ $ time bin/git clone https://github.com/git/git /sami/test\nCloning into '/sami/test'...\nremote: Counting objects: 186015, done.\nremote: Compressing objects: 100% (8/8), done.\nremote: Total 186015 (delta 8), reused 7 (delta 7), pack-reused 186000\nReceiving objects: 100% (186015/186015), 61.33 MiB | 29.91 MiB/s, done.\nResolving deltas: 100% (135512/135512), done.\nChecking connectivity... done.\nChecking out files: 100% (2795/2795), done.\n\nreal    0m13.169s\nuser    0m9.961s\nsys     0m3.480s\n\nSo  I would say that was in line with the timings for the other versions I have tested.\n\nAnd looking back at the email from the morning I did run the wrong bin/git.   Sorry, totally my fault on that. \n\nIf I can test anything else then let me know.\n\nThanks,\n\nSteve\n--\n"},{"id":"261965","messageId":"20150522235116.GA4300@peff.net","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C551975B851@C111KXTEMBX51.ERF.thomson.com","subject":"[PATCH/RFC 0/3] using stat() to avoid re-scanning pack dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T23:51:16Z","receivedAt":"2015-05-22T23:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2015 at 03:02:45PM +0000, steve.norman@thomsonreuters.com wrote:\n\n> On Friday, May 22, 2015 @ 11:06 AM Duy Nguyen did write:\n> \n> > Strange. Maybe there is something else... Anyway some numbers from me.\n> > This is nfs3 hosted by Raspberry Pi, accessed over wireless. I just\n> > run index-pack on git.git pack instead of full clone.\n> > \n> >  - v1.8.4.1 34s\n> >  - v1.8.4.2 519s (oh.. wireless..)\n> >  - v1.8.4.2 without has_sha1_file() in index-pack.c 33s\n> >  - v1.8.4.2 + Jeff's mtime patch 36s\n> \n> Just ran the test again and it was 12 seconds.   Too many versions of git available on the \n> machine and I think I might have run the wrong one:\n\nThanks for re-checking. Here's a series that fixes the rough edges of\nthe patch I sent earlier. I'd appreciate it if you can re-confirm that\nit improves things for you.\n\n  [1/3]: stat_validity: handle non-regular files\n  [2/3]: cache.h: move stat_validity definition up\n  [3/3]: prepare_packed_git: use stat_validity to avoid re-reading packs\n\nI'm adding Michael to the cc, as I'm abusing the stat_validity code\nwhich we worked on in 2013.\n\nMy big concern here is that using stat_validity with a directory is\nracy. It works for a regular file like packed-refs because that file is\nreplaced atomically.  We fill the validity using fstat() on the same\ndescriptor we read the data from, and nobody modifies an already-written\nfile. So we know it matches what we read.\n\nFor a directory, I don't think that atomicity guarantee is there.\nSomebody can modify the direct while we're reading. For the most part, I\nthink that is OK. We fstat() before reading any entries, so our fstat\ndata will then become out of date, and we'll re-read next time. It would\nbe a problem if opendir() looked at the entries at all (e.g., if it\ncalled getdents() under Linux before our first readdir() call, then our\nfstat is happening after the first read, and wouldn't match what we\nread). I don't have any reason to believe that any libc does that, but\nit is making an assumption about how opendir() is implemented.\n\nThe other problem is that I'm not sure stat data is enough to notice\nwhen a directory changes. Certainly the mtime should change, but if you\nhave only one-second resolution on your mtimes, we can be fooled. For a\nregular file that is replaced atomically, the inode will change (and\nprobably the size, too). But I don't know if that is the case for a\ndirectory. Can writing an entry go undetected between two stat() calls\n(or something even more pathological, like deleting one entry and\nwriting another one)?\n\n-Peff\n"},{"id":"261966","messageId":"20150522235143.GA4818@peff.net","threadId":"39395","inReplyTo":"20150522235116.GA4300@peff.net","subject":"[PATCH 1/3] stat_validity: handle non-regular files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T23:51:43Z","receivedAt":"2015-05-22T23:51:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The stat_validity code was originally written to avoid\nre-reading the packed-refs file when it has not changed. It\nmakes sure that the file continues to match S_ISREG() when\nwe check it.\n\nHowever, we can use the same concept on a directory to see\nwhether it has been modified. Even though we still have to\ntouch the filesystem to do the stat, this can provide a\nspeedup even over opendir/readdir/closedir, especially on\nhigh-latency filesystems like NFS.\n\nThis patch adds a \"mode\" field to stat_validity, which lets\nus check that the mode has stayed the same (rather than\nexplicitly checking that it is a regular file).\n\nAs a bonus cleanup, we can stop allocating the embedded\n\"stat_data\" on the heap. Prior to this patch, we needed to\nrepresent the case where the file did not exist, and we used\n\"sv->sd == NULL\" for that. Now we can simply check that\n\"sv->mode\" is 0.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h      |  3 ++-\n read-cache.c | 16 ++++++----------\n 2 files changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c97d807..2941e7e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1660,7 +1660,8 @@ int sane_execvp(const char *file, char *const argv[]);\n  * for the index.\n  */\n struct stat_validity {\n-\tstruct stat_data *sd;\n+\tstruct stat_data sd;\n+\tunsigned mode;\n };\n \n void stat_validity_clear(struct stat_validity *sv);\ndiff --git a/read-cache.c b/read-cache.c\nindex 36ff89f..115b000 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2279,8 +2279,7 @@ void *read_blob_data_from_index(struct index_state *istate, const char *path, un\n \n void stat_validity_clear(struct stat_validity *sv)\n {\n-\tfree(sv->sd);\n-\tsv->sd = NULL;\n+\tmemset(sv, 0, sizeof(*sv));\n }\n \n int stat_validity_check(struct stat_validity *sv, const char *path)\n@@ -2288,21 +2287,18 @@ int stat_validity_check(struct stat_validity *sv, const char *path)\n \tstruct stat st;\n \n \tif (stat(path, &st) < 0)\n-\t\treturn sv->sd == NULL;\n-\tif (!sv->sd)\n-\t\treturn 0;\n-\treturn S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);\n+\t\treturn sv->mode == 0;\n+\treturn sv->mode == st.st_mode && !match_stat_data(&sv->sd, &st);\n }\n \n void stat_validity_update(struct stat_validity *sv, int fd)\n {\n \tstruct stat st;\n \n-\tif (fstat(fd, &st) < 0 || !S_ISREG(st.st_mode))\n+\tif (fstat(fd, &st) < 0)\n \t\tstat_validity_clear(sv);\n \telse {\n-\t\tif (!sv->sd)\n-\t\t\tsv->sd = xcalloc(1, sizeof(struct stat_data));\n-\t\tfill_stat_data(sv->sd, &st);\n+\t\tsv->mode = st.st_mode;\n+\t\tfill_stat_data(&sv->sd, &st);\n \t}\n }\n-- \n2.4.1.538.g69ac333\n"},{"id":"261967","messageId":"20150522235207.GB4818@peff.net","threadId":"39395","inReplyTo":"20150522235116.GA4300@peff.net","subject":"[PATCH 2/3] cache.h: move stat_validity definition up","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T23:52:07Z","receivedAt":"2015-05-22T23:52:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It would be nice to embed stat_validity structs inside other\nstructs defined in cache.h. We cannot get away with a\nforward declaration, because using it in a struct definition\nmeans the compiler needs the real size.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h | 56 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 2941e7e..cdd279a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1185,6 +1185,34 @@ extern int has_dirs_only_path(const char *name, int len, int prefix_len);\n extern void schedule_dir_for_removal(const char *name, int len);\n extern void remove_scheduled_dirs(void);\n \n+/*\n+ * A struct to encapsulate the concept of whether a file has changed\n+ * since we last checked it. This uses criteria similar to those used\n+ * for the index.\n+ */\n+struct stat_validity {\n+\tstruct stat_data sd;\n+\tunsigned mode;\n+};\n+\n+void stat_validity_clear(struct stat_validity *sv);\n+\n+/*\n+ * Returns 1 if the path is a regular file (or a symlink to a regular\n+ * file) and matches the saved stat_validity, 0 otherwise.  A missing\n+ * or inaccessible file is considered a match if the struct was just\n+ * initialized, or if the previous update found an inaccessible file.\n+ */\n+int stat_validity_check(struct stat_validity *sv, const char *path);\n+\n+/*\n+ * Update the stat_validity from a file opened at descriptor fd. If\n+ * the file is missing, inaccessible, or not a regular file, then\n+ * future calls to stat_validity_check will match iff one of those\n+ * conditions continues to be true.\n+ */\n+void stat_validity_update(struct stat_validity *sv, int fd);\n+\n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\n \tchar *name;\n@@ -1654,34 +1682,6 @@ int checkout_fast_forward(const unsigned char *from,\n \n int sane_execvp(const char *file, char *const argv[]);\n \n-/*\n- * A struct to encapsulate the concept of whether a file has changed\n- * since we last checked it. This uses criteria similar to those used\n- * for the index.\n- */\n-struct stat_validity {\n-\tstruct stat_data sd;\n-\tunsigned mode;\n-};\n-\n-void stat_validity_clear(struct stat_validity *sv);\n-\n-/*\n- * Returns 1 if the path is a regular file (or a symlink to a regular\n- * file) and matches the saved stat_validity, 0 otherwise.  A missing\n- * or inaccessible file is considered a match if the struct was just\n- * initialized, or if the previous update found an inaccessible file.\n- */\n-int stat_validity_check(struct stat_validity *sv, const char *path);\n-\n-/*\n- * Update the stat_validity from a file opened at descriptor fd. If\n- * the file is missing, inaccessible, or not a regular file, then\n- * future calls to stat_validity_check will match iff one of those\n- * conditions continues to be true.\n- */\n-void stat_validity_update(struct stat_validity *sv, int fd);\n-\n int versioncmp(const char *s1, const char *s2);\n \n #endif /* CACHE_H */\n-- \n2.4.1.538.g69ac333\n"},{"id":"261968","messageId":"20150522235404.GA4973@peff.net","threadId":"39395","inReplyTo":"20150522235116.GA4300@peff.net","subject":"[PATCH 3/3] prepare_packed_git: use stat_validity to avoid re-reading packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-22T23:54:05Z","receivedAt":"2015-05-22T23:54:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we try to read an object and fail, we call\nreprepare_packed_git() to re-scan the list of packfiles.\nThis catches any \"racy\" cases in which somebody is repacking\nor making new objects. The performance implications of\nre-scanning the pack directory are not usually a big deal,\nbecause failing to find an object is the unlikely case;\nwe typically die if the re-scan does not help.\n\nSince 45e8a74 (has_sha1_file: re-check pack directory before\ngiving up, 2013-08-30), we do the same thing for\nhas_sha1_file.  Some calls to has_sha1_file are in the same\nboat: they expect most calls to return true. But some sites,\nsuch as the collision test in index-pack.c, may make a large\nnumber of calls, most of which they expect to be false. On a\nlocal system, this can cause a performance slowdown of\naround 5%. But on a system with high-latency system calls\n(like NFS), it can be much worse.\n\nSince the common case is that the pack directory has _not_\nchanged, we can improve the performance by using stat()\nbefore doing the opendir()/readdir()/closedir() to re-scan\nthe directory. It's valid to check stat() for just the\nsingle \"objects/pack\" directory because:\n\n  1. We know that packfiles themselves are not changed in\n     place; only new files are written, which would update\n     the mtime of the directory.\n\n  2. There are no subdirectories inside the pack directory.\n     Checking the single top-level directory can tell us\n     whether anything changed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not sure what we want to do for the case where we don't have DIRFD.\nWe have to separately open the directory, but I'm not sure if regular\nopen() is even valid on a regular directory on all systems. We could\njust skip the stat_validity entirely in that case.\n\n cache.h           |  1 +\n git-compat-util.h |  4 ++++\n sha1_file.c       | 27 ++++++++++++++++++++++++---\n 3 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex cdd279a..9c34a17 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1216,6 +1216,7 @@ void stat_validity_update(struct stat_validity *sv, int fd);\n extern struct alternate_object_database {\n \tstruct alternate_object_database *next;\n \tchar *name;\n+\tstruct stat_validity pack_validity;\n \tchar base[FLEX_ARRAY]; /* more */\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b7a97fb..8631e75 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -941,4 +941,8 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n+#if _POSIX_VERSION >= 200809L\n+#define HAVE_DIRFD\n+#endif\n+\n #endif\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ccc6dac..0e77838 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1225,7 +1225,8 @@ static void report_pack_garbage(struct string_list *list)\n \treport_helper(list, seen_bits, first, list->nr);\n }\n \n-static void prepare_packed_git_one(char *objdir, int local)\n+static void prepare_packed_git_one(char *objdir, int local,\n+\t\t\t\t   struct stat_validity *validity)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tsize_t dirnamelen;\n@@ -1235,6 +1236,12 @@ static void prepare_packed_git_one(char *objdir, int local)\n \n \tstrbuf_addstr(&path, objdir);\n \tstrbuf_addstr(&path, \"/pack\");\n+\n+\tif (stat_validity_check(validity, path.buf)) {\n+\t\tstrbuf_release(&path);\n+\t\treturn;\n+\t}\n+\n \tdir = opendir(path.buf);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n@@ -1243,6 +1250,19 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tstrbuf_release(&path);\n \t\treturn;\n \t}\n+\n+#ifdef HAVE_DIRFD\n+\tstat_validity_update(validity, dirfd(dir));\n+#else\n+\t{\n+\t\tint fd = open(path.buf, O_RDONLY);\n+\t\tif (fd >= 0) {\n+\t\t\tstat_validity_update(validity, fd);\n+\t\t\tclose(fd);\n+\t\t}\n+\t}\n+#endif\n+\n \tstrbuf_addch(&path, '/');\n \tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n@@ -1348,15 +1368,16 @@ static void rearrange_packed_git(void)\n static int prepare_packed_git_run_once = 0;\n void prepare_packed_git(void)\n {\n+\tstatic struct stat_validity validity;\n \tstruct alternate_object_database *alt;\n \n \tif (prepare_packed_git_run_once)\n \t\treturn;\n-\tprepare_packed_git_one(get_object_directory(), 1);\n+\tprepare_packed_git_one(get_object_directory(), 1, &validity);\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\talt->name[-1] = 0;\n-\t\tprepare_packed_git_one(alt->base, 0);\n+\t\tprepare_packed_git_one(alt->base, 0, &alt->pack_validity);\n \t\talt->name[-1] = '/';\n \t}\n \trearrange_packed_git();\n-- \n2.4.1.538.g69ac333\n"},{"id":"261976","messageId":"CACsJy8BjFM_OecoVU9DV3GmJafatSR2yPt6Xb6dETEpYjc1ODA@mail.gmail.com","threadId":"39395","inReplyTo":"20150522235116.GA4300@peff.net","subject":"Re: [PATCH/RFC 0/3] using stat() to avoid re-scanning pack dir","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-23T01:19:03Z","receivedAt":"2015-05-23T01:19:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 23, 2015 at 6:51 AM, Jeff King <peff@peff.net> wrote:\n> The other problem is that I'm not sure stat data is enough to notice\n> when a directory changes. Certainly the mtime should change, but if you\n> have only one-second resolution on your mtimes, we can be fooled.\n\nmtime may or may not change. I based my untracked-cache series\nentirely on this directory mtime and did some research about it. For\nUNIXy filesystems on UNIXy OSes, mtime should work as you expect. FAT\non Windows does not (but FAT on Linux does..). NTFS works fine\naccording to some MS document. No idea about CIFS. But people often\njust do open operation of a time and this racy is not an issue. It is\nonly an issue on the busy server side, and you can make sure you run\non the right fs+OS.\n-- \nDuy\n"},{"id":"261977","messageId":"CACsJy8CY-_b6NzQrd==vrVZnTx-r7jYu4dtep8wEcgGRX79_Zg@mail.gmail.com","threadId":"39395","inReplyTo":"CACsJy8BjFM_OecoVU9DV3GmJafatSR2yPt6Xb6dETEpYjc1ODA@mail.gmail.com","subject":"Re: [PATCH/RFC 0/3] using stat() to avoid re-scanning pack dir","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-23T01:21:20Z","receivedAt":"2015-05-23T01:21:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 23, 2015 at 8:19 AM, Duy Nguyen <pclouds@gmail.com> wrote:\n> But people often just do open operation of a time and this racy is not an issue.\n\nVery bad proof reading. This should read \"But people often do one\noperation at a time..\"\n-- \nDuy\n"},{"id":"261987","messageId":"55605DB6.6040909@alum.mit.edu","threadId":"39395","inReplyTo":"20150522235143.GA4818@peff.net","subject":"Re: [PATCH 1/3] stat_validity: handle non-regular files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-23T11:00:06Z","receivedAt":"2015-05-23T11:00:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/23/2015 01:51 AM, Jeff King wrote:\n> The stat_validity code was originally written to avoid\n> re-reading the packed-refs file when it has not changed. It\n> makes sure that the file continues to match S_ISREG() when\n> we check it.\n> \n> However, we can use the same concept on a directory to see\n> whether it has been modified. Even though we still have to\n> touch the filesystem to do the stat, this can provide a\n> speedup even over opendir/readdir/closedir, especially on\n> high-latency filesystems like NFS.\n> \n> This patch adds a \"mode\" field to stat_validity, which lets\n> us check that the mode has stayed the same (rather than\n> explicitly checking that it is a regular file).\n\nI don't have any insight about whether mtimes are reliable change\nindicators for directories.\n\nBut if you make this change, you are changing the contract of the\nstat_validity functions:\n\n* Have you verified that no callers rely on the old stat_validity's\ncheck that the file is a regular file?\n\n* The docstrings in cache.h need to be updated.\n\n> As a bonus cleanup, we can stop allocating the embedded\n> \"stat_data\" on the heap. Prior to this patch, we needed to\n> represent the case where the file did not exist, and we used\n> \"sv->sd == NULL\" for that. Now we can simply check that\n> \"sv->mode\" is 0.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  cache.h      |  3 ++-\n>  read-cache.c | 16 ++++++----------\n>  2 files changed, 8 insertions(+), 11 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index c97d807..2941e7e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1660,7 +1660,8 @@ int sane_execvp(const char *file, char *const argv[]);\n>   * for the index.\n>   */\n>  struct stat_validity {\n> -\tstruct stat_data *sd;\n> +\tstruct stat_data sd;\n> +\tunsigned mode;\n>  };\n\nCould the mode be stored directly in stat_data?\n\nBy comparing modes, you are not only comparing file type (S_ISREG vs\nS_ISDIR etc.) but also all of the file permissions. That seems OK with\nme but you might want to document that fact. Plus, I don't know whether\nPOSIX allows additional, implementation-defined information to be stored\nin st_mode. If so, you would be comparing that, too.\n\n> [...]\n\nOtherwise, looks OK.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"262012","messageId":"20150524082016.GA8718@peff.net","threadId":"39395","inReplyTo":"CACsJy8BjFM_OecoVU9DV3GmJafatSR2yPt6Xb6dETEpYjc1ODA@mail.gmail.com","subject":"Re: [PATCH/RFC 0/3] using stat() to avoid re-scanning pack dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-24T08:20:16Z","receivedAt":"2015-05-24T08:20:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 23, 2015 at 08:19:03AM +0700, Duy Nguyen wrote:\n\n> On Sat, May 23, 2015 at 6:51 AM, Jeff King <peff@peff.net> wrote:\n> > The other problem is that I'm not sure stat data is enough to notice\n> > when a directory changes. Certainly the mtime should change, but if you\n> > have only one-second resolution on your mtimes, we can be fooled.\n> \n> mtime may or may not change. I based my untracked-cache series\n> entirely on this directory mtime and did some research about it. For\n> UNIXy filesystems on UNIXy OSes, mtime should work as you expect. FAT\n> on Windows does not (but FAT on Linux does..). NTFS works fine\n> according to some MS document. No idea about CIFS. But people often\n> just do open operation of a time and this racy is not an issue. It is\n> only an issue on the busy server side, and you can make sure you run\n> on the right fs+OS.\n\nEven on Linux+ext4, I think there is some raciness.\n\nFor instance, the program at the end of this mail will loop forever,\nrunning \"stat\" on an open directory fd, then enumerating the entries in\nthe directory.  If we ever get the same stat data as the last iteration\nbut different contents, then it complains. If you run it alongside\nsomething simple, like touching 10,000 files in the directory, it fails\npretty quickly.\n\nThis is because we have no atomicity guarantees with directories. We can\nstat() them, and then while we are reading them, somebody else can be\nchanging the entries. Whether we see the \"before\" or \"after\" state\ndepends on the timing.\n\nI'm not 100% sure this translates into real-world problems for\npackfiles. If you notice that an object is missing and re-scan the pack\ndirectory (stat-ing it during the re-scan), then the change that made\nthe object go missing must have happened _before_ the stat, and would\npresumably be reflected in it (modulo mtime resolution issues). But I\nhaven't thought that hard about it yet, and I have a nagging feeling\nthat there will be problem cases.\n\nIt might be that you could get an \"atomic\" read of the directory by\ndoing a stat before and after and making sure they're the same (and if\nnot, repeating the readdir() calls). But I think that suffers from the\nsame mtime-resolution problems.\n\nLinux+ext4 _does_ have nanosecond mtimes, which perhaps is enough to\nassume that any change will be reflected.\n\nI dunno. I guess the most interesting test would be something closer to\nthe real world: one process repeatedly making sure the object pointed to\nby \"master\" exists, and another one constantly rewriting \"master\" and\nthen repacking the object.\n\n-- >8 --\n#include \"cache.h\"\n#include \"string-list.h\"\n\nstatic void get_data(const char *path, struct string_list *entries,\n\t\t     struct stat_validity *validity)\n{\n\tDIR *dir = opendir(path);\n\tstruct dirent *de;\n\n\tstat_validity_update(validity, dirfd(dir));\n\twhile ((de = readdir(dir)))\n\t\tstring_list_insert(entries, de->d_name);\n\tclosedir(dir);\n}\n\nstatic int list_eq(const struct string_list *a,\n\t\t    const struct string_list *b)\n{\n\tint i;\n\n\tif (a->nr != b->nr)\n\t\treturn 0;\n\tfor (i = 0; i < a->nr; i++)\n\t\tif (strcmp(a->items[i].string, b->items[i].string))\n\t\t\treturn 0;\n\treturn 1;\n}\n\nstatic void monitor(const char *path)\n{\n\tstruct string_list last_entries = STRING_LIST_INIT_DUP;\n\tstruct stat_validity last_validity;\n\n\tget_data(path, &last_entries, &last_validity);\n\twhile (1) {\n\t\tstruct string_list cur_entries = STRING_LIST_INIT_DUP;\n\t\tstruct stat_validity cur_validity;\n\n\t\tget_data(path, &cur_entries, &cur_validity);\n\t\tif (!memcmp(&last_validity, &cur_validity, sizeof(last_validity)) &&\n\t\t    !list_eq(&cur_entries, &last_entries))\n\t\t\tdie(\"mismatch\");\n\n\t\tstring_list_clear(&last_entries, 0);\n\t\tmemcpy(&last_entries, &cur_entries, sizeof(last_entries));\n\t\tstat_validity_clear(&last_validity);\n\t\tmemcpy(&last_validity, &cur_validity, sizeof(last_validity));\n\t}\n}\n\nint main(int argc, const char **argv)\n{\n\tmonitor(*++argv);\n\treturn 0;\n}\n"},{"id":"262013","messageId":"20150524082935.GB8718@peff.net","threadId":"39395","inReplyTo":"55605DB6.6040909@alum.mit.edu","subject":"Re: [PATCH 1/3] stat_validity: handle non-regular files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-24T08:29:35Z","receivedAt":"2015-05-24T08:29:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 23, 2015 at 01:00:06PM +0200, Michael Haggerty wrote:\n\n> I don't have any insight about whether mtimes are reliable change\n> indicators for directories.\n> \n> But if you make this change, you are changing the contract of the\n> stat_validity functions:\n> \n> * Have you verified that no callers rely on the old stat_validity's\n> check that the file is a regular file?\n\nI think they are OK if a directory appears (they notice the error in\nreading and call stat_validity_clear to throw away the result). But it\nwould be a problem, I suppose, if you put a FIFO or something into\n$GIT_DIR/packed-refs. OTOH, that would suffer from the stat data\nchanging constantly, so we would fall back to safe behavior.\n\nI don't know how careful we want to be. I guess the conservative choice\nwould be to barf if it's not a file or directory, both of which can be\nhandled pretty sanely.\n\n> * The docstrings in cache.h need to be updated.\n\nThanks, will fix if I re-roll (I'm still not convinced this is a robust\nthing to be doing overall).\n\n> >  struct stat_validity {\n> > -\tstruct stat_data *sd;\n> > +\tstruct stat_data sd;\n> > +\tunsigned mode;\n> >  };\n> \n> Could the mode be stored directly in stat_data?\n\nI'd rather not. stat_data is shared with the cache_entry code, which\nholds the mode separately. I'd like to avoid impacting the index code at\nall.\n\n> By comparing modes, you are not only comparing file type (S_ISREG vs\n> S_ISDIR etc.) but also all of the file permissions. That seems OK with\n> me but you might want to document that fact. Plus, I don't know whether\n> POSIX allows additional, implementation-defined information to be stored\n> in st_mode. If so, you would be comparing that, too.\n\nYeah, I considered that. My feeling is that testing _more_ information\nis probably OK. Even if there is extra data there, it presumably doesn't\nchange from call to call of stat() unless the directory changes. And if\nwe are wrong, we fail safely (e.g., if the permissions change we don't\ntechnically need to re-read the pack directory, but it is OK to do so).\n\n-Peff\n"},{"id":"262014","messageId":"CACsJy8Bb1O7QZtiPWdzYwYgOdV0dLDgD3Xu_YaWNUbsuTqJB5g@mail.gmail.com","threadId":"39395","inReplyTo":"20150522071224.GA10734@peff.net","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-05-24T09:00:36Z","receivedAt":"2015-05-24T09:00:36Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 22, 2015 at 2:12 PM, Jeff King <peff@peff.net> wrote:\n> So I think there are two possibilities for improving this:\n>\n>   1. Find places where we expect the object will not exist (like the\n>      collision_test check you pointed out) and use a\n>      \"has_sha1_file_fast\" that accepts that it may very occasionally\n>      erroneously return false. In this case it would mean potentially\n>      skipping a collision check, but I think that is OK. That could have\n>      security implications, but only if an attacker:\n>\n>        a. has broken sha1 to generate a colliding object\n>\n>        b. can manipulate the victim into repacking in a loop\n>\n>        c. can manipulate the victim into fetching (or receiving a push)\n>           simultaneously with (b)\n>\n>      at which point they can try to race the repack procedure to add\n>      their colliding object to the repository. It seems rather unlikely\n>      (especially part a).\n\nIn case you want to back away from option 2 because it starts to leak\nraciness, which your old commit tried to fix in the first place. I\nthink the only other place that tests for lots of non-existent loose\nobjects is write_sha1_file (e.g. \"tar -xf bigtarball.tar.gz; cd\nbigtarball; git init; git add .\"). But the number of calls should be\nmuch smaller compared to index-pack and it does not use has_sha1_file,\nit uses check_and_freshen_file() instead.\n\nThere are other places where has_sha1_file() may return 0, but I think\nthe number of calls is even smaller to bother (shallow.c,\nfetch-pack.c, apply.c, buik-checkin.c)\n-- \nDuy\n"},{"id":"263007","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C55197764FE@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":"CACsJy8Bb1O7QZtiPWdzYwYgOdV0dLDgD3Xu_YaWNUbsuTqJB5g@mail.gmail.com","subject":"RE: Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-06-05T12:01:16Z","receivedAt":"2015-06-05T12:01:16Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"On Sunday, May 24, 2015 @ 10:01 AM Duy Nguyen [mailto:pclouds@gmail.com] did scribble:\n\n> In case you want to back away from option 2 because it starts to leak\n> raciness, which your old commit tried to fix in the first place. I\n> think the only other place that tests for lots of non-existent loose\n> objects is write_sha1_file (e.g. \"tar -xf bigtarball.tar.gz; cd\n> bigtarball; git init; git add .\"). But the number of calls should be\n> much smaller compared to index-pack and it does not use has_sha1_file,\n> it uses check_and_freshen_file() instead.\n> \n> There are other places where has_sha1_file() may return 0, but I think\n> the number of calls is even smaller to bother (shallow.c,\n> fetch-pack.c, apply.c, buik-checkin.c)\n\nAny updates / further thoughts on this?\n\nThanks,\n\nSteve\n"},{"id":"263008","messageId":"20150605121817.GA22125@peff.net","threadId":"39395","inReplyTo":"7FAE15F0A93C0144AD8B5FBD584E1C55197764FE@C111KXTEMBX51.ERF.thomson.com","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-05T12:18:17Z","receivedAt":"2015-06-05T12:18:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 05, 2015 at 12:01:16PM +0000, steve.norman@thomsonreuters.com wrote:\n\n> On Sunday, May 24, 2015 @ 10:01 AM Duy Nguyen [mailto:pclouds@gmail.com] did scribble:\n> \n> > In case you want to back away from option 2 because it starts to leak\n> > raciness, which your old commit tried to fix in the first place. I\n> > think the only other place that tests for lots of non-existent loose\n> > objects is write_sha1_file (e.g. \"tar -xf bigtarball.tar.gz; cd\n> > bigtarball; git init; git add .\"). But the number of calls should be\n> > much smaller compared to index-pack and it does not use has_sha1_file,\n> > it uses check_and_freshen_file() instead.\n> > \n> > There are other places where has_sha1_file() may return 0, but I think\n> > the number of calls is even smaller to bother (shallow.c,\n> > fetch-pack.c, apply.c, buik-checkin.c)\n> \n> Any updates / further thoughts on this?\n\nSorry, I haven't had a chance to look at it further. It still on my todo\nlist. My plan is:\n\n  1. Devise some torture to tests to see whether my patch series is in\n     fact racy on Linux.\n\n  2. Assuming it is, scrap it and make a has_sha1_file_quick() which\n     might sometimes return a false negative (if somebody else is\n     repacking). Use that in index-pack (and possibly other places, but\n     we can start with index-pack).\n\nIf we skip step 1 out of pessimism (which I think is a reasonable thing\nto do), then step 2 should not be all that much work. I'm going to be\noffline for a few days, though, so I won't get to it until next week at\nthe earliest. If you (or someone else) wants to take a stab at it,\nplease feel free.\n\n-Peff\n"},{"id":"263009","messageId":"20150605122921.GA7586@peff.net","threadId":"39395","inReplyTo":"20150605121817.GA22125@peff.net","subject":"[PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-05T12:29:21Z","receivedAt":"2015-06-05T12:29:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 05, 2015 at 08:18:17AM -0400, Jeff King wrote:\n\n>   1. Devise some torture to tests to see whether my patch series is in\n>      fact racy on Linux.\n>\n>   2. Assuming it is, scrap it and make a has_sha1_file_quick() which\n>      might sometimes return a false negative (if somebody else is\n>      repacking). Use that in index-pack (and possibly other places, but\n>      we can start with index-pack).\n> \n> If we skip step 1 out of pessimism (which I think is a reasonable thing\n> to do), then step 2 should not be all that much work. I'm going to be\n> offline for a few days, though, so I won't get to it until next week at\n> the earliest. If you (or someone else) wants to take a stab at it,\n> please feel free.\n\nActually, it really is a tiny bit of work. I knocked this out in less\nthan 10 minutes. It passes the test suite, but it's entirely possible\nthat I did something boneheaded that does not fix your problem. ;)\n\nPlease test and confirm that it makes things better on your system.\n\n-- >8 --\nSubject: [PATCH] index-pack: avoid excessive re-reading of pack directory\n\nSince 45e8a74 (has_sha1_file: re-check pack directory before\ngiving up, 2013-08-30), we spend extra effort for\nhas_sha1_file to give the right answer when somebody else is\nrepacking. Usually this effort does not matter, because\nafter finding that the object does not exist, the next step\nis usually to die().\n\nHowever, some code paths make a large number of\nhas_sha1_file checks which are _not_ expected to return 1.\nThe collision test in index-pack.c is such a case. On a\nlocal system, this can cause a performance slowdown of\naround 5%. But on a system with high-latency system calls\n(like NFS), it can be much worse.\n\nThis patch introduces a \"quick\" flag to has_sha1_file which\ncallers can use when they would prefer high performance at\nthe cost of false negatives during repacks. There may be\nother code paths that can use this, but the index-pack one\nis the most obviously critical, so we'll start with\nswitching that one.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c |  2 +-\n cache.h              | 11 ++++++++++-\n sha1_file.c          |  4 +++-\n 3 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 7ea2020..0761dfd 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -784,7 +784,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \tassert(data || obj_entry);\n \n \tread_lock();\n-\tcollision_test_needed = has_sha1_file(sha1);\n+\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n \tread_unlock();\n \n \tif (collision_test_needed && !data) {\ndiff --git a/cache.h b/cache.h\nindex 571c98f..98edc2c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -943,8 +943,17 @@ extern int has_sha1_pack(const unsigned char *sha1);\n  * Return true iff we have an object named sha1, whether local or in\n  * an alternate object database, and whether packed or loose.  This\n  * function does not respect replace references.\n+ *\n+ * If the QUICK flag is set, do not re-check the pack directory\n+ * when we cannot find the object (this means we may give a false\n+ * negative answer if another process is simultaneously repacking).\n  */\n-extern int has_sha1_file(const unsigned char *sha1);\n+#define HAS_SHA1_QUICK 0x1\n+extern int has_sha1_file_with_flags(const unsigned char *sha1, int flags);\n+static inline int has_sha1_file(const unsigned char *sha1)\n+{\n+\treturn has_sha1_file_with_flags(sha1, 0);\n+}\n \n /*\n  * Return true iff an alternate object database has a loose object\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7e38148..dc8746b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3161,7 +3161,7 @@ int has_sha1_pack(const unsigned char *sha1)\n \treturn find_pack_entry(sha1, &e);\n }\n \n-int has_sha1_file(const unsigned char *sha1)\n+int has_sha1_file_with_flags(const unsigned char *sha1, int flags)\n {\n \tstruct pack_entry e;\n \n@@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n \t\treturn 1;\n \tif (has_loose_object(sha1))\n \t\treturn 1;\n+\tif (flags & HAS_SHA1_QUICK)\n+\t\treturn 0;\n \treprepare_packed_git();\n \treturn find_pack_entry(sha1, &e);\n }\n-- \n2.4.2.752.geeb594a\n"},{"id":"263018","messageId":"7FAE15F0A93C0144AD8B5FBD584E1C5519776893@C111KXTEMBX51.ERF.thomson.com","threadId":"39395","inReplyTo":"20150605121817.GA22125@peff.net","subject":"RE: Troubleshoot clone issue to NFS.","fromName":"","fromEmail":"steve.norman@thomsonreuters.com","sentAt":"2015-06-05T14:20:54Z","receivedAt":"2015-06-05T14:20:54Z","isPatch":false,"sender":{"key":"steve.norman@thomsonreuters.com","avatar":null},"body":"On Friday, June 05, 2015 @ 1:18 PM Jeff King [mailto:peff@peff.net] did scribble:\n\n> Sorry, I haven't had a chance to look at it further. It still on my todo\n> list. My plan is:\n> \n>   1. Devise some torture to tests to see whether my patch series is in\n>      fact racy on Linux.\n> \n>   2. Assuming it is, scrap it and make a has_sha1_file_quick() which\n>      might sometimes return a false negative (if somebody else is\n>      repacking). Use that in index-pack (and possibly other places, but\n>      we can start with index-pack).\n> \n> If we skip step 1 out of pessimism (which I think is a reasonable thing\n> to do), then step 2 should not be all that much work. I'm going to be\n> offline for a few days, though, so I won't get to it until next week at\n> the earliest. If you (or someone else) wants to take a stab at it,\n> please feel free.\n\nI've been off myself and wanted to make sure I hadn't missed anything in \nthe email threads while I was away as there we rather a lot of them.\n\nNot feeling confident enough to make the changes myself at the moment. \nI think what you are saying is that has_sha1_file_quick() would be the version\nfrom before your change in 45e8a74.  And then use that, but I could be\nbarking up the wrong tree completely.\n\nThanks,\n\nSteve\n"},{"id":"263381","messageId":"20150609172436.GA32034@peff.net","threadId":"39395","inReplyTo":"20150605122921.GA7586@peff.net","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-09T17:24:37Z","receivedAt":"2015-06-09T17:24:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 05, 2015 at 08:29:21AM -0400, Jeff King wrote:\n\n> On Fri, Jun 05, 2015 at 08:18:17AM -0400, Jeff King wrote:\n> \n> >   1. Devise some torture to tests to see whether my patch series is in\n> >      fact racy on Linux.\n> >\n> >   2. Assuming it is, scrap it and make a has_sha1_file_quick() which\n> >      might sometimes return a false negative (if somebody else is\n> >      repacking). Use that in index-pack (and possibly other places, but\n> >      we can start with index-pack).\n> > \n> > If we skip step 1 out of pessimism (which I think is a reasonable thing\n> > to do), then step 2 should not be all that much work. I'm going to be\n> > offline for a few days, though, so I won't get to it until next week at\n> > the earliest. If you (or someone else) wants to take a stab at it,\n> > please feel free.\n> \n> Actually, it really is a tiny bit of work. I knocked this out in less\n> than 10 minutes. It passes the test suite, but it's entirely possible\n> that I did something boneheaded that does not fix your problem. ;)\n> \n> Please test and confirm that it makes things better on your system.\n\nI tested this on my system, and confirmed that for a \"git clone\n--no-local --bare git.git\":\n\n  1. It cuts the number of openat/getdents/close syscalls by several\n     orders of magnitude.\n\n  2. The overall time drops from ~11.4s to ~10.5s. I suppose if I timed\n     only the `index-pack` process, it would be even higher (as a\n     percentage improvement).\n\nSo I expect it should fix the problem on your NFS system.\n\nHere's the patch again. Same as before, but I'm cc-ing Junio, as I think\nit is good to apply.\n\n-- >8 --\nSubject: index-pack: avoid excessive re-reading of pack directory\n\nSince 45e8a74 (has_sha1_file: re-check pack directory before\ngiving up, 2013-08-30), we spend extra effort for\nhas_sha1_file to give the right answer when somebody else is\nrepacking. Usually this effort does not matter, because\nafter finding that the object does not exist, the next step\nis usually to die().\n\nHowever, some code paths make a large number of\nhas_sha1_file checks which are _not_ expected to return 1.\nThe collision test in index-pack.c is such a case. On a\nlocal system, this can cause a performance slowdown of\naround 5%. But on a system with high-latency system calls\n(like NFS), it can be much worse.\n\nThis patch introduces a \"quick\" flag to has_sha1_file which\ncallers can use when they would prefer high performance at\nthe cost of false negatives during repacks. There may be\nother code paths that can use this, but the index-pack one\nis the most obviously critical, so we'll start with\nswitching that one.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c |  2 +-\n cache.h              | 11 ++++++++++-\n sha1_file.c          |  4 +++-\n 3 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 7ea2020..0761dfd 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -784,7 +784,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \tassert(data || obj_entry);\n \n \tread_lock();\n-\tcollision_test_needed = has_sha1_file(sha1);\n+\tcollision_test_needed = has_sha1_file_with_flags(sha1, HAS_SHA1_QUICK);\n \tread_unlock();\n \n \tif (collision_test_needed && !data) {\ndiff --git a/cache.h b/cache.h\nindex 571c98f..98edc2c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -943,8 +943,17 @@ extern int has_sha1_pack(const unsigned char *sha1);\n  * Return true iff we have an object named sha1, whether local or in\n  * an alternate object database, and whether packed or loose.  This\n  * function does not respect replace references.\n+ *\n+ * If the QUICK flag is set, do not re-check the pack directory\n+ * when we cannot find the object (this means we may give a false\n+ * negative answer if another process is simultaneously repacking).\n  */\n-extern int has_sha1_file(const unsigned char *sha1);\n+#define HAS_SHA1_QUICK 0x1\n+extern int has_sha1_file_with_flags(const unsigned char *sha1, int flags);\n+static inline int has_sha1_file(const unsigned char *sha1)\n+{\n+\treturn has_sha1_file_with_flags(sha1, 0);\n+}\n \n /*\n  * Return true iff an alternate object database has a loose object\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7e38148..dc8746b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3161,7 +3161,7 @@ int has_sha1_pack(const unsigned char *sha1)\n \treturn find_pack_entry(sha1, &e);\n }\n \n-int has_sha1_file(const unsigned char *sha1)\n+int has_sha1_file_with_flags(const unsigned char *sha1, int flags)\n {\n \tstruct pack_entry e;\n \n@@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n \t\treturn 1;\n \tif (has_loose_object(sha1))\n \t\treturn 1;\n+\tif (flags & HAS_SHA1_QUICK)\n+\t\treturn 0;\n \treprepare_packed_git();\n \treturn find_pack_entry(sha1, &e);\n }\n-- \n2.4.2.752.geeb594a\n"},{"id":"263383","messageId":"20150609174106.GB32034@peff.net","threadId":"39395","inReplyTo":"20150609172436.GA32034@peff.net","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-09T17:41:06Z","receivedAt":"2015-06-09T17:41:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 09, 2015 at 01:24:36PM -0400, Jeff King wrote:\n\n> I tested this on my system, and confirmed that for a \"git clone\n> --no-local --bare git.git\":\n> \n>   1. It cuts the number of openat/getdents/close syscalls by several\n>      orders of magnitude.\n> \n>   2. The overall time drops from ~11.4s to ~10.5s. I suppose if I timed\n>      only the `index-pack` process, it would be even higher (as a\n>      percentage improvement).\n\nJust for fun, I did a \"git pack-objects --all --stdout\" from linux.git,\nand then timed \"git index-pack --stdin\" on it in an empty repo. With a\nconfigured alternate pointing to another empty repo, just to make it\nmore unfair. And then I stored it all on a ramdisk to emphasize the cost\nof the syscalls versus hitting the disk. The numbers I got were:\n\n  [before]\n  real    2m13.093s\n  user    3m31.884s\n  sys     0m55.208s\n\n  [after]\n  real    1m40.389s\n  user    3m10.776s\n  sys     0m26.012s\n\nThat's sort of a ridiculous test, but it does show that this was having\nsome impact even on \"normal\" systems without insane syscall latencies.\n\n-Peff\n"},{"id":"263431","messageId":"CAJo=hJv6O66yFC_O_4aeL2JxBOtk2f+k1wGt3VW7dk71Q1ov3A@mail.gmail.com","threadId":"39395","inReplyTo":"20150605122921.GA7586@peff.net","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-06-10T03:46:24Z","receivedAt":"2015-06-10T03:46:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Jun 5, 2015 at 5:29 AM, Jeff King <peff@peff.net> wrote:\n>\n> However, some code paths make a large number of\n> has_sha1_file checks which are _not_ expected to return 1.\n> The collision test in index-pack.c is such a case. On a\n> local system, this can cause a performance slowdown of\n> around 5%. But on a system with high-latency system calls\n> (like NFS), it can be much worse.\n>\n> This patch introduces a \"quick\" flag to has_sha1_file which\n> callers can use when they would prefer high performance at\n> the cost of false negatives during repacks. There may be\n> other code paths that can use this, but the index-pack one\n> is the most obviously critical, so we'll start with\n> switching that one.\n\nHilarious. We did this in JGit back in ... uhm before 2009. :)\n\nBut its Java. So of course we had to do optimizations.\n\n\n> @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n>                 return 1;\n>         if (has_loose_object(sha1))\n>                 return 1;\n> +       if (flags & HAS_SHA1_QUICK)\n> +               return 0;\n>         reprepare_packed_git();\n>         return find_pack_entry(sha1, &e);\n\nSomething else we do is readdir() over the loose objects and store\nthem in a map in memory. That way we avoid stat() calls during that\nhas_loose_object() path. This is apparently a win enough of the time\nthat we always do that when receiving a pack over the wire (client or\nserver).\n"},{"id":"263465","messageId":"20150610140024.GA9395@peff.net","threadId":"39395","inReplyTo":"CAJo=hJv6O66yFC_O_4aeL2JxBOtk2f+k1wGt3VW7dk71Q1ov3A@mail.gmail.com","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-10T14:00:26Z","receivedAt":"2015-06-10T14:00:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:\n\n> > This patch introduces a \"quick\" flag to has_sha1_file which\n> > callers can use when they would prefer high performance at\n> > the cost of false negatives during repacks. There may be\n> > other code paths that can use this, but the index-pack one\n> > is the most obviously critical, so we'll start with\n> > switching that one.\n> \n> Hilarious. We did this in JGit back in ... uhm before 2009. :)\n> \n> But its Java. So of course we had to do optimizations.\n\nThis is actually how Git did it up until v1.8.4.2, in 2013. I changed it\nthen because the old way was racy (and git could flakily report refs as\nbroken and skip them during repacks!).\n\nIf you are doing it the \"quick\" way everywhere in JGit, you may want to\nreexamine the possibility for races. :)\n\n> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n> >                 return 1;\n> >         if (has_loose_object(sha1))\n> >                 return 1;\n> > +       if (flags & HAS_SHA1_QUICK)\n> > +               return 0;\n> >         reprepare_packed_git();\n> >         return find_pack_entry(sha1, &e);\n> \n> Something else we do is readdir() over the loose objects and store\n> them in a map in memory. That way we avoid stat() calls during that\n> has_loose_object() path. This is apparently a win enough of the time\n> that we always do that when receiving a pack over the wire (client or\n> server).\n\nYeah, I thought about that while writing this. It would be a win as long\nas you have a small number of loose objects and were going to make a\nlarge number of requests (otherwise you are traversing even though\nnobody is going to look it up). According to perf, though, loose object\nlookups are not a large expenditure[1].\n\nI'm also hesitant to go that route because it's basically caching, which\nintroduces new opportunities for race conditions when the cache is stale\n(we do the same thing with loose refs, and we have indeed run into races\nthere).\n\n-Peff\n\n[1] As measured mostly by __d_lookup_rcu calls. Of course, my patch\n    gives a 5% improvement over the original, and we were not spending\n    5% of the time there originally. I suspect part of the problem is\n    that we do the lookup under a lock, so the longer we spend there,\n    the more contention we have between threads, and the less\n    parallelism. Indeed, I just did a quick repeat of my tests with\n    pack.threads=1, and the size of the improvement shrinks.\n"},{"id":"263466","messageId":"CACsJy8AfyYFcDdXN8jyRedhAx9f7c2n9Mmxs2W_FzfYd-f=AEg@mail.gmail.com","threadId":"39395","inReplyTo":"20150610140024.GA9395@peff.net","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-10T14:36:23Z","receivedAt":"2015-06-10T14:36:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 10, 2015 at 9:00 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:\n>\n>> > This patch introduces a \"quick\" flag to has_sha1_file which\n>> > callers can use when they would prefer high performance at\n>> > the cost of false negatives during repacks. There may be\n>> > other code paths that can use this, but the index-pack one\n>> > is the most obviously critical, so we'll start with\n>> > switching that one.\n>>\n>> Hilarious. We did this in JGit back in ... uhm before 2009. :)\n>>\n>> But its Java. So of course we had to do optimizations.\n>\n> This is actually how Git did it up until v1.8.4.2, in 2013. I changed it\n> then because the old way was racy (and git could flakily report refs as\n> broken and skip them during repacks!).\n>\n> If you are doing it the \"quick\" way everywhere in JGit, you may want to\n> reexamine the possibility for races. :)\n\nI was expecting this :D\n\n>> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n>> >                 return 1;\n>> >         if (has_loose_object(sha1))\n>> >                 return 1;\n>> > +       if (flags & HAS_SHA1_QUICK)\n>> > +               return 0;\n>> >         reprepare_packed_git();\n>> >         return find_pack_entry(sha1, &e);\n>>\n>> Something else we do is readdir() over the loose objects and store\n>> them in a map in memory. That way we avoid stat() calls during that\n>> has_loose_object() path. This is apparently a win enough of the time\n>> that we always do that when receiving a pack over the wire (client or\n>> server).\n>\n> Yeah, I thought about that while writing this. It would be a win as long\n> as you have a small number of loose objects and were going to make a\n> large number of requests (otherwise you are traversing even though\n> nobody is going to look it up). According to perf, though, loose object\n> lookups are not a large expenditure[1].\n>\n> I'm also hesitant to go that route because it's basically caching, which\n> introduces new opportunities for race conditions when the cache is stale\n> (we do the same thing with loose refs, and we have indeed run into races\n> there).\n\nWatchman may help avoid races _with_ caching. But we need to support\nboth ways in that case, falling back to normal file poking when\nwatchman gives up, or when we're on Windows. Extra work needs big\nenough performance gain to justify. I haven't seen that gain yet.\n\n> [1] As measured mostly by __d_lookup_rcu calls. Of course, my patch\n>     gives a 5% improvement over the original, and we were not spending\n>     5% of the time there originally. I suspect part of the problem is\n>     that we do the lookup under a lock, so the longer we spend there,\n>     the more contention we have between threads, and the less\n>     parallelism. Indeed, I just did a quick repeat of my tests with\n>     pack.threads=1, and the size of the improvement shrinks.\n-- \nDuy\n"},{"id":"263536","messageId":"CAJo=hJs51hG15MSdB-kdDy0NKn=50cbDpVS6o+VRx-6M-C0twA@mail.gmail.com","threadId":"39395","inReplyTo":"20150610140024.GA9395@peff.net","subject":"Re: [PATCH] index-pack: avoid excessive re-reading of pack directory","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2015-06-10T21:34:37Z","receivedAt":"2015-06-10T21:34:37Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Jun 10, 2015 at 7:00 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote:\n>\n>> > This patch introduces a \"quick\" flag to has_sha1_file which\n>> > callers can use when they would prefer high performance at\n>> > the cost of false negatives during repacks. There may be\n>> > other code paths that can use this, but the index-pack one\n>> > is the most obviously critical, so we'll start with\n>> > switching that one.\n>>\n>> Hilarious. We did this in JGit back in ... uhm before 2009. :)\n>>\n>> But its Java. So of course we had to do optimizations.\n>\n> This is actually how Git did it up until v1.8.4.2, in 2013. I changed it\n> then because the old way was racy (and git could flakily report refs as\n> broken and skip them during repacks!).\n>\n> If you are doing it the \"quick\" way everywhere in JGit, you may want to\n> reexamine the possibility for races. :)\n\nCorrect, fortunately we are not this naive.\n\nJGit always does the reprepare_packed_git() + retry search on a miss.\n\nBut we have a code path to bypass that inside critical loops like our\nequivalent of index-pack pulling off the wire. We snapshot the object\ntree at the start of the operation before we read in the pack header,\nand then require that the incoming pack be completed with that\nsnapshot. Since the snapshot was taking after ref\nnegotiation/advertisement, we should be at least as current as the\nrefs that were exchanged on the wire.\n\n>> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1)\n>> >                 return 1;\n>> >         if (has_loose_object(sha1))\n>> >                 return 1;\n>> > +       if (flags & HAS_SHA1_QUICK)\n>> > +               return 0;\n>> >         reprepare_packed_git();\n>> >         return find_pack_entry(sha1, &e);\n>>\n>> Something else we do is readdir() over the loose objects and store\n>> them in a map in memory. That way we avoid stat() calls during that\n>> has_loose_object() path. This is apparently a win enough of the time\n>> that we always do that when receiving a pack over the wire (client or\n>> server).\n>\n> Yeah, I thought about that while writing this. It would be a win as long\n> as you have a small number of loose objects and were going to make a\n> large number of requests (otherwise you are traversing even though\n> nobody is going to look it up). According to perf, though, loose object\n> lookups are not a large expenditure[1].\n\nInteresting. We were getting hurt by this in JGit. For most\nrepositories it was cheaper to issue 256 readdir() and build a set of\nSHA-1s we found. I think we even just hit every directory 00..ff, we\ndon't even bother with a readdir() on the $GIT_DIR/objects itself.\n\n> I'm also hesitant to go that route because it's basically caching, which\n> introduces new opportunities for race conditions when the cache is stale\n> (we do the same thing with loose refs, and we have indeed run into races\n> there).\n\nYes. But see above, we do this only after we snapshot the packs, and\nonly after the ref negotiation, and only for the duration of parsing\nthe pack off the wire. So we should never have a data race.\n\nSince JGit is multi-threaded, this cache is also effectively a\nthread-local. Its never shared across threads.\n\n> [1] As measured mostly by __d_lookup_rcu calls. Of course, my patch\n>     gives a 5% improvement over the original, and we were not spending\n>     5% of the time there originally. I suspect part of the problem is\n>     that we do the lookup under a lock, so the longer we spend there,\n>     the more contention we have between threads, and the less\n>     parallelism. Indeed, I just did a quick repeat of my tests with\n>     pack.threads=1, and the size of the improvement shrinks.\n\nInteresting. Yea, fine-grained locking can hurt parallel execution... :(\n"},{"id":"263972","messageId":"20150616205006.GA4864@peff.net","threadId":"39395","inReplyTo":"20150605121817.GA22125@peff.net","subject":"Re: Troubleshoot clone issue to NFS.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-16T20:50:06Z","receivedAt":"2015-06-16T20:50:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 05, 2015 at 08:18:17AM -0400, Jeff King wrote:\n\n> On Fri, Jun 05, 2015 at 12:01:16PM +0000, steve.norman@thomsonreuters.com wrote:\n> \n> > On Sunday, May 24, 2015 @ 10:01 AM Duy Nguyen [mailto:pclouds@gmail.com] did scribble:\n> > \n> > > In case you want to back away from option 2 because it starts to leak\n> > > raciness, which your old commit tried to fix in the first place. I\n> > > think the only other place that tests for lots of non-existent loose\n> > > objects is write_sha1_file (e.g. \"tar -xf bigtarball.tar.gz; cd\n> > > bigtarball; git init; git add .\"). But the number of calls should be\n> > > much smaller compared to index-pack and it does not use has_sha1_file,\n> > > it uses check_and_freshen_file() instead.\n> > > \n> > > There are other places where has_sha1_file() may return 0, but I think\n> > > the number of calls is even smaller to bother (shallow.c,\n> > > fetch-pack.c, apply.c, buik-checkin.c)\n> > \n> > Any updates / further thoughts on this?\n> \n> Sorry, I haven't had a chance to look at it further. It still on my todo\n> list. My plan is:\n> \n>   1. Devise some torture to tests to see whether my patch series is in\n>      fact racy on Linux.\n\nI do not know that we needed further convincing that the patch series to\nstat() the objects/pack directory was flaky, but just as an extra data\npoint in case somebody tries this approach later: it is indeed flaky.\n\nI've been running with it for a week or so, and I noticed that a simple\nclone of a small repository fails racily:\n\n  $ git clone --bare --no-local . foo.git\n  Cloning into bare repository 'child.git'...\n  remote: Counting objects: 210, done.\n  remote: Compressing objects: 100% (98/98), done.\n  remote: Total 210 (delta 99), reused 210 (delta 99)\n  Receiving objects: 100% (210/210), 68.00 KiB | 0 bytes/s, done.\n  Resolving deltas: 100% (99/99), done.\n  Checking connectivity... done.\n  error: internal error: refs/heads/master is not a valid packed reference!\n\nSometimes it works, and sometimes not.  It looks like we're failing to\nre-scan the pack directory when we should (presumably because the open()\nand readdir() operations are not atomic).\n\n-Peff\n"}]}