git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] Increase core.packedGitLimit
@ 2017-04-20 20:41 David Turner
  2017-04-20 21:02 ` Jeff King
  0 siblings, 1 reply; 5+ messages in thread
From: David Turner @ 2017-04-20 20:41 UTC (permalink / raw)
  To: git; +Cc: David Turner

When core.packedGitLimit is exceeded, git will close packs.  If there
is a repack operation going on in parallel with a fetch, the fetch
might open a pack, and then be forced to close it due to
packedGitLimit being hit.  The repack could then delete the pack
out from under the fetch, causing the fetch to fail.

Increase core.packedGitLimit's default value to prevent
this.

On current 64-bit x86_64 machines, 48 bits of address space are
available.  It appears that 64-bit ARM machines have no standard
amount of address space (that is, it varies by manufacturer), and IA64
and POWER machines have the full 64 bits.  So 48 bits is the only
limit that we can reasonably care about.  We reserve a few bits of the
48-bit address space for the kernel's use (this is not strictly
necessary, but it's better to be safe), and use up to the remaining
45.  No git repository will be anywhere near this large any time soon,
so this should prevent the failure.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: David Turner <dturner@twosigma.com>
---
 git-compat-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..1c5de153a5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -616,7 +616,7 @@ extern int git_lstat(const char *, struct stat *);
 #endif
 
 #define DEFAULT_PACKED_GIT_LIMIT \
-	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
+	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? (32 * 1024L * 1024L) : 256))
 
 #ifdef NO_PREAD
 #define pread git_pread
-- 
2.11.GIT


^ permalink raw reply	[flat|threaded] 5+ messages in thread

* Re: [PATCH] Increase core.packedGitLimit
  2017-04-20 20:41 [PATCH] Increase core.packedGitLimit David Turner
@ 2017-04-20 21:02 ` Jeff King
  2017-04-20 21:58   ` Johannes Schindelin
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2017-04-20 21:02 UTC (permalink / raw)
  To: David Turner; +Cc: git

On Thu, Apr 20, 2017 at 04:41:18PM -0400, David Turner wrote:

> When core.packedGitLimit is exceeded, git will close packs.  If there
> is a repack operation going on in parallel with a fetch, the fetch
> might open a pack, and then be forced to close it due to
> packedGitLimit being hit.  The repack could then delete the pack
> out from under the fetch, causing the fetch to fail.
> 
> Increase core.packedGitLimit's default value to prevent
> this.
> 
> On current 64-bit x86_64 machines, 48 bits of address space are
> available.  It appears that 64-bit ARM machines have no standard
> amount of address space (that is, it varies by manufacturer), and IA64
> and POWER machines have the full 64 bits.  So 48 bits is the only
> limit that we can reasonably care about.  We reserve a few bits of the
> 48-bit address space for the kernel's use (this is not strictly
> necessary, but it's better to be safe), and use up to the remaining
> 45.  No git repository will be anywhere near this large any time soon,
> so this should prevent the failure.

Yep, I think this is a reasonable direction.

> ---
>  git-compat-util.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This probably needs an update to the core.packedGitLimit section of
Documentation/config.txt.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 8a4a3f85e7..1c5de153a5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -616,7 +616,7 @@ extern int git_lstat(const char *, struct stat *);
>  #endif
>  
>  #define DEFAULT_PACKED_GIT_LIMIT \
> -	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
> +	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? (32 * 1024L * 1024L) : 256))

I wondered if we would run afoul of integer sizes on 64-bit systems where
"long" is still only 32-bits (i.e., Windows). But I think it's OK,
because the values before we cast to size_t are in megabytes. So your
32*1024*1024 needs only 25 bits to store it. And then after we cast to
size_t, everything is in 64-bit.

-Peff

^ permalink raw reply	[flat|threaded] 5+ messages in thread

* Re: [PATCH] Increase core.packedGitLimit
  2017-04-20 21:02 ` Jeff King
@ 2017-04-20 21:58   ` Johannes Schindelin
  2017-04-20 22:04     ` David Turner
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Schindelin @ 2017-04-20 21:58 UTC (permalink / raw)
  To: Jeff King; +Cc: David Turner, git

Hi Peff,

On Thu, 20 Apr 2017, Jeff King wrote:

> On Thu, Apr 20, 2017 at 04:41:18PM -0400, David Turner wrote:
> 
> > When core.packedGitLimit is exceeded, git will close packs.  If there
> > is a repack operation going on in parallel with a fetch, the fetch
> > might open a pack, and then be forced to close it due to
> > packedGitLimit being hit.  The repack could then delete the pack
> > out from under the fetch, causing the fetch to fail.
> > 
> > Increase core.packedGitLimit's default value to prevent
> > this.
> > 
> > On current 64-bit x86_64 machines, 48 bits of address space are
> > available.  It appears that 64-bit ARM machines have no standard
> > amount of address space (that is, it varies by manufacturer), and IA64
> > and POWER machines have the full 64 bits.  So 48 bits is the only
> > limit that we can reasonably care about.  We reserve a few bits of the
> > 48-bit address space for the kernel's use (this is not strictly
> > necessary, but it's better to be safe), and use up to the remaining
> > 45.  No git repository will be anywhere near this large any time soon,
> > so this should prevent the failure.
> 
> Yep, I think this is a reasonable direction.
> 
> > ---
> >  git-compat-util.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This probably needs an update to the core.packedGitLimit section of
> Documentation/config.txt.
> 
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 8a4a3f85e7..1c5de153a5 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -616,7 +616,7 @@ extern int git_lstat(const char *, struct stat *);
> >  #endif
> >  
> >  #define DEFAULT_PACKED_GIT_LIMIT \
> > -	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
> > +	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? (32 * 1024L * 1024L) : 256))
> 
> I wondered if we would run afoul of integer sizes on 64-bit systems where
> "long" is still only 32-bits (i.e., Windows). But I think it's OK,
> because the values before we cast to size_t are in megabytes. So your
> 32*1024*1024 needs only 25 bits to store it. And then after we cast to
> size_t, everything is in 64-bit.

Indeed, when I patch a local Git checkout accordingly, I see that
packed_git_limit is set to 35184372088832.

The bigger problem in this regard is that users are allowed to override
this via core.packedgitlimit but that value is parsed as an unsigned long.

Ciao,
Dscho

^ permalink raw reply	[flat|threaded] 5+ messages in thread

* RE: [PATCH] Increase core.packedGitLimit
  2017-04-20 21:58   ` Johannes Schindelin
@ 2017-04-20 22:04     ` David Turner
  2017-04-21  9:34       ` Johannes Schindelin
  0 siblings, 1 reply; 5+ messages in thread
From: David Turner @ 2017-04-20 22:04 UTC (permalink / raw)
  To: Johannes Schindelin, Jeff King; +Cc: git


> -----Original Message-----
> From: Johannes Schindelin [mailto:Johannes.Schindelin@gmx.de]
> Sent: Thursday, April 20, 2017 5:58 PM
> To: Jeff King <peff@peff.net>
> Cc: David Turner <David.Turner@twosigma.com>; git@vger.kernel.org
> Subject: Re: [PATCH] Increase core.packedGitLimit
> 
> Hi Peff,
> 
> On Thu, 20 Apr 2017, Jeff King wrote:
> 
> > On Thu, Apr 20, 2017 at 04:41:18PM -0400, David Turner wrote:
> >
> > > When core.packedGitLimit is exceeded, git will close packs.  If
> > > there is a repack operation going on in parallel with a fetch, the
> > > fetch might open a pack, and then be forced to close it due to
> > > packedGitLimit being hit.  The repack could then delete the pack out
> > > from under the fetch, causing the fetch to fail.
> > >
> > > Increase core.packedGitLimit's default value to prevent this.
> > >
> > > On current 64-bit x86_64 machines, 48 bits of address space are
> > > available.  It appears that 64-bit ARM machines have no standard
> > > amount of address space (that is, it varies by manufacturer), and
> > > IA64 and POWER machines have the full 64 bits.  So 48 bits is the
> > > only limit that we can reasonably care about.  We reserve a few bits
> > > of the 48-bit address space for the kernel's use (this is not
> > > strictly necessary, but it's better to be safe), and use up to the
> > > remaining 45.  No git repository will be anywhere near this large
> > > any time soon, so this should prevent the failure.
> >
> > Yep, I think this is a reasonable direction.
> >
> > > ---
> > >  git-compat-util.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > This probably needs an update to the core.packedGitLimit section of
> > Documentation/config.txt.
> >
> > > diff --git a/git-compat-util.h b/git-compat-util.h index
> > > 8a4a3f85e7..1c5de153a5 100644
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -616,7 +616,7 @@ extern int git_lstat(const char *, struct stat
> > > *);  #endif
> > >
> > >  #define DEFAULT_PACKED_GIT_LIMIT \
> > > -	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? 8192 : 256))
> > > +	((1024L * 1024L) * (size_t)(sizeof(void*) >= 8 ? (32 * 1024L *
> > > +1024L) : 256))
> >
> > I wondered if we would run afoul of integer sizes on 64-bit systems
> > where "long" is still only 32-bits (i.e., Windows). But I think it's
> > OK, because the values before we cast to size_t are in megabytes. So
> > your
> > 32*1024*1024 needs only 25 bits to store it. And then after we cast to
> > size_t, everything is in 64-bit.
> 
> Indeed, when I patch a local Git checkout accordingly, I see that
> packed_git_limit is set to 35184372088832.
> 
> The bigger problem in this regard is that users are allowed to override this via
> core.packedgitlimit but that value is parsed as an unsigned long.

We might want to think about replacing git_config_ulong with 
git_config_size_t in nearly all cases. "long" has ceased to be 
useful.  More modern versions of C prefer uint64_t, but I
think that we'll usually want size_t because these values will
be used as memory limits of various sorts.


^ permalink raw reply	[flat|threaded] 5+ messages in thread

* RE: [PATCH] Increase core.packedGitLimit
  2017-04-20 22:04     ` David Turner
@ 2017-04-21  9:34       ` Johannes Schindelin
  0 siblings, 0 replies; 5+ messages in thread
From: Johannes Schindelin @ 2017-04-21  9:34 UTC (permalink / raw)
  To: David Turner; +Cc: Jeff King, git

Hi Dave,

On Thu, 20 Apr 2017, David Turner wrote:
> 
> We might want to think about replacing git_config_ulong with 
> git_config_size_t in nearly all cases. "long" has ceased to be 
> useful.  More modern versions of C prefer uint64_t, but I
> think that we'll usually want size_t because these values will
> be used as memory limits of various sorts.

Indeed, `unsigned long` has ceased to be useful ever since the
introduction of inttypes.h. It is just too undefined.

We probably also need git_config_off_t(), though, and maybe even
git_config_ssize_t().

I would definitely welcome a change in direction where we use datatypes
also as a documentation of the variables' purpose.

Ciao,
Dscho

^ permalink raw reply	[flat|threaded] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-20 20:41 [PATCH] Increase core.packedGitLimit David Turner
2017-04-20 21:02 ` Jeff King
2017-04-20 21:58   ` Johannes Schindelin
2017-04-20 22:04     ` David Turner
2017-04-21  9:34       ` Johannes Schindelin

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox