Re: [PATCH] SMP Caching: Core, IPC, Shared memory cache, and Rock Store

On 07/09/11 12:08, Alex Rousskov wrote:
> Hello,
>
> Attached compressed patch contains changes related to SMP shared
> memory cache and Rock Store support. Most of these changes have been
> posted earlier for preview. The patch accumulates more than two years
> worth of work and, at 20K lines, is probably too huge and too difficult
> to audit correctly, for which I do apologize. I should have found a way
> to merge parts of this earlier.
>
> The patch preamble is several pages long and attempts to document all
> major changes.
>

Second block of fixes...

src/cache_cf.cc:
  * free_YesNoNone() is useless. Please use a macro to elide:
     #define free_YesNoNone(x) // do nothing.

src/cf.data.pre:
  * please use the IFDEF: config functionality when compiler build
dependencies are involved.
  * please add the new DEFAULT_DOC: tag to clarify the default value.

NAME: memory_cache_shared
IFDEF: HAVE_ATOMIC_OPS
DEFAULT_DOC: Automatic detection in SMP mode. Disabled otherwise.

NAME: disk_io_timeout
IFDEF: HAVE_FS_ROCK
DEFAULT_DOC: No disk I/O time limit.

(also matching updates to cf.data.defines)

  ** NP: this IFDEF addition may impact removal of the #if
!USE_DNSSERVERS macros from src/cache_cf.cc (addition of conditions
instead of full removal).

src/fs/Module.cc:
  * s/#ifdef/#if/

src/fs/rock/RockFile.h:
  * please implement or erase the "XXX: rename" before merge.
  * the class also appears to be an exact on-disk bit format or not.
Please document that clearly on the class itself. With similar warnings
to StoreMeta about not using virtuals etc.

src/fs/rock/RockRebuild.cc:
  * s/debugs(47,0, "/debugs(47, DBG_CRITICAL, "ERROR:/

src/fs/rock/RockSwapDir.cc:
  * please move <iomanip> include below the squid "local" files. If it
is required by any of the headers it should be included there.

  * s/debugs(47,0, "/debugs(47, DBG_CRITICAL, "/
  * also all the nearby critical "Failed to" need an ERROR: or FATAL:
prefix for log monitors.

  * drop the _SQUID_MSWIN_ around mkdir(). We have a portability wrapper
now that maps from the unix version.

  * re: "XXX: should we support resize?" - I think right now we should
at minimum drop max_objsize down to what the map supports and warn about
the change.

  * in Rock::SwapDir::validateOptions() - the wasted space warning and
dump should be DBG_IMPORTANT by the looks of it. Possibly using
opt_parse_cfg_only to bump up to critical on -k parse if its that important.

  * HERE << "Rock::SwapDir::createStoreIO:' - is redundant output

  * 'DBG_IMPORTANT, "Internal ERROR:' - IMO we should display "BUG:
blah" for this type of message. Change if you agree.

  * "XXX: otherwise too expensive to count" - 100 cache entries is way
too small if this is expected to be useful info. Why bother even
producing these stats if its not?

src/fs/rock/RockSwapDir.h:
  * re: "return 0xFFFFFF; } /// Core sfileno maximum" - please define as
MAX_SFILENO in Store.h. Could be useful elsewhere.

Thats up to src/fs/ done I think. More to come.

Extra:
   My understanding from last year was that rock would be designed to
replace COSS. Is that still correct or are we going to have to still
port the COSS fixes from 2.7?

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.15
   Beta testers wanted for 3.2.0.11