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 <[email protected]>
Tested-by: Commit checker robot <[email protected]>

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, DoublyLinkedListCLink<vm_page,
+		SplayTreeLink<vm_page>, &vm_page::cache_link> > RemovedPagesQueue;
+
 private:
 			int32				fRefCount;
 			mutex				fLock;
 			PageEventWaiter*	fPageEventWaiters;
 			void*				fUserData;
 			VMCacheRef*			fCacheRef;
+			RemovedPagesQueue	fRemovedBusyPages;
 
 			page_num_t			fWiredPagesCount;
 			uint64				fFaultCount;

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) {
+			if (!page->busy_writing) {
-				// We cannot wait for the page to become available
+				// wait for page to become unbusy
-				// as we might cause a deadlock this way
+				WaitForPageEvents(page, PAGE_EVENT_NOT_BUSY, true);
-				page->busy_writing = false;
+				return true;
-					// this will notify the writer to free the page
-				if (freedPages != NULL)
-					(*freedPages)++;
-				continue;
 			}
 
-			// wait for page to become unbusy
+			// We cannot wait for the page to become available
-			WaitForPageEvents(page, PAGE_EVENT_NOT_BUSY, true);
+			// as we might cause a deadlock this way
-			return true;
+			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)++;
 	}

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
+		// Writing the page failed -- mark the page modified and move it to
-		// shrunk and the page does no longer belong to the file. Otherwise the
+		// an appropriate queue other than the modified queue, so we don't
-		// actual I/O failed, in which case we'll simply keep the page modified.
+		// 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) {
+		fPage->modified = true;
-			// The busy_writing flag was cleared. That means the cache has been
+		if (!fCache->temporary)
-			// shrunk while we were trying to write the page and we have to free
+			set_page_state(fPage, PAGE_STATE_MODIFIED);
-			// it now.
+		else if (fPage->IsMapped())
-			vm_remove_all_page_mappings(fPage);
+			set_page_state(fPage, PAGE_STATE_ACTIVE);
-// TODO: Unmapping should already happen when resizing the cache!
+		else
-			fCache->RemovePage(fPage);
+			set_page_state(fPage, PAGE_STATE_INACTIVE);
-			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;
+		fPage->busy_writing = false;
-			if (!fCache->temporary)
+		DEBUG_PAGE_ACCESS_END(fPage);
-				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;
+		fCache->NotifyPageEvents(fPage, PAGE_EVENT_NOT_BUSY);
-			DEBUG_PAGE_ACCESS_END(fPage);
+		success = false;
-
-			success = false;
-		}
 	}
 
-	fCache->NotifyPageEvents(fPage, PAGE_EVENT_NOT_BUSY);
 	fIsActive = false;
 
 	return success;