feat: allow replicas on full disks reclaim the space and squash all data into the heads - #572
feat: allow replicas on full disks reclaim the space and squash all data into the heads#572apoorvajagtap wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #572 +/- ##
=========================================
- Coverage 3.14% 2.80% -0.34%
=========================================
Files 42 51 +9
Lines 3629 4057 +428
=========================================
Hits 114 114
- Misses 3512 3940 +428
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c39b569 to
43801d9
Compare
apoorvajagtap
changed the title
684faa7 to
217130e
Compare
apoorvajagtap
changed the title
4e7548b to
799d6dc
Compare
We usually put a commit for updating vendor packages in the same PR. The commit title can be |
799d6dc to
5a83463
Compare
apoorvajagtap
changed the title
1e30f3a to
46482f6
Compare
|
This pull request is now in conflict. Could you fix it @apoorvajagtap? 🙏 |
| func dumpExtentsForFile(name string, f *os.File, totalSectors int64) error { | ||
| extents, err := getAllExtents(f, uint64(totalSectors)*sectorSize) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read extents for %s: %w", name, err) | ||
| } | ||
| fmt.Printf("=== %s: %d extents ===\n", name, len(extents)) | ||
| for _, e := range extents { | ||
| startSector := int64(e.Logical) / sectorSize | ||
| lengthSectors := int64(e.Length) / sectorSize | ||
| fmt.Printf(" logical=%d length=%d -> sectors [%d, %d)\n", | ||
| e.Logical, e.Length, startSector, startSector+lengthSectors) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Can we directly ask SectorMapping to dump its ExtentCache[fName] when it is not empty? getAllExtents is resource-consuming and should not be called as long as there is cache already.
Besides, whenExtentCache[fName] is empty, it means that the corresponding snapshot file has all data expired. There is no need to intentionally print out its extent. Instead, you can print a single line directly, like file xxx all data (size xxx) is expired, will skip retrieve its extents.
There was a problem hiding this comment.
I actually introduced a separate field to maintain the list of obsoleteFiles, to make the deletion of these files easier. Given that, modified the dumpExtentsForFile logic as well to print the range per ExtentCache[fName], and print the warning for all files in smap.ObsoleteFiles (for better checks). Let me know in case the printing of obsoleteFiles does not seem very relevant anymore.
| location, names := smap.Location, smap.Names | ||
|
|
||
| runStart := int64(0) | ||
| runOwner := Owner(location, names, 0, fallbackName) |
There was a problem hiding this comment.
I am fine with directly skipping this location if it's not in snap/head files. There is no need to figure out if it is in the backing file
There was a problem hiding this comment.
Updated in the latest commit. PTAL.
| fmt.Println("[dry-run] No holes to punch.") | ||
| return nil | ||
| } | ||
| if !common.Confirm("Do you want to proceed with hole punching?") { |
There was a problem hiding this comment.
I am not sure if we need to cache ops for this confirmation. The SectorMapping may already have consumed lots of memory if the file data is scattered (e.g., the data is like 4k hole 4k hole...)
There was a problem hiding this comment.
If we wanna skip the caching of ops, then scanning the extents twice could be an alternate, with just maintaining the opCount (instead of whole offset, length details).
This might be cheaper given the location & ExtentCache are already in memory?
There was a problem hiding this comment.
I tried scanning twice, so we don't pile up the memory with ops cache, will push the changes for further review. Along with this, I am planning to eliminate the extra dry-run scan for PromoteToHead(), as that seems a little overkill, given the sectors have already been punched, and it's now just about squashing everything at one place. So, probably, just a question about Do you want to proceed with promoting sectors to head? would do at this stage, without printing each sector being copied. WDYT?
| } | ||
|
|
||
| for _, ancestor := range ancestors { | ||
| allocated := intersect(runStart, runEnd, smap.ExtentCache[ancestor]) |
There was a problem hiding this comment.
This operation in this loop in total may be expensive. But currently, I have no better idea
There was a problem hiding this comment.
Can we use a binary search for func intersect? Then each intersect() takes O(log n) time.
// first interval whose runEnd > smap.ExtentCache[ancestor].Start — the 1st candidate that can overlap
i := sort.Search(len(smap.ExtentCache[ancestor]), func(i int) bool { return smap.ExtentCache[ancestor][i].End > runStart })
46482f6 to
90babc4
Compare
Signed-off-by: Apoorva Jagtap <apoorva.jagtap@suse.com>
Signed-off-by: Apoorva Jagtap <apoorva.jagtap@suse.com>
Signed-off-by: Apoorva Jagtap <apoorva.jagtap@suse.com>
90babc4 to
7afeaa9
Compare
|
The following are still WIP, I'll update the comment once all the comments are addressed. If b/w permits in the meantime, please feel free to look through the latest commits with rest of the changes. Thanks! |
Signed-off-by: Apoorva Jagtap <apoorva.jagtap@suse.com>
8b57d80 to
11ce150
Compare
Signed-off-by: Apoorva Jagtap <apoorva.jagtap@suse.com>
11ce150 to
a52484d
Compare
3 participants
Which issue(s) this PR fixes:
Issue longhorn/longhorn#13636
What this PR does / why we need it:
This PR so far:
Special notes for your reviewer:
Additional documentation or context