Re: [PATCH] rock fixes and improvements

On 04/29/2014 05:29 AM, Amos Jeffries wrote:

> +1. All good apart form one minor nit:
>
> src/tests/stub_store.cc
> * the new checkCacheable() needs to be STUB_RETVAL(false).
>
> That can be added on merge.

Finally merged as trunk r13475.

Please note that another, unaudited fix for a frequent shared memory
cache assertion was included in this merge:

> Avoid store_client.cc "entry->swap_filen > -1 || entry->swappingOut()" asserts.
>
> A client may hit on an incomplete shared memory cache entry. Such entry is
> fully backed by the shared memory cache, but the copy of its data in local RAM
> may be trimmed. When that trimMemory() happens, StoreEntry::storeClientType()
> assumes DISK_CLIENT due to positive inmem_lo, and the store_client constructor
> asserts upon discovering that there is no disk backing.
>
> To improve shared cache effectiveness for "being cached" entries, we need to
> prevent local memory trimming while the shared cache entry is being filled
> (possibly by another worker, so this is far from trivial!) or, better, stop
> using the local memory for entries feeding off the shared memory cache. The
> latter would also require revising DISK_CLIENT designation to include entries
> backed by a shared memory cache.

This production-tested fix essentially reorders two checks in
StoreEntry::validToSend():

> if (!mem_obj) // not backed by a memory cache and not collapsed
> return 0;
>
> - if (mem_obj->memCache.index >= 0) // backed by a shared memory cache
> - return 1;
> -
> // StoreEntry::storeClientType() assumes DISK_CLIENT here, but there is no
> - // disk cache backing so we should not rely on the store cache at all. This
> - // is wrong for range requests that could feed off nibbled memory (XXX).
> - if (mem_obj->inmem_lo) // in local memory cache, but got nibbled at
> + // disk cache backing that store_client constructor will assert. XXX: This
> + // is wrong for range requests (that could feed off nibbled memory) and for
> + // entries backed by the shared memory cache (that could, in theory, get
> + // nibbled bytes from that cache, but there is no such "memoryIn" code).
> + if (mem_obj->inmem_lo) // in memory cache, but got nibbled at
> return 0;
>
> + // The following check is correct but useless at this position. TODO: Move
> + // it up when the shared memory cache can either replenish locally nibbled
> + // bytes or, better, does not use local RAM copy at all.
> + // if (mem_obj->memCache.index >= 0) // backed by a shared memory cache
> + // return 1;
> +
> return 1;
> }

I can adjust or revert this if needed, of course.

Thank you,

Alex.
Received on Tue Jun 24 2014 - 23:48:10 MDT