From dcf4948bff8c906937cd5b463d59dc81e9b5f208 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Fri, 27 Mar 2026 00:07:05 -0400 Subject: [PATCH] kernel/vm: Add better handling for busy-writing pages on cache resizes. Previously, busy-writing pages just stayed in the cache until I/O finished. This wasn't right, as they should've been unmapped (and removed) immediately, as a TODO in the PageWriteWrapper noted. Now they are, and they're moved into a queue on the cache pending the I/O completing. This paves the way for fixing #18390. Change-Id: I51778ba1cfc1d26f34dd44f1ee293381be59937f Reviewed-on: https://review.haiku-os.org/c/haiku/+/8627 Reviewed-by: waddlesplash Tested-by: Commit checker robot --- headers/private/kernel/vm/VMCache.h | 6 +++ src/system/kernel/vm/VMCache.cpp | 51 +++++++++++++++++------- src/system/kernel/vm/vm_page.cpp | 61 ++++++++++++----------------- 3 files changed, 68 insertions(+), 50 deletions(-) diff --git a/headers/private/kernel/vm/VMCache.h b/headers/private/kernel/vm/VMCache.h index 2180081b88..99bb2f638e 100644 --- a/headers/private/kernel/vm/VMCache.h +++ b/headers/private/kernel/vm/VMCache.h @@ -115,6 +115,7 @@ public: { if (fPageEventWaiters != NULL) _NotifyPageEvents(page, events); } inline void MarkPageUnbusy(vm_page* page); + void FreeRemovedPage(vm_page* page); vm_page* LookupPage(off_t offset); void InsertPage(vm_page* page, off_t offset); @@ -231,12 +232,17 @@ private: bool _FreePageRange(VMCachePagesTree::Iterator it, page_num_t* toPage, page_num_t* freedPages); +private: + typedef DoublyLinkedQueue, &vm_page::cache_link> > RemovedPagesQueue; + private: int32 fRefCount; mutex fLock; PageEventWaiter* fPageEventWaiters; void* fUserData; VMCacheRef* fCacheRef; + RemovedPagesQueue fRemovedBusyPages; page_num_t fWiredPagesCount; uint64 fFaultCount; diff --git a/src/system/kernel/vm/VMCache.cpp b/src/system/kernel/vm/VMCache.cpp index 174f5fd667..d5b0abf207 100644 --- a/src/system/kernel/vm/VMCache.cpp +++ b/src/system/kernel/vm/VMCache.cpp @@ -678,6 +678,8 @@ VMCache::Delete() panic("cache %p to be deleted still has areas", this); if (!consumers.IsEmpty()) panic("cache %p to be deleted still has consumers", this); + if (!fRemovedBusyPages.IsEmpty()) + panic("cache %p to be deleted still has removed busy pages", this); T(Delete(this)); @@ -818,6 +820,20 @@ VMCache::InsertPage(vm_page* page, off_t offset) } +/*! Frees a page that was removed by _FreePageRange(), but which was busy + and couldn't be freed then. +*/ +void +VMCache::FreeRemovedPage(vm_page* page) +{ + AssertLocked(); + + NotifyPageEvents(page, PAGE_EVENT_NOT_BUSY); + fRemovedBusyPages.Remove(page); + vm_page_free(this, page); +} + + /*! Removes the vm_page from this cache. Of course, the page must really be in this cache or evil things will happen. The cache lock must be held. @@ -1103,33 +1119,38 @@ VMCache::_FreePageRange(VMCachePagesTree::Iterator it, page = it.Next()) { if (page->busy) { - if (page->busy_writing) { - // We cannot wait for the page to become available - // as we might cause a deadlock this way - page->busy_writing = false; - // this will notify the writer to free the page - if (freedPages != NULL) - (*freedPages)++; - continue; + if (!page->busy_writing) { + // wait for page to become unbusy + WaitForPageEvents(page, PAGE_EVENT_NOT_BUSY, true); + return true; } - // wait for page to become unbusy - WaitForPageEvents(page, PAGE_EVENT_NOT_BUSY, true); - return true; + // We cannot wait for the page to become available + // as we might cause a deadlock this way + page->busy_writing = false; + // this will notify the writer to free the page } - // remove the page and put it into the free queue - DEBUG_PAGE_ACCESS_START(page); - vm_remove_all_page_mappings(page); ASSERT(page->WiredCount() == 0); // TODO: Find a real solution! If the page is wired // temporarily (e.g. by lock_memory()), we actually must not // unmap it! + + // remove the page and put it into the free queue + DEBUG_PAGE_ACCESS_START(page); + vm_remove_all_page_mappings(page); + RemovePage(page); // Note: When iterating through a IteratableSplayTree // removing the current node is safe. - vm_page_free(this, page); + if (page->busy) { + fRemovedBusyPages.Add(page); + DEBUG_PAGE_ACCESS_END(page); + } else { + vm_page_free(this, page); + } + if (freedPages != NULL) (*freedPages)++; } diff --git a/src/system/kernel/vm/vm_page.cpp b/src/system/kernel/vm/vm_page.cpp index 2a57003ea1..58bd71fbc9 100644 --- a/src/system/kernel/vm/vm_page.cpp +++ b/src/system/kernel/vm/vm_page.cpp @@ -2087,54 +2087,45 @@ PageWriteWrapper::Done(status_t result) DEBUG_PAGE_ACCESS_START(fPage); fPage->busy = false; - // Set unbusy and notify later by hand, since we might free the page. + // Set unbusy and notify later by hand. bool success = true; - if (result == B_OK) { + if (!fPage->busy_writing) { + // The busy_writing flag was cleared. That means the cache tried to remove + // the page while we were trying to write it. Let the cache handle the rest. + fCache->FreeRemovedPage(fPage); + } else if (result == B_OK) { // put it into the active/inactive queue move_page_to_appropriate_queue(fPage); fPage->busy_writing = false; DEBUG_PAGE_ACCESS_END(fPage); + + fCache->NotifyPageEvents(fPage, PAGE_EVENT_NOT_BUSY); } else { - // Writing the page failed. One reason would be that the cache has been - // shrunk and the page does no longer belong to the file. Otherwise the - // actual I/O failed, in which case we'll simply keep the page modified. + // Writing the page failed -- mark the page modified and move it to + // an appropriate queue other than the modified queue, so we don't + // keep trying to write it over and over again. We keep + // non-temporary pages in the modified queue, though, so they don't + // get lost in the inactive queue. + dprintf("PageWriteWrapper: Failed to write page %p: %s\n", fPage, + strerror(result)); - if (!fPage->busy_writing) { - // The busy_writing flag was cleared. That means the cache has been - // shrunk while we were trying to write the page and we have to free - // it now. - vm_remove_all_page_mappings(fPage); -// TODO: Unmapping should already happen when resizing the cache! - fCache->RemovePage(fPage); - free_page(fPage, false); - unreserve_pages(1); - } else { - // Writing the page failed -- mark the page modified and move it to - // an appropriate queue other than the modified queue, so we don't - // keep trying to write it over and over again. We keep - // non-temporary pages in the modified queue, though, so they don't - // get lost in the inactive queue. - dprintf("PageWriteWrapper: Failed to write page %p: %s\n", fPage, - strerror(result)); + fPage->modified = true; + if (!fCache->temporary) + set_page_state(fPage, PAGE_STATE_MODIFIED); + else if (fPage->IsMapped()) + set_page_state(fPage, PAGE_STATE_ACTIVE); + else + set_page_state(fPage, PAGE_STATE_INACTIVE); - fPage->modified = true; - if (!fCache->temporary) - set_page_state(fPage, PAGE_STATE_MODIFIED); - else if (fPage->IsMapped()) - set_page_state(fPage, PAGE_STATE_ACTIVE); - else - set_page_state(fPage, PAGE_STATE_INACTIVE); + fPage->busy_writing = false; + DEBUG_PAGE_ACCESS_END(fPage); - fPage->busy_writing = false; - DEBUG_PAGE_ACCESS_END(fPage); - - success = false; - } + fCache->NotifyPageEvents(fPage, PAGE_EVENT_NOT_BUSY); + success = false; } - fCache->NotifyPageEvents(fPage, PAGE_EVENT_NOT_BUSY); fIsActive = false; return success;