[io] Several fixes to handling of gaps and recovery#22481
Conversation
jblomer
changed the title
jblomer
force-pushed
the
tfile-fix-gaps-v2
branch
2 times, most recently
from
84edc84 to
b13a691
Compare
| } | ||
| if (fWritable) { | ||
| const Long64_t last = idcur - nbytes - 1; | ||
| if (fFree->Last() && static_cast<TFree *>(fFree->Last())->GetLast() + 1 == idcur) { |
There was a problem hiding this comment.
It feels like rather than equality a greater than would be more 'stable', isn't it?
There was a problem hiding this comment.
I think I'd prefer the strict equality because we construct it this way in the loop before. I.e., we want to pass the condition exactly when we added a free segment in the last loop iteration and in this case we constructed idcur to point just after the last free byte.
| if (nrecov) Write(); | ||
| Long64_t max_file_size = Long64_t(kStartBigFile); | ||
| if (max_file_size < fEND) | ||
| max_file_size = fEND + 1000000000; |
There was a problem hiding this comment.
What is the semantic of 1000000000? Should we either add a comment and/or use a const to express it?
There was a problem hiding this comment.
I couldn't figure out what the semantics of this value is. This constant was there before and it is sprinkled here and there. I guess it was meant to be "large enough gap so that we can fit any possible key" but mistakenly using 1GB instead of 1GiB. That's just my guess though.
There was a problem hiding this comment.
Historical review.
When supporting only small files, max_file_size was 2GB and a node in the free list was added from fEND to 2GB.
Then max_file_size was updated for (only) 64 bits platforms to 500 times 2GB with the correspond node in the free list (from fEND to 500*2GB)
Then this was 'simplifiedtomax_file_sizebeingkStartBigFileorkStartBigFIle + 1GBiffEND > kStartBigFile`.
So I guess you are right and we ought to 'fix' it to be 1GiB.
Test Results 22 files 22 suites 3d 12h 25m 41s ⏱️ For more details on these failures, see this check. Results for commit 2f9f3a1. ♻️ This comment has been updated with latest results. |
The free segment header for gaps >2GB was previously truncated, leaving the chain of segments broken. Now we will link together several smaller gaps on disk to keep the chain intact. The free list, however, will still contain one large gap.
In TFile::Recover(), incorrect free segments were added during recovery. The file offset counter should be incremented _after_ adding the free segment, as it is done now. Also, remove an unnecessary `Seek()`, which will take place at the next loop iteration anyway.
Do not use an existing free list during file recovery. It will result in stale and/or duplicated entries in the recovered free list. Instead, only use the information picked up from the segment headers.
jblomer
force-pushed
the
tfile-fix-gaps-v2
branch
from
b13a691 to
2f9f3a1
Compare
2 participants
Fixes
Replaces #19251
Fixes #19245