fsfs block size and page size config parameters

Hi Stefan.

fs_fs.c:
> "### When a specific piece of information needs to be read from disk,  a"    NL
> "### data block is being read at once and its contents are being cached."    NL
> "### ...
> "### block-size is 64 kBytes by default."                                    NL
> "# " CONFIG_OPTION_BLOCK_SIZE " = 64"                                        NL

^^ Can you state the size units explicitly in the commentary please? (If an admin has changed the value to say "4" or "4096" then the implicit clue of "64" matching "64 kBytes" would be lost.)

> "###"                                                                        NL
> "### The log-to-phys index maps data item numbers to offsets within the"     NL
> "### rev or pack file.  ...
> "### l2p-page-size is 8192 entries by default."                              NL
> "# " CONFIG_OPTION_L2P_PAGE_SIZE " = 8192"                                   NL
> "###"                                                                        NL
> "### The phys-to-log index maps positions within the rev or pack file to"    NL
> "### to data items, ...
> "### For source code repositories, this should be about 16x the block-size." NL
> "### Must be a power of 2."                                                  NL
> "### p2l-page-size is 1024 kBytes by default."                               NL
> "# " CONFIG_OPTION_P2L_PAGE_SIZE " = 1024"                                   NL

^^ Same for this one.

fs.h:
> typedef struct fs_fs_data_t
> { ...
>   /* Rev / pack file read granularity. */
>   apr_int64_t block_size;

And here? (Here it's bytes not kilobytes, I believe.)

>   /* Capacity in entries of log-to-phys index pages */
>   apr_int64_t l2p_page_size;

For disambiguation, maybe call it "l2p_page_entries" or "l2p_page_size_entries" instead? Otherwise my instinct is to assume the same units as p2l_page_size.

>   /* Rev / pack file granularity covered by phys-to-log index pages */
>   apr_int64_t p2l_page_size;

And here?

> ... }

The initialization code converts kilobytes to bytes:

fs_fs.c:
>   if (ffd->format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
>     {
>       SVN_ERR(svn_config_get_int64(config, &ffd->block_size,
>                                    CONFIG_SECTION_IO,
>                                    CONFIG_OPTION_BLOCK_SIZE,
>                                    64));
>       SVN_ERR(svn_config_get_int64(config, &ffd->l2p_page_size,
>                                    CONFIG_SECTION_IO,
>                                    CONFIG_OPTION_L2P_PAGE_SIZE,
>                                    0x2000));
>       SVN_ERR(svn_config_get_int64(config, &ffd->p2l_page_size,
>                                    CONFIG_SECTION_IO,
>                                    CONFIG_OPTION_P2L_PAGE_SIZE,
>                                    0x400));
>
>       ffd->block_size *= 0x400;
>       ffd->p2l_page_size *= 0x400;

But no such multiplier for l2p_page_size? Oh, that's because it's measured in entries rather than bytes/kilobytes. That confused me when I first read this: it looked like an accidental omission.

>     }
>   else
>     {
>       /* should be irrelevant but we initialize them anyway */
>       ffd->block_size = 0x1000;
>       ffd->l2p_page_size = 0x2000;
>       ffd->p2l_page_size = 0x100000;

At first I thought these numbers were different from above, then I noticed the multipliers above and decided they're the same end result, but then hours later I noticed the 'block size' here is _not_ equal to 64 x 0x400 (which would be 0x10000 not 0x1000).

I know this last set of values "should be irrelevant" but we should avoid hard-to-spot differences like this one. Can you write this so there are not so many different numbers in the source code? How about using defined constants, since each one appears approx. 4 times including the config file commentary?

- Julian
Received on 2014-06-30 18:43:45 CEST