Re: [PREVIEW] Making Store work with SMP-shared cache_dirs

On 02/15/2011 01:24 AM, Amos Jeffries wrote:
> On 15/02/11 17:41, Alex Rousskov wrote:

>> The same method is used for cache_dir load reporting (if it returns
>> true). Load management needs more work, but the current code is no worse
>> than the old one in this aspect, and further improvements are outside
>> this change scope.
>
> In UFSSwapDir::canStore I think load needs to be set regardless of the
> shedLoad() results. If I understand it right that would help balance
> things better when they are all overloaded.

It is possible that reporting cache_dir load regardless of object
acceptability would be better, but the way the current/old code is
written, it would not make a difference because if the object cannot be
stored by cache_dir, the cache_dir load is ignored: We simply switch to
the next cache_dir. And in storeDirSelectSwapDirRoundRobin() we did not
even compute the load until the object was accepted.

We preserved that logic because (a) it kind of make sense and (b)
changing/optimizing it would be outside this work scope.

>> * When swapout initiation fails, release StoreEntry. This prevents the
>> caller code from trying to swap out again and again because swap_status
>> becomes SWAPOUT_NONE.
>>
>> TODO: Consider add SWAPOUT_ERROR, STORE_ERROR, and similar states. It
>> may solve several problems where the code sees _NONE or _OK and thinks
>> everything is peachy when in fact there was an error.
>>
>
> in StoreEntry::swapoutPossible the logic tests of (currentEnd >
> store_maxobjsize) is broken. To reach it we already have checked
> (expectedEnd > 0 and expectedEnd < store_maxobjsize) so if (expectedEnd
> < currentEnd) there is a bug somewhere.

I see what you are saying, but I am not 100% sure that endOffset() may
not exceed expectedReplySize() for range responses with known
Content-Length. If you are correct, then the currentEnd check should be
indeed removed, but leaving it should not break things either.

Do you remember whether endOffset() may be very large for, say, a
"last-byte" Content-Range response when the underlying entity is huge?

>> * Always call StoreEntry::abort() instead of setting ENTRY_ABORTED
>> manually.
>>
>
> ? I see no changes implementing that statement.

My bad. Apparently, I did not include the corresponding changes in the
posted patch. They are quoted below and will be included in Part 1 patch
to be posted today:

> === modified file 'src/neighbors.cc'
> --- src/neighbors.cc 2011-04-03 12:17:09 +0000
> +++ src/neighbors.cc 2011-04-14 16:58:28 +0000
> @@ -1515,9 +1516,8 @@
>
> cbdataReferenceDone(psstate->callback_data);
>
> - EBIT_SET(fake->flags, ENTRY_ABORTED);
> + fake->abort(); // sets ENTRY_ABORTED and initiates releated cleanup
> HTTPMSGUNLOCK(fake->mem_obj->request);
> - fake->releaseRequest();
> fake->unlock();
> HTTPMSGUNLOCK(psstate->request);
> cbdataFree(psstate);

>> * Added operator<< for dumping StoreEntry summary into the debugging
>> log. Needs more work to report more info (and not report yet-unknown
>> info).
>
> ? I see no changes implementing that statement.

Same problem, I think. Here is the matching store.cc code (except for
the declaration) from Part1 patch to be posted shortly:

> +std::ostream &operator <<(std::ostream &os, const StoreEntry &e)
> +{
> + return os << e.swap_filen << '@' << e.swap_dirn << '=' <<
> + e.mem_status << '/' << e.ping_status << '/' << e.store_status << '/' <<
> + e.swap_status;
> +}
> +

> in storeSwapOutFileClosed the assert commented "we checked that above"
> needs to be dropped. We are trying to remove useless asserts remember?

Will remove before posting the update patch today.

Thank you,

Alex.
Received on Sun May 15 2011 - 19:47:43 MDT