Skip to content

Conversation

@mcyork
Copy link

@mcyork mcyork commented Dec 13, 2025

  • Remove redundant Flush() call before ForceSync() in periodic sync
  • Skip sync operations when using direct I/O (no page cache to flush)
  • Optimize macOS writes: use buffered I/O with /dev/rdisk for faster writes
  • Enable direct I/O only for verification reads to ensure accuracy

These optimizations improve write performance while maintaining data integrity of verify.

- Remove redundant Flush() call before ForceSync() in periodic sync
- Skip sync operations when using direct I/O (no page cache to flush)
- Optimize macOS writes: use buffered I/O with /dev/rdisk for faster writes
- Enable direct I/O only for verification reads to ensure accuracy

These optimizations improve write performance while maintaining data integrity.
@mcyork mcyork force-pushed the optimize-io-operations branch from 98afd57 to 9afc647 Compare December 13, 2025 06:03
Comment on lines +121 to +129
// Convert /dev/diskX to /dev/rdiskX for raw device access (like dd uses)
// Raw devices (/dev/rdisk) have better buffering behavior than block devices (/dev/disk)
std::string rawPath = path;
size_t diskPos = rawPath.find("/dev/disk");
if (diskPos != std::string::npos) {
rawPath.replace(diskPos + 5, 4, "rdisk"); // Replace "disk" with "rdisk"
std::cout << "Using raw device path: " << rawPath << " (instead of " << path << ")" << std::endl;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh? This is already happening:

// Use raw disk device for direct I/O - bypasses macOS buffer cache

Comment on lines +1499 to +1500
// ForceSync() already handles both flushing and syncing to disk
// No need for redundant Flush() call - ForceSync() does everything
Copy link
Collaborator

Choose a reason for hiding this comment

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

The point of this functionality is to avoid excessive page cache usage, which was cited in early testing as causing problems on memory-constrained systems.

Comment on lines +164 to +168
// Use buffered I/O for writes (like v1.9.6) - much faster!
// Direct I/O (F_NOCACHE) will be enabled only for verification reads
// in PrepareForSequentialRead() to ensure accurate verification.
// This gives us: fast writes + accurate verification
using_direct_io_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As below, the intent here was to have more control over the memory pressure Imager was exacting on the system, but clearly on macOS that comes with a substantial write cost.

At this stage, I'm going to have to consider the balance of harms - and that might take a while.

@tdewey-rpi
Copy link
Collaborator

Thanks for the contribution, @mcyork!

I particularly appreciate someone taking the time to model out their ideas in a PR, and measure the output! (Honestly, given the scarcity of community-sourced PRs, I nearly fell out of my chair when I saw this!)

That said, I think there's a block you can probably remove - and the Buffered I/O change will need to be considered against earlier testing results (where systems under memory pressure behaved poorly - extremely long 'flush' cycles, allocation thrashing in other applications, etc). Imager 2.0 tries to be very deliberate about it's memory allocation and management - mostly so that if it's causing problems, I can hold my hands up and consider biasing the algorithm further.

tldr; Not an immediate merge, but a valued contribution that I'll evaluate further.

@mcyork
Copy link
Author

mcyork commented Dec 17, 2025 via email

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants