Fix double deorphan caused by relocation mid dir remove #1099
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These are the same as the related reentrant variants, but by opting out of powerloss testing, we can test a much larger number of states without having to worry about the impact on powerloss testing runtime. Bumped CYCLES from 20 -> 2000. This reveals an orphan remove bug found by Hugh-Baoa.
Long story short: There is a specific case where removing a directory
can trigger a deorphan pass, but lfs_remove did not check for this,
would try to clean up the (already cleaned) directory orphan, and
trigger an assert:
lfs.c:4890:assert: assert failed with false, expected eq true
LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) > 0x000 || orphans >= 0);
The specific case being a remove commit that triggers a relocation that
creates an orphan.
This is also possible in lfs_rename, but only if you're renaming a
directory that implies a remove, which is a pretty rare operation.
---
This was probably an oversight introduced in the non-recursive commit
logic rework.
Fortunately the fix is to just check if we even have an orphan before
trying to remove it. We can rely on this instead of the file type, so
this fix shouldn't even increase the code size.
Found and root-caused by Hugh-Baoa
Tests passed ✓, Code: 17104 B (-0.1%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Long story short: There is a specific case where removing a directory can trigger a deorphan pass, but
lfs_removedid not check for this, would try to clean up the (already cleaned) directory orphan, and trigger an assert:The specific case being a remove commit that triggers a relocation that creates an orphan.
This is also possible in lfs_rename, but only if you're renaming a directory that implies a remove, which is a pretty rare operation.
This was probably an oversight introduced in the non-recursive commit logic rework (#658).
Fortunately the fix is to just check if we even have an orphan before trying to remove it. We can rely on this instead of the file type, so this fix shouldn't even increase the code size.
I've also significantly increased the state size of the orphan/relocation tests, by splitting them into reentrant and non-reentrant variants. Previously these were artificially limited by the impact to powerloss testing runtime. (Unrelated work introduces TEST_PLS to avoid this tradeoff, so eventually this should be a non-issue.)
Found and root-caused by @Hugh-Baoa
See #1086 for more info