Re: [PATCH] Allow a ufs cache_dir entry to coexist with a shared memory cache entry

On 10/15/2012 03:59 PM, Amos Jeffries wrote:
> On 16.10.2012 09:04, Alex Rousskov wrote:
>> Hello,
>>
>> The attached patch allows a ufs cache_dir entry to coexist with a
>> shared memory cache entry instead of being released when it becomes idle.
>>
>> The original boolean version of the StoreController::dereference() code
>> (r11730) was written to make sure that idle unlocked local store_table
>> entries are released if nobody needs them (to avoid creating
>> inconsistencies with shared caches that could be modified in a different
>> process).
>>
>> Then, in r11786, we realized that the original code was destroying
>> non-shared memory cache entries if there were no cache_dirs to vote for
>> keeping them in store_table. I fixed that by changing the
>> StoreController::dereference() logic from "remove if nobody needs it" to
>> "remove if somebody objects to keeping it". That solved the problem at
>> hand, but prohibited an entry to exist in a non-shared cache_dir and in
>> a shared memory cache at the same time.
>>
>> We now go back to the original "remove if nobody needs it" design but
>> also give non-shared memory cache a vote so that it can protect idle but
>> suitable for memory cache entries from being released if there are no
>> cache_dirs to vote for them.
>>
>>
>> This is a second revision of the fix. The first one (trunk r12231) was
>> reverted because it did not pass tests/testRock unit tests on some
>> platforms. The unit tests assume that the entry slot is not locked after
>> the entry is stored, but the first revision of the fix allowed idle
>> entries to remain in store_table and, hence, their slots were locked and
>> could not be replaced, causing testRock assertions. This revision
>> allows the idle entry to be destroyed (and its slot unlocked) if
>> [non-shared] memory caching is disabled.
>>
>> It is not clear to me why only some of the platforms were affected by
>> this. Should not memory caching be disabled everywhere during testRock
>> (because testRock does not set memory cache capacity and memory cache
>> entry size limits)?
>>
>> The changes passed build farm tests (except a couple of nodes with
>> Jenkins-related build problems unrelated to the code).
>>
>>
>> Thank you,
>>
>> Alex.
>
>
> Cool, lets give it a whirl. +1.

Committed as trunk r12395. I will watch for testRock failures after this
change (combined with the flexible arrays fix).

Thank you,

Alex.
Received on Tue Oct 16 2012 - 00:23:10 MDT