[PATCH 2/2] Don't close pack fd when free'ing pack windows
From: Brandon Casey draf...@gmail.com
Now that close_one_pack() has been introduced to handle file
descriptor pressure, it is not strictly necessary to close the
pack file descriptor in unuse_one_window() when we're under memory
pressure.
Jeff King provided a justification for leaving the pack file open:
If you close packfile descriptors, you can run into racy situations
where somebody else is repacking and deleting packs, and they go away
while you are trying to access them. If you keep a descriptor open,
you're fine; they last to the end of the process. If you don't, then
they disappear from under you.
For normal object access, this isn't that big a deal; we just rescan
the packs and retry. But if you are packing yourself (e.g., because
you are a pack-objects started by upload-pack for a clone or fetch),
it's much harder to recover (and we print some warnings).
Let's do so (or uh, not do so).
Signed-off-by: Brandon Casey draf...@gmail.com
---
builtin/pack-objects.c | 2 +-
git-compat-util.h | 2 +-
sha1_file.c| 21 +++--
3 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..4eb0521 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1809,7 +1809,7 @@ static void find_deltas(struct object_entry **list,
unsigned *list_size,
static void try_to_free_from_threads(size_t size)
{
read_lock();
- release_pack_memory(size, -1);
+ release_pack_memory(size);
read_unlock();
}
diff --git a/git-compat-util.h b/git-compat-util.h
index cc4ba4d..29b1ee3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -517,7 +517,7 @@ int inet_pton(int af, const char *src, void *dst);
const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif
-extern void release_pack_memory(size_t, int);
+extern void release_pack_memory(size_t);
typedef void (*try_to_free_t)(size_t);
extern try_to_free_t set_try_to_free_routine(try_to_free_t);
diff --git a/sha1_file.c b/sha1_file.c
index 7731ab1..d26121a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -614,7 +614,7 @@ static void scan_windows(struct packed_git *p,
}
}
-static int unuse_one_window(struct packed_git *current, int keep_fd)
+static int unuse_one_window(struct packed_git *current)
{
struct packed_git *p, *lru_p = NULL;
struct pack_window *lru_w = NULL, *lru_l = NULL;
@@ -628,15 +628,8 @@ static int unuse_one_window(struct packed_git *current,
int keep_fd)
pack_mapped -= lru_w-len;
if (lru_l)
lru_l-next = lru_w-next;
- else {
+ else
lru_p-windows = lru_w-next;
- if (!lru_p-windows lru_p-pack_fd != -1
-lru_p-pack_fd != keep_fd) {
- close(lru_p-pack_fd);
- pack_open_fds--;
- lru_p-pack_fd = -1;
- }
- }
free(lru_w);
pack_open_windows--;
return 1;
@@ -644,10 +637,10 @@ static int unuse_one_window(struct packed_git *current,
int keep_fd)
return 0;
}
-void release_pack_memory(size_t need, int fd)
+void release_pack_memory(size_t need)
{
size_t cur = pack_mapped;
- while (need = (cur - pack_mapped) unuse_one_window(NULL, fd))
+ while (need = (cur - pack_mapped) unuse_one_window(NULL))
; /* nothing */
}
@@ -658,7 +651,7 @@ void *xmmap(void *start, size_t length,
if (ret == MAP_FAILED) {
if (!length)
return NULL;
- release_pack_memory(length, fd);
+ release_pack_memory(length);
ret = mmap(start, length, prot, flags, fd, offset);
if (ret == MAP_FAILED)
die_errno(Out of memory? mmap failed);
@@ -954,7 +947,7 @@ unsigned char *use_pack(struct packed_git *p,
win-len = (size_t)len;
pack_mapped += win-len;
while (packed_git_limit pack_mapped
-unuse_one_window(p, p-pack_fd))
+unuse_one_window(p))
; /* nothing */
win-base = xmmap(NULL, win-len,
PROT_READ, MAP_PRIVATE,
@@ -1000,7 +993,7 @@ static struct packed_git *alloc_packed_git(int extra)
static void try_to_free_pack_memory(size_t size)
{
- release_pack_memory(size, -1);
+ release_pack_memory(size);
}
struct packed_git *add_packed_git(const char *path, int path_len, int local)
--
1.8.4.rc0.2.g6cf5c31
---
This email message is for the sole use of the intended
Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows
On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote: --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- I'm certainly not a lawyer, and I'm sorry for not reviewing the content of the patch instead, but is that not a problem from a legal point of view ? I remember a video of Greg Kroah-Hartman where he talked about that (the video was posted by Junio on G+). -- 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 2/2] Don't close pack fd when free'ing pack windows
On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote: On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote: --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- I'm certainly not a lawyer, and I'm sorry for not reviewing the content of the patch instead, but is that not a problem from a legal point of view ? Talking about legal, is it a problem if a commit isn't signed-off by it's committer or author e-mail? Like in this case where the sign-off is from gmail.com and the committer from nvidia.com? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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 2/2] Don't close pack fd when free'ing pack windows
On Wed, Jul 31, 2013 at 2:08 PM, Antoine Pelisse apeli...@gmail.com wrote: On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote: --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- I'm certainly not a lawyer, and I'm sorry for not reviewing the content of the patch instead, but is that not a problem from a legal point of view ? I remember a video of Greg Kroah-Hartman where he talked about that (the video was posted by Junio on G+). Me either thank God. Are those footers even enforceable? I mean, really, if someone mistakenly sends me their corporate financial numbers am I supposed to be under some legal obligation not to share it? I always assumed it was a scare tactic that lawyers like to use. To address the text of the footer, I'd say the intended recipient(s) are those on the to line which includes git@vger.kernel.org and the implicit use is for inclusion and distribution in the git source code. Anyway, I doubt I would have any influence on getting the footer removed. If Junio would rather me not submit patches with that footer, then I'd try to find a workaround. -Brandon -- 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 2/2] Don't close pack fd when free'ing pack windows
Antoine Pelisse apeli...@gmail.com writes:
On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote:
---
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information. Any unauthorized review, use, disclosure or
distribution
is prohibited. If you are not the intended recipient, please contact the
sender by
reply email and destroy all copies of the original message.
---
I'm certainly not a lawyer, and I'm sorry for not reviewing the
content of the patch instead, but is that not a problem from a legal
point of view ?
I remember a video of Greg Kroah-Hartman where he talked about that
(the video was posted by Junio on G+).
It's this video:
http://www.youtube.com/watch?v=fMeH7wqOwXA
The comment starts at 13:55.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 2/2] Don't close pack fd when free'ing pack windows
On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote: On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote: --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- I'm certainly not a lawyer, and I'm sorry for not reviewing the content of the patch instead, but is that not a problem from a legal point of view ? Talking about legal, is it a problem if a commit isn't signed-off by it's committer or author e-mail? Like in this case where the sign-off is from gmail.com and the committer from nvidia.com? It never has been. My commits should have the author and committer set to my gmail address actually. Others have sometimes used the two fields to distinguish between a corporate identity (i.e. m...@somecompany.com) that represents the funder of the work and a canonical identity (m...@personalemail.com) that identifies the person that performed the work. -Brandon -- 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 2/2] Don't close pack fd when free'ing pack windows
On Wed, Jul 31, 2013 at 02:31:34PM -0700, Brandon Casey wrote: On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote: On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote: --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- I'm certainly not a lawyer, and I'm sorry for not reviewing the content of the patch instead, but is that not a problem from a legal point of view ? Talking about legal, is it a problem if a commit isn't signed-off by it's committer or author e-mail? Like in this case where the sign-off is from gmail.com and the committer from nvidia.com? It never has been. My commits should have the author and committer set to my gmail address actually. Oh, that's why the extra From: - field below the header is for. Others have sometimes used the two fields to distinguish between a corporate identity (i.e. m...@somecompany.com) that represents the funder of the work and a canonical identity (m...@personalemail.com) that identifies the person that performed the work. In some contries your work when you're employed does not belong to you but to your employer and when you're acting for your employer you're representing the corporate legal person. Therefore two different e-mails can be seen as two different (legal not physical) persons. At least that's how I understand those legal tips for developers I've got. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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
