Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 4 additions & 16 deletions src/downloadthread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1471,11 +1471,8 @@ void DownloadThread::_periodicSync()
return;
}

// Skip periodic sync when direct I/O is enabled (F_NOCACHE on macOS, O_DIRECT on Linux).
// With direct I/O, data bypasses the page cache entirely, so periodic fsync() provides
// no benefit - the data is already going directly to the device. This avoids the severe
// performance penalty of frequent fsync() calls on slow media like SD cards over USB.
// The final sync at the end of the write is still performed to ensure data integrity.
// Skip periodic syncs when using direct I/O - there's no page cache to flush
// Direct I/O bypasses the OS page cache, so syncing is unnecessary and wasteful
if (_file && _file->IsDirectIOEnabled()) {
return;
}
Expand All @@ -1499,17 +1496,8 @@ void DownloadThread::_periodicSync()
<< timeSinceLastSync << "ms elapsed)"
<< "on" << SystemMemoryManager::instance().getPlatformName();

// Use unified FileOperations for flushing and syncing
if (_file->Flush() != rpi_imager::FileError::kSuccess) {
quint64 syncMs = static_cast<quint64>(syncTimer.elapsed());
_writeTimingStats.totalSyncMs.fetch_add(syncMs);
_writeTimingStats.syncCount.fetch_add(1);
emit eventPeriodicSync(static_cast<quint32>(syncMs), false, currentBytes);
qDebug() << "Warning: Flush() failed during periodic sync";
return;
}

// Force filesystem sync using unified FileOperations
// ForceSync() already handles both flushing and syncing to disk
// No need for redundant Flush() call - ForceSync() does everything
Comment on lines +1499 to +1500
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.

if (_file->ForceSync() != rpi_imager::FileError::kSuccess) {
quint64 syncMs = static_cast<quint64>(syncTimer.elapsed());
_writeTimingStats.totalSyncMs.fetch_add(syncMs);
Expand Down
29 changes: 19 additions & 10 deletions src/mac/file_operations_macos.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,18 @@ FileError MacOSFileOperations::OpenDevice(const std::string& path) {
if (isBlockDevice) {
std::cout << "Device path detected, using macOS authorization..." << std::endl;

// 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;
}

Comment on lines +121 to +129
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

// Create a MacFile instance to handle authorization
MacFile macfile;
QByteArray devicePath = QByteArray::fromStdString(path);
QByteArray devicePath = QByteArray::fromStdString(rawPath);

auto authResult = macfile.authOpen(devicePath);
if (authResult == MacFile::authOpenCancelled) {
Expand Down Expand Up @@ -152,17 +161,14 @@ FileError MacOSFileOperations::OpenDevice(const std::string& path) {
fd_ = duplicated_fd;
current_path_ = path;

// Enable direct I/O via F_NOCACHE for block devices
// This bypasses the unified buffer cache for:
// 1. Better performance by avoiding double-buffering
// 2. More accurate verification (reads from actual device, not cache)
// 3. Reduced memory pressure on the system
if (!EnableDirectIO()) {
std::cout << "Warning: Could not enable direct I/O, continuing with buffered I/O" << std::endl;
}
// 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;
Comment on lines +164 to +168
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.


std::cout << "Successfully opened device with authorization, fd=" << fd_
<< (using_direct_io_ ? " (direct I/O enabled)" : " (buffered I/O)") << std::endl;
<< " (buffered I/O for writes, direct I/O for verification)" << std::endl;
return FileError::kSuccess;
}

Expand Down Expand Up @@ -304,6 +310,9 @@ FileError MacOSFileOperations::WriteSequential(const std::uint8_t* data, std::si
return FileError::kOpenError;
}

// With buffered I/O (no F_NOCACHE), let the kernel handle buffering.
// Write the full size at once - kernel will buffer and write asynchronously.
// This matches v1.9.6 behavior which was much faster.
std::size_t bytes_written = 0;
while (bytes_written < size) {
ssize_t result = write(fd_, data + bytes_written, size - bytes_written);
Expand Down