kernel/vm: Rework ordering of _RemoveConsumer and drop "unmergeable" flag.

This reverts a8877df135.

Previously, the "unmergeable" flag was necessary for the RAMFS,
because if the last vnode reference was released while there
was still a consumer (as the old ordering of _RemoveConsumer
had), then the release of the cache reference when the vnode
was removed would result in the cache trying to merge with
its now-only consumer and sole referrer.

Now, instead, we remove the consumer before releasing the store
reference, so that there's no chance the cache will be merged
inside this method.

mmap_cut_tests still pass, web browsers using ramfs shared_memory
still seem to work.
This commit is contained in:
Augustin Cavalier
2024-12-17 19:12:40 -05:00
parent 7eeb28152e
commit f836917f47
3 changed files with 10 additions and 12 deletions
-1
View File
@@ -200,7 +200,6 @@ public:
// TODO: Remove! // TODO: Remove!
uint32 page_count; uint32 page_count;
uint32 temporary : 1; uint32 temporary : 1;
uint32 unmergeable : 1;
uint32 type : 6; uint32 type : 6;
#if DEBUG_CACHE_LIST #if DEBUG_CACHE_LIST
@@ -303,7 +303,6 @@ DataContainer::_SwitchToCacheMode()
fCache = cache; fCache = cache;
fCache->temporary = 1; fCache->temporary = 1;
fCache->unmergeable = 1;
fCache->virtual_end = fSize; fCache->virtual_end = fSize;
error = fCache->Commit(fSize, VM_PRIORITY_USER); error = fCache->Commit(fSize, VM_PRIORITY_USER);
+10 -10
View File
@@ -611,7 +611,7 @@ VMCacheRef::VMCacheRef(VMCache* cache)
bool bool
VMCache::_IsMergeable() const VMCache::_IsMergeable() const
{ {
return areas.IsEmpty() && temporary && !unmergeable return areas.IsEmpty() && temporary
&& !consumers.IsEmpty() && consumers.Head() == consumers.Tail(); && !consumers.IsEmpty() && consumers.Head() == consumers.Tail();
} }
@@ -642,7 +642,6 @@ VMCache::Init(uint32 cacheType, uint32 allocationFlags)
virtual_end = 0; virtual_end = 0;
committed_size = 0; committed_size = 0;
temporary = 0; temporary = 0;
unmergeable = 0;
page_count = 0; page_count = 0;
fWiredPagesCount = 0; fWiredPagesCount = 0;
type = cacheType; type = cacheType;
@@ -1561,21 +1560,22 @@ void
VMCache::_RemoveConsumer(VMCache* consumer) VMCache::_RemoveConsumer(VMCache* consumer)
{ {
TRACE(("remove consumer vm cache %p from cache %p\n", consumer, this)); TRACE(("remove consumer vm cache %p from cache %p\n", consumer, this));
consumer->AssertLocked();
T(RemoveConsumer(this, consumer)); T(RemoveConsumer(this, consumer));
// Remove the store ref before locking the cache. Otherwise we'd call into consumer->AssertLocked();
// the VFS while holding the cache lock, which would reverse the usual
// locking order.
ReleaseStoreRef();
// remove the consumer from the cache, but keep its reference until later // Remove the consumer from the cache, but keep its reference until the end.
Lock(); Lock();
consumers.Remove(consumer); consumers.Remove(consumer);
consumer->source = NULL; consumer->source = NULL;
Unlock();
ReleaseRefAndUnlock(); // Release the store ref without holding the cache lock, as calling into
// the VFS while holding the cache lock would reverse the usual locking order.
ReleaseStoreRef();
// Now release the consumer's reference.
ReleaseRef();
} }