Implemented force-realloc-tail option for sync#31
Implemented force-realloc-tail option for sync#31Ralf1108 wants to merge 6 commits intoamadvance:masterfrom
Conversation
|
Thanks! I'll review it next week! |
There was a problem hiding this comment.
Pull request overview
This PR implements a new --force-realloc-tail (-X) option for the sync command that allows selective reallocation of only the files occupying the parity tail, rather than forcing a full parity reallocation. This feature aims to shrink oversized parity files more efficiently by reallocating only the blocks that prevent shrinking, avoiding the time-consuming process of either full reallocation or manual file movement.
Changes:
- Added
force_realloc_tailflag to the snapraid_option structure - Implemented command-line option
-X/--force-realloc-tailwith associated validation - Created
state_locate_mark_tail_blocks_for_resync()function to mark tail blocks for reallocation during sync
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| cmdline/state.h | Adds force_realloc_tail flag to snapraid_option struct |
| cmdline/snapraid.c | Implements command-line parsing, validation, and integration of force-realloc-tail option |
| cmdline/locate.h | Declares new state_locate_mark_tail_blocks_for_resync function |
| cmdline/locate.c | Implements tail block marking logic and refactors state_locate to extract common functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @Ralf1108 I started a Copilot review. Ignore all review comments about white spacing, they will be fixed automatically. Check the others. No idea if they are valid or not. Use your human common sense :) |
| struct snapraid_block* block = fs_file2block_get(file, f); | ||
|
|
||
| // TODO: check: is condition correct or should we always set BLOCK_STATE_REP? | ||
| if (block_state_get(block) == BLOCK_STATE_BLK) { |
There was a problem hiding this comment.
@amadvance please check if this is correct
| // TODO: check if this is the correct place for marking, before or after regular scan? | ||
| // "scan_file_keep()"" will be called inside the following "state_scan()" so we have to mark the file before | ||
| if(state.opt.force_realloc_tail) | ||
| state_locate_mark_tail_blocks_for_resync(&state, parity_tail); |
There was a problem hiding this comment.
@amadvance please check if this is correct place to mark the blocks
|
Hi @amadvance, I added 2 comments where you should check if the logic is correct and does not mess other stuff up 👍 |
Ralf1108
left a comment
There was a problem hiding this comment.
fixed all issues
|
btw. @amadvance can you convert the manual test in starting post into an automated test? |
|
@Ralf1108 Merged! The only significant change is that, when a file needs to be reallocated, it must now be reallocated in full. I've also added corresponding tests and updated the documentation. Thanks again! |
|
Thx for the merge! Question regarding data safety during sync with "--force-realloc-tail": docs I thought the parity of the affected files are valid until the parity file was shrunk which happens at the end of the sync process. |
|
The issue is that at the startup the files are reallocated, and the information of where was their parity is lost, even if the parity is still there. The program keeps track only of the new position in the parity. |
|
So a hint added to the docs for the "--force-realloc-tail" option to precheck the affected files could be helpful. |
Follow up from #30
I implemented the suggested "force-realloc-tail" option for the sync command.
It works but it should be checked that this marking of files is always safe and screws nothing up.
Here the manual test:
Init test raid
snapraid.conf:
DRU1:
PPU1
First sync
DRU1:
PPU1:
Adding big file ~ 20mb
DRU1:
PPU1:
Adding small file to be at the parity tail
DRU1:
PPU1:
Delete big file -> notice that the parity won't shrink
DRU1:
PPU1:
Checking for blocking files -> AnyFileAfterBigFile.txt blocks the shrinking because it was placed at the end of the parity file
Output:
Run sync with "--force-realloc-tail" to try to shrink parity file by at least 1 mb
DRU1:
PPU1:
Voila, parity shrunk to 3 files * 256kb blocksize/file => 768 kb
This operation won't have to realloc the whole raid which could save hours/days of time.
Also the moving of the files out of the raid, sync and move back and sync is also unnecessary. I would assume all data is now parity-protected all the time.
Via the "locate" operation it can be checked if this operation is necessary.
Feel free to check the code and adapt it to your snapraid guidelines 🙂