Skip to content

NIOFileSystem: Add on-disk size to FileInfo - #3656

Open
thliu21 wants to merge 3 commits into
apple:mainfrom
thliu21:fix/3650-on-disk-size
Open

NIOFileSystem: Add on-disk size to FileInfo#3656
thliu21 wants to merge 3 commits into
apple:mainfrom
thliu21:fix/3650-on-disk-size

Conversation

@thliu21

@thliu21 thliu21 commented Jul 10, 2026

Copy link
Copy Markdown

Summary

Add FileInfo.onDiskSize so callers can retrieve the number of bytes a file actually occupies on disk, independently of its apparent size.

This is useful for sparse and compressed files, where FileInfo.size can differ significantly from the allocated storage.

Fixes #3650.

Changes

  • Add the public FileInfo.onDiskSize property to both NIOFS and _NIOFileSystem.
  • Calculate the value from S_BLKSIZE * stat.st_blocks, without using stat.st_blksize.
  • Use musl's equivalent DEV_BSIZE constant where S_BLKSIZE is unavailable.
  • Extend the value-based FileInfo initializer with an onDiskSize argument.
  • Add a conversion test that verifies 9 allocated blocks produce 4,608 bytes even when st_blksize is set to 10.

Testing

  • swift test --jobs 2 --filter NIOFSTests.FileInfoTests
    • 3 tests passed, 0 failures.
  • swift test --skip-build --filter NIOFSTests
    • 135 tests passed, 3 platform-specific tests skipped, 0 failures.
  • swift format lint --strict --configuration .swift-format Sources/NIOFS/FileInfo.swift Sources/_NIOFileSystem/FileInfo.swift Tests/NIOFSTests/FileInfoTests.swift
    • Passed.
  • git diff --check
    • Passed.

@thliu21
thliu21 marked this pull request as ready for review July 10, 2026 20:10
@weissi
weissi requested a review from glbrntt July 11, 2026 10:56
XCTAssertEqual(info.userID, FileInfo.UserID(rawValue: 5))
XCTAssertEqual(info.groupID, FileInfo.GroupID(rawValue: 6))
XCTAssertEqual(info.size, 8)
XCTAssertEqual(info.onDiskSize, 9 * 512)

@weissi weissi Jul 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we maybe add a test case which creates a file with a hole? Should be pretty easy: FileSystem.shared.withFileOpen(forWriting: ".../stuff") { handle in handle.write("hi"); handle.resize(128 MB) } then check that the on-disk size is under 1MB for UNIX platforms

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an integration regression test that writes hi, resizes the file to 128 MiB, and verifies the logical size while asserting the on-disk allocation remains below 1 MiB.

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jul 20, 2026
private let S_BLKSIZE = Musl.DEV_BSIZE
#elseif canImport(Android)
private let S_BLKSIZE = Android.S_BLKSIZE
#endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakepetroules To avoid an in-flight regression of Windows, can you identify what the appropriate operation is there?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the Windows port. The corresponding API for actual allocated bytes is GetCompressedFileSizeW. Because FileInfo currently receives only BY_HANDLE_FILE_INFORMATION while that API is path-based, I need to thread the value through the path/handle information flow and add a Windows sparse-file test rather than add an inaccurate fallback.

@thliu21

thliu21 commented Jul 22, 2026

Copy link
Copy Markdown
Author

Small correction to my previous reply: GetCompressedFileSizeW is path-based, but the closest documented handle-based API is GetFileInformationByHandleEx with FileCompressionInfo, reading FILE_COMPRESSION_INFO.CompressedFileSize. That would avoid path races.

The newly landed NIOFileSystem Windows port is currently compile-only and its stat/fstat paths are still stubs, so I cannot validate a real Windows sparse-file implementation yet. Would you prefer onDiskSize to be optional and return nil on Windows temporarily, or should this PR wait until the Windows backend can query FileCompressionInfo? I can implement whichever direction you prefer.

PR #3668 currently takes the optional/nil route, but does not provide a real Windows implementation or the sparse-file integration test.

@jakepetroules

Copy link
Copy Markdown
Member

Small correction to my previous reply: GetCompressedFileSizeW is path-based, but the closest documented handle-based API is GetFileInformationByHandleEx with FileCompressionInfo, reading FILE_COMPRESSION_INFO.CompressedFileSize. That would avoid path races.

The newly landed NIOFileSystem Windows port is currently compile-only and its stat/fstat paths are still stubs, so I cannot validate a real Windows sparse-file implementation yet. Would you prefer onDiskSize to be optional and return nil on Windows temporarily, or should this PR wait until the Windows backend can query FileCompressionInfo? I can implement whichever direction you prefer.

PR #3668 currently takes the optional/nil route, but does not provide a real Windows implementation or the sparse-file integration test.

I think you can fatalError for now.

@thliu21
thliu21 force-pushed the fix/3650-on-disk-size branch from 1ef50f0 to 93757ec Compare July 30, 2026 21:01
@thliu21

thliu21 commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks. I’ve rebased onto the current main and added the temporary Windows-only fatalError("FileInfo.onDiskSize is not implemented on Windows") to both NIOFS and _NIOFileSystem, while keeping the POSIX allocation calculation unchanged.

The focused FileInfoTests pass 3/3, the sparse-file integration test passes 1/1, strict formatting and git diff --check pass, and both NIOFS targets build locally. I can’t complete an actual Windows build on this host because it lacks the Windows SDK, so Windows compilation will still need CI confirmation.

Could you please trigger CI and re-review when convenient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NIOFileSystem: No API to get the on-disk size

4 participants