[patch] munmap-before-rename, cygwin need

[patch] munmap-before-rename, cygwin need

[Yakov Lerner]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

I found that mmap() works on cygwin, but needs a patch.
On Cygwin, rename() fails if target file has active mmap().
The patch below adds  munmap() before rename().

If you excuse me for posting not in git-generated format
(I did not yet completely make git working on my cygwin).
I'm adding the copy as attachment, I'm afraid the mailer
may interfere with spaces ?

Yakov Lerner

Makefile     |    5 +++++
cache.h      |    1 +
index.c      |    3 +++
read-cache.c |   14 ++++++++++++++

--- Makefile.000 2006-05-07 22:32:04.000000000 +0300
+++ Makefile 2006-05-07 22:30:38.000000000 +0300
@@ -46,6 +46,9 @@
#
# Define NO_MMAP if you want to avoid mmap.
#
+# Define MUMNAP_BEFORE_RENAME if munmap() on target file is required
+# before rename().
+#
# Define WITH_OWN_SUBPROCESS_PY if you want to use with python 2.3.
#
# Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
@@ -270,6 +273,8 @@
# NO_MMAP = YesPlease
NO_IPV6 = YesPlease
X = .exe
+ MUMNAP_BEFORE_RENAME = YesPlease
+ NO_SOCKADDR_STORAGE = YesPlease
endif
ifeq ($(uname_S),FreeBSD)
NEEDS_LIBICONV = YesPlease
--- cache.h.000 2006-05-02 11:35:50.000000000 +0300
+++ cache.h 2006-05-02 11:33:34.000000000 +0300
@@ -140,6 +140,7 @@

/* Initialize and use the cache information */
extern int read_cache(void);
+extern void unmap_cache(void);
extern int write_cache(int newfd, struct cache_entry **cache, int entries);
extern int cache_name_pos(const char *name, int namelen);
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
--- read-cache.c.000 2006-05-07 22:33:42.000000000 +0300
+++ read-cache.c 2006-05-02 11:32:56.000000000 +0300
@@ -513,6 +513,9 @@
return 0;
}

+static void *mapaddr = MAP_FAILED;
+static unsigned long mapsize;
+
int read_cache(void)
{
int fd, i;
@@ -541,6 +544,8 @@
errno = EINVAL;
if (size >= sizeof(struct cache_header) + 20)
map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+ mapaddr = map;
+ mapsize = size;
}
close(fd);
if (map == MAP_FAILED)
@@ -565,10 +570,19 @@

unmap:
munmap(map, size);
+ mapaddr = MAP_FAILED;
errno = EINVAL;
die("index file corrupt");
}

+void unmap_cache(void)
+{
+ if ( mapaddr != MAP_FAILED ) {
+ munmap(mapaddr, mapsize);
+ mapaddr = MAP_FAILED;
+ }
+}
+
#define WRITE_BUFFER_SIZE 8192
static unsigned char write_buffer[WRITE_BUFFER_SIZE];
static unsigned long write_buffer_len;
--- index.c.000 2006-05-07 22:36:04.000000000 +0300
+++ index.c 2006-05-07 22:36:40.000000000 +0300
@@ -43,6 +43,9 @@
strcpy(indexfile, cf->lockfile);
i = strlen(indexfile) - 5; /* .lock */
indexfile[i] = 0;
+#ifdef MUMNAP_BEFORE_RENAME
+ unmap_cache();
+#endif
i = rename(cf->lockfile, indexfile);
cf->lockfile[0] = 0;
return i;

[-- Attachment #2: patch-munmap-before-rename --]
[-- Type: text/plain, Size: 2237 bytes --]

--- Makefile.000	2006-05-07 22:32:04.000000000 +0300
+++ Makefile	2006-05-07 22:30:38.000000000 +0300
@@ -46,6 +46,9 @@
 #
 # Define NO_MMAP if you want to avoid mmap.
 #
+# Define MUMNAP_BEFORE_RENAME if munmap() on target file is required
+# before rename().
+#
 # Define WITH_OWN_SUBPROCESS_PY if you want to use with python 2.3.
 #
 # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
@@ -270,6 +273,8 @@
 	# NO_MMAP = YesPlease
 	NO_IPV6 = YesPlease
 	X = .exe
+	MUMNAP_BEFORE_RENAME = YesPlease
+	NO_SOCKADDR_STORAGE = YesPlease
 endif
 ifeq ($(uname_S),FreeBSD)
 	NEEDS_LIBICONV = YesPlease
--- cache.h.000	2006-05-02 11:35:50.000000000 +0300
+++ cache.h	2006-05-02 11:33:34.000000000 +0300
@@ -140,6 +140,7 @@
 
 /* Initialize and use the cache information */
 extern int read_cache(void);
+extern void unmap_cache(void);
 extern int write_cache(int newfd, struct cache_entry **cache, int entries);
 extern int cache_name_pos(const char *name, int namelen);
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
--- read-cache.c.000	2006-05-07 22:33:42.000000000 +0300
+++ read-cache.c	2006-05-02 11:32:56.000000000 +0300
@@ -513,6 +513,9 @@
 	return 0;
 }
 
+static void *mapaddr = MAP_FAILED;
+static unsigned long mapsize;
+
 int read_cache(void)
 {
 	int fd, i;
@@ -541,6 +544,8 @@
 		errno = EINVAL;
 		if (size >= sizeof(struct cache_header) + 20)
 			map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+		mapaddr = map;
+		mapsize = size;
 	}
 	close(fd);
 	if (map == MAP_FAILED)
@@ -565,10 +570,19 @@
 
 unmap:
 	munmap(map, size);
+	mapaddr = MAP_FAILED;
 	errno = EINVAL;
 	die("index file corrupt");
 }
 
+void unmap_cache(void)
+{
+	if ( mapaddr != MAP_FAILED ) {
+		munmap(mapaddr, mapsize);
+		mapaddr = MAP_FAILED;
+	}
+}
+
 #define WRITE_BUFFER_SIZE 8192
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;
--- index.c.000	2006-05-07 22:36:04.000000000 +0300
+++ index.c	2006-05-07 22:36:40.000000000 +0300
@@ -43,6 +43,9 @@
 	strcpy(indexfile, cf->lockfile);
 	i = strlen(indexfile) - 5; /* .lock */
 	indexfile[i] = 0;
+#ifdef MUMNAP_BEFORE_RENAME
+	unmap_cache();
+#endif
 	i = rename(cf->lockfile, indexfile);
 	cf->lockfile[0] = 0;
 	return i;

Re: [patch] munmap-before-rename, cygwin need

[Junio C Hamano]

"Yakov Lerner" <iler.ml@gmail.com> writes:

> I found that mmap() works on cygwin, but needs a patch.
> On Cygwin, rename() fails if target file has active mmap().
> The patch below adds  munmap() before rename().

This is interesting in three counts.

 - I from time to time test Cygwin version on my day-job machine
   (W2K) and my wife's machine (XP); on both machines I usually
   have less than two weeks old Cygwin installation, and I have
   not seen the breakage.  I wonder how reproducible this is.
   Also previously people reported mmap() works for some and
   fake mmap is needed for others.  Would this patch make things
   work for everybody?

 - The part you patched is commit_index_file().  This typically
   is called just before program exit, but some callers, like
   apply.c, may want to still look at the index after calling
   it, fully aware that the changes after commit_index will not
   be written out.  Although I haven't traced the codepath fully
   in apply.c yet, unmapping would break the access to the index
   (i.e. active_cache[]).  Does apply still work with your
   patch?

 - As long as you can clear the second point, I do not see a
   particular reason to make this an option; we should be able
   to do so everywhere.

Re: [patch] munmap-before-rename, cygwin need

[Yakov Lerner]

On 5/8/06, Junio C Hamano <junkio@cox.net> wrote:
> "Yakov Lerner" <iler.ml@gmail.com> writes:
>
> > I found that mmap() works on cygwin, but needs a patch.
> > On Cygwin, rename() fails if target file has active mmap().
> > The patch below adds  munmap() before rename().
>
> This is interesting in three counts.
>
>  - I from time to time test Cygwin version on my day-job machine
>   (W2K) and my wife's machine (XP); on both machines I usually
>   have less than two weeks old Cygwin installation, and I have
>   not seen the breakage.  I wonder how reproducible this is.
>   Also previously people reported mmap() works for some and
>   fake mmap is needed for others.  Would this patch make things
>   work for everybody?
>
>  - The part you patched is commit_index_file().  This typically
>   is called just before program exit, but some callers, like
>   apply.c, may want to still look at the index after calling
>   it, fully aware that the changes after commit_index will not
>   be written out.  Although I haven't traced the codepath fully
>   in apply.c yet, unmapping would break the access to the index
>   (i.e. active_cache[]).  Does apply still work with your
>   patch?

I am checking this. I am trying different options and different
scenarios.

Yakov

Re: [patch] munmap-before-rename, cygwin need

[Yakov Lerner]

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

On 5/7/06, Junio C Hamano <junkio@cox.net> wrote:
> "Yakov Lerner" <iler.ml@gmail.com> writes:
> > I found that mmap() works on cygwin, but needs a patch.
> > On Cygwin, rename() fails if target file has active mmap().
> > The patch below adds  munmap() before rename().
> This is interesting in three counts.
>
>  - I from time to time test Cygwin version on my day-job machine
>    (W2K) and my wife's machine (XP); on both machines I usually
>    have less than two weeks old Cygwin installation, and I have
>    not seen the breakage.  I wonder how reproducible this is.
>    Also previously people reported mmap() works for some and
>    fake mmap is needed for others.  Would this patch make things
>    work for everybody?
>
>  - The part you patched is commit_index_file().  This typically
>    is called just before program exit, but some callers, like
>    apply.c, may want to still look at the index after calling
>    it, fully aware that the changes after commit_index will not
>    be written out.  Although I haven't traced the codepath fully
>    in apply.c yet, unmapping would break the access to the index
>    (i.e. active_cache[]).  Does apply still work with your
>    patch?

You are right. Apply did not work when I gave it more than one patchfile on
commandline (and --index option). I fixed this by zeroing active_nr and freeing
active_cache in unmap_cache(). Then I got infinite loop in
remove_lock_file (after multiple calls to hold_index_file_for_update
with same cf, cache_file_list points to cf and cf->next points to
cf creating infinite loop.) The fix in index.c is easy.

The patch below works for me. However, it changes internal
working of apply.c in the scenario 'git-apply --index patch1 patch2 ...'.

(1) With the patch below, apply.c repeats mmap() on index after every patch
argument (because index gets unmapped after every patchfile argument).

(2) Current apply.c does single mmap() at the beginning. It modfies index
on disk and cache in memory and it does not repeat mmap. This mmap
is to original (now deleted) index, if i understand correctly (the
no-name inode).

But (2) this does not work in cygwin. The end results of (1) and
(2) are the same, I think. (2) looks to me bit faster (I didn't do
measurements).

It's up to you whether to make it under #ifdef MUNMAP_BEFORE_RENAME
of not. The changes are now bigger that in original patch.

The fix to index.c prevents circular list. I got infinite loop in
cache_file_list
every time I tried more than 1 patch on commandline of git-apply. I tried
other solution with the function below, but what I put in the atached patch
is shorter than the alternative below.

Yakov

P.S. I am attaching not inlining bacause
gmail totally removes leading tabs from inline text.

P.P.S.
Alternate fix for index.c:

static void clean_cache_file_list(struct cache_file *cf)
{
        struct cache_file *ppcf = &cache_file_list;
        cf->lockfile[0] = 0;
        while( *ppcf ) {
            if(*ppcf == cf ) {
                *ppcf = cf->next;
            } else
                ppcf = &(cf->next);
        }
}
-         cf->lockfile[0] = 0;
+        clean_cache_file_list(cf);
-         cf->lockfile[0] = 0;
+        clean_cache_file_list(cf);

[-- Attachment #2: patch2-unmap-before-rename --]
[-- Type: application/octet-stream, Size: 2712 bytes --]

--- cache.h.000	2006-05-08 16:59:09.000000000 +0000
+++ cache.h	2006-05-08 16:42:15.000000000 +0000
@@ -141,6 +141,7 @@
 
 /* Initialize and use the cache information */
 extern int read_cache(void);
+extern void unmap_cache(void);
 extern int write_cache(int newfd, struct cache_entry **cache, int entries);
 extern int cache_name_pos(const char *name, int namelen);
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
@@ -161,6 +162,7 @@
 struct cache_file {
 	struct cache_file *next;
 	char lockfile[PATH_MAX];
+	int enlisted;
 };
 extern int hold_index_file_for_update(struct cache_file *, const char *path);
 extern int commit_index_file(struct cache_file *);
--- index.c.000	2006-05-08 16:19:22.000000000 +0000
+++ index.c	2006-05-08 16:57:14.000000000 +0000
@@ -5,6 +5,7 @@
 #include "cache.h"
 
 static struct cache_file *cache_file_list;
+static int cleanup_installed = 0;
 
 static void remove_lock_file(void)
 {
@@ -27,11 +28,17 @@
 	int fd;
 	sprintf(cf->lockfile, "%s.lock", path);
 	fd = open(cf->lockfile, O_RDWR | O_CREAT | O_EXCL, 0666);
-	if (fd >=0 && !cf->next) {
-		cf->next = cache_file_list;
-		cache_file_list = cf;
-		signal(SIGINT, remove_lock_file_on_signal);
-		atexit(remove_lock_file);
+	if (fd >=0) {
+		if (!cf->enlisted) {
+			cf->next = cache_file_list;
+			cache_file_list = cf;
+			cf->enlisted = 1;
+		}
+		if (!cleanup_installed) {
+			signal(SIGINT, remove_lock_file_on_signal);
+			atexit(remove_lock_file);
+			cleanup_installed = 1;
+		}
 	}
 	return fd;
 }
@@ -43,6 +50,9 @@
 	strcpy(indexfile, cf->lockfile);
 	i = strlen(indexfile) - 5; /* .lock */
 	indexfile[i] = 0;
+//#ifdef MUMNAP_BEFORE_RENAME
+	unmap_cache();
+//#endif
 	i = rename(cf->lockfile, indexfile);
 	cf->lockfile[0] = 0;
 	return i;
--- read-cache.c.000	2006-05-08 17:01:28.000000000 +0000
+++ read-cache.c	2006-05-08 17:02:24.000000000 +0000
@@ -513,6 +513,9 @@
 	return 0;
 }
 
+static void *mapaddr = MAP_FAILED;
+static unsigned long mapsize;
+
 int read_cache(void)
 {
 	int fd, i;
@@ -541,6 +544,8 @@
 		errno = EINVAL;
 		if (size >= sizeof(struct cache_header) + 20)
 			map = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+		mapaddr = map;
+		mapsize = size;
 	}
 	close(fd);
 	if (map == MAP_FAILED)
@@ -565,10 +570,23 @@
 
 unmap:
 	munmap(map, size);
+	mapaddr = MAP_FAILED;
 	errno = EINVAL;
 	die("index file corrupt");
 }
 
+void unmap_cache(void)
+{
+	if ( mapaddr != MAP_FAILED ) {
+		munmap(mapaddr, mapsize);
+		mapaddr = MAP_FAILED;
+	}
+	active_cache_changed = 0;
+	active_nr = 0;
+	free(active_cache);
+	active_cache = NULL;
+}
+
 #define WRITE_BUFFER_SIZE 8192
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;

Re: [patch] munmap-before-rename, cygwin need

[Junio C Hamano]

"Yakov Lerner" <iler.ml@gmail.com> writes:

> You are right. Apply did not work when I gave it more than one
> patchfile on commandline (and --index option). I fixed this by
> zeroing active_nr and freeing active_cache in
> unmap_cache().

I suspect that essentially means you are forcing the cache to be
read again after writing it out, in which case I think you are
better off using NO_MMAP as Alex suggests.

Re: [patch] munmap-before-rename, cygwin need

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> You really want  "NO_MMAP = YesPlease" in your config.mak.
> Take a look into compat/mmap.c. That's why Junio has never seen
> the breakage.

I do not have custom config.mak for my Cygwin builds and NO_MMAP
is disabled in the default Makefile, so that does not explain
why it does not break for me.  Very puzzled.