{"thread":{"id":"42108","subject":"[PATCH 1/3] win32mmap: set errno appropriately","startedAt":"2016-04-22T14:31:17Z","lastAt":"2016-04-28T08:03:23Z","messageCount":8,"participants":["Johannes Schindelin","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"284130","messageId":"cover.1461335463.git.johannes.schindelin@gmx.de","threadId":"42108","inReplyTo":null,"subject":"[PATCH 0/3] Improve the mmap() emulation on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T14:31:17Z","receivedAt":"2016-04-22T14:31:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Kevin David reported a problem last October where a simple git fetch\nwould produce this error output:\n\n\tfatal: mmap failed: No error\n\tfatal: write error: Invalid argument\n\nThe reason was that several bits of our mmap() emulation left room for\nimprovement. These patches fix it, and made it already into Git\nfor Windows v2.6.2 and have been working without problems ever since.\n\n\nJohannes Schindelin (3):\n win32mmap: set errno appropriately\n mmap(win32): avoid copy-on-write when it is unnecessary\n mmap(win32): avoid expensive fstat() call\n\n compat/win32mmap.c | 31 ++++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 13 deletions(-)\n\n-- \n2.8.1.306.gff998f2\n"},{"id":"284129","messageId":"ff12776a7f76c0e924222a0a04b0ae6f571b571c.1461335463.git.johannes.schindelin@gmx.de","threadId":"42108","inReplyTo":"cover.1461335463.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] win32mmap: set errno appropriately","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T14:31:22Z","receivedAt":"2016-04-22T14:31:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is not really helpful when a `git fetch` fails with the message:\n\n\tfatal: mmap failed: No error\n\nIn the particular instance encountered by a colleague of yours truly,\nthe Win32 error code was ERROR_COMMITMENT_LIMIT which means that the\npage file is not big enough.\n\nLet's make the message\n\n\tfatal: mmap failed: File too large\n\ninstead, which is only marginally better, but which can be associated\nwith the appropriate work-around: setting `core.packedGitWindowSize` to\na relatively small value.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32mmap.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/win32mmap.c b/compat/win32mmap.c\nindex 80a8c9a..3a39f0f 100644\n--- a/compat/win32mmap.c\n+++ b/compat/win32mmap.c\n@@ -24,15 +24,21 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \thmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n \t\tPAGE_WRITECOPY, 0, 0, NULL);\n \n-\tif (!hmap)\n+\tif (!hmap) {\n+\t\terrno = EINVAL;\n \t\treturn MAP_FAILED;\n+\t}\n \n \ttemp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);\n \n \tif (!CloseHandle(hmap))\n \t\twarning(\"unable to close file mapping handle\");\n \n-\treturn temp ? temp : MAP_FAILED;\n+\tif (temp)\n+\t\treturn temp;\n+\n+\terrno = GetLastError() == ERROR_COMMITMENT_LIMIT ? EFBIG : EINVAL;\n+\treturn MAP_FAILED;\n }\n \n int git_munmap(void *start, size_t length)\n-- \n2.8.1.306.gff998f2\n"},{"id":"284131","messageId":"3e2a45e60e2905f52f962604cf19a0e5e39b9b1b.1461335463.git.johannes.schindelin@gmx.de","threadId":"42108","inReplyTo":"cover.1461335463.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T14:31:26Z","receivedAt":"2016-04-22T14:31:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Often we are mmap()ing read-only. In those cases, it is wasteful to map in\ncopy-on-write mode. Even worse: it can cause errors where we run out of\nspace in the page file.\n\nSo let's be extra careful to map files in read-only mode whenever\npossible.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32mmap.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/win32mmap.c b/compat/win32mmap.c\nindex 3a39f0f..b836169 100644\n--- a/compat/win32mmap.c\n+++ b/compat/win32mmap.c\n@@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \t\tdie(\"Invalid usage of mmap when built with USE_WIN32_MMAP\");\n \n \thmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n-\t\tPAGE_WRITECOPY, 0, 0, NULL);\n+\t\tprot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);\n \n \tif (!hmap) {\n \t\terrno = EINVAL;\n \t\treturn MAP_FAILED;\n \t}\n \n-\ttemp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);\n+\ttemp = MapViewOfFileEx(hmap, prot == PROT_READ ?\n+\t\t\tFILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);\n \n \tif (!CloseHandle(hmap))\n \t\twarning(\"unable to close file mapping handle\");\n-- \n2.8.1.306.gff998f2\n"},{"id":"284132","messageId":"54698524fa95e2f83167ca78205749f855bfaee6.1461335463.git.johannes.schindelin@gmx.de","threadId":"42108","inReplyTo":"cover.1461335463.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] mmap(win32): avoid expensive fstat() call","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T14:31:32Z","receivedAt":"2016-04-22T14:31:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, we have to emulate the fstat() call to fill out information\nthat takes extra effort to obtain, such as the file permissions/type.\n\nIf all we want is the file size, we can use the much cheaper\nGetFileSizeEx() function (available since Windows XP).\n\nSuggested by Philip Kelley.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32mmap.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/compat/win32mmap.c b/compat/win32mmap.c\nindex b836169..519d51f 100644\n--- a/compat/win32mmap.c\n+++ b/compat/win32mmap.c\n@@ -2,26 +2,24 @@\n \n void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)\n {\n-\tHANDLE hmap;\n+\tHANDLE osfhandle, hmap;\n \tvoid *temp;\n-\toff_t len;\n-\tstruct stat st;\n+\tLARGE_INTEGER len;\n \tuint64_t o = offset;\n \tuint32_t l = o & 0xFFFFFFFF;\n \tuint32_t h = (o >> 32) & 0xFFFFFFFF;\n \n-\tif (!fstat(fd, &st))\n-\t\tlen = st.st_size;\n-\telse\n+\tosfhandle = (HANDLE)_get_osfhandle(fd);\n+\tif (!GetFileSizeEx(osfhandle, &len))\n \t\tdie(\"mmap: could not determine filesize\");\n \n-\tif ((length + offset) > len)\n-\t\tlength = xsize_t(len - offset);\n+\tif ((length + offset) > len.QuadPart)\n+\t\tlength = xsize_t(len.QuadPart - offset);\n \n \tif (!(flags & MAP_PRIVATE))\n \t\tdie(\"Invalid usage of mmap when built with USE_WIN32_MMAP\");\n \n-\thmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n+\thmap = CreateFileMapping(osfhandle, NULL,\n \t\tprot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);\n \n \tif (!hmap) {\n-- \n2.8.1.306.gff998f2\n"},{"id":"284569","messageId":"571FB923.9040808@kdbg.org","threadId":"42108","inReplyTo":"3e2a45e60e2905f52f962604cf19a0e5e39b9b1b.1461335463.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-04-26T18:53:23Z","receivedAt":"2016-04-26T18:53:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.04.2016 um 16:31 schrieb Johannes Schindelin:\n> Often we are mmap()ing read-only. In those cases, it is wasteful to map in\n> copy-on-write mode. Even worse: it can cause errors where we run out of\n> space in the page file.\n>\n> So let's be extra careful to map files in read-only mode whenever\n> possible.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/win32mmap.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/compat/win32mmap.c b/compat/win32mmap.c\n> index 3a39f0f..b836169 100644\n> --- a/compat/win32mmap.c\n> +++ b/compat/win32mmap.c\n> @@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n> \t\tdie(\"Invalid usage of mmap when built with USE_WIN32_MMAP\");\n>\n> \thmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n> -\t\tPAGE_WRITECOPY, 0, 0, NULL);\n> +\t\tprot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);\n\nAs long as we use this implementation with MAP_PRIVATE, PAGE_WRITECOPY \nis the right setting. Should we insert a check for MAP_PRIVATE to catch \nunexpected use-cases (think of the index-helper daemon effort)?\n\n>\n> \tif (!hmap) {\n> \t\terrno = EINVAL;\n> \t\treturn MAP_FAILED;\n> \t}\n>\n> -\ttemp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);\n> +\ttemp = MapViewOfFileEx(hmap, prot == PROT_READ ?\n> +\t\t\tFILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);\n\nSame here: FILE_MAP_COPY is the right choice for MAP_SHARED mmaps.\n\n>\n> \tif (!CloseHandle(hmap))\n> \t\twarning(\"unable to close file mapping handle\");\n>\n\nExcept for these mental notes, I've no comments on this series. Looks good.\n\n-- Hannes\n"},{"id":"284632","messageId":"alpine.DEB.2.20.1604270834440.2896@virtualbox","threadId":"42108","inReplyTo":"571FB923.9040808@kdbg.org","subject":"Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-27T06:43:01Z","receivedAt":"2016-04-27T06:43:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 26 Apr 2016, Johannes Sixt wrote:\n\n> Am 22.04.2016 um 16:31 schrieb Johannes Schindelin:\n> > Often we are mmap()ing read-only. In those cases, it is wasteful to map in\n> > copy-on-write mode. Even worse: it can cause errors where we run out of\n> > space in the page file.\n> >\n> > So let's be extra careful to map files in read-only mode whenever\n> > possible.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > compat/win32mmap.c | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/compat/win32mmap.c b/compat/win32mmap.c\n> > index 3a39f0f..b836169 100644\n> > --- a/compat/win32mmap.c\n> > +++ b/compat/win32mmap.c\n> > @@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int\n> > flags, int fd, off_t of\n> > die(\"Invalid usage of mmap when built with USE_WIN32_MMAP\");\n> >\n> > \thmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,\n> > -\t\tPAGE_WRITECOPY, 0, 0, NULL);\n> > +\t\tprot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0,\n> > NULL);\n> \n> As long as we use this implementation with MAP_PRIVATE, PAGE_WRITECOPY\n> is the right setting. Should we insert a check for MAP_PRIVATE to catch\n> unexpected use-cases (think of the index-helper daemon effort)?\n\nI agree, we should have such a check. The line above the `die(\"Invalid\nusage ...\")` that you can read as first line in above-quoted hunk reads:\n\n\tif (!(flags & MAP_PRIVATE))\n\nSo I think we're fine :-)\n\nAnd yes, I am worrying about the index-helper support, too: I need this\nmyself, so I will have to make mmap() work for that use case, too. But\nthat is a story for another day ;-)\n\n> > if (!hmap) {\n> > errno = EINVAL;\n> > return MAP_FAILED;\n> > }\n> >\n> > -\ttemp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);\n> > +\ttemp = MapViewOfFileEx(hmap, prot == PROT_READ ?\n> > +\t\t\tFILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);\n> \n> Same here: FILE_MAP_COPY is the right choice for MAP_SHARED mmaps.\n\nI agree ;-)\n\n> > if (!CloseHandle(hmap))\n> > warning(\"unable to close file mapping handle\");\n> >\n> \n> Except for these mental notes, I've no comments on this series. Looks good.\n\nThanks for reviewing!\n\nDo you think we should add a note to the commit message that we'll have to\ndo something about this when MAP_PRIVATE is not the only way mmap() will\nbe used?\n\nI am torn: on the one hand, it is the appropriate thing to do, on the\nother hand, it is easy to forget such notes in commit messages. On the\nthird hand, I hope to find time to work on the index-helper this week,\nmeaning that I will still know about this when touching\ncompat/win32mmap.c.\n\nSo maybe I can just leave things as are here, and focus on the\nindex-helper?\n\nCiao,\nDscho\n"},{"id":"284708","messageId":"57210A38.4080203@kdbg.org","threadId":"42108","inReplyTo":"alpine.DEB.2.20.1604270834440.2896@virtualbox","subject":"Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-04-27T18:51:36Z","receivedAt":"2016-04-27T18:51:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:\n> On Tue, 26 Apr 2016, Johannes Sixt wrote:\n>> Should we insert a check for MAP_PRIVATE to catch\n>> unexpected use-cases (think of the index-helper daemon effort)?\n>\n> I agree, we should have such a check. The line above the `die(\"Invalid\n> usage ...\")` that you can read as first line in above-quoted hunk reads:\n>\n> \tif (!(flags & MAP_PRIVATE))\n>\n> So I think we're fine :-)\n\nOh, well... I thought I had checked the code before I wrote my question, \nbut it seems I was blind... ;)\n\nThanks,\n-- Hannes\n"},{"id":"284779","messageId":"alpine.DEB.2.20.1604281001220.2896@virtualbox","threadId":"42108","inReplyTo":"57210A38.4080203@kdbg.org","subject":"Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-28T08:03:23Z","receivedAt":"2016-04-28T08:03:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Wed, 27 Apr 2016, Johannes Sixt wrote:\n\n> Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:\n> > On Tue, 26 Apr 2016, Johannes Sixt wrote:\n> > > Should we insert a check for MAP_PRIVATE to catch\n> > > unexpected use-cases (think of the index-helper daemon effort)?\n> >\n> > I agree, we should have such a check. The line above the `die(\"Invalid\n> > usage ...\")` that you can read as first line in above-quoted hunk reads:\n> >\n> > if (!(flags & MAP_PRIVATE))\n> >\n> > So I think we're fine :-)\n> \n> Oh, well... I thought I had checked the code before I wrote my question, but\n> it seems I was blind... ;)\n\nDon't worry! I really appreciate your review!\n\nCiao,\nJohannes\n"}]}