Comment 1 Matthias Sohn 2021-05-25 19:44:40 EDT

- Did you use git or jgit to run gc ?
- Which gc configuration was used ?
- How were NFS clients configured ?

Comment 2 Luca Milanesio 2021-05-25 20:08:01 EDT

> Did you use git or jgit to run gc ?
> Which gc configuration was used ?

JGit GC, with prunepackexpiry = 1.day.ago

> How were NFS clients configured ?

You could mount the NFS volume with 'noac' but the performance would be awful.

---

The issue is: this wasn't happening before: the removed packfile was just removed from the in-memory packlist.

By looking at the call stack, I do not see any of the caller that is using the FileUtils.isStaleFileHandle(ioe).

I am wondering if one recent optimisations in the clone has missed this use-case?
Anyway, I'm working on a fix of the problem.

P.S. The packfile that fails with "stale file handle" just needs to be silently discarded because it was for sure an old duplicate removed by a JGit GC cycle.

Comment 3 Matthias Sohn 2021-05-26 03:53:12 EDT

Did you try the option

pack.preserveOldPacks [1]

with jgit gc and Martin's change [2].
 

[1] https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/refs/heads/master/Documentation/config-options.md
https://git.eclipse.org/r/c/jgit/jgit/+/87969
https://git.eclipse.org/r/c/jgit/jgit/+/122288
[2] https://git.eclipse.org/r/c/jgit/jgit/+/122288

Comment 4 Matthias Sohn 2021-05-26 03:56:40 EDT

Could you bisect which jgit version introduced the problem to sort out if this is a regression ?

Comment 5 Luca Milanesio 2021-05-26 04:19:10 EDT

> Could you bisect which jgit version introduced the problem to sort out if this is a regression ?

Yes, that's the plan :-)

Comment 6 Luca Milanesio 2021-05-26 04:21:43 EDT

> Did you try the option
> pack.preserveOldPacks [1]
> with jgit gc and Martin's change [2].

Thanks for the pointer, that's an option to avoid the "stale file handle" exception altogether: moving the file instead of removing it. I recall that Martin mentioned that many times at the hackathons.

I am wondering what happens though when those files are eventually removed, as you can't afford having them growing indefinitely.
NFS is notoriously slow in listing files in directories, so listing *that* preserved directory would then become a performance issue I believe.

I'll do some testing on that also, thanks again for the feedback and hints.

Luca.

Comment 7 Matthias Sohn 2021-05-26 04:57:12 EDT

(In reply to Luca Milanesio from comment #6)
> > Did you try the option
> > pack.preserveOldPacks [1]
> > with jgit gc and Martin's change [2].
> 
> Thanks for the pointer, that's an option to avoid the "stale file handle"
> exception altogether: moving the file instead of removing it. I recall that
> Martin mentioned that many times at the hackathons.
> 
> I am wondering what happens though when those files are eventually removed,
> as you can't afford having them growing indefinitely.
> NFS is notoriously slow in listing files in directories, so listing *that*
> preserved directory would then become a performance issue I believe.

As far as I understand this option it's deferring deletion to the next run of gc instead of immediately deleting it in the gc run moving it to the preserved folder.

1. pass: move garbage to preserved folder
2. pass: first delete content of preserved folder from last round, then do gc

If between these steps an object is found to be missing but can be found in the preserved folder the corresponding pack is restored back to the packs folder including its index.

Comment 8 Luca Milanesio 2021-05-26 06:30:16 EDT

> As far as I understand this option it's deferring deletion to the next run of gc > instead of immediately deleting it in the gc run moving it to the preserved folder.

> 1. pass: move garbage to preserved folder
> 2. pass: first delete content of preserved folder from last round, then do gc

Yes, but then I do not understand how is that different from a prunepackexpire settings.

We already keep the packfiles for 1 more cycle (GC is done more frequently than the value of the prunepackexpire), which would already avoid the problem of removing packfiles that could be actually used.

The issue here is that *WHEN* they are removed, they remain visible as "stale file handle" on the other nodes. Moving them solves the issue BUT decreases the performance. If they are removed afterwards from the 'preserved' folder, the "stale file handle" would reappear.

IMHO the problem is unavoidable when you remove (sooner or later) a file from NFS.
That's why we should rather just "ignore the exception and close the file" as we do in other parts of the code.

Comment 9 Luca Milanesio 2021-05-26 07:23:54 EDT

I believe I have a bit more light on the situation where this problem happens.

What has happened is that:
- packfile was a "stale object"
- bitmap was NOT a "stale object" (I need to understand how that happened)

In that situation, WindowCursor finds the bitmap to serve the clone, get the corresponding packfile and send it back to the client 'as-is'.

See in fact:
WindowCursor.copyPackAsIs(WindowCursor.java:239)

If the objects were packed in memory and read from the packfile, the ObjectDirectory would have noticed the stale file handle and removed the invalid packfile from memory.

However, in this case, the packing of individual objects isn't needed and therefore JGit tries to send the whole packfile through the wire: that fails because it is a stale object.

Let me know investigate for how long this problem has been there.

Comment 10 Luca Milanesio 2021-05-26 08:33:34 EDT

> Let me know investigate for how long this problem has been there.

By looking at the history of WindowCursor.java, the problem has been there from the very beginning. The thing is that we learned about the nuances of NFS just later when we started using JGit from Gerrit with multiple primary nodes in HA.

@Matthias does the troubleshooting and analysis make sense to you? Should I now start drafting a fix? Which stable branch would you suggest to target?

Comment 11 Nasser Grainawi 2021-05-26 10:43:50 EDT

(In reply to Luca Milanesio from comment #8)
> > As far as I understand this option it's deferring deletion to the next run of gc > instead of immediately deleting it in the gc run moving it to the preserved folder.
> 
> > 1. pass: move garbage to preserved folder
> > 2. pass: first delete content of preserved folder from last round, then do gc
> 
> Yes, but then I do not understand how is that different from a
> prunepackexpire settings.
> 
> We already keep the packfiles for 1 more cycle (GC is done more frequently
> than the value of the prunepackexpire), which would already avoid the
> problem of removing packfiles that could be actually used.
> 
> The issue here is that *WHEN* they are removed, they remain visible as
> "stale file handle" on the other nodes. Moving them solves the issue BUT
> decreases the performance. If they are removed afterwards from the
> 'preserved' folder, the "stale file handle" would reappear.
> 
> IMHO the problem is unavoidable when you remove (sooner or later) a file
> from NFS.
> That's why we should rather just "ignore the exception and close the file"
> as we do in other parts of the code.

One difference is that with the preserved dir, those packs aren't referenced unless we can't find the object anywhere else, whereas pack expiry keeps them available and referenceable for longer. With preserved, it's expected that any used objects are duplicated between preserved and non-preserved dirs. When it turns out a used object is only available in preserved, we move that pack back to non-preserved. Since we never delete a file from non-preserved, you cannot have a stale file handle.

We've been running this code for many years now and have eliminated pack stale file handles with it.

Comment 12 Luca Milanesio 2021-05-26 14:55:07 EDT

> One difference is that with the preserved dir, those packs aren't referenced unless we can't find the object anywhere else

Gotcha: so preserved dir contains packfiles with ALL duplicate objects and, potentially, unreachable objects, correct?

> whereas pack expiry keeps them available and referenceable for longer.

Do you see the preserved approach as an alternative or a complement to the prunepackexpire mechanism?

> With preserved, it's expected that any 
> used objects are duplicated between preserved and non-preserved dirs. When it turns out a used object is only available in preserved, we move that pack back to non-preserved. Since we never delete a file from non-preserved, you cannot have a stale file handle.

Very neat indeed. The non-preserved dir is basically the "attic" of all packfiles of the Git repository.
With a very busy and large repo, GCed very often, I presume that there is *a lot* of disk space used for preserved and non-preserved packfiles.

> We've been running this code for many years now and have eliminated pack stale file handles with it.

Of course, by never removing any packfile you can never ever create a stale file handle.
Do you do any cleanup once in a while, where you remove the non-preserved packfiles? If yes, do you shutdown Gerrit for that?

Comment 13 Matthias Sohn 2021-05-26 16:34:47 EDT

(In reply to Luca Milanesio from comment #12)
> > One difference is that with the preserved dir, those packs aren't referenced unless we can't find the object anywhere else
> 
> Gotcha: so preserved dir contains packfiles with ALL duplicate objects and,
> potentially, unreachable objects, correct?
> 
> > whereas pack expiry keeps them available and referenceable for longer.
> 
> Do you see the preserved approach as an alternative or a complement to the
> prunepackexpire mechanism?
> 
> > With preserved, it's expected that any 
> > used objects are duplicated between preserved and non-preserved dirs. When it turns out a used object is only available in preserved, we move that pack back to non-preserved. Since we never delete a file from non-preserved, you cannot have a stale file handle.
> 
> Very neat indeed. The non-preserved dir is basically the "attic" of all
> packfiles of the Git repository.
> With a very busy and large repo, GCed very often, I presume that there is *a
> lot* of disk space used for preserved and non-preserved packfiles.
> 
> > We've been running this code for many years now and have eliminated pack stale file handles with it.
> 
> Of course, by never removing any packfile you can never ever create a stale
> file handle.
> Do you do any cleanup once in a while, where you remove the non-preserved
> packfiles? If yes, do you shutdown Gerrit for that?

AFAICS preserved packs are pruned in the next gc by the method GC.prunePreserved()

Comment 14 Luca Milanesio 2021-05-26 16:41:46 EDT

> AFAICS preserved packs are pruned in the next gc by the method GC.prunePreserved()

That would risk again to trigger yet another "stale file handle" exception, if any of the other nodes are trying to access them.

My impression is that the ONLY solution at the moment, apart from solving this bug, is to keep the non-preserved packfiles forever, or until you restart Gerrit.

Comment 15 Nasser Grainawi 2021-05-26 19:26:19 EDT

(In reply to Luca Milanesio from comment #14)
> > AFAICS preserved packs are pruned in the next gc by the method GC.prunePreserved()
> 
> That would risk again to trigger yet another "stale file handle" exception,
> if any of the other nodes are trying to access them.

No, as I noted above, only packs still in the preserved dir are removed. Since any referenced pack will be moved to the non-preserved dir, a referenced pack will never be removed.

Comment 16 Luca Milanesio 2021-05-26 19:31:28 EDT

> No, as I noted above, only packs still in the preserved dir are removed.
> Since any referenced pack will be moved to the non-preserved dir, a referenced pack will never be removed.

Exactly, that's what I've learned; hence, you will eventually run out of disk space, unless you do some cleanup and actually prune them from the non-preserved dir.

Comment 17 Luca Milanesio 2021-05-26 19:59:33 EDT

Let me get more into the code and see how the preserved/non-preserved works and when pack files are effectively removed or not.

Comment 18 Luca Milanesio 2021-05-26 20:17:25 EDT

OK, I believe I should have now clear how it works:

1. Run a jgit.sh gc --preserve-oldpacks (with prune = now)

The new packfiles are created under objects/pack
The existing packfiles are moved under objects/pack/preserved

2. Run a jgit.sh gc --preserve-oldpacks --prune-preserved (with prune = now)

The old packfiles (moved at step 1.) are removed from objects/pack/preserved
The new packfiles are generated under objects/pack
The existing packfiles are moved to objects/pack/preserved

Bottom line: you just delay the removal of the packfiles, you do not accumulate them.

I need to do some testing to see what happens if the second node (on NFS) is holding an open file pointing to an existing packfile that is moved at 1. and then removed at 2.

I do believe that it would still trigger a "stale file handle" error, unless I restart Gerrit. The list of packfiles in memory is cached and not always refreshed. It will continue to hold an open file to a file that is then removed at 2. and, once you try to access that file, it would return a "stale file handle".

Anyway, I need to verify that with a test case.

Thanks for the pointers, the time to answer my questions and the guidance on how to address this issue :-) it is all much appreciated and useful.

Comment 19 Nasser Grainawi 2021-05-26 22:34:24 EDT

(In reply to Luca Milanesio from comment #18)
> OK, I believe I should have now clear how it works:
> 
> 1. Run a jgit.sh gc --preserve-oldpacks (with prune = now)
> 
> The new packfiles are created under objects/pack
> The existing packfiles are moved under objects/pack/preserved
> 
> 2. Run a jgit.sh gc --preserve-oldpacks --prune-preserved (with prune = now)
> 
> The old packfiles (moved at step 1.) are removed from objects/pack/preserved
> The new packfiles are generated under objects/pack
> The existing packfiles are moved to objects/pack/preserved
> 
> Bottom line: you just delay the removal of the packfiles, you do not
> accumulate them.
> 
> I need to do some testing to see what happens if the second node (on NFS) is
> holding an open file pointing to an existing packfile that is moved at 1.
> and then removed at 2.

Unless you have an exceptionally long running operation (i.e. longer than your gc cycle), so long as the second node is also running this updated JGit code, it will move any packs it accesses from preserved back to objects/pack as well, thus avoiding them being removed by node 1.

> 
> I do believe that it would still trigger a "stale file handle" error, unless
> I restart Gerrit. The list of packfiles in memory is cached and not always
> refreshed. It will continue to hold an open file to a file that is then
> removed at 2. and, once you try to access that file, it would return a
> "stale file handle".

If that's the case, I'd consider it a bug and would help to fix that.

Comment 20 Luca Milanesio 2021-05-27 04:01:24 EDT

>> I need to do some testing to see what happens if the second node (on NFS) is
>> holding an open file pointing to an existing packfile that is moved at 1.
>> and then removed at 2.

>Unless you have an exceptionally long running operation (i.e. longer than your gc
>cycle), so long as the second node is also running this updated JGit code, it >will move any packs it accesses from preserved back to objects/pack as well, thus
>avoiding them being removed by node 1.

The updated JGit code isn't on stable-5.9, or am I mistaken?
I will test with the current stable-5.9 *and* with the update JGit code and revert back with the results.

It could well be that it has fixed my issue :-) I hope so !

>> I do believe that it would still trigger a "stale file handle" error, unless
>> I restart Gerrit. The list of packfiles in memory is cached and not always
>> refreshed. It will continue to hold an open file to a file that is then
>> removed at 2. and, once you try to access that file, it would return a
>> "stale file handle".


>If that's the case, I'd consider it a bug and would help to fix that.

From what I see in the code, there are isn't a validation of the packfile before using it and sending it over the wire.

But I'll do another check and report the results.

Comment 21 Nasser Grainawi 2021-05-27 10:10:28 EDT

(In reply to Luca Milanesio from comment #20)
> The updated JGit code isn't on stable-5.9, or am I mistaken?
> I will test with the current stable-5.9 *and* with the update JGit code and
> revert back with the results.

No, stable-5.11, specifically this release onwards: v5.11.0.202103091610-r

> It could well be that it has fixed my issue :-) I hope so !
> 
> >> I do believe that it would still trigger a "stale file handle" error, unless
> >> I restart Gerrit. The list of packfiles in memory is cached and not always
> >> refreshed. It will continue to hold an open file to a file that is then
> >> removed at 2. and, once you try to access that file, it would return a
> >> "stale file handle".
> 
> 
> >If that's the case, I'd consider it a bug and would help to fix that.
> 
> From what I see in the code, there are isn't a validation of the packfile
> before using it and sending it over the wire.
> 
> But I'll do another check and report the results.

Sounds good, thanks!

Comment 22 Luca Milanesio 2021-05-27 18:22:50 EDT

I did a few experiments with JGit from stable-5.9 (this bug is filed against that version) and:

1. The jgit gc --preserve-oldpacks helps on the 1st round to avoid the "Stale file handle" because the packfile is moved rather than removed.

2. The jgit gc --prune-preserved triggers again the "stale file handle" when the 'moved file' is getting removed

The question is: how comes that at 2. the JGit code is still referring to a packfile that isn't in the objects/pack directory anymore? Well, that's a bug :-(

If you look at this interface:

interface ObjectsReuseAsIs {

	/**
	 * Obtain the available cached packs that match the bitmap and update
	 * the bitmap by removing the items that are in the CachedPack.
	 * <p>
	 * A cached pack has known starting points and may be sent entirely as-is,
	 * with almost no effort on the sender's part.
	 *
	 * @param needBitmap
	 *            the bitmap that contains all of the objects the client wants.
	 * @return the available cached packs.
	 * @throws java.io.IOException
	 *             the cached packs cannot be listed from the repository.
	 *             Callers may choose to ignore this and continue as-if there
	 *             were no cached packs.
	 */
	Collection<CachedPack> getCachedPacksAndUpdate(
			BitmapBuilder needBitmap) throws IOException;
}

The method says in its documentation that it should throw an IOException if the packfiles cannot be listed from the repository, which makes you think that the implementation should check that what is in the cached packs corresponds to what is listed on the filesystem.

However, the WindowCursor implementation doesn't do that:

	/** {@inheritDoc} */
	@Override
	public Collection<CachedPack> getCachedPacksAndUpdate(
			BitmapBuilder needBitmap) throws IOException {
		for (PackFile pack : db.getPacks()) {
			PackBitmapIndex index = pack.getBitmapIndex();
			if (needBitmap.removeAllOrNone(index))
				return Collections.<CachedPack> singletonList(
						new LocalCachedPack(Collections.singletonList(pack)));
		}
		return Collections.emptyList();
	}

And if you suspect that maybe the 'db.getPacks()' does it, you would be disappointed too when you look in ObjectDirectory:

	@Override
	public Collection<PackFile> getPacks() {
		PackList list = packList.get();
		if (list == NO_PACKS)
			list = scanPacks(list);
		PackFile[] packs = list.packs;
		return Collections.unmodifiableCollection(Arrays.asList(packs));
	}

--- * ---

Bottom line: moving the packfile to the objects/pack/preserved *solves temporarily* the issue, but then *IF* the cached pack list isn't getting refreshed, it would then fail with "Stale file handle" after the jgit gc --prune-preserved stage.

I believe the issue here is that there is a bug in WindowCursor.getCachedPacksAndUpdate() because it should check that the cached pack list corresponds to what is found in the repository at *that* moment.

I am now going to check the stable-5.11 version of the code to see if the problem still exists or not.

Comment 23 Luca Milanesio 2021-05-27 18:33:10 EDT

I checked in stable-5.11 and master: the problem is still there :-(

By looking at [1], Martin's change that moves the preserved pack back into the non-preserved directory, that operates only when an object is referenced but NOT when the entire pack is selected for being transferred over the wire and therefore doesn't help.

I am going now to write a unit-test and then a fix for the issue.

@Matthias is stable-5.9 good enough? Or should I go further back in time? What is the earliest non-EOL version of JGit where the problem should be fixed?

Luca.

[1] https://git.eclipse.org/r/c/jgit/jgit/+/122288

Comment 24 Eclipse Genie 2021-05-27 21:59:12 EDT

New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/181123

Comment 25 Luca Milanesio 2021-05-27 22:00:11 EDT

I have attempted a possible fix and added a test-case.
I am not sure though that the test case is in the correct suite :-(

@Matthias what do you think?

Comment 26 Luca Milanesio 2021-05-27 22:00:48 EDT

The build is green for me (locally)

Comment 27 Conal McLaughlin 2021-06-02 05:21:10 EDT

We have observed this issue on non NFS filesystems as well during out of band CGit GC runs. This resulted in Gerrit holding onto (Deleted) file descriptors. 
We didn't observe the Stale file handle exception explicitly, but accumulating stale file handles were observed in the system.

It appeared to be that git was doing a rewrite of existing packfiles and leaving stale handles in the cache.

In fact, since CGit rewrote the file (with the same name), the cached entry existed in name, but pointed to a different inode on the filesystem.

To replicate, you can browse the Gerrit repositories (to populate WindowCache and open file descriptors), trigger a CGit GC (or maybe a jgit gc in your case) on one of the repositories. lsof command will now show a (Deleted) file descriptor and this entry will still be in the WindowCache and never cleaned up.

Comment 28 Fabio Ponciroli 2021-06-25 11:32:04 EDT

>To replicate, you can browse the Gerrit repositories (to populate WindowCache and >open file descriptors), trigger a CGit GC (or maybe a jgit gc in your case) on one >of the repositories. lsof command will now show a (Deleted) file descriptor and >this entry will still be in the WindowCache and never cleaned up.

Hi @Conal,
can you give me a hand to better understand how you manage to reproduce it without NFS? I am trying to get at the bottom of the issue.

1) What do you mean by browse a Gerrit repository?
2) How exactly do you get the Deleted file descriptor with lsof?
3) Where exactly in WindowCache you check the file descriptor is still there?

Thanks,
Ponch

Comment 29 Eclipse Genie 2021-07-16 05:44:53 EDT

New Gerrit change created: https://git.eclipse.org/r/c/jgit/jgit/+/183118

Comment 30 Matthias Sohn 2024-01-29 04:14:44 EST

https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/205553
merged as c701c01b49d92993f1c3df0a0e26a2dd68b8cec1