You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2013/08/02 05:11:11 UTC
Re: svn commit: r1509342 - in
/subversion/branches/log-addressing/subversion/libsvn_fs_fs: fs.h
fs_fs.c util.c util.h
stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
> Author: stefan2
> Date: Thu Aug 1 17:36:42 2013
> New Revision: 1509342
>
> URL: http://svn.apache.org/r1509342
> Log:
> On the log-addressing branch:
High-level question: how does this compare to fsx? Is this a feature
fsx already has that is now being backported to fsfs?
> Bump FSFS format number and introduce
> the new "addressing" option to fsfs format files. Make that available
> to our internal code through the new svn_fs_fs__use_log_addressing API.
> IOW, we support mixed addressing repos from the beginning.
> +++ subversion/branches/log-addressing/subversion/libsvn_fs_fs/fs_fs.c Thu Aug 1 17:36:42 2013
> @@ -323,17 +329,42 @@ read_format(int *pformat, int *max_files
> + /* non-shared repositories never use logical addressing */
> + if (!*max_files_per_dir)
> + *min_log_addressing_rev = SVN_INVALID_REVNUM;
Can we detect
7
layout linear
addressing logical 42
and make it an error?
> @@ -987,14 +1042,59 @@ svn_fs_fs__create(svn_fs_t *fs,
> + /* set compatible version according to generic option */
> + compatible = svn_hash_gets(fs->config, SVN_FS_CONFIG_COMPATIBLE_VERSION);
...
> else if (svn_hash_gets(fs->config, SVN_FS_CONFIG_PRE_1_8_COMPATIBLE))
> - format = 4;
> + compatible_version->minor = 7;
> +
> + /* select format number */
> + switch(compatible_version->minor)
> + {
What about case 0? Right now it'll fall to the "default" case, I think
we should either make it an error or funnel it into the 1.1 case.
> + case 1:
> + case 2:
> + case 3: format = 1;
> + break;
> +
> + case 4: format = 2;
> + break;
> +
> + case 5: format = 3;
> + break;
> +
> + case 6:
> + case 7: format = 4;
> + break;
> +
> + case 8: format = 5;
> + break;
Format 5 was never released, I think you meant 6.
Should the definition of SVN_FS_FS__FORMAT_NUMBER point to this switch()
statement? eg, "If you increment this, update svn_fs_fs__create()"
> +
> + default:format = SVN_FS_FS__FORMAT_NUMBER;
> + }
> }
> ffd->format = format;
>
> @@ -1002,6 +1102,12 @@ svn_fs_fs__create(svn_fs_t *fs,
> if (format >= SVN_FS_FS__MIN_LAYOUT_FORMAT_OPTION_FORMAT)
> ffd->max_files_per_dir = SVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR;
>
> + /* Select the addressing mode depending on the format. */
> + if (format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
> + ffd->min_log_addressing_rev = 0;
> + else
> + ffd->min_log_addressing_rev = SVN_INVALID_REVNUM;
Shouldn't you set this to SVN_INVALID_REVNUM in initialize_fs_struct()
as well?
Re: svn commit: r1509342 - in /subversion/branches/log-addressing/subversion/libsvn_fs_fs:
fs.h fs_fs.c util.c util.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 2, 2013 at 5:16 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <da...@elego.de> wrote:
>
>> stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
>> > Author: stefan2
>> > Date: Thu Aug 1 17:36:42 2013
>> > New Revision: 1509342
>> >
>> > URL: http://svn.apache.org/r1509342
>> > Log:
>> > On the log-addressing branch:
>>
>> High-level question: how does this compare to fsx? Is this a feature
>> fsx already has that is now being backported to fsfs?
>>
>
(forgot to answer that part)
FSX and FSFS f7 do indeed share that feature and will provide
the following common features based on it but with different
implementations:
* pack reorders data
* block read & prefetch
* checksum all data
FSX however replaces the "isolated" FS on-disk items with
containers for massively reduced on-disk and in-memory size.
Also, the txdelta algorithm will differ (fixed window v1 in FSFS
vs. sliding window v2 in FSX).
Basically, f7 looks just like f6 but offsets need to get though
an index translation to get the physical offset. Everything else
is mere optimization that's enabled by this indirection.
See also: http://svn.haxx.se/dev/archive-2013-07/0059.shtml
-- Stefan^2.
Re: svn commit: r1509342 - in /subversion/branches/log-addressing/subversion/libsvn_fs_fs:
fs.h fs_fs.c util.c util.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 2, 2013 at 5:16 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <da...@elego.de> wrote:
>
>> stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
>> > Author: stefan2
>> > Date: Thu Aug 1 17:36:42 2013
>> > New Revision: 1509342
>> >
>> > URL: http://svn.apache.org/r1509342
>> > Log:
>> > On the log-addressing branch:
>>
>> High-level question: how does this compare to fsx? Is this a feature
>> fsx already has that is now being backported to fsfs?
>>
>
(forgot to answer that part)
FSX and FSFS f7 do indeed share that feature and will provide
the following common features based on it but with different
implementations:
* pack reorders data
* block read & prefetch
* checksum all data
FSX however replaces the "isolated" FS on-disk items with
containers for massively reduced on-disk and in-memory size.
Also, the txdelta algorithm will differ (fixed window v1 in FSFS
vs. sliding window v2 in FSX).
Basically, f7 looks just like f6 but offsets need to get though
an index translation to get the physical offset. Everything else
is mere optimization that's enabled by this indirection.
See also: http://svn.haxx.se/dev/archive-2013-07/0059.shtml
-- Stefan^2.
Re: svn commit: r1509342 - in /subversion/branches/log-addressing/subversion/libsvn_fs_fs:
fs.h fs_fs.c util.c util.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
> > Author: stefan2
> > Date: Thu Aug 1 17:36:42 2013
> > New Revision: 1509342
> >
> > URL: http://svn.apache.org/r1509342
> > Log:
> > On the log-addressing branch:
>
> High-level question: how does this compare to fsx? Is this a feature
> fsx already has that is now being backported to fsfs?
>
> > Bump FSFS format number and introduce
> > the new "addressing" option to fsfs format files. Make that available
> > to our internal code through the new svn_fs_fs__use_log_addressing API.
> > IOW, we support mixed addressing repos from the beginning.
>
> > +++ subversion/branches/log-addressing/subversion/libsvn_fs_fs/fs_fs.c
> Thu Aug 1 17:36:42 2013
> > @@ -323,17 +329,42 @@ read_format(int *pformat, int *max_files
> > + /* non-shared repositories never use logical addressing */
> > + if (!*max_files_per_dir)
> > + *min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Can we detect
>
> 7
> layout linear
> addressing logical 42
>
> and make it an error?
>
Yes. We should catch that kind inconsistency instead of implementing a
fallback.
Done in r1509609,-27.
> @@ -987,14 +1042,59 @@ svn_fs_fs__create(svn_fs_t *fs,
> > + /* set compatible version according to generic option */
> > + compatible = svn_hash_gets(fs->config,
> SVN_FS_CONFIG_COMPATIBLE_VERSION);
> ...
> > else if (svn_hash_gets(fs->config,
> SVN_FS_CONFIG_PRE_1_8_COMPATIBLE))
> > - format = 4;
> > + compatible_version->minor = 7;
> > +
> > + /* select format number */
> > + switch(compatible_version->minor)
> > + {
>
> What about case 0? Right now it'll fall to the "default" case, I think
> we should either make it an error or funnel it into the 1.1 case.
>
I opted for the error. That's also consistent with FSX's behaviour.
> + case 1:
> > + case 2:
> > + case 3: format = 1;
> > + break;
> > +
> > + case 4: format = 2;
> > + break;
> > +
> > + case 5: format = 3;
> > + break;
> > +
> > + case 6:
> > + case 7: format = 4;
> > + break;
> > +
> > + case 8: format = 5;
> > + break;
>
> Format 5 was never released, I think you meant 6.
>
Oops.
Should the definition of SVN_FS_FS__FORMAT_NUMBER point to this switch()
> statement? eg, "If you increment this, update svn_fs_fs__create()"
>
Yep.
> > +
> > + default:format = SVN_FS_FS__FORMAT_NUMBER;
> > + }
> > }
> > ffd->format = format;
> >
> > @@ -1002,6 +1102,12 @@ svn_fs_fs__create(svn_fs_t *fs,
> > if (format >= SVN_FS_FS__MIN_LAYOUT_FORMAT_OPTION_FORMAT)
> > ffd->max_files_per_dir = SVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR;
> >
> > + /* Select the addressing mode depending on the format. */
> > + if (format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
> > + ffd->min_log_addressing_rev = 0;
> > + else
> > + ffd->min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Shouldn't you set this to SVN_INVALID_REVNUM in initialize_fs_struct()
> as well?
>
As the struct gets filled with the right values in *_create(), *_open()
and friends, it should not be strictly necessary to init anything in
initialize_fs_struct(). Doing it is more robust, though.
Committed all of the suggested changes in r1509595.
Thanks for the review!
-- Stefan^2.
Re: svn commit: r1509342 - in /subversion/branches/log-addressing/subversion/libsvn_fs_fs:
fs.h fs_fs.c util.c util.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Aug 2, 2013 at 5:11 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@apache.org wrote on Thu, Aug 01, 2013 at 17:36:42 -0000:
> > Author: stefan2
> > Date: Thu Aug 1 17:36:42 2013
> > New Revision: 1509342
> >
> > URL: http://svn.apache.org/r1509342
> > Log:
> > On the log-addressing branch:
>
> High-level question: how does this compare to fsx? Is this a feature
> fsx already has that is now being backported to fsfs?
>
> > Bump FSFS format number and introduce
> > the new "addressing" option to fsfs format files. Make that available
> > to our internal code through the new svn_fs_fs__use_log_addressing API.
> > IOW, we support mixed addressing repos from the beginning.
>
> > +++ subversion/branches/log-addressing/subversion/libsvn_fs_fs/fs_fs.c
> Thu Aug 1 17:36:42 2013
> > @@ -323,17 +329,42 @@ read_format(int *pformat, int *max_files
> > + /* non-shared repositories never use logical addressing */
> > + if (!*max_files_per_dir)
> > + *min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Can we detect
>
> 7
> layout linear
> addressing logical 42
>
> and make it an error?
>
Yes. We should catch that kind inconsistency instead of implementing a
fallback.
Done in r1509609,-27.
> @@ -987,14 +1042,59 @@ svn_fs_fs__create(svn_fs_t *fs,
> > + /* set compatible version according to generic option */
> > + compatible = svn_hash_gets(fs->config,
> SVN_FS_CONFIG_COMPATIBLE_VERSION);
> ...
> > else if (svn_hash_gets(fs->config,
> SVN_FS_CONFIG_PRE_1_8_COMPATIBLE))
> > - format = 4;
> > + compatible_version->minor = 7;
> > +
> > + /* select format number */
> > + switch(compatible_version->minor)
> > + {
>
> What about case 0? Right now it'll fall to the "default" case, I think
> we should either make it an error or funnel it into the 1.1 case.
>
I opted for the error. That's also consistent with FSX's behaviour.
> + case 1:
> > + case 2:
> > + case 3: format = 1;
> > + break;
> > +
> > + case 4: format = 2;
> > + break;
> > +
> > + case 5: format = 3;
> > + break;
> > +
> > + case 6:
> > + case 7: format = 4;
> > + break;
> > +
> > + case 8: format = 5;
> > + break;
>
> Format 5 was never released, I think you meant 6.
>
Oops.
Should the definition of SVN_FS_FS__FORMAT_NUMBER point to this switch()
> statement? eg, "If you increment this, update svn_fs_fs__create()"
>
Yep.
> > +
> > + default:format = SVN_FS_FS__FORMAT_NUMBER;
> > + }
> > }
> > ffd->format = format;
> >
> > @@ -1002,6 +1102,12 @@ svn_fs_fs__create(svn_fs_t *fs,
> > if (format >= SVN_FS_FS__MIN_LAYOUT_FORMAT_OPTION_FORMAT)
> > ffd->max_files_per_dir = SVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR;
> >
> > + /* Select the addressing mode depending on the format. */
> > + if (format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
> > + ffd->min_log_addressing_rev = 0;
> > + else
> > + ffd->min_log_addressing_rev = SVN_INVALID_REVNUM;
>
> Shouldn't you set this to SVN_INVALID_REVNUM in initialize_fs_struct()
> as well?
>
As the struct gets filled with the right values in *_create(), *_open()
and friends, it should not be strictly necessary to init anything in
initialize_fs_struct(). Doing it is more robust, though.
Committed all of the suggested changes in r1509595.
Thanks for the review!
-- Stefan^2.