Re: fsfs block size and page size config parameters

[Stefan Fuhrmann <st...@wandisco.com>]

On Mon, Jun 30, 2014 at 6:33 PM, Julian Foad <ju...@btopenworld.com>
wrote:

> 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?
>

Most of this is covered by r1616613. I did not, however, rename
l2p_page_size to keep the actual code churn low.

-- Stefan^2.

Re: fsfs block size and page size config parameters

[Julian Foad <ju...@btopenworld.com>]

Stefan Fuhrmann wrote:
> Most of this is covered by r1616613. I did not, however, rename l2p_page_size to keep the actual code churn low.

Thanks.

- Julian