fix: accept blocksize=4096 in fat32.Create for 4k native block devices#378
fix: accept blocksize=4096 in fat32.Create for 4k native block devices#378jfroy wants to merge 1 commit intodiskfs:masterfrom
Conversation
This patch allows creating FAT32 filesystems with 4K block size. This is intended to create UEFI System Partitions (ESPs) on NVME namespaces with a 4Kn format. This is a follow-up to PR diskfs#320 which added support for reading FAT32 filesystems with 4K block size. Signed-off-by: Jean-Francois Roy <jf@devklog.net> Assisted-by: Claude:claude-opus-4-7
|
I will work on and send tests after an initial review of the main patch. |
|
Looking at the Linux failure. I did the initial work on macOS, not surprised I didn’t catch everything. And then I’ll be submitting new tests for 4k creation. |
deitch
left a comment
There was a problem hiding this comment.
Overall, this is quite good. Thank you for submitting it. I had only a few questions about things that are unclear in one file.
| sectorsPerCluster = 128 | ||
| clusterBytes = 64 * KB | ||
| } | ||
| // FAT32 limits clusters to 32 KiB, but maintain 64 KiB behavior for 512 bps. |
There was a problem hiding this comment.
I don't understand this comment. If clusters are max 32K, then how does that last case make sense?
There was a problem hiding this comment.
Based on the next line, and the comment above, it looks like you are saying, it might be that the filesystem size is > 32GB, sector size != 512, in which case, force it back to 32KB cluster size? So is the logic?
- if sector size is 4K, then cluster bytes can be up to 32K
- if sector size is 512B, then cluster bytes can be up to 64K
Is that it? If not, kindly do explain; if not, do you mind updating the comments to make it clearer?
There was a problem hiding this comment.
64 KiB clusters are out of spec, but were allowed for 512 bps filesystems before this patch. So the current patch keeps the existing behavior for backward compat but is stricter for 4K bps.
I had Claude do an analysis on mkfs.fat and the behavior of go-diskfs is different. The thresholds are hardcoded as if sector size is 512, but divided into the actual sector count. This means the byte thresholds scale with sector size and mkfs.fat violates the spec for bps > 512:
┌──────┬──────────────┬───────────────┬─────┬─────────────────────┬──────────────────────┐
│ Tier │ 512B sectors │ 4096B sectors │ spc │ Cluster size (512B) │ Cluster size (4096B) │
├──────┼──────────────┼───────────────┼─────┼─────────────────────┼──────────────────────┤
│ 1 │ ≤260 MiB │ ≤2 GiB │ 1 │ 512 B │ 4 KiB │
├──────┼──────────────┼───────────────┼─────┼─────────────────────┼──────────────────────┤
│ 2 │ ≤8 GiB │ ≤64 GiB │ 8 │ 4 KiB │ 32 KiB │
├──────┼──────────────┼───────────────┼─────┼─────────────────────┼──────────────────────┤
│ 3 │ ≤16 GiB │ ≤128 GiB │ 16 │ 8 KiB │ 64 KiB │
├──────┼──────────────┼───────────────┼─────┼─────────────────────┼──────────────────────┤
│ 4 │ ≤32 GiB │ ≤256 GiB │ 32 │ 16 KiB │ 128 KiB │
├──────┼──────────────┼───────────────┼─────┼─────────────────────┼──────────────────────┤
│ 5 │ >32 GiB │ >256 GiB │ 64 │ 32 KiB │ 256 KiB │
└──────┴──────────────┴───────────────┴─────┴─────────────────────┴──────────────────────┘
See also https://github.com/dosfstools/dosfstools/blob/289a48b9cb5b3c589391d28aa2515c325c932c7a/src/mkfs.fat.c#L659 and https://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc (alt: https://aeb.win.tue.nl/linux/fs/fat/fatgen103.pdf)
Linux's fat driver does not care about bytes per cluster. So the concern is likely for Windows and for UEFI firmware implementations.
| clusterBytes = 32 * KB | ||
| } | ||
| sectorsPerCluster := uint8(clusterBytes / blocksize) | ||
| if sectorsPerCluster == 0 { |
There was a problem hiding this comment.
At this point, is it an error if sectorsPerCluster == 0? You just set it as:
sectorsPerCluster := uint8(clusterBytes / blocksize)And earlier, you handled setting clusterBytes for every possible case, except if filesystem size > maxFAT32Size, but we handled that on line 86. So how can this line ever resolve as true?
There was a problem hiding this comment.
It's an integer division. So if the requested filesystem size is very small, you can get clusterBytes < blocksize, which will yield zero. I can update this to use max instead.
This patch allows creating FAT32 filesystems with 4K block size. This is intended to create UEFI System Partitions (ESPs) on NVME namespaces with a 4Kn format.
This is a follow-up to PR #320 which added support for reading FAT32 filesystems with 4K block size.