compact: copy-forward packs, retire Repository.delete#9777
Conversation
…p#8572 Reclaim space at pack granularity: drop fully-unused packs whole and rewrite mixed packs by copying referenced objects forward (N>1 path, max_count-guarded). check --repair and debug delete-obj now drop by pack_id; Repository.delete() is gone.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9777 +/- ##
==========================================
+ Coverage 84.87% 84.90% +0.03%
==========================================
Files 92 92
Lines 15167 15194 +27
Branches 2271 2275 +4
==========================================
+ Hits 12873 12901 +28
+ Misses 1590 1589 -1
Partials 704 704 ☔ View full report in Codecov by Harness. |
| pack_id = self.chunks[defect_chunk].pack_id | ||
| del self.chunks[defect_chunk] | ||
| self.repository.delete(defect_chunk) | ||
| self.repository.delete_pack(pack_id) |
There was a problem hiding this comment.
The goal is to get rid of the N=1 limitation, so I suggest you don't add code that makes no sense and is even very harmful when N!=1.
| def compact_packs(self): | ||
| """Reclaim space at the granularity the store supports: whole pack files. | ||
|
|
||
| The store is append-only and content-addressed, so we never delete a single object in |
There was a problem hiding this comment.
append-only is a term from the borg 1.x past, don't use it if it makes no sense anymore, that just adds confusion.
at the store level, full packs are just added usually. removed only by special borg operations like compact or check.
there is also no ordering anymore as in 1.x segment files 0..N, so when looking at the whole store, one maybe can say add-only, but append-only implies an "end" of the store that does not exist anymore.
ThomasWaldmann
left a comment
ThomasWaldmann
left a comment
There was a problem hiding this comment.
needs optimisation or RAM usage will explode.
| for id, entry in self.chunks.iteritems(): | ||
| bucket = packs.setdefault(entry.pack_id, {"used": [], "unused": []}) |
There was a problem hiding this comment.
Never do whole-chunkindex transformations, building new datastructures that contain most of the information from the chunks index. The chunks index is a highly optimized datastructure and even then, it can be many gigabytes in size. If you are transforming that into another arrangement using unoptimized python datastructures, you will likely use too much RAM.
| packs_to_drop = [pid for pid, b in packs.items() if b["unused"] and not b["used"]] | ||
| packs_to_rewrite = [pid for pid, b in packs.items() if b["unused"] and b["used"]] |
There was a problem hiding this comment.
Basically the right idea, just needs a more efficient implementation.
| del self.chunks[id] | ||
| pi.finish() | ||
|
|
||
| def _copy_forward_pack(self, pack_id, used_ids): |
There was a problem hiding this comment.
usually this is named "compaction".
| writer = PackWriter(store, max_count=self.repository.pack_max_count, chunks=self.chunks) | ||
| for id in used_ids: | ||
| entry = self.chunks[id] | ||
| key = "packs/" + bin_to_hex(entry.pack_id) | ||
| cdata = store.load(key, offset=entry.obj_offset, size=entry.obj_size) | ||
| writer.add(id, cdata) | ||
| writer.flush() |
There was a problem hiding this comment.
Use borgstore.Store.defrag method, that is way more efficient.
|
|
||
| def delete(self, id, wait=True): | ||
| """delete a repo object | ||
| def delete_pack(self, pack_id, wait=True): |
There was a problem hiding this comment.
guess you could just use .store_delete at the few places that need this.
2 participants
Description
Reclaiming space in a borg2 repo used to go through
Repository.delete(id), which removes one object at a time. That works today because every pack holds exactly one object (N=1): the pack id equals the chunk id, so deleting the object and deleting the pack are the same thing. It stops working once a pack bundles several objects (N>1), because you cannot cut a single object out of an append-only pack file.This moves compaction to pack granularity:
The mixed-pack case can only show up once a pack holds more than one object, so it sits behind the
max_countguard and is unreachable at N=1. Rather than ship that branch untested, there is a test that forcesmax_count > 1to build a real mixed pack and run copy-forward on it, so the design is proven now instead of promised for later.Repository.delete()is gone. In its place isRepository.delete_pack(pack_id). The two callers that still worked at object level now drop by pack id:check --repairwhen it hits a defective object, anddebug delete-obj.The crash-safety fix from #9748 still holds: cached chunk indexes are invalidated before the first store change, so an interrupted compact stays on the safe side and the next client rebuilds from what is actually in the repo.
Changes:
repository.py: replacedelete(id)withdelete_pack(pack_id); add apack_max_countproperty so compact can size its writer.compact_cmd.py: group surviving objects by pack id, drop the all-unused packs, and copy-forward the mixed ones with aPackWriterbound to the GC chunk index.archive.py:check --repairdrops the bad object's pack by pack id.debug_cmd.py:delete-objroutes throughdelete_pack.test_compact_drops_whole_unused_packscovers the N=1 whole-pack drop,test_compact_copy_forward_mixed_packforcesmax_countto build and compact a mixed pack, and the directdelete()callers in the suite switch todelete_pack().refs #8572
Checklist
master(or maintenance branch if only applicable there)toxor the relevant test subset)