repart: Properly pre-calculate auto size of images#42972
Conversation
When passing --size=auto to repart, it will pre-calculate the image size and resize the image to that size before partitioning. Currently, that fails when passing a large grain size, complaining that the auto-sized image is too small to fit the data. The reason for this is that the current code simply assumes the GPT metadata size taken away from the usable size by fdisk is static (1044KiB), when it actually is more complicated than that: There's two ranges of GPT metadata: One at the beginning of the image, and one at the end of the image. And there's the first usable block that is defined by fdisk when creating the partition table. The static value of 1044KiB usually works, because fdisk sets the first usable block to 1MiB (so 1024KiB), leaving 20KiB of leeway for the secondary GPT at the end of the image. Now as soon as the first partition starts at an offset higher than 1024KiB, we lose the 20KiB leeway for the secondary GPT, and the partitions will no longer fit. What we should do, is first of all round up to the grain size instead of 4096 (as that's the minimum offset our first partition will start at), and second of all properly subtract the secondary GPT at the end. Also confirm we don't regress on this anymore by adding a test that uses a 2MiB grain size, breaking the old code.
Claude review of PR #42972 (cdfca35)The core GPT-geometry rework looks correct: forcing the leading reservation to Suggestions
Nits
|
| } | ||
|
|
||
| /* At the end, add Secondary partition table (minimum 16KiB) + GPT header (1 block) */ | ||
| minimal_size += (16 * 1024) + c->sector_size; |
There was a problem hiding this comment.
Claude: suggestion: The trailing secondary-GPT reservation ((16 * 1024) + c->sector_size) is added only to minimal_size, never to current_size. current_size is seeded from minimal_size at the point where it only holds the leading round_up(1MiB, grain_size) reservation, then accumulates the existing partitions' sizes. In the old code both were seeded from round_up_size(GPT_METADATA_SIZE, 4096), and the 1044KiB constant covered both the leading and trailing metadata regions, so the two stayed consistent. Now current_size under-accounts for the backup GPT by 16KiB + sector_size relative to minimal_size. Since current_size feeds the reported current size and need_free = LESS_BY(minimal_size, current_size), need_free ends up slightly inflated. Consider adding the trailing reservation to current_size as well (or seeding both consistently).
| * decides to set it lower, we made the image a bit larger than necessary, | ||
| * if it sets it higher, we'd have a problem. | ||
| */ | ||
| minimal_size = 1024 * 1024; |
There was a problem hiding this comment.
Claude: suggestion: This removes the named GPT_METADATA_SIZE constant in favor of inline literals (1024 * 1024 here and (16 * 1024) + c->sector_size at line 11274), which diverges from the file's idiom of defining size constants at the top (e.g. HARD_MIN_SIZE, LUKS2_METADATA_SIZE). The 16 * 1024 value is also duplicated between the comment above and line 11274, so the two can drift. Consider introducing named constants for the forced 1MiB first-usable-LBA and the minimum GPT partition-table size. Minor: the surrounding size macros use the ULL suffix while the new literals do not.
| output=$(systemd-repart --offline="$OFFLINE" \ | ||
| --empty=create \ | ||
| --size=auto \ | ||
| --definitions="$defs" \ | ||
| --seed="$seed" \ | ||
| --dry-run=no \ | ||
| --json=pretty \ | ||
| --grain-size=2097152 \ | ||
| "$imgs/auto") | ||
|
|
||
| output=$(sfdisk --dump "$imgs/auto") |
There was a problem hiding this comment.
Claude: suggestion: The output captured from the systemd-repart invocation is immediately overwritten by output=$(sfdisk --dump "$imgs/auto") on the next line and never used, so --json=pretty only produces output that is discarded. Either drop the capture entirely (like testcase_zero_uuid/testcase_copy_blocks, and remove --json=pretty), or assert against the repart JSON — the auto-size geometry it just computed is exactly the code path this PR fixes, so validating it directly would strengthen the regression test.
| } | ||
|
|
||
| /* At the end, add Secondary partition table (minimum 16KiB) + GPT header (1 block) */ | ||
| minimal_size += (16 * 1024) + c->sector_size; |
There was a problem hiding this comment.
Claude: nit: Unlike the per-partition accumulation just above (guarded by if (m > UINT64_MAX - minimal_size)), this addition has no overflow check. If the loop leaves minimal_size within 16KiB + sector_size of UINT64_MAX, it wraps to a tiny value and silently bypasses the "too large, refusing" protection. Only reachable with absurd (~16 EiB) configured partition sizes, so this is a consistency/defense-in-depth point rather than a practical bug; guarding it with the same UINT64_MAX - check would match the surrounding code.
When passing --size=auto to repart, it will pre-calculate the image size and resize the image to that size before partitioning. Currently, that fails when passing a large grain size, complaining that the auto-sized image is too small to fit the data.
The reason for this is that the current code simply assumes the GPT metadata size taken away from the usable size by fdisk is static (1044KiB), when it actually is more complicated than that:
There's two ranges of GPT metadata: One at the beginning of the image, and one at the end of the image. And there's the first usable block that is defined by fdisk when creating the partition table.
The static value of 1044KiB usually works, because fdisk sets the first usable block to 1MiB (so 1024KiB), leaving 20KiB of leeway for the secondary GPT at the end of the image.
Now as soon as the first partition starts at an offset higher than 1024KiB, we lose the 20KiB leeway for the secondary GPT, and the partitions will no longer fit.
What we should do, is first of all round up to the grain size instead of 4096 (as that's the minimum offset our first partition will start at), and second of all properly subtract the secondary GPT at the end.
Also confirm we don't regress on this anymore by adding a test that uses a 2MiB grain size, breaking the old code.