{"thread":{"id":"16038","subject":"Re* ext3: fix ext3_dx_readdir hash collision handling - Regression","startedAt":"2008-10-24T22:00:24Z","lastAt":"2008-10-24T22:35:03Z","messageCount":3,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"93887","messageId":"7vwsfx1wnb.fsf_-_@gitster.siamese.dyndns.org","threadId":"16038","inReplyTo":"alpine.LFD.2.00.0810240853310.3287@nehalem.linux-foundation.org","subject":"Re* ext3: fix ext3_dx_readdir hash collision handling - Regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-24T22:00:24Z","receivedAt":"2008-10-24T22:00:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> The \"git buglet\" looks liek this:\n>\n> - build a kernel\n>\n> - do \"git clean -dqfx\". This fails with\n>\n> \twarning: failed to remove 'include/config/'\n>\n> - do \"git clean -dqfx\" again. And now it works - apparently because the \n> first invocation deleted enough of the big directory.\n>\n> Doing an 'strace' to see what is going on, I see:\n>\n> \t..\n> \tgetdents(3, /* 132 entries */, 4096) = 3888\n> \tlstat(\"include/config/sgi\", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0\n> \topen(\"include/config/sgi\", O_RDONLY|O_NONBLOCK|O_DIRECTORY|0x80000) = 4\n> \tfstat(4, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0\n> \tgetdents(4, /* 3 entries */, 4096) = 80\n> \tlstat(\"include/config/sgi/partition.h\", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0\n> \tunlink(\"include/config/sgi/partition.h\") = 0\n> \tgetdents(4, /* 0 entries */, 4096) = 0\n> \tclose(4) = 0\n> \trmdir(\"include/config/sgi\") = 0\n> \tlstat(\"include/config/sgi\", 0x7fffdc4d2110) = -1 ENOENT (No such file or directory)\n> \tclose(3) = 0\n> \twrite(2, \"warning: failed to remove \\'include/config/\\'\\n\", 44) = 44\n> \t..\n>\n> and notice how it does that\n>\n> \tlstat(\"include/config/sgi\", ..\n>\n> _twice_. That, in turn (knowing the git implementation), implies that it \n> was returned twice from that first \"getdents(3, ...)\" call.\n>\n> So what git clean does is to scan over the readdir() return values, see if \n> it's a file it knows about, try to remove it if not, lstat() every entry, \n> recurse into them if they are directories, and then remove it. If the \n> lstat() fails, \"git clean\" will fail.\n\nHmm, interesting.\n\n-- >8 --\nSubject: allow readdir(3) to return the same entry twice\n\nWhen removing a large directory recursively, repeatedly running unlink(2)\non what we read from readdir(3) can cause readdir(3) (or underlying\ngetdents(2)) to return the same entry twice. If we have already removed\nit, running lstat() on the entry would fail with ENOENT, but there is no\nharm if we ignore such an error.\n\nSigned-off-by: Junio C Hamano \n---\n\n dir.c | 7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git i/dir.c w/dir.c\nindex 0131983..293df4e 100644\n--- i/dir.c\n+++ w/dir.c\n@@ -800,8 +800,11 @@ int remove_dir_recursively(struct strbuf *path, int only_empty)\n \n \t\tstrbuf_setlen(path, len);\n \t\tstrbuf_addstr(path, e->d_name);\n-\t\tif (lstat(path->buf, &st))\n-\t\t\t; /* fall thru */\n+\t\tif (lstat(path->buf, &st)) {\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\tcontinue;\n+\t\t\t/* otherwise fall thru */\n+\t\t}\n \t\telse if (S_ISDIR(st.st_mode)) {\n \t\t\tif (!remove_dir_recursively(path, only_empty))\n \t\t\t\tcontinue; /* happy */\n"},{"id":"93892","messageId":"alpine.LFD.2.00.0810241524350.3287@nehalem.linux-foundation.org","threadId":"16038","inReplyTo":"7vwsfx1wnb.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Re* ext3: fix ext3_dx_readdir hash collision handling - Regression","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-10-24T22:26:46Z","receivedAt":"2008-10-24T22:26:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 24 Oct 2008, Junio C Hamano wrote:\n>\n> Subject: allow readdir(3) to return the same entry twice\n\nThe thing is, this really is a kernel bug. We have even bisected it (and \nit hasn't hit any released kernel). The original reporter showed it with a \nsimple \"rm -r\".\n\nSo it really isn't a git bug, even though I initially thought it might be, \nbefore I looked closer.\n\nThat said, the git patch may be worth it just because two *concurrent* \ninvocations of \"git clean\" could then cause one (or both) to fail this \nway.\n\n\t\tLinus\n"},{"id":"93894","messageId":"7vk5bx1v1k.fsf@gitster.siamese.dyndns.org","threadId":"16038","inReplyTo":"alpine.LFD.2.00.0810241524350.3287@nehalem.linux-foundation.org","subject":"Re: Re* ext3: fix ext3_dx_readdir hash collision handling - Regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-24T22:35:03Z","receivedAt":"2008-10-24T22:35:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, 24 Oct 2008, Junio C Hamano wrote:\n>>\n>> Subject: allow readdir(3) to return the same entry twice\n>\n> The thing is, this really is a kernel bug. We have even bisected it (and \n> it hasn't hit any released kernel). The original reporter showed it with a \n> simple \"rm -r\".\n>\n> So it really isn't a git bug, even though I initially thought it might be, \n> before I looked closer.\n>\n> That said, the git patch may be worth it just because two *concurrent* \n> invocations of \"git clean\" could then cause one (or both) to fail this \n> way.\n\nYeah, or on perhaps a buggy implementation of readdir(3) on somebody\nelse's system. In either case, I just thought it might be a low impact\nbelt-and-suspenders fix that is worth considering.\n"}]}