{"thread":{"id":"4097","subject":"[patch] munmap-before-rename, cygwin need","startedAt":"2006-05-07T19:58:48Z","lastAt":"2006-05-08T20:52:28Z","messageCount":6,"participants":["Yakov Lerner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"19662","messageId":"f36b08ee0605071258s7a0cb085r3f08e9981234255a@mail.gmail.com","threadId":"4097","inReplyTo":null,"subject":"[patch] munmap-before-rename, cygwin need","fromName":"Yakov Lerner","fromEmail":"iler.ml@gmail.com","sentAt":"2006-05-07T19:58:48Z","receivedAt":"2006-05-07T19:58:48Z","isPatch":true,"sender":{"key":"iler.ml@gmail.com","avatar":null},"body":"I found that mmap() works on cygwin, but needs a patch.\nOn Cygwin, rename() fails if target file has active mmap().\nThe patch below adds  munmap() before rename().\n\nIf you excuse me for posting not in git-generated format\n(I did not yet completely make git working on my cygwin).\nI'm adding the copy as attachment, I'm afraid the mailer\nmay interfere with spaces ?\n\nYakov Lerner\n\nMakefile     |    5 +++++\ncache.h      |    1 +\nindex.c      |    3 +++\nread-cache.c |   14 ++++++++++++++\n\n--- Makefile.000 2006-05-07 22:32:04.000000000 +0300\n+++ Makefile 2006-05-07 22:30:38.000000000 +0300\n@@ -46,6 +46,9 @@\n#\n# Define NO_MMAP if you want to avoid mmap.\n#\n+# Define MUMNAP_BEFORE_RENAME if munmap() on target file is required\n+# before rename().\n+#\n# Define WITH_OWN_SUBPROCESS_PY if you want to use with python 2.3.\n#\n# Define NO_IPV6 if you lack IPv6 support and getaddrinfo().\n@@ -270,6 +273,8 @@\n# NO_MMAP = YesPlease\nNO_IPV6 = YesPlease\nX = .exe\n+ MUMNAP_BEFORE_RENAME = YesPlease\n+ NO_SOCKADDR_STORAGE = YesPlease\nendif\nifeq ($(uname_S),FreeBSD)\nNEEDS_LIBICONV = YesPlease\n--- cache.h.000 2006-05-02 11:35:50.000000000 +0300\n+++ cache.h 2006-05-02 11:33:34.000000000 +0300\n@@ -140,6 +140,7 @@\n\n/* Initialize and use the cache information */\nextern int read_cache(void);\n+extern void unmap_cache(void);\nextern int write_cache(int newfd, struct cache_entry **cache, int entries);\nextern int cache_name_pos(const char *name, int namelen);\n#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */\n--- read-cache.c.000 2006-05-07 22:33:42.000000000 +0300\n+++ read-cache.c 2006-05-02 11:32:56.000000000 +0300\n@@ -513,6 +513,9 @@\nreturn 0;\n}\n\n+static void *mapaddr = MAP_FAILED;\n+static unsigned long mapsize;\n+\nint read_cache(void)\n{\nint fd, i;\n@@ -541,6 +544,8 @@\nerrno = EINVAL;\nif (size >= sizeof(struct cache_header) + 20)\nmap = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+ mapaddr = map;\n+ mapsize = size;\n}\nclose(fd);\nif (map == MAP_FAILED)\n@@ -565,10 +570,19 @@\n\nunmap:\nmunmap(map, size);\n+ mapaddr = MAP_FAILED;\nerrno = EINVAL;\ndie(\"index file corrupt\");\n}\n\n+void unmap_cache(void)\n+{\n+ if ( mapaddr != MAP_FAILED ) {\n+ munmap(mapaddr, mapsize);\n+ mapaddr = MAP_FAILED;\n+ }\n+}\n+\n#define WRITE_BUFFER_SIZE 8192\nstatic unsigned char write_buffer[WRITE_BUFFER_SIZE];\nstatic unsigned long write_buffer_len;\n--- index.c.000 2006-05-07 22:36:04.000000000 +0300\n+++ index.c 2006-05-07 22:36:40.000000000 +0300\n@@ -43,6 +43,9 @@\nstrcpy(indexfile, cf->lockfile);\ni = strlen(indexfile) - 5; /* .lock */\nindexfile[i] = 0;\n+#ifdef MUMNAP_BEFORE_RENAME\n+ unmap_cache();\n+#endif\ni = rename(cf->lockfile, indexfile);\ncf->lockfile[0] = 0;\nreturn i;\n\n\n--- Makefile.000\t2006-05-07 22:32:04.000000000 +0300\n+++ Makefile\t2006-05-07 22:30:38.000000000 +0300\n@@ -46,6 +46,9 @@\n #\n # Define NO_MMAP if you want to avoid mmap.\n #\n+# Define MUMNAP_BEFORE_RENAME if munmap() on target file is required\n+# before rename().\n+#\n # Define WITH_OWN_SUBPROCESS_PY if you want to use with python 2.3.\n #\n # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().\n@@ -270,6 +273,8 @@\n \t# NO_MMAP = YesPlease\n \tNO_IPV6 = YesPlease\n \tX = .exe\n+\tMUMNAP_BEFORE_RENAME = YesPlease\n+\tNO_SOCKADDR_STORAGE = YesPlease\n endif\n ifeq ($(uname_S),FreeBSD)\n \tNEEDS_LIBICONV = YesPlease\n--- cache.h.000\t2006-05-02 11:35:50.000000000 +0300\n+++ cache.h\t2006-05-02 11:33:34.000000000 +0300\n@@ -140,6 +140,7 @@\n \n /* Initialize and use the cache information */\n extern int read_cache(void);\n+extern void unmap_cache(void);\n extern int write_cache(int newfd, struct cache_entry **cache, int entries);\n extern int cache_name_pos(const char *name, int namelen);\n #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\n--- read-cache.c.000\t2006-05-07 22:33:42.000000000 +0300\n+++ read-cache.c\t2006-05-02 11:32:56.000000000 +0300\n@@ -513,6 +513,9 @@\n \treturn 0;\n }\n \n+static void *mapaddr = MAP_FAILED;\n+static unsigned long mapsize;\n+\n int read_cache(void)\n {\n \tint fd, i;\n@@ -541,6 +544,8 @@\n \t\terrno = EINVAL;\n \t\tif (size >= sizeof(struct cache_header) + 20)\n \t\t\tmap = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);\n+\t\tmapaddr = map;\n+\t\tmapsize = size;\n \t}\n \tclose(fd);\n \tif (map == MAP_FAILED)\n@@ -565,10 +570,19 @@\n \n unmap:\n \tmunmap(map, size);\n+\tmapaddr = MAP_FAILED;\n \terrno = EINVAL;\n \tdie(\"index file corrupt\");\n }\n \n+void unmap_cache(void)\n+{\n+\tif ( mapaddr != MAP_FAILED ) {\n+\t\tmunmap(mapaddr, mapsize);\n+\t\tmapaddr = MAP_FAILED;\n+\t}\n+}\n+\n #define WRITE_BUFFER_SIZE 8192\n static unsigned char write_buffer[WRITE_BUFFER_SIZE];\n static unsigned long write_buffer_len;\n--- index.c.000\t2006-05-07 22:36:04.000000000 +0300\n+++ index.c\t2006-05-07 22:36:40.000000000 +0300\n@@ -43,6 +43,9 @@\n \tstrcpy(indexfile, cf->lockfile);\n \ti = strlen(indexfile) - 5; /* .lock */\n \tindexfile[i] = 0;\n+#ifdef MUMNAP_BEFORE_RENAME\n+\tunmap_cache();\n+#endif\n \ti = rename(cf->lockfile, indexfile);\n \tcf->lockfile[0] = 0;\n \treturn i;\n"},{"id":"19664","messageId":"7vslnlk04v.fsf@assigned-by-dhcp.cox.net","threadId":"4097","inReplyTo":"f36b08ee0605071258s7a0cb085r3f08e9981234255a@mail.gmail.com","subject":"Re: [patch] munmap-before-rename, cygwin need","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-07T21:14:56Z","receivedAt":"2006-05-07T21:14:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Yakov Lerner\" <iler.ml@gmail.com> writes:\n\n> I found that mmap() works on cygwin, but needs a patch.\n> On Cygwin, rename() fails if target file has active mmap().\n> The patch below adds  munmap() before rename().\n\nThis is interesting in three counts.\n\n - I from time to time test Cygwin version on my day-job machine\n   (W2K) and my wife's machine (XP); on both machines I usually\n   have less than two weeks old Cygwin installation, and I have\n   not seen the breakage.  I wonder how reproducible this is.\n   Also previously people reported mmap() works for some and\n   fake mmap is needed for others.  Would this patch make things\n   work for everybody?\n\n - The part you patched is commit_index_file().  This typically\n   is called just before program exit, but some callers, like\n   apply.c, may want to still look at the index after calling\n   it, fully aware that the changes after commit_index will not\n   be written out.  Although I haven't traced the codepath fully\n   in apply.c yet, unmapping would break the access to the index\n   (i.e. active_cache[]).  Does apply still work with your\n   patch?\n\n - As long as you can clear the second point, I do not see a\n   particular reason to make this an option; we should be able\n   to do so everywhere.\n"},{"id":"19708","messageId":"f36b08ee0605080358y3830f1aep879331df806211e0@mail.gmail.com","threadId":"4097","inReplyTo":"7vslnlk04v.fsf@assigned-by-dhcp.cox.net","subject":"Re: [patch] munmap-before-rename, cygwin need","fromName":"Yakov Lerner","fromEmail":"iler.ml@gmail.com","sentAt":"2006-05-08T10:58:00Z","receivedAt":"2006-05-08T10:58:00Z","isPatch":true,"sender":{"key":"iler.ml@gmail.com","avatar":null},"body":"On 5/8/06, Junio C Hamano <junkio@cox.net> wrote:\n> \"Yakov Lerner\" <iler.ml@gmail.com> writes:\n>\n> > I found that mmap() works on cygwin, but needs a patch.\n> > On Cygwin, rename() fails if target file has active mmap().\n> > The patch below adds  munmap() before rename().\n>\n> This is interesting in three counts.\n>\n>  - I from time to time test Cygwin version on my day-job machine\n>   (W2K) and my wife's machine (XP); on both machines I usually\n>   have less than two weeks old Cygwin installation, and I have\n>   not seen the breakage.  I wonder how reproducible this is.\n>   Also previously people reported mmap() works for some and\n>   fake mmap is needed for others.  Would this patch make things\n>   work for everybody?\n>\n>  - The part you patched is commit_index_file().  This typically\n>   is called just before program exit, but some callers, like\n>   apply.c, may want to still look at the index after calling\n>   it, fully aware that the changes after commit_index will not\n>   be written out.  Although I haven't traced the codepath fully\n>   in apply.c yet, unmapping would break the access to the index\n>   (i.e. active_cache[]).  Does apply still work with your\n>   patch?\n\nI am checking this. I am trying different options and different\nscenarios.\n\nYakov\n"},{"id":"19713","messageId":"f36b08ee0605080747r24668152t20cc406e017454a9@mail.gmail.com","threadId":"4097","inReplyTo":"7vslnlk04v.fsf@assigned-by-dhcp.cox.net","subject":"Re: [patch] munmap-before-rename, cygwin need","fromName":"Yakov Lerner","fromEmail":"iler.ml@gmail.com","sentAt":"2006-05-08T14:47:01Z","receivedAt":"2006-05-08T14:47:01Z","isPatch":true,"sender":{"key":"iler.ml@gmail.com","avatar":null},"body":"On 5/7/06, Junio C Hamano <junkio@cox.net> wrote:\n> \"Yakov Lerner\" <iler.ml@gmail.com> writes:\n> > I found that mmap() works on cygwin, but needs a patch.\n> > On Cygwin, rename() fails if target file has active mmap().\n> > The patch below adds  munmap() before rename().\n> This is interesting in three counts.\n>\n>  - I from time to time test Cygwin version on my day-job machine\n>    (W2K) and my wife's machine (XP); on both machines I usually\n>    have less than two weeks old Cygwin installation, and I have\n>    not seen the breakage.  I wonder how reproducible this is.\n>    Also previously people reported mmap() works for some and\n>    fake mmap is needed for others.  Would this patch make things\n>    work for everybody?\n>\n>  - The part you patched is commit_index_file().  This typically\n>    is called just before program exit, but some callers, like\n>    apply.c, may want to still look at the index after calling\n>    it, fully aware that the changes after commit_index will not\n>    be written out.  Although I haven't traced the codepath fully\n>    in apply.c yet, unmapping would break the access to the index\n>    (i.e. active_cache[]).  Does apply still work with your\n>    patch?\n\nYou are right. Apply did not work when I gave it more than one patchfile on\ncommandline (and --index option). I fixed this by zeroing active_nr and freeing\nactive_cache in unmap_cache(). Then I got infinite loop in\nremove_lock_file (after multiple calls to hold_index_file_for_update\nwith same cf, cache_file_list points to cf and cf->next points to\ncf creating infinite loop.) The fix in index.c is easy.\n\nThe patch below works for me. However, it changes internal\nworking of apply.c in the scenario 'git-apply --index patch1 patch2 ...'.\n\n(1) With the patch below, apply.c repeats mmap() on index after every patch\nargument (because index gets unmapped after every patchfile argument).\n\n(2) Current apply.c does single mmap() at the beginning. It modfies index\non disk and cache in memory and it does not repeat mmap. This mmap\nis to original (now deleted) index, if i understand correctly (the\nno-name inode).\n\nBut (2) this does not work in cygwin. The end results of (1) and\n(2) are the same, I think. (2) looks to me bit faster (I didn't do\nmeasurements).\n\nIt's up to you whether to make it under #ifdef MUNMAP_BEFORE_RENAME\nof not. The changes are now bigger that in original patch.\n\nThe fix to index.c prevents circular list. I got infinite loop in\ncache_file_list\nevery time I tried more than 1 patch on commandline of git-apply. I tried\nother solution with the function below, but what I put in the atached patch\nis shorter than the alternative below.\n\nYakov\n\nP.S. I am attaching not inlining bacause\ngmail totally removes leading tabs from inline text.\n\nP.P.S.\nAlternate fix for index.c:\n\nstatic void clean_cache_file_list(struct cache_file *cf)\n{\n        struct cache_file *ppcf = &cache_file_list;\n        cf->lockfile[0] = 0;\n        while( *ppcf ) {\n            if(*ppcf == cf ) {\n                *ppcf = cf->next;\n            } else\n                ppcf = &(cf->next);\n        }\n}\n-         cf->lockfile[0] = 0;\n+        clean_cache_file_list(cf);\n-         cf->lockfile[0] = 0;\n+        clean_cache_file_list(cf);\n"},{"id":"19722","messageId":"7vmzdsfdfl.fsf@assigned-by-dhcp.cox.net","threadId":"4097","inReplyTo":"f36b08ee0605080747r24668152t20cc406e017454a9@mail.gmail.com","subject":"Re: [patch] munmap-before-rename, cygwin need","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-08T20:51:10Z","receivedAt":"2006-05-08T20:51:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Yakov Lerner\" <iler.ml@gmail.com> writes:\n\n> You are right. Apply did not work when I gave it more than one\n> patchfile on commandline (and --index option). I fixed this by\n> zeroing active_nr and freeing active_cache in\n> unmap_cache().\n\nI suspect that essentially means you are forcing the cache to be\nread again after writing it out, in which case I think you are\nbetter off using NO_MMAP as Alex suggests.\n"},{"id":"19723","messageId":"7vfyjkfddf.fsf@assigned-by-dhcp.cox.net","threadId":"4097","inReplyTo":"81b0412b0605080635r40868f18ua88b33558368f82b@mail.gmail.com","subject":"Re: [patch] munmap-before-rename, cygwin need","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-08T20:52:28Z","receivedAt":"2006-05-08T20:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" <raa.lkml@gmail.com> writes:\n\n> You really want  \"NO_MMAP = YesPlease\" in your config.mak.\n> Take a look into compat/mmap.c. That's why Junio has never seen\n> the breakage.\n\nI do not have custom config.mak for my Cygwin builds and NO_MMAP\nis disabled in the default Makefile, so that does not explain\nwhy it does not break for me.  Very puzzled.\n"}]}