[PATCH 0/3] Improve the mmap() emulation on Windows

[PATCH 0/3] Improve the mmap() emulation on Windows

[Johannes Schindelin]

Kevin David reported a problem last October where a simple git fetch
would produce this error output:

	fatal: mmap failed: No error
	fatal: write error: Invalid argument

The reason was that several bits of our mmap() emulation left room for
improvement. These patches fix it, and made it already into Git
for Windows v2.6.2 and have been working without problems ever since.


Johannes Schindelin (3):
  win32mmap: set errno appropriately
  mmap(win32): avoid copy-on-write when it is unnecessary
  mmap(win32): avoid expensive fstat() call

 compat/win32mmap.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

-- 
2.8.1.306.gff998f2

[PATCH 1/3] win32mmap: set errno appropriately

[Johannes Schindelin]

It is not really helpful when a `git fetch` fails with the message:

	fatal: mmap failed: No error

In the particular instance encountered by a colleague of yours truly,
the Win32 error code was ERROR_COMMITMENT_LIMIT which means that the
page file is not big enough.

Let's make the message

	fatal: mmap failed: File too large

instead, which is only marginally better, but which can be associated
with the appropriate work-around: setting `core.packedGitWindowSize` to
a relatively small value.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32mmap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index 80a8c9a..3a39f0f 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -24,15 +24,21 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
 		PAGE_WRITECOPY, 0, 0, NULL);
 
-	if (!hmap)
+	if (!hmap) {
+		errno = EINVAL;
 		return MAP_FAILED;
+	}
 
 	temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
 
 	if (!CloseHandle(hmap))
 		warning("unable to close file mapping handle");
 
-	return temp ? temp : MAP_FAILED;
+	if (temp)
+		return temp;
+
+	errno = GetLastError() == ERROR_COMMITMENT_LIMIT ? EFBIG : EINVAL;
+	return MAP_FAILED;
 }
 
 int git_munmap(void *start, size_t length)
-- 
2.8.1.306.gff998f2

[PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Schindelin]

Often we are mmap()ing read-only. In those cases, it is wasteful to map in
copy-on-write mode. Even worse: it can cause errors where we run out of
space in the page file.

So let's be extra careful to map files in read-only mode whenever
possible.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32mmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index 3a39f0f..b836169 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
 		die("Invalid usage of mmap when built with USE_WIN32_MMAP");
 
 	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
-		PAGE_WRITECOPY, 0, 0, NULL);
+		prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);
 
 	if (!hmap) {
 		errno = EINVAL;
 		return MAP_FAILED;
 	}
 
-	temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
+	temp = MapViewOfFileEx(hmap, prot == PROT_READ ?
+			FILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);
 
 	if (!CloseHandle(hmap))
 		warning("unable to close file mapping handle");
-- 
2.8.1.306.gff998f2

[PATCH 3/3] mmap(win32): avoid expensive fstat() call

[Johannes Schindelin]

On Windows, we have to emulate the fstat() call to fill out information
that takes extra effort to obtain, such as the file permissions/type.

If all we want is the file size, we can use the much cheaper
GetFileSizeEx() function (available since Windows XP).

Suggested by Philip Kelley.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32mmap.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index b836169..519d51f 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -2,26 +2,24 @@
 
 void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)
 {
-	HANDLE hmap;
+	HANDLE osfhandle, hmap;
 	void *temp;
-	off_t len;
-	struct stat st;
+	LARGE_INTEGER len;
 	uint64_t o = offset;
 	uint32_t l = o & 0xFFFFFFFF;
 	uint32_t h = (o >> 32) & 0xFFFFFFFF;
 
-	if (!fstat(fd, &st))
-		len = st.st_size;
-	else
+	osfhandle = (HANDLE)_get_osfhandle(fd);
+	if (!GetFileSizeEx(osfhandle, &len))
 		die("mmap: could not determine filesize");
 
-	if ((length + offset) > len)
-		length = xsize_t(len - offset);
+	if ((length + offset) > len.QuadPart)
+		length = xsize_t(len.QuadPart - offset);
 
 	if (!(flags & MAP_PRIVATE))
 		die("Invalid usage of mmap when built with USE_WIN32_MMAP");
 
-	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
+	hmap = CreateFileMapping(osfhandle, NULL,
 		prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);
 
 	if (!hmap) {
-- 
2.8.1.306.gff998f2

Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Sixt]

Am 22.04.2016 um 16:31 schrieb Johannes Schindelin:
> Often we are mmap()ing read-only. In those cases, it is wasteful to map in
> copy-on-write mode. Even worse: it can cause errors where we run out of
> space in the page file.
>
> So let's be extra careful to map files in read-only mode whenever
> possible.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/win32mmap.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/compat/win32mmap.c b/compat/win32mmap.c
> index 3a39f0f..b836169 100644
> --- a/compat/win32mmap.c
> +++ b/compat/win32mmap.c
> @@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
>   		die("Invalid usage of mmap when built with USE_WIN32_MMAP");
>
>   	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
> -		PAGE_WRITECOPY, 0, 0, NULL);
> +		prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);

As long as we use this implementation with MAP_PRIVATE, PAGE_WRITECOPY 
is the right setting. Should we insert a check for MAP_PRIVATE to catch 
unexpected use-cases (think of the index-helper daemon effort)?

>
>   	if (!hmap) {
>   		errno = EINVAL;
>   		return MAP_FAILED;
>   	}
>
> -	temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
> +	temp = MapViewOfFileEx(hmap, prot == PROT_READ ?
> +			FILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);

Same here: FILE_MAP_COPY is the right choice for MAP_SHARED mmaps.

>
>   	if (!CloseHandle(hmap))
>   		warning("unable to close file mapping handle");
>

Except for these mental notes, I've no comments on this series. Looks good.

-- Hannes

Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Schindelin]

Hi Hannes,

On Tue, 26 Apr 2016, Johannes Sixt wrote:

> Am 22.04.2016 um 16:31 schrieb Johannes Schindelin:
> > Often we are mmap()ing read-only. In those cases, it is wasteful to map in
> > copy-on-write mode. Even worse: it can cause errors where we run out of
> > space in the page file.
> >
> > So let's be extra careful to map files in read-only mode whenever
> > possible.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   compat/win32mmap.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/compat/win32mmap.c b/compat/win32mmap.c
> > index 3a39f0f..b836169 100644
> > --- a/compat/win32mmap.c
> > +++ b/compat/win32mmap.c
> > @@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int
> > flags, int fd, off_t of
> >     die("Invalid usage of mmap when built with USE_WIN32_MMAP");
> >
> >   	hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
> > -		PAGE_WRITECOPY, 0, 0, NULL);
> > +		prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0,
> > NULL);
> 
> As long as we use this implementation with MAP_PRIVATE, PAGE_WRITECOPY
> is the right setting. Should we insert a check for MAP_PRIVATE to catch
> unexpected use-cases (think of the index-helper daemon effort)?

I agree, we should have such a check. The line above the `die("Invalid
usage ...")` that you can read as first line in above-quoted hunk reads:

	if (!(flags & MAP_PRIVATE))

So I think we're fine :-)

And yes, I am worrying about the index-helper support, too: I need this
myself, so I will have to make mmap() work for that use case, too. But
that is a story for another day ;-)

> >    if (!hmap) {
> >     errno = EINVAL;
> >     return MAP_FAILED;
> >    }
> >
> > -	temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
> > +	temp = MapViewOfFileEx(hmap, prot == PROT_READ ?
> > +			FILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);
> 
> Same here: FILE_MAP_COPY is the right choice for MAP_SHARED mmaps.

I agree ;-)

> >    if (!CloseHandle(hmap))
> >     warning("unable to close file mapping handle");
> >
> 
> Except for these mental notes, I've no comments on this series. Looks good.

Thanks for reviewing!

Do you think we should add a note to the commit message that we'll have to
do something about this when MAP_PRIVATE is not the only way mmap() will
be used?

I am torn: on the one hand, it is the appropriate thing to do, on the
other hand, it is easy to forget such notes in commit messages. On the
third hand, I hope to find time to work on the index-helper this week,
meaning that I will still know about this when touching
compat/win32mmap.c.

So maybe I can just leave things as are here, and focus on the
index-helper?

Ciao,
Dscho

Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Sixt]

Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:
> On Tue, 26 Apr 2016, Johannes Sixt wrote:
>> Should we insert a check for MAP_PRIVATE to catch
>> unexpected use-cases (think of the index-helper daemon effort)?
>
> I agree, we should have such a check. The line above the `die("Invalid
> usage ...")` that you can read as first line in above-quoted hunk reads:
>
> 	if (!(flags & MAP_PRIVATE))
>
> So I think we're fine :-)

Oh, well... I thought I had checked the code before I wrote my question, 
but it seems I was blind... ;)

Thanks,
-- Hannes

Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Schindelin]

Hi Hannes,

On Wed, 27 Apr 2016, Johannes Sixt wrote:

> Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:
> > On Tue, 26 Apr 2016, Johannes Sixt wrote:
> > > Should we insert a check for MAP_PRIVATE to catch
> > > unexpected use-cases (think of the index-helper daemon effort)?
> >
> > I agree, we should have such a check. The line above the `die("Invalid
> > usage ...")` that you can read as first line in above-quoted hunk reads:
> >
> >  if (!(flags & MAP_PRIVATE))
> >
> > So I think we're fine :-)
> 
> Oh, well... I thought I had checked the code before I wrote my question, but
> it seems I was blind... ;)

Don't worry! I really appreciate your review!

Ciao,
Johannes