{"thread":{"id":"48451","subject":"[PATCH 1/2] packfile: close and free packs upon releasing an object store","startedAt":"2018-05-10T00:12:22Z","lastAt":"2018-05-10T05:12:06Z","messageCount":3,"participants":["Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"347151","messageId":"20180510001211.163692-1-sbeller@google.com","threadId":"48451","inReplyTo":null,"subject":"[PATCH 1/2] packfile: close and free packs upon releasing an object store","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-10T00:12:10Z","receivedAt":"2018-05-10T00:12:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In d0b59866223 (object-store: close all packs upon clearing the object\nstore, 2018-03-23), we made sure to close all packfiles on releasing\nan object store, but we also have to free the memory of the closed packs.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n object.c   |  2 +-\n packfile.c | 11 +++++++++++\n packfile.h |  1 +\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/object.c b/object.c\nindex 66cffaf6e51..3e64a4a26dd 100644\n--- a/object.c\n+++ b/object.c\n@@ -485,6 +485,6 @@ void raw_object_store_clear(struct raw_object_store *o)\n \to->alt_odb_tail = NULL;\n \n \tINIT_LIST_HEAD(&o->packed_git_mru);\n-\tclose_all_packs(o);\n+\tclose_and_free_packs(o);\n \to->packed_git = NULL;\n }\ndiff --git a/packfile.c b/packfile.c\nindex 6c3ddc3c31d..ba4baa55531 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -322,6 +322,17 @@ void close_all_packs(struct raw_object_store *o)\n \t\t\tclose_pack(p);\n }\n \n+void close_and_free_packs(struct raw_object_store *o)\n+{\n+\tclose_all_packs(o);\n+\n+\twhile (o->packed_git) {\n+\t\tstruct packed_git *p = o->packed_git;\n+\t\to->packed_git = p->next;\n+\t\tfree(p);\n+\t}\n+}\n+\n /*\n  * The LRU pack is the one with the oldest MRU window, preferring packs\n  * with no used windows, or the oldest mtime if it has no windows allocated.\ndiff --git a/packfile.h b/packfile.h\nindex 9c2f8859945..cdab0557979 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -67,6 +67,7 @@ extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t\n extern void close_pack_windows(struct packed_git *);\n extern void close_pack(struct packed_git *);\n extern void close_all_packs(struct raw_object_store *o);\n+extern void close_and_free_packs(struct raw_object_store *o);\n extern void unuse_pack(struct pack_window **);\n extern void clear_delta_base_cache(void);\n extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n-- \n2.17.0.255.g8bfb7c0704\n\n"},{"id":"347152","messageId":"20180510001211.163692-2-sbeller@google.com","threadId":"48451","inReplyTo":"20180510001211.163692-1-sbeller@google.com","subject":"[PATCH 2/2] packfile.h: remove all extern keywords","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-10T00:12:11Z","receivedAt":"2018-05-10T00:12:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Per our coding guidelines we prefer to only use the extern keyword\nwhen needed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n packfile.h | 80 +++++++++++++++++++++++++++---------------------------\n 1 file changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/packfile.h b/packfile.h\nindex cdab0557979..eb3b1154501 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -10,32 +10,32 @@\n  *\n  * Example: odb_pack_name(out, sha1, \"idx\") => \".git/objects/pack/pack-1234..idx\"\n  */\n-extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n+char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);\n \n /*\n  * Return the name of the (local) packfile with the specified sha1 in\n  * its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_name(const unsigned char *sha1);\n+char *sha1_pack_name(const unsigned char *sha1);\n \n /*\n  * Return the name of the (local) pack index file with the specified\n  * sha1 in its name.  The return value is a pointer to memory that is\n  * overwritten each time this function is called.\n  */\n-extern char *sha1_pack_index_name(const unsigned char *sha1);\n+char *sha1_pack_index_name(const unsigned char *sha1);\n \n-extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n #define PACKDIR_FILE_GARBAGE 4\n-extern void (*report_garbage)(unsigned seen_bits, const char *path);\n+void (*report_garbage)(unsigned seen_bits, const char *path);\n \n-extern void reprepare_packed_git(struct repository *r);\n-extern void install_packed_git(struct repository *r, struct packed_git *pack);\n+void reprepare_packed_git(struct repository *r);\n+void install_packed_git(struct repository *r, struct packed_git *pack);\n \n struct packed_git *get_packed_git(struct repository *r);\n struct list_head *get_packed_git_mru(struct repository *r);\n@@ -46,31 +46,31 @@ struct list_head *get_packed_git_mru(struct repository *r);\n  */\n unsigned long approximate_object_count(void);\n \n-extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n+struct packed_git *find_sha1_pack(const unsigned char *sha1,\n \t\t\t\t\t struct packed_git *packs);\n \n-extern void pack_report(void);\n+void pack_report(void);\n \n /*\n  * mmap the index file for the specified packfile (if it is not\n  * already mmapped).  Return 0 on success.\n  */\n-extern int open_pack_index(struct packed_git *);\n+int open_pack_index(struct packed_git *);\n \n /*\n  * munmap the index file for the specified packfile (if it is\n  * currently mmapped).\n  */\n-extern void close_pack_index(struct packed_git *);\n+void close_pack_index(struct packed_git *);\n \n-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n-extern void close_pack_windows(struct packed_git *);\n-extern void close_pack(struct packed_git *);\n-extern void close_all_packs(struct raw_object_store *o);\n-extern void close_and_free_packs(struct raw_object_store *o);\n-extern void unuse_pack(struct pack_window **);\n-extern void clear_delta_base_cache(void);\n-extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n+unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n+void close_pack_windows(struct packed_git *);\n+void close_pack(struct packed_git *);\n+void close_all_packs(struct raw_object_store *o);\n+void close_and_free_packs(struct raw_object_store *o);\n+void unuse_pack(struct pack_window **);\n+void clear_delta_base_cache(void);\n+struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n \n /*\n  * Make sure that a pointer access into an mmap'd index file is within bounds,\n@@ -80,7 +80,7 @@ extern struct packed_git *add_packed_git(const char *path, size_t path_len, int\n  * (like the 64-bit extended offset table), as we compare the size to the\n  * fixed-length parts when we open the file.\n  */\n-extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n+void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n \n /*\n  * Perform binary search on a pack-index for a given oid. Packfile is expected to\n@@ -96,51 +96,51 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32\n  * at the SHA-1 within the mmapped index.  Return NULL if there is an\n  * error.\n  */\n-extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n+const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n /*\n  * Like nth_packed_object_sha1, but write the data into the object specified by\n  * the the first argument.  Returns the first argument on success, and NULL on\n  * error.\n  */\n-extern const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n+const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n \n /*\n  * Return the offset of the nth object within the specified packfile.\n  * The index must already be opened.\n  */\n-extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n+off_t nth_packed_object_offset(const struct packed_git *, uint32_t n);\n \n /*\n  * If the object named sha1 is present in the specified packfile,\n  * return its offset within the packfile; otherwise, return 0.\n  */\n-extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n+off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git *);\n \n-extern int is_pack_valid(struct packed_git *);\n-extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);\n-extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n-extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n-extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n+int is_pack_valid(struct packed_git *);\n+void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);\n+unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n+unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n+int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n \n-extern void release_pack_memory(size_t);\n+void release_pack_memory(size_t);\n \n /* global flag to enable extra checks when accessing packed objects */\n-extern int do_check_packed_object_crc;\n+int do_check_packed_object_crc;\n \n-extern int packed_object_info(struct packed_git *pack, off_t offset, struct object_info *);\n+int packed_object_info(struct packed_git *pack, off_t offset, struct object_info *);\n \n-extern void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n-extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);\n+void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1);\n+const struct packed_git *has_packed_and_bad(const unsigned char *sha1);\n \n /*\n  * Iff a pack file in the given repository contains the object named by sha1,\n  * return true and store its location to e.\n  */\n-extern int find_pack_entry(struct repository *r, const unsigned char *sha1, struct pack_entry *e);\n+int find_pack_entry(struct repository *r, const unsigned char *sha1, struct pack_entry *e);\n \n-extern int has_sha1_pack(const unsigned char *sha1);\n+int has_sha1_pack(const unsigned char *sha1);\n \n-extern int has_pack_index(const unsigned char *sha1);\n+int has_pack_index(const unsigned char *sha1);\n \n /*\n  * Only iterate over packs obtained from the promisor remote.\n@@ -156,13 +156,13 @@ typedef int each_packed_object_fn(const struct object_id *oid,\n \t\t\t\t  struct packed_git *pack,\n \t\t\t\t  uint32_t pos,\n \t\t\t\t  void *data);\n-extern int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data);\n-extern int for_each_packed_object(each_packed_object_fn, void *, unsigned flags);\n+int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn, void *data);\n+int for_each_packed_object(each_packed_object_fn, void *, unsigned flags);\n \n /*\n  * Return 1 if an object in a promisor packfile is or refers to the given\n  * object, 0 otherwise.\n  */\n-extern int is_promisor_object(const struct object_id *oid);\n+int is_promisor_object(const struct object_id *oid);\n \n #endif\n-- \n2.17.0.255.g8bfb7c0704\n\n"},{"id":"347173","messageId":"20180510051201.GB27259@sigill.intra.peff.net","threadId":"48451","inReplyTo":"20180510001211.163692-1-sbeller@google.com","subject":"Re: [PATCH 1/2] packfile: close and free packs upon releasing an object store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-10T05:12:02Z","receivedAt":"2018-05-10T05:12:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 09, 2018 at 05:12:10PM -0700, Stefan Beller wrote:\n\n> In d0b59866223 (object-store: close all packs upon clearing the object\n> store, 2018-03-23), we made sure to close all packfiles on releasing\n> an object store, but we also have to free the memory of the closed packs.\n\nI know we've assumed in a few places that a \"struct packed_git\" will\nnever go away. The one that comes to mind is the mru list.\n\nIt looks like we'll be OK here:\n\n> diff --git a/object.c b/object.c\n> index 66cffaf6e51..3e64a4a26dd 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -485,6 +485,6 @@ void raw_object_store_clear(struct raw_object_store *o)\n>  \to->alt_odb_tail = NULL;\n>  \n>  \tINIT_LIST_HEAD(&o->packed_git_mru);\n> -\tclose_all_packs(o);\n> +\tclose_and_free_packs(o);\n>  \to->packed_git = NULL;\n>  }\n\nbecause we clear the list above. But it would be dangerous for anybody\nelse to call close_and_free_packs(). Should that INIT_LIST_HEAD get\nmoved down into that function?\n\nProbably the same applies to setting NULL here; you're left with a\ndangling pointer if you just call close_and_free_packs(). Should\nthat helper maybe just be a static function in object.c?\n\n\nJust brainstorming other places where the immutability of \"struct\npacked_git\" might be important:\n\n  - pack-objects keeps a pointer from each object_entry to its\n    containing packed_git. That's probably OK, as you wouldn't expect to\n    be able to close the object store in the middle of that operation.\n\n  - the reachability bitmap code holds a pointer to the pack that has a\n    bitmap. Probably that whole \"struct bitmap_index\" needs to be part\n    of the object_store (arguably it should all just be _inside_ the\n    packed_git, but the current implementation avoids complexity by just\n    having a single bitmap-per-repo).\n\n-Peff\n"}]}