From 835f899fe12546a89f99c8299d10022d5fa8ebff Mon Sep 17 00:00:00 2001 From: Daniel Hammer Date: Thu, 9 Jul 2026 23:20:19 +0200 Subject: [PATCH] fix: eliminate desktop stall when deleting large files Deleting a big file froze the desktop for seconds: ext2 FreeBlock did 4 synchronous disk I/Os per data block (bitmap + BGDT read/write), and the file manager deleted single files inline on the desktop main thread. - Ext2: batch block frees per block group; keep the bitmap resident, clear bits in memory, flush bitmap + BGDT once per group transition. Also covers the truncate-on-overwrite path. - Ext2: refuse to mount volumes with block size > 4096; temp buffers throughout the driver are single 4 KiB pages, so larger blocks would overflow them (our mkfs always uses 4K). - Files: route single-file deletes past 4 MiB to the background worker + progress dialog; refuse (instead of stalling inline) when another file operation already owns the worker. Co-Authored-By: Claude Fable 5 --- kernel/src/Api/BuildNo.hpp | 2 +- kernel/src/Fs/Ext2.cpp | 111 ++++++++++++++++-- .../src/desktop/apps/filemanager/actions.cpp | 27 ++++- .../apps/filemanager/filemanager_internal.hpp | 6 + 4 files changed, 133 insertions(+), 13 deletions(-) diff --git a/kernel/src/Api/BuildNo.hpp b/kernel/src/Api/BuildNo.hpp index 47643ad..9aa0514 100644 --- a/kernel/src/Api/BuildNo.hpp +++ b/kernel/src/Api/BuildNo.hpp @@ -12,4 +12,4 @@ #pragma once -#define MONTAUK_BUILD_NUMBER 1 +#define MONTAUK_BUILD_NUMBER 3 diff --git a/kernel/src/Fs/Ext2.cpp b/kernel/src/Fs/Ext2.cpp index e37b750..81083a7 100644 --- a/kernel/src/Fs/Ext2.cpp +++ b/kernel/src/Fs/Ext2.cpp @@ -568,33 +568,111 @@ namespace Fs::Ext2 { return false; } + // Batched block-free state. FreeBlock() does 4 disk I/Os per block (read + + // write of both the block bitmap and the BGDT), so freeing a large file's + // thousands of blocks one at a time is pathologically slow -- the observed + // multi-second stall when deleting a big file. This batch keeps the current + // group's bitmap resident, clears bits in memory, and flushes the bitmap + + // BGDT once per block-group transition instead of once per block. Files are + // largely contiguous, so a delete collapses to a handful of I/Os. + struct BlockFreeBatch { + uint8_t* bitmap; // dedicated page holding the loaded group's bitmap + int group; // currently loaded group, -1 if none + uint32_t freedCount; // bits cleared since load (added to bgdt on flush) + bool dirty; + }; + + // Write the loaded group's bitmap and updated free-count to disk. Uses + // inst.blockBuf as scratch for the BGDT (callers keep indirect-tree data in + // their own temp buffers, so blockBuf is free here). + static void FreeBatchFlush(Ext2Instance& inst, BlockFreeBatch& batch) { + if (batch.group < 0) return; + if (batch.dirty) { + uint32_t g = (uint32_t)batch.group; + WriteBlock(inst, inst.bgdt[g].bg_block_bitmap, batch.bitmap); + + inst.bgdt[g].bg_free_blocks_count += batch.freedCount; + + uint32_t bgdtBlock = inst.firstDataBlock + 1; + uint32_t bgdtOffset = g * sizeof(BlockGroupDescriptor); + uint32_t bgdtBlockIdx = bgdtBlock + bgdtOffset / inst.blockSize; + uint32_t bgdtOffInBlock = bgdtOffset % inst.blockSize; + if (ReadBlock(inst, bgdtBlockIdx, inst.blockBuf)) { + memcpy(inst.blockBuf + bgdtOffInBlock, &inst.bgdt[g], + sizeof(BlockGroupDescriptor)); + WriteBlock(inst, bgdtBlockIdx, inst.blockBuf); + } + } + batch.group = -1; + batch.freedCount = 0; + batch.dirty = false; + } + + // Clear one block's bitmap bit in the batch, loading its group's bitmap on + // demand (flushing the previously loaded group first). + static void FreeBlockBatched(Ext2Instance& inst, BlockFreeBatch& batch, + uint32_t blockNum) { + if (blockNum < inst.firstDataBlock || blockNum >= inst.totalBlocks) return; + + uint32_t adjusted = blockNum - inst.firstDataBlock; + uint32_t g = adjusted / inst.blocksPerGroup; + uint32_t bit = adjusted % inst.blocksPerGroup; + if (g >= inst.groupCount) return; + + if (batch.group != (int)g) { + FreeBatchFlush(inst, batch); + if (!ReadBlock(inst, inst.bgdt[g].bg_block_bitmap, batch.bitmap)) { + batch.group = -1; // leave unloaded rather than corrupt a bitmap + return; + } + batch.group = (int)g; + } + + uint32_t byteIdx = bit / 8; + uint8_t bitMask = 1 << (bit % 8); + if (batch.bitmap[byteIdx] & bitMask) { + batch.bitmap[byteIdx] &= ~bitMask; + batch.freedCount++; + batch.dirty = true; + } + } + // Free all data blocks belonging to an inode (direct + indirect trees) static void FreeInodeBlocks(Ext2Instance& inst, Inode& inode) { uint32_t ptrsPerBlock = inst.blockSize / 4; + // A dedicated page holds the batch's resident bitmap. If it can't be + // allocated, fall back to the per-block FreeBlock path (correct, slow). + uint8_t* bitmapPage = (uint8_t*)Memory::g_pfa->AllocateZeroed(); + BlockFreeBatch batch{ bitmapPage, -1, 0, false }; + auto freeOne = [&](uint32_t b) { + if (bitmapPage) FreeBlockBatched(inst, batch, b); + else FreeBlock(inst, b); + }; + // Free direct blocks for (int i = 0; i < 12; i++) { if (inode.i_block[i]) { - FreeBlock(inst, inode.i_block[i]); + freeOne(inode.i_block[i]); inode.i_block[i] = 0; } } // Free single indirect if (inode.i_block[12]) { - // Need a temporary buffer since blockBuf is used by FreeBlock + // Indirect-tree data lives in its own temp page (not blockBuf, which + // the batch flush uses as BGDT scratch). uint32_t indBlock = inode.i_block[12]; - // Allocate a temp page for reading the indirect block uint8_t* tmpBuf = (uint8_t*)Memory::g_pfa->AllocateZeroed(); if (tmpBuf && ReadBlock(inst, indBlock, tmpBuf)) { for (uint32_t i = 0; i < ptrsPerBlock; i++) { uint32_t b; memcpy(&b, tmpBuf + i * 4, 4); - if (b) FreeBlock(inst, b); + if (b) freeOne(b); } } if (tmpBuf) Memory::g_pfa->Free(tmpBuf); - FreeBlock(inst, indBlock); + freeOne(indBlock); inode.i_block[12] = 0; } @@ -612,15 +690,15 @@ namespace Fs::Ext2 { for (uint32_t j = 0; j < ptrsPerBlock; j++) { uint32_t b; memcpy(&b, tmpBuf2 + j * 4, 4); - if (b) FreeBlock(inst, b); + if (b) freeOne(b); } } - FreeBlock(inst, indBlock); + freeOne(indBlock); } if (tmpBuf2) Memory::g_pfa->Free(tmpBuf2); } if (tmpBuf1) Memory::g_pfa->Free(tmpBuf1); - FreeBlock(inst, dblBlock); + freeOne(dblBlock); inode.i_block[13] = 0; } @@ -628,10 +706,13 @@ namespace Fs::Ext2 { if (inode.i_block[14]) { // For simplicity, just free the top-level triple indirect block. // Full triple-indirect traversal is rare and complex. - FreeBlock(inst, inode.i_block[14]); + freeOne(inode.i_block[14]); inode.i_block[14] = 0; } + FreeBatchFlush(inst, batch); + if (bitmapPage) Memory::g_pfa->Free(bitmapPage); + inode.i_blocks = 0; inode.i_size = 0; } @@ -1667,8 +1748,18 @@ namespace Fs::Ext2 { if (sb->s_inodes_count == 0 || sb->s_blocks_count == 0) return nullptr; if (sb->s_inodes_per_group == 0 || sb->s_blocks_per_group == 0) return nullptr; + // Reject block sizes above 4096: temp buffers throughout this driver + // (indirect-tree pages, dirBuf, the free-batch bitmap page) are single + // 4 KiB PFA pages, so a larger block would overflow them. Our own mkfs + // always uses 4K blocks; only foreign-formatted volumes are affected. uint32_t blockSize = 1024 << sb->s_log_block_size; - if (blockSize < 1024 || blockSize > 65536) return nullptr; + if (blockSize < 1024 || blockSize > 4096) { + if (blockSize > 4096) { + KernelLogStream(WARNING, "Ext2") << "Refusing to mount volume with " + << blockSize << " bytes/block (max supported is 4096)"; + } + return nullptr; + } // Determine inode size uint16_t inodeSize = 128; // default for rev 0 diff --git a/programs/src/desktop/apps/filemanager/actions.cpp b/programs/src/desktop/apps/filemanager/actions.cpp index b330a5f..e82a55b 100644 --- a/programs/src/desktop/apps/filemanager/actions.cpp +++ b/programs/src/desktop/apps/filemanager/actions.cpp @@ -154,10 +154,33 @@ void filemanager_delete_selected(FileManagerState* fm) { return; } - // Single plain file: delete immediately. + // Single plain file. Freeing a large file's blocks is a slow syscall + // (the fs driver rewrites allocation bitmaps per block), so deleting it + // inline would stall the desktop -- Files runs inside desktop.elf, so a + // blocked main thread freezes the compositor and cursor. Hand anything + // past the inline threshold to the background worker + progress dialog; + // small files still delete inline so there's no dialog flash. + int only = filtered[0]; char fullpath[512]; filemanager_build_fullpath(fullpath, 512, - fm->current_path, fm->entry_names[filtered[0]]); + fm->current_path, fm->entry_names[only]); + + if (fm->entry_sizes[only] > FILEMANAGER_DELETE_INLINE_MAX_BYTES) { + const char* one = fullpath; + if (filemanager_start_delete_job(fm, &one, 1)) { + montauk::mfree(filtered); + return; + } + if (fm->active_fileop) { + // Another operation owns the worker. Deleting a big file inline + // here would stall the desktop (the exact bug this path avoids), + // so refuse; its progress dialog tells the user we're busy. + montauk::mfree(filtered); + return; + } + // Job allocation failed (OOM): fall through to the inline delete. + } + montauk::mfree(filtered); montauk::fdelete(fullpath); filemanager_clear_multi_selection(fm); diff --git a/programs/src/desktop/apps/filemanager/filemanager_internal.hpp b/programs/src/desktop/apps/filemanager/filemanager_internal.hpp index ae62a24..3631879 100644 --- a/programs/src/desktop/apps/filemanager/filemanager_internal.hpp +++ b/programs/src/desktop/apps/filemanager/filemanager_internal.hpp @@ -54,6 +54,12 @@ struct FileManagerVirtualEntry { // misbehaving driver and bounds memory. Far above any realistic directory. inline constexpr int FM_ENTRY_HARD_CAP = 8192; +// Single-file deletes at or below this size are freed inline; larger files go +// to the background worker + progress dialog. Freeing a big file rewrites the +// filesystem allocation bitmaps and can take seconds on real disks, which would +// otherwise stall the desktop (Files runs inside desktop.elf). +inline constexpr uint64_t FILEMANAGER_DELETE_INLINE_MAX_BYTES = 4ull * 1024 * 1024; + struct FileManagerState { char current_path[256]; FileManagerLocation history[16];