object-file: reprepare alternates when necessary

From: Derrick Stolee <[email protected]>

When an object is not found in a repository's object store, we sometimes call reprepare_packed_git() to see if the object was temporarily moved into a new pack-file (and its old pack-file or loose object was deleted). This process does a scan of each pack directory within each odb, but does not reevaluate if the odb list needs updating.

Create a new reprepare_alt_odb() method that is a similar wrapper around prepare_alt_odb(). Call it from reprepare_packed_git() under the object read lock to avoid readers from interacting with a potentially incomplete odb being added to the odb list.

prepare_alt_odb() already avoids adding duplicate odbs to the list during its progress, so it is safe to call it again from reprepare_alt_odb() without worrying about duplicate odbs.

This change is specifically for concurrent changes to the repository, so it is difficult to create a test that guarantees this behavior is correct. I manually verified by introducing a reprepare_packed_git() call into get_revision() and stepped into that call in a debugger with a parent 'git log' process. Multiple runs of reprepare_alt_odb() kept the_repository->objects->odb as a single-item chain until I added a .git/objects/info/alternates file in a different process. The next run added the new odb to the chain and subsequent runs did not add to the chain.

---
object-file: reprepare alternates when necessary

This subtlety was notice by Michael Haggerty due to how alternates are used server-side at $DAYJOB. Moving pack-files from a repository to the alternate occasionally causes failures because processes that start before the alternate exists don't know how to find that alternate at run-time.

Thanks,

* Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1490

 object-file.c  | 6 ++++++
 object-store.h | 1 +
 packfile.c     | 1 +
 3 files changed, 8 insertions(+)

index 939865c1ae0..22acc7fd8e9 100644
@@ -944,6 +944,12 @@ void prepare_alt_odb(struct repository *r)
 	r->objects->loaded_alternates = 1;
 }
 
+void reprepare_alt_odb(struct repository *r)
+{
+	r->objects->loaded_alternates = 0;
+	prepare_alt_odb(r);
+}
+
 /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
 static int freshen_file(const char *fn)
 {

index 1a713d89d7c..750c29daa54 100644
@@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,
 	struct object_directory *, 1, fspathhash, fspatheq)
 
 void prepare_alt_odb(struct repository *r);
+void reprepare_alt_odb(struct repository *r);
 char *compute_alternate_path(const char *path, struct strbuf *err);
 struct object_directory *find_odb(struct repository *r, const char *obj_dir);
 typedef int alt_odb_fn(struct object_directory *, void *);

index 79e21ab18e7..2b28918a05e 100644
@@ -1008,6 +1008,7 @@ void reprepare_packed_git(struct repository *r)
 	struct object_directory *odb;
 
 	obj_read_lock();
+	reprepare_alt_odb(r);
 	for (odb = r->objects->odb; odb; odb = odb->next)
 		odb_clear_loose_cache(odb);
 

base-commit: d15644fe0226af7ffc874572d968598564a230dd

gitgitgadget

Based on my understanding of the patch, we are only prepared to see "more" alternates, rather than some existing alternate going away.

That being said, I am not certain that is how it works. Perhaps an alternate "goes away", but does not actually get removed from the list of alternate ODBs. If that's the case, any object lookup in that now-missing ODB would fail, but any subsequent ODBs which were added after calling reprepare_alt_odb() would succeed on that object lookup.

So, I don't know. I don't have the implementation details of the alternates ODB mechanism paged in enough to say for sure. Hopefully Stolee can point us in the right direction.

Agreed.

Thanks,
Taylor

I found this a bit hard to read, as one migh think from just this explanation that you're adding ODB locking as a new concept here, adding "existing" in front of "read lock" above might help.

But in fact we've been doing the locking since 6c307626f1e (grep:
protect packed_git [re-]initialization, 2020-01-15). So the only thing that really needs justification here is that putting the alternates re-reading under the same lock

There is a really interesting potential caveat here which you're not discussing, which is...

This seems reasonable, but wouldn't this do the same without introducing an API function just for this one use-case?

That's of course a nit, and you seem to have been adding this for consistency with reprepare_packed_git(), but it already "owns" the "approximate_object_count_valid" and "packed_git_initialized" flags in "struct raw_object_store".

So as we'll only need this from reprepare_packed_git() isn't it better to declare that "loaded_alternates" is another such flag?

Perhaps not, but as the resulting patch is much shorter it seems worth considering.

...but to continue the above, the *really* important thing here (and correct me if I'm wrong) is that we really need to *first* prepare the alternates, and *then* do the rest, as our new alternates might point to new loose objects and packs.

So with both of the above (the same could be done with your new helper) something like this would IMO make that much clearer:

	diff --git a/packfile.c b/packfile.c
	index 79e21ab18e7..50cb46ca4b7 100644
	--- a/packfile.c
	+++ b/packfile.c
	@@ -1008,6 +1008,13 @@ void reprepare_packed_git(struct repository *r)
	 	struct object_directory *odb;
	 
	 	obj_read_lock();
	+	/*
	+	 * New alternates might point to new loose & pack dirs, so we
	+	 * must first read those.
	+	 */
	+	r->objects->loaded_alternates = 0;
	+	prepare_alt_odb(r);
	+
	 	for (odb = r->objects->odb; odb; odb = odb->next)
	 		odb_clear_loose_cache(odb);

And, I think this is an exsiting edge case, but we're only locking the ODB of the "parent" repository in this case, so if we have alternates in play aren't we going to racily compute the rest here, the loose objects and packs of the alternates we're about to consult aren't under the same lock?

The prepare_alt_odb() call only _adds_ to the linked odb list. It will not remove any existing ODBs. Adding this reprepare_*() method makes it such that we can use the union of the alternates available across the lifetime of the process.

Thanks,
-Stolee

Ah, thanks. That was what I missed and led to the question.

I am not sure I got what you want to say 100%, but if you are saying that this "drop the 'loaded' flag and force prepare_*() function to redo its thing" must not be done only in reprepare_packed_git(), and that inlining the code there without introducing a helper function that anybody can casually call without thinking its consequenced through, then I tend to agree in principle. But it is just as easy to lift two lines of code from the rewritten/inlined code to a new place---to ensure people follow the obj_read_lock() rule, the comment before it may have to be a bit stronger, I wonder?

Yes, and as Derrick explained in another message, we only have to worry about new ones getting added, not existing ones going away.

Oops, "must be done only", of course. Sorry.

The fact that we do it in a lock in reprepare_packed_git() in the only current caller does raise suspicion that someone could call it later and not realize that the lock could be helpful. Inlining the change within reprepare_packed_git() makes the most sense here instead of mimicking the pattern.

The shortness of the patch is metric of quality, to me. The other reason (we might introduce a footgun) is more interesting.

I'll be sure to clarify that in my next version.

I don't understand what you are saying here. odb_read_lock() does not specify a repository and is instead a global lock on reading from any object database.

Here is its implementation:

extern int obj_read_use_lock;
extern pthread_mutex_t obj_read_mutex;

static inline void obj_read_lock(void)

{
	if(obj_read_use_lock)
		pthread_mutex_lock(&obj_read_mutex);
}

static inline void obj_read_unlock(void)

{
	if(obj_read_use_lock)
		pthread_mutex_unlock(&obj_read_mutex);
}

So I don't believe that your proposed edge case exists.

Thanks,
-Stolee

Right, that matches my understanding. What I am asking is: since we only add ODBs to the list, what happens if we can no longer access an *existing* alternate at the time we call reprepare_alt_odb()?

It's clear that that now-inaccessible alternate remains in our list of alternate ODBs, but do all object lookups hitting that ODB fail-over to the new ODB? I believe so, but it isn't totally clear to me.

Thanks,
Taylor

It's the same as the pack-file list: if we fail to load something from one, then we continue to the next one. If an alternate dir is completely removed during the process, then looking for pack- files again will fail to see any and continue without error.

This is already possible by deleting an alternate directory while a Git process is running and might try to open files in it. Git already recovers from this scenario.

If you're instead talking about the .git/objects/info/alternates file being modified to remove an alternate from the list, then Git's current behavior is to keep that alternate around for the life of the process, and I recommend continuing that behavior.

There's nothing special that we are adding here that doesn't already exist as protections when files are removed beneath the Git process.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

When an object is not found in a repository's object store, we sometimes call reprepare_packed_git() to see if the object was temporarily moved into a new pack-file (and its old pack-file or loose object was deleted). This process does a scan of each pack directory within each odb, but does not reevaluate if the odb list needs updating.

Extend reprepare_packed_git() to also reprepate the alternate odb list by setting loaded_alternates to zero and calling prepare_alt_odb(). This will add newly-discoverd odbs to the linked list, but will not duplicate existing ones nor will it remove existing ones that are no longer listed in the alternates file. Do this under the object read lock to avoid readers from interacting with a potentially incomplete odb being added to the odb list.

If the alternates file was edited to _remove_ some alternates during the course of the Git process, Git will continue to see alternates that were ever valid for that repository. ODBs are not removed from the list, the same as the existing behavior before this change. Git already has protections against an alternate directory disappearing from the filesystem during the lifetime of a process, and those are still in effect.

This change is specifically for concurrent changes to the repository, so it is difficult to create a test that guarantees this behavior is correct. I manually verified by introducing a reprepare_packed_git() call into get_revision() and stepped into that call in a debugger with a parent 'git log' process. Multiple runs of prepare_alt_odb() kept the_repository->objects->odb as a single-item chain until I added a .git/objects/info/alternates file in a different process. The next run added the new odb to the chain and subsequent runs did not add to the chain.

---
object-file: reprepare alternates when necessary

This subtlety was notice by Michael Haggerty due to how alternates are used server-side at $DAYJOB. Moving pack-files from a repository to the alternate occasionally causes failures because processes that start before the alternate exists don't know how to find that alternate at run-time.

Update in v2
============

* Instead of creating a new public reprepare_alt_odb() method, inline its implementation inside reprepare_packed_git().
* Update commit message to be explicit about behavior with alternates being removed from the alternates file or from disk.

Thanks,

* Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1490%2Fderrickstolee%2Fstolee%2Freprepare-alternates-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1490/derrickstolee/stolee/reprepare-alternates-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1490

Range-diff vs v1:

1: 3f42c9cdef7 ! 1: 9a5e1d9a9da object-file: reprepare alternates when necessary
@@ Commit message
deleted). This process does a scan of each pack directory within each odb, but does not reevaluate if the odb list needs updating.

- Create a new reprepare_alt_odb() method that is a similar wrapper around
- prepare_alt_odb(). Call it from reprepare_packed_git() under the object
- read lock to avoid readers from interacting with a potentially
- incomplete odb being added to the odb list.
-
- prepare_alt_odb() already avoids adding duplicate odbs to the list
- during its progress, so it is safe to call it again from
- reprepare_alt_odb() without worrying about duplicate odbs.
+ Extend reprepare_packed_git() to also reprepate the alternate odb list
+ by setting loaded_alternates to zero and calling prepare_alt_odb(). This
+ will add newly-discoverd odbs to the linked list, but will not duplicate
+ existing ones nor will it remove existing ones that are no longer listed
+ in the alternates file. Do this under the object read lock to avoid
+ readers from interacting with a potentially incomplete odb being added
+ to the odb list.
+
+ If the alternates file was edited to _remove_ some alternates during the
+ course of the Git process, Git will continue to see alternates that were
+ ever valid for that repository. ODBs are not removed from the list, the
+ same as the existing behavior before this change. Git already has
+ protections against an alternate directory disappearing from the
+ filesystem during the lifetime of a process, and those are still in
+ effect.

This change is specifically for concurrent changes to the repository, so it is difficult to create a test that guarantees this behavior is correct. I manually verified by introducing a reprepare_packed_git() call into get_revision() and stepped into that call in a debugger with a
- parent 'git log' process. Multiple runs of reprepare_alt_odb() kept
+ parent 'git log' process. Multiple runs of prepare_alt_odb() kept
the_repository->objects->odb as a single-item chain until I added a .git/objects/info/alternates file in a different process. The next run added the new odb to the chain and subsequent runs did not add to the
@@ Commit message

- ## object-file.c ##
-@@ object-file.c: void prepare_alt_odb(struct repository *r)
- r->objects->loaded_alternates = 1;
- }
-
-+void reprepare_alt_odb(struct repository *r)
-+{
-+ r->objects->loaded_alternates = 0;
-+ prepare_alt_odb(r);
-+}
-+
- /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
- static int freshen_file(const char *fn)
- {
-
- ## object-store.h ##
-@@ object-store.h: KHASH_INIT(odb_path_map, const char * /* key: odb_path */,
- struct object_directory *, 1, fspathhash, fspatheq)
-
- void prepare_alt_odb(struct repository *r);
-+void reprepare_alt_odb(struct repository *r);
- char *compute_alternate_path(const char *path, struct strbuf *err);
- struct object_directory *find_odb(struct repository *r, const char *obj_dir);
- typedef int alt_odb_fn(struct object_directory *, void *);
-
## packfile.c ##
@@ packfile.c: void reprepare_packed_git(struct repository *r)
struct object_directory *odb;

obj_read_lock();
-+ reprepare_alt_odb(r);
++
++ /*
++ * Reprepare alt odbs, in case the alternates file was modified
++ * during the course of this process. This only _adds_ odbs to
++ * the linked list, so existing odbs will continue to exist for
++ * the lifetime of the process.
++ */
++ r->objects->loaded_alternates = 0;
++ prepare_alt_odb(r);
++
for (odb = r->objects->odb; odb; odb = odb->next)
odb_clear_loose_cache(odb);

 packfile.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 79e21ab18e7..06419c8e8ca 100644
@@ -1008,6 +1008,16 @@ void reprepare_packed_git(struct repository *r)
 	struct object_directory *odb;
 
 	obj_read_lock();
+
+	/*
+	 * Reprepare alt odbs, in case the alternates file was modified
+	 * during the course of this process. This only _adds_ odbs to
+	 * the linked list, so existing odbs will continue to exist for
+	 * the lifetime of the process.
+	 */
+	r->objects->loaded_alternates = 0;
+	prepare_alt_odb(r);
+
 	for (odb = r->objects->odb; odb; odb = odb->next)
 		odb_clear_loose_cache(odb);
 

base-commit: d15644fe0226af7ffc874572d968598564a230dd

gitgitgadget

Very well explained, except I do not know what is meant by "temporarily", which to me implies what was moved is moved back later. Without "temporarily", it reads perfectly well. While I do not think of a good word to use to tell the readers that the moving is done by somebody else while we are trying to find the object, if there were one, that would replace "temporarily" very well.

But other than that tiny nit that does not have to be addressed at all, everything in this patch looks good.

Thanks.

s/temporarily/concurrently?

I think that concurrently conveys that another process was doing the moving. But I agree that it doesn't matter here to aid in the overall understanding of what's happening in this patch.

So I'd be fine with this patch as-is, with s/temporarily/concurrently, or with s/temporarily//.

Thanks,
Taylor

s/reprepate/reprepare/

Unless you were inventing a new word. :)

One thing that test wouldn't cover is loading alternates from $GIT_ALTERNATE_OBJECT_DIRECTORIES. Once upon a time, I think we were pretty inconsistent in finding duplicates. But these days it looks like it's all done centrally in link_alt_odb_entry(), via alt_odb_usable(). So it should be good.

Yeah, I don't think there's any real reason not to do this. It simply doesn't come up in the same way that packfile-repreparing does, because there it is routine for a concurrent gc to remove an existing packfile and add the objects elsewhere.

Whereas modifying alternates was never seen as something that would happen often or automatically. I guess in GitHub's case it is from converting a repository from stand-alone into a fork, migrating its objects to a shared one, and adding an alternate.

The only downside might be performance. For sane cases, I think scanning the new alternates is OK. I know Eric (cc'd) has some crazy 100k-alternate setup (from 407532f82d, etc), but I'd expect a reprepare there is already expensive (we already have to re-scan every one of those directories for packfiles, and throw out any loose object caches).

The patch itself looks good to me (and I agree with the sentiment to just inline the lines rather than adding a function).

-Peff

I'm not sure if that 100k alternate thing is happening, yet... (initial specs called for ~30k, but I figured it might grow)

If it does, I'm thinking about enhancing --batch-command, to support `add-alternate' to dynamically add alternates while running cat-file.

Right now, my biggest use case is only 250 alternates or so.

[snip]

I think the original reason why we did not clear was because we never knew (and we do not know) what is still in use. Can anybody explain why it would be a safe thing to do to rebuild the alt-odb list from scratch? Surely we can have a big central lock to do the "list manipulation" part safely, but is it safe to access the objects we obtained from one of the odb's that no longer is listed on the alt-odb list, for example? The code that this patch touches clears the loose object cache after updating the odb list, so loose object cache of any odb that goes away will not be cleared here, which means that the code that reconstruts alt-odb list would need to clear the loose object cache automatically? What should we do to packfiles that are mmaped from these alt-odbs that goes away? Etc. etc.

Sounds good.

Thanks.

Ah, I meant that prepare_alt_odb() assumes that the data structures are as they were originally initialized. So I can think of at least 2 ways that we could implement prepare_alt_odb():

(a) Repeatedly modify r->objects->odb_tail and repeatedly assign to

     kh_value(r->objects->odb_by_path, pos).
 (b) Call make_odb(), which returns a pointer that we assign to
     r->objects->odb, and call make_odb_hashmap(), which returns a
     pointer that we assign to r->objects->odb_by_path.

Remember that prepare_alt_odb() is meant to start from scratch, so it is reasonable to do either.

In Java, for example, it would be very reasonable to do (b). But this patch assumes that we have implemented it as (a). Indeed we have implemented it as (a) (although we have never documented it as such), and my quoted statement above is meant to say that it might be reasonable to keep assuming that it would be (a).