Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-05 06:57, Max Kirillov wrote:
> On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:
>> I guess then we would need two different patches for the
>> two different fixes, at least.
>>
>> So now I am unsure how to proceed: I do not want to step
>> on your toes, but I also want to see my use case fixed and
>> I want to move forward on this. At the moment, it looks as
>> if we are at an impasse.
> 
> Just continue with your patches then. I do not hurry.

Okay!
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-02 21:21, Max Kirillov wrote:
> On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:
>> On 2015-10-02 12:05, Johannes Schindelin wrote:
>>
>> > On 2015-10-01 05:29, Max Kirillov wrote:
 When a builtin has done its job, but waits for pager or not waited
 by its caller and still hanging it keeps pack files opened.
 This can cause a number of issues, for example on Windows git gc
 cannot remove the packs.
>>
>> Could you do me another favor? It seems that you want to
>> work on this, so I will step back (I have to take off for
>> the weekend very soon anyway, so I am really glad that you
>> take care of it). But I would really love to see the line
> 
> As I explained in other message, your case is a bit
> different.

I guess then we would need two different patches for the two different fixes, 
at least.

So now I am unsure how to proceed: I do not want to step on your toes, but I 
also want to see my use case fixed and I want to move forward on this. At the 
moment, it looks as if we are at an impasse.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote:
> I guess then we would need two different patches for the
> two different fixes, at least.
> 
> So now I am unsure how to proceed: I do not want to step
> on your toes, but I also want to see my use case fixed and
> I want to move forward on this. At the moment, it looks as
> if we are at an impasse.

Just continue with your patches then. I do not hurry.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-01 05:29, Max Kirillov wrote:
> When a builtin has done its job, but waits for pager or not waited
> by its caller and still hanging it keeps pack files opened.
> This can cause a number of issues, for example on Windows git gc
> cannot remove the packs.

I did not experience that issue. In any case, closing the packs after the 
builtin function has returned might not change anything anyway because we are 
about to `exit()` and that's that.

So I would like to skip this:

> diff --git a/git.c b/git.c
> index 5feba41..ad34680 100644
> --- a/git.c
> +++ b/git.c
> @@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int
> argc, const char **argv)
>   trace_argv_printf(argv, "trace: built-in: git");
>  
>   status = p->fn(argc, argv, prefix);
> + close_all_packs();
>   if (status)
>   return status;
>  

Also, I would move the new declaration directly before `close_pack_windows()`:

> diff --git a/cache.h b/cache.h
> index 79066e5..153bc46 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
>  extern void free_pack_by_name(const char *);
>  extern void clear_delta_base_cache(void);
>  extern struct packed_git *add_packed_git(const char *, int, int);
> +extern void close_all_packs(void);
>  
>  /*
>   * Return the SHA-1 of the nth object within the specified packfile.

The convention in Git seems to call things _gently rather than _nodie:

> diff --git a/sha1_file.c b/sha1_file.c
> index 08302f5..62f1dad 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
>   return ret;
>  }
>  
> -void close_pack_windows(struct packed_git *p)
> +static int close_pack_windows_nodie(struct packed_git *p)
>  {
>   while (p->windows) {
>   struct pack_window *w = p->windows;
>  
>   if (w->inuse_cnt)
> - die("pack '%s' still has open windows to it",
> - p->pack_name);
> + return 1;
> +
>   munmap(w->base, w->len);
>   pack_mapped -= w->len;
>   pack_open_windows--;
>   p->windows = w->next;
>   free(w);
>   }
> +
> + return 0;
> +}

And while we're at it, why not teach that function a new parameter `int 
close_pack_fd`?

There is another problem: when we cannot close the pack window, we cannot 
really continue, can we? Because if we do, we *still* have the lock, and we'll 
just fail later, most likely with an unhelpful error message because we do not 
know where the pack closing failed. I do not think that the warning really 
helps...

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Johannes Schindelin]

Hi Max,

On 2015-10-02 12:05, Johannes Schindelin wrote:

> On 2015-10-01 05:29, Max Kirillov wrote:
>> When a builtin has done its job, but waits for pager or not waited
>> by its caller and still hanging it keeps pack files opened.
>> This can cause a number of issues, for example on Windows git gc
>> cannot remove the packs.

Could you do me another favor? It seems that you want to work on this, so I 
will step back (I have to take off for the weekend very soon anyway, so I am 
really glad that you take care of it). But I would really love to see the line

This fixes https://github.com/git-for-windows/git/issues/446

in the commit message, as this will keep the connection between the fix and the 
original report.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 12:05:55PM +0200, Johannes Schindelin wrote:
> Hi Max,
> 
> On 2015-10-01 05:29, Max Kirillov wrote:
>> When a builtin has done its job, but waits for pager or not waited
>> by its caller and still hanging it keeps pack files opened.
>> This can cause a number of issues, for example on Windows git gc
>> cannot remove the packs.
> 
> I did not experience that issue. In any case, closing the
> packs after the builtin function has returned might not
> change anything anyway because we are about to `exit()`
> and that's that.

Steps to reproduce:
1. install the latest git-for-windows, 2.6.0.windows.1
2. run the script in the end of message
3. remember the directory name, press the enter as it asks
4. in the less scrool down

Then inspect the processes git.exe and less.exe with Process
Explorer of something you find convenient. Both processes
should have opened the pack file, and git.exe should have it
mapped (I found it in the "View"->"Lower Pane views"->"Dlls").

Then, if you run another bash window, cd to the repository
and run "git repack -a -d", it should print:


$ git repack -a -d
Counting objects: 3001, done.
Compressing objects: 100% (1003/1003), done.
Writing objects: 100% (3001/3001), done.
Total 3001 (delta 997), reused 3001 (delta 997)
Unlink of file 
'.git/objects/pack/pack-e1b0e3ac01ff8d79a77648de3370f49b93c58a8b.pack' failed. 
Should I try again? (y/n)


I would like the git gc to succeed, because I run it as a
scheduled task nightly and feel a bit annoyed by the opened
windows which wait for me to say yes (after exiting pager).

So, the fix is needed approximately in that place, after
running any builtin command.

For your case, I think, the same close_all_packs() should be
invoked before the repacking.

> The convention in Git seems to call things _gently rather
> than _nodie:

Thanks, will change it if the idea is welcomed.

>> -void close_pack_windows(struct packed_git *p)
>> +static int close_pack_windows_nodie(struct packed_git *p)
>>  {
>>  while (p->windows) {
>>  struct pack_window *w = p->windows;
>>  
>>  if (w->inuse_cnt)
>> -die("pack '%s' still has open windows to it",
>> -p->pack_name);
>> +return 1;
>> +
>>  munmap(w->base, w->len);
>>  pack_mapped -= w->len;
>>  pack_open_windows--;
>>  p->windows = w->next;
>>  free(w);
>>  }
>> +
>> +return 0;
>> +}
> 
> And while we're at it, why not teach that function a new
> parameter `int close_pack_fd`?

I think "close windows" should close windows, if it also
closes pack fd probably should be another name. But current
code seems quite logical. Close the packs, and run closing
windows from it.

> There is another problem: when we cannot close the pack
> window, we cannot really continue, can we?

Yes we can, unlocking of the window is not needed for the
current process to do what it intended to do, it would just
interfere with concurrent git gc.

For the clone case probably die would be appropriate. If you
feel like it worth complicating the code we might search for
some solution.

-- 
Max

The script:
- ./t-windows-pack-close.sh -
#!/bin/sh

set -e

TEST_DIR=`mktemp -d`

t_git() {
git --work-tree="$TEST_DIR" --git-dir="$TEST_DIR/.git" "$@"
}

t_git init

for i in $(seq 1000)
do
echo "commit$i" >"$TEST_DIR/commit.$i"
t_git add "commit.$i"
t_git commit -m "commit$i" -q
done

t_git repack

pack_path=$(find "$TEST_DIR/.git/objects/pack" -name "pack-.pack")

echo "dir: $TEST_DIR"
echo press enter to start git log
read

t_git -c core.packedGitWindowSize=100 log
-
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote:
> Hi Max,
> 
> On 2015-10-02 12:05, Johannes Schindelin wrote:
> 
> > On 2015-10-01 05:29, Max Kirillov wrote:
>>> When a builtin has done its job, but waits for pager or not waited
>>> by its caller and still hanging it keeps pack files opened.
>>> This can cause a number of issues, for example on Windows git gc
>>> cannot remove the packs.
> 
> Could you do me another favor? It seems that you want to
> work on this, so I will step back (I have to take off for
> the weekend very soon anyway, so I am really glad that you
> take care of it). But I would really love to see the line

As I explained in other message, your case is a bit
different.

I could add another call of close_all_packs() as a separate
commit with the "fixes" link, but I'm not so sure about it
if it turns out that additional efforts are required.

-- 
Max
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

On Fri, Oct 02, 2015 at 10:06:46PM +0300, Max Kirillov wrote:
> for i in $(seq 1000)
> t_git -c core.packedGitWindowSize=100 log

It was 32-bit build.
I cannot promise those exactly numbers will work, because I don not clearly
understand what do they mean. With 100 commits the pack size was 20K, but it
was mapped fully with the window=100.

But I believe with the same build same numbers should reproduce the issue.

For 32-bit builds I can see it on any significantly big repository, like the
git itself. Maybe for 64bit it is less likely.

The code which decides whether to close the pack is in use_pack() in 
sha1_file.c:
---
if (!win->offset && win->len == p->pack_size
&& !p->do_not_close) {
close(p->pack_fd);
pack_open_fds--;
p->pack_fd = -1;
}
---
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 1/2] sha1_file: close all pack files after running

[Max Kirillov]

When a builtin has done its job, but waits for pager or not waited
by its caller and still hanging it keeps pack files opened.
This can cause a number of issues, for example on Windows git gc
cannot remove the packs.

Fix this by explicitly closing all pack files and unmapping memory
from the packs after running builtin. Do not die() if the memory region
is still used though, just print a warning, because failure to close
a file should not prevent the currently running program from finishing
its task.

Signed-off-by: Max Kirillov 
---
 cache.h |  1 +
 git.c   |  2 ++
 sha1_file.c | 32 +---
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 79066e5..153bc46 100644
--- a/cache.h
+++ b/cache.h
@@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
 extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *, int, int);
+extern void close_all_packs(void);
 
 /*
  * Return the SHA-1 of the nth object within the specified packfile.
diff --git a/git.c b/git.c
index 5feba41..ad34680 100644
--- a/git.c
+++ b/git.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "cache.h"
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
@@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int argc, 
const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
 
status = p->fn(argc, argv, prefix);
+   close_all_packs();
if (status)
return status;
 
diff --git a/sha1_file.c b/sha1_file.c
index 08302f5..62f1dad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
return ret;
 }
 
-void close_pack_windows(struct packed_git *p)
+static int close_pack_windows_nodie(struct packed_git *p)
 {
while (p->windows) {
struct pack_window *w = p->windows;
 
if (w->inuse_cnt)
-   die("pack '%s' still has open windows to it",
-   p->pack_name);
+   return 1;
+
munmap(w->base, w->len);
pack_mapped -= w->len;
pack_open_windows--;
p->windows = w->next;
free(w);
}
+
+   return 0;
+}
+
+void close_pack_windows(struct packed_git *p)
+{
+   if (close_pack_windows_nodie(p))
+   die("pack '%s' still has open windows to it", p->pack_name);
 }
 
 /*
@@ -866,6 +874,24 @@ static int close_one_pack(void)
return 0;
 }
 
+void close_all_packs(void)
+{
+   struct packed_git *p = NULL;
+
+   for (p = packed_git; p; p = p->next) {
+   if (close_pack_windows_nodie(p))
+   warning("pack '%s' still has open windows to it", 
p->pack_name);
+
+   if (p->pack_fd != -1) {
+   if (close(p->pack_fd) != 0)
+   warning("close(%s) failed: %d", p->pack_name, 
errno);
+   p->pack_fd = -1;
+   }
+
+   close_pack_index(p);
+   }
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
struct pack_window *w = *w_cursor;
-- 
2.3.4.2801.g3d0809b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html