Re: Inconsistencies in FSFS cache-* options

[Stefan Fuhrmann]

On 16.03.2017 15:08, Julian Foad wrote:

Stefan2 et al.:

Nodeprops caching is new in 1.10. This caused me to have a look at the code for enabling it, and I noticed the following in FSFS (and the same in FSX).

In subversion/libsvn_fs_fs/caching.c@1787000, around line 100:
>   /* don't cache text deltas by default.

Wrong: the default was changed to TRUE in r1517479, and this new default is contrary to the reasoning given in this comment.

>    * Once we reconstructed the fulltexts from the deltas,
>    * these deltas are rarely re-used. Therefore, only tools
>    * like svnadmin will activate this to speed up operations
>    * dump and verify.

Yup, that comment is outdated. Fixed in r1791793.

>    */
>   *cache_txdeltas
>     = svn_hash__get_bool(fs->config,
>                          SVN_FS_CONFIG_FSFS_CACHE_DELTAS,
>                          TRUE);
[...]
>   /* by default, cache nodeprops: this will match pre-1.10
>    * behavior where node properties caching was controlled
>    * by SVN_FS_CONFIG_FSFS_CACHE_FULLTEXTS configuration option.

The default of TRUE doesn't match that behaviour, does it? To do so, should the last argument (below) be "*cache_txdeltas" instead of "TRUE"?

In r1791793, I tried to make Ivan's intention more clear.

>    */
>   *cache_nodeprops
>     = svn_hash__get_bool(fs->config,
>                          SVN_FS_CONFIG_FSFS_CACHE_NODEPROPS,
>                          TRUE);

Also, the handling of these flags in mod_dav_svn (around line 990) is odd:

svn_boolean_t
dav_svn__get_txdelta_cache_flag(request_rec *r)
{
  dir_conf_t *conf;

  conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);

  /* txdelta caching is enabled by default. */
  return get_conf_flag(conf->txdelta_cache, TRUE);
}


svn_boolean_t
dav_svn__get_fulltext_cache_flag(request_rec *r)
{
  dir_conf_t *conf;

  conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
  return conf->fulltext_cache == CONF_FLAG_ON;

This construction ("==ON") means the default here is OFF, which is the opposite of the default that FSFS would apply if we didn't pass it any value for this flag.

}


svn_boolean_t
dav_svn__get_revprop_cache_flag(request_rec *r)
{
  dir_conf_t *conf;

  conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
  return conf->revprop_cache == CONF_FLAG_ON;
}

svn_boolean_t
dav_svn__get_nodeprop_cache_flag(request_rec *r)
{
  dir_conf_t *conf;

  conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
  /* node properties caching is enabled by default. */
  return get_conf_flag(conf->nodeprop_cache, FALSE);

The code here uses a default value of "FALSE", which is different from the fsfs.conf default value (see above), and doesn't match the comment here.

Yes, the whole section is quite inconsistent. r1791794 should fix this.

Thanks for the review!

-- Stefan^2.