{"thread":{"id":"25610","subject":"[PATCH 1/2] Use git_open_noatime when accessing pack data","startedAt":"2010-11-01T22:54:19Z","lastAt":"2010-11-05T17:26:21Z","messageCount":11,"participants":["Shawn O. Pearce","Johannes Sixt","Junio C Hamano","Jonathan Nieder","Kevin Ballard"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"154947","messageId":"1288652061-19614-1-git-send-email-spearce@spearce.org","threadId":"25610","inReplyTo":null,"subject":"[PATCH 0/2] Work around too many file descriptors","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-11-01T22:54:19Z","receivedAt":"2010-11-01T22:54:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This tiny series tries to work around not having enough file\ndescriptors to open every pack in a repository. It is the logical\nconclusion of Johannes Schindelin's commit fd73ccf2 (\"Cope better\nwith a _lot_ of packs\").\n\nThe patch was prepared on top of gitster's recent two changes to\nimprove error handling in read_sha1_file().\n\nShawn O. Pearce (2):\n Use git_open_noatime when accessing pack data\n Work around EMFILE when there are too many pack files\n\n sha1_file.c | 43 ++++++++++++++++++++++++++++---------------\n 1 files changed, 28 insertions(+), 15 deletions(-)\n\n-- \n1.7.3.2.191.g2d0e5\n"},{"id":"154946","messageId":"1288652061-19614-2-git-send-email-spearce@spearce.org","threadId":"25610","inReplyTo":"1288652061-19614-1-git-send-email-spearce@spearce.org","subject":"[PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-11-01T22:54:20Z","receivedAt":"2010-11-01T22:54:20Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This utility function avoids an unnecessary update of the access time\nfor a loose object file. Just as the atime isn't useful on a loose\nobject, its not useful on the pack or the corresonding idx file.\n\nSigned-off-by: Shawn O. Pearce \n---\n sha1_file.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f709ed6..a6c1934 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -35,6 +35,8 @@ static size_t sz_fmt(size_t s) { return s; }\n \n const unsigned char null_sha1[20];\n \n+static int git_open_noatime(const char *name);\n+\n int safe_create_leading_directories(char *path)\n {\n \tchar *pos = path + offset_1st_component(path);\n@@ -298,7 +300,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tsprintf(path, \"%s/%s\", relative_base, alt_file_name);\n-\tfd = open(path, O_RDONLY);\n+\tfd = git_open_noatime(path);\n \tif (fd < 0)\n \t\treturn;\n \tif (fstat(fd, &st) || (st.st_size == 0)) {\n@@ -411,7 +413,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = open(path, O_RDONLY);\n+\tint fd = git_open_noatime(path);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -655,9 +657,9 @@ static int open_packed_git_1(struct packed_git *p)\n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n-\tp->pack_fd = open(p->pack_name, O_RDONLY);\n+\tp->pack_fd = git_open_noatime(p->pack_name);\n \twhile (p->pack_fd < 0 && errno == EMFILE && unuse_one_window(p, -1))\n-\t\tp->pack_fd = open(p->pack_name, O_RDONLY);\n+\t\tp->pack_fd = git_open_noatime(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \n-- \n1.7.3.2.191.g2d0e5\n"},{"id":"154948","messageId":"1288652061-19614-3-git-send-email-spearce@spearce.org","threadId":"25610","inReplyTo":"1288652061-19614-1-git-send-email-spearce@spearce.org","subject":"[PATCH 2/2] Work around EMFILE when there are too many pack files","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-11-01T22:54:21Z","receivedAt":"2010-11-01T22:54:21Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"When opening any files in the object database, release unused pack\nwindows if the open(2) syscall fails due to EMFILE (too many open\nfiles in this process). This allows Git to degrade gracefully on\na repository with thousands of pack files, and a commit stored in\na loose object in the middle of the history.\n\nSigned-off-by: Shawn O. Pearce \n---\n sha1_file.c | 43 +++++++++++++++++++++++++++----------------\n 1 files changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a6c1934..43d68e0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -35,7 +35,7 @@ static size_t sz_fmt(size_t s) { return s; }\n \n const unsigned char null_sha1[20];\n \n-static int git_open_noatime(const char *name);\n+static int git_open_noatime(const char *name, struct packed_git *p);\n \n int safe_create_leading_directories(char *path)\n {\n@@ -300,7 +300,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tsprintf(path, \"%s/%s\", relative_base, alt_file_name);\n-\tfd = git_open_noatime(path);\n+\tfd = git_open_noatime(path, NULL);\n \tif (fd < 0)\n \t\treturn;\n \tif (fstat(fd, &st) || (st.st_size == 0)) {\n@@ -413,7 +413,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = git_open_noatime(path);\n+\tint fd = git_open_noatime(path, p);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -657,9 +657,7 @@ static int open_packed_git_1(struct packed_git *p)\n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n-\tp->pack_fd = git_open_noatime(p->pack_name);\n-\twhile (p->pack_fd < 0 && errno == EMFILE && unuse_one_window(p, -1))\n-\t\tp->pack_fd = git_open_noatime(p->pack_name);\n+\tp->pack_fd = git_open_noatime(p->pack_name, p);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \n@@ -876,7 +874,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tsprintf(path, \"%s/pack\", objdir);\n \tlen = strlen(path);\n \tdir = opendir(path);\n-\twhile (!dir && errno == EMFILE && unuse_one_window(packed_git, -1))\n+\twhile (!dir && errno == EMFILE && unuse_one_window(NULL, -1))\n \t\tdir = opendir(path);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n@@ -1024,18 +1022,31 @@ int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long siz\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-static int git_open_noatime(const char *name)\n+static int git_open_noatime(const char *name, struct packed_git *p)\n {\n \tstatic int sha1_file_open_flag = O_NOATIME;\n-\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n \n-\t/* Might the failure be due to O_NOATIME? */\n-\tif (fd < 0 && errno != ENOENT && sha1_file_open_flag) {\n-\t\tfd = open(name, O_RDONLY);\n+\tfor (;;) {\n+\t\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n \t\tif (fd >= 0)\n+\t\t\treturn fd;\n+\n+\t\t/* Might the failure be insufficient file descriptors? */\n+\t\tif (errno == EMFILE) {\n+\t\t\tif (unuse_one_window(p, -1))\n+\t\t\t\tcontinue;\n+\t\t\telse\n+\t\t\t\treturn -1;\n+\t\t}\n+\n+\t\t/* Might the failure be due to O_NOATIME? */\n+\t\tif (errno != ENOENT && sha1_file_open_flag) {\n \t\t\tsha1_file_open_flag = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\treturn -1;\n \t}\n-\treturn fd;\n }\n \n static int open_sha1_file(const unsigned char *sha1)\n@@ -1044,7 +1055,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tchar *name = sha1_file_name(sha1);\n \tstruct alternate_object_database *alt;\n \n-\tfd = git_open_noatime(name);\n+\tfd = git_open_noatime(name, NULL);\n \tif (fd >= 0)\n \t\treturn fd;\n \n@@ -1053,7 +1064,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tname = alt->name;\n \t\tfill_sha1_path(name, sha1);\n-\t\tfd = git_open_noatime(alt->base);\n+\t\tfd = git_open_noatime(alt->base, NULL);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t}\n@@ -2314,7 +2325,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \n \tfilename = sha1_file_name(sha1);\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n-\twhile (fd < 0 && errno == EMFILE && unuse_one_window(packed_git, -1))\n+\twhile (fd < 0 && errno == EMFILE && unuse_one_window(NULL, -1))\n \t\tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n \t\tif (errno == EACCES)\n-- \n1.7.3.2.191.g2d0e5\n"},{"id":"154957","messageId":"4CCFCF8A.20608@viscovery.net","threadId":"25610","inReplyTo":"1288652061-19614-3-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 2/2] Work around EMFILE when there are too many pack files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-11-02T08:44:58Z","receivedAt":"2010-11-02T08:44:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11/1/2010 23:54, schrieb Shawn O. Pearce:\n> -static int git_open_noatime(const char *name)\n> +static int git_open_noatime(const char *name, struct packed_git *p)\n> {\n> \tstatic int sha1_file_open_flag = O_NOATIME;\n> -\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n> \n> -\t/* Might the failure be due to O_NOATIME? */\n> -\tif (fd < 0 && errno != ENOENT && sha1_file_open_flag) {\n> -\t\tfd = open(name, O_RDONLY);\n> +\tfor (;;) {\n> +\t\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n> \t\tif (fd >= 0)\n> +\t\t\treturn fd;\n> +\n> +\t\t/* Might the failure be insufficient file descriptors? */\n> +\t\tif (errno == EMFILE) {\n> +\t\t\tif (unuse_one_window(p, -1))\n> +\t\t\t\tcontinue;\n> +\t\t\telse\n> +\t\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\t/* Might the failure be due to O_NOATIME? */\n> +\t\tif (errno != ENOENT && sha1_file_open_flag) {\n> \t\t\tsha1_file_open_flag = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\treturn -1;\n> \t}\n> -\treturn fd;\n> }\n\nOh, boy! A function that returns a value, but does not have a return\nstatement at the end? Even a goto would be easier to read:\n\nretry:\n\tfd = open(name, O_RDONLY | sha1_file_open_flag);\n\tif (fd >= 0)\n\t\treturn fd;\n\n\tif (errno == EMFILE) {\n\t\tif (unuse_one_window(p, -1))\n\t\t\tgoto retry;\n\t\treturn -1;\n\t}\n\n\tif (errno != ENOENT && sha1_file_open_flag) {\n\t\tsha1_file_open_flag = 0;\n\t\tgoto retry;\n\t}\n\n\treturn -1;\n\nIMHO, of course.\n\n-- Hannes\n"},{"id":"155073","messageId":"7vaalqxro0.fsf@alter.siamese.dyndns.org","threadId":"25610","inReplyTo":"1288652061-19614-3-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 2/2] Work around EMFILE when there are too many pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-03T17:06:55Z","receivedAt":"2010-11-03T17:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> @@ -876,7 +874,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n> \tsprintf(path, \"%s/pack\", objdir);\n> \tlen = strlen(path);\n> \tdir = opendir(path);\n> -\twhile (!dir && errno == EMFILE && unuse_one_window(packed_git, -1))\n> +\twhile (!dir && errno == EMFILE && unuse_one_window(NULL, -1))\n\nHmmmm, why?\n\n> @@ -2314,7 +2325,7 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n> \n> \tfilename = sha1_file_name(sha1);\n> \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n> -\twhile (fd < 0 && errno == EMFILE && unuse_one_window(packed_git, -1))\n> +\twhile (fd < 0 && errno == EMFILE && unuse_one_window(NULL, -1))\n\nAgain, why?\n"},{"id":"155074","messageId":"7v8w1axrnp.fsf@alter.siamese.dyndns.org","threadId":"25610","inReplyTo":"1288652061-19614-2-git-send-email-spearce@spearce.org","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-03T17:07:06Z","receivedAt":"2010-11-03T17:07:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> This utility function avoids an unnecessary update of the access time\n> for a loose object file. Just as the atime isn't useful on a loose\n> object, its not useful on the pack or the corresonding idx file.\n>\n> Signed-off-by: Shawn O. Pearce \n\nHearing the name \"git-open-noatime\", one would naturally assume that it is\na way to open files without burdening the filesystem with inode metadata\nupdate traffic, as that was the original reason why we open loose objects\nwithout atime update. We historically anticipated to have very many of\nthe loose objects lying around, and this optimization made sense.\n\nAs any sane repository would have far fewer packfiles than loose objects,\none would think that, while it may not hurt, using git-open-noatime to\nopen packfiles is just a misguided performance measure. Not.\n\nThis patch (and the next patch) adds \"we unuse pack windows to retry\nopening if we have too many files already open\" logic, which is a lot more\nimportant side effect, especially when this function is used for packfiles\n(because they tend to stay open for a long time, unlike loose object files\nthat are opened, read/mapped, and then immediately closed) than what the\nname of this function says it does.\n\nEven though I think the issue you are solving is worth addressing, I do\nnot think I like the structure of the API resulting from these two\npatches. Most of the callers, except for the ones in check-packed-git-idx\nand open-packed-git-1, do not care about \"keeping one packfile\" interface,\nso I would prefer to see a two-patch series along the lines of ...\n\n (1) introduce \"int git_open_ro(const char *)\" to replace the current\n git_open_noatime(). The point is that the function no longer is\n about avoiding from smudging the inode metadata. Instead, it becomes\n the preferred way for us to get a read-only fd.\n\n (2) call your git_open_noatime() implementation git_open_rowpf() or\n something. Make git_open_ro() a thin wrapper of this function that\n passes NULL for its packed_git parameter. Two callers that care\n about protecting a pack they are operating on will call this function\n directly.\n\nWe can of course do without s/git_open_noatime/git_open_ro/; and it will\nmake the patch much smaller. The rename is purely a clarification of the\nAPI and is optional. It may make it easier to explain the name of the new\nfunction, though.\n\nBy the way, I think I still owe you a patch to selectively pack-ref only\nold ones.\n"},{"id":"155078","messageId":"20101103174148.GB13377@burratino","threadId":"25610","inReplyTo":"7v8w1axrnp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-03T17:41:48Z","receivedAt":"2010-11-03T17:41:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> (1) introduce \"int git_open_ro(const char *)\" to replace the current\n> git_open_noatime(). The point is that the function no longer is\n> about avoiding from smudging the inode metadata. Instead, it becomes\n> the preferred way for us to get a read-only fd.\n[...]\n> We can of course do without s/git_open_noatime/git_open_ro/; and it will\n> make the patch much smaller. The rename is purely a clarification of the\n> API and is optional. It may make it easier to explain the name of the new\n> function, though.\n\nProbably a silly question, but should all readonly open()s actually use\nnoatime? Some uses for atime:\n\n - tmpwatch. That one's a bit insane, anyway, but I think it might\n work because loose objects and packs are read-only.\n\n - mail clients. For this case, noatime is the right thing to do ---\n git's access does mean the mail was read.\n\n - listing important files, as in popularity-contest. For this,\n noatime is also the right thing to do.\n\nJudging from these three use cases, readonly open()s to the worktree\nshould indeed use noatime, but open()s of .git/config, say, should\nnot. Hmm.\n"},{"id":"155082","messageId":"7vvd4ew687.fsf@alter.siamese.dyndns.org","threadId":"25610","inReplyTo":"20101103174148.GB13377@burratino","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-03T19:35:20Z","receivedAt":"2010-11-03T19:35:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Judging from these three use cases, readonly open()s to the worktree\n> should indeed use noatime, but open()s of .git/config, say, should\n> not. Hmm.\n\nWhy not, when you are talking about readonly open?\n"},{"id":"155135","messageId":"20101104050437.GB15018@burratino","threadId":"25610","inReplyTo":"7vvd4ew687.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-04T05:04:37Z","receivedAt":"2010-11-04T05:04:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Judging from these three use cases, readonly open()s to the worktree\n>> should indeed use noatime, but open()s of .git/config, say, should\n>> not. Hmm.\n>\n> Why not, when you are talking about readonly open?\n\nA .git/config frequently accessed by git is being used, so tmpwatch\nshouldn't delete it. (Meanwhile, a worktree frequently accessed by\ngit is not precious on that account, and letting git touch the atime\nthere might mask more useful information about when other programs\ntouched the files.)\n\nBut that is all very fuzzy to me. I tend to mount noatime except on\n/usr (for popularity-contest).\n\nI guess I should put it another way. What if anything does readonly\nhave to do with O_NOATIME? Why shouldn't we always use O_NOATIME?\nWhy should the operating system provide atime at all?\n"},{"id":"155138","messageId":"95C8862A-11A8-436C-AAD9-C231DA2BC3E4@sb.org","threadId":"25610","inReplyTo":"20101104050437.GB15018@burratino","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2010-11-04T05:23:26Z","receivedAt":"2010-11-04T05:23:26Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"On Nov 3, 2010, at 10:04 PM, Jonathan Nieder wrote:\n\n> I guess I should put it another way. What if anything does readonly\n> have to do with O_NOATIME? Why shouldn't we always use O_NOATIME?\n> Why should the operating system provide atime at all?\n\natime is useful for user-level files that are opened or modified as a\ndirect result of actions the user took. I don't think a took like git\nhas any reason to ever want to touch atime.\n\n-Kevin Ballard\n"},{"id":"155265","messageId":"7vr5ezu1fm.fsf@alter.siamese.dyndns.org","threadId":"25610","inReplyTo":"20101104050437.GB15018@burratino","subject":"Re: [PATCH 1/2] Use git_open_noatime when accessing pack data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-05T17:26:21Z","receivedAt":"2010-11-05T17:26:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I guess I should put it another way. What if anything does readonly\n> have to do with O_NOATIME? Why shouldn't we always use O_NOATIME?\n> Why should the operating system provide atime at all?\n\nWell, I think what you are getting at is that the proposed API in Shawn's\npatch and my suggestion is upside down, and it should be layered more like\nthis (from lower to higher layers) from the interface's point of view:\n\n - git_open_wpf(): an API to get a file descriptor to a file, while\n protecting open fds to a packfile from getting reclaimed;\n\n - git_open(): a thin wrapper of the previous, for callers that do not\n need any \"with-pack-file\" aspect of it;\n\n - git_open_noatime(): a thin wrapper of the previous, for callers that do\n not want to incur inode metainformation traffic to the filesystem.\n \neven though the bulk of implementation, including the logic to handle\nno-atime, probably needs to happen in the lowermost layer.\n"}]}