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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by 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 <[hidden email]>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
In reply to this post by 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 <[hidden email]>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Sixt-3
In reply to this post by Johannes Schindelin
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 <[hidden email]>
> ---
>   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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
> > ---
> >   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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Sixt-3
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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html