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 2011/07/06 18:47:23 UTC
fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer
allow usage of traditional backup software?
This thread is now the only non-FSFS release blocker (filed as #3944).
Last I checked there were at least three solutions suggested, but no
consensus on which solution to implement.
Some suggestions were
0. Leave things as they are
1. Allow packing revisions without packing revprops.
(revprops/ remains as in 1.6/f4)
2. Have all revprops in the DB all the time, never in plain files.
3. Swap the DB for some other "A thousand revision's revprops in one
file" solution. [several suggestions as to that file's format]
Can we decide on what to do here?
Thanks,
Daniel
---------
My opinion:
* (1) is orthogonal to the others, but may be a good idea if we refactor
the FS so shortly before the release
* (2) simplifies things, doesn't solve the problems with having an
SQLite db authoritative for parts of the FS storage
(read: cp(1) unsafe)
* (3) has my +1, assuming it's sufficiently performant and the concrete
design is reasonable
* (0) would mean that if we refactor revprop storage later, 1.8 servers
will have to try and read revprops from *three* places; and lots of
headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
So, if f5 should be improved, I'd rather do that /before/ it's
released (and has to be indefinitely supported).
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Peter Samuelson <pe...@p12n.org>.
[C. Michael Pilato]
> Boo! Layer knowledge violation. Ignoring, oh, the tiny percentage
> of Subversion code that does something special with r0 props, why
> would we special-case this?
It's special because we do ship svnsync officially. (:
It's special because r0 never has anything in it _except_ revprops.
It's special because it does not get svn:author or svn:log by default
like every other revision does.
r0 revprops _are_ a natural place to store metadata about the
repository as a whole that one does not want to store in the repo
filesystem itself. I hardly think svnsync is the only tool that uses
it that way. Just as I proposed handling HEAD revprops in a special
way to accommodate post-commit hooks that immediately edit their own
revision's revprops (the mechanics of this as relates to the
implementation of packing is beyond the scope of this message),
special-casing the other endpoint, r0, is no less strange.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Mark Phippard wrote on Wed, Jul 06, 2011 at 13:27:05 -0400:
> On Wed, Jul 6, 2011 at 1:20 PM, Daniel Shahaf <da...@elego.de> wrote:
> > Mark Phippard wrote on Wed, Jul 06, 2011 at 13:01:58 -0400:
> >> On Wed, Jul 6, 2011 at 12:47 PM, Daniel Shahaf <da...@elego.de> wrote:
> >> > This thread is now the only non-FSFS release blocker (filed as #3944).
> >> > Last I checked there were at least three solutions suggested, but no
> >> > consensus on which solution to implement.
> >> >
> >> > Some suggestions were
> >> >
> >> >
> >> > 0. Leave things as they are
> >> >
> >> > 1. Allow packing revisions without packing revprops.
> >> > (revprops/ remains as in 1.6/f4)
> >> >
> >> > 2. Have all revprops in the DB all the time, never in plain files.
> >> >
> >> > 3. Swap the DB for some other "A thousand revision's revprops in one
> >> > file" solution. [several suggestions as to that file's format]
> >>
> >> Should there be a new discussion about what is wrong with option 0?
> >> My recollection of the thread is that there were no issues raised that
> >> really "stuck". Meaning there was speculation and concern that turned
> >> out to be fairly manageable and a non-issue. So what is wrong with
> >> the current design approach?
> >>
> >
> > What about your own answers to points (2) and (3)?
>
> Not sure the question. I think we answered Kevin's original concerns.
> Namely that:
>
> 1) This is an optional feature that does not come into play unless you
> pack the repository.
> 2) If you do pack the repository, the SQLite databases are only
> written to by someone that edits a revprop.
>
[ also by commits ]
> >> 3) As long as we had a good design, this would have been my
> >> preference.
> >
> > Okay. Hyrum and I raised a few designs late on Friday, but I don't
> > recall discussion on which one was better.
> >
> >> Mainly because I think adding SQLite adds some unknowns.
> >> However, given that rep sharing is there, I am not sure it matters at
> >> this point.
> >>
> >
> > rep-cache.db isn't authoritative; it is consulted while writing the rev
> > files but never afterwards. revprops.db is authoritative (removing it
> > is comparable to deleting a rev file).
>
> That is mainly only a backup question though. A single packed
> revision file would also be a critical file that has to be preserved.
> Given that the current packing design is relatively easy to backup, a
> new design would only be slightly better in this area and that it
> would be easier still.
>
> I am not against a new design that does not use SQLite. I am against
> expanding the SQLite usage. My main objection to a new design is that
> it feels too late. I do not want to wait for it to be agreed upon and
> coded.
>
I'm not worried that it would take too long to code.
I am, though, worried about introducing bugs.
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Mark Phippard <ma...@gmail.com>.
On Wed, Jul 6, 2011 at 1:20 PM, Daniel Shahaf <da...@elego.de> wrote:
> Mark Phippard wrote on Wed, Jul 06, 2011 at 13:01:58 -0400:
>> On Wed, Jul 6, 2011 at 12:47 PM, Daniel Shahaf <da...@elego.de> wrote:
>> > This thread is now the only non-FSFS release blocker (filed as #3944).
>> > Last I checked there were at least three solutions suggested, but no
>> > consensus on which solution to implement.
>> >
>> > Some suggestions were
>> >
>> >
>> > 0. Leave things as they are
>> >
>> > 1. Allow packing revisions without packing revprops.
>> > (revprops/ remains as in 1.6/f4)
>> >
>> > 2. Have all revprops in the DB all the time, never in plain files.
>> >
>> > 3. Swap the DB for some other "A thousand revision's revprops in one
>> > file" solution. [several suggestions as to that file's format]
>>
>> Should there be a new discussion about what is wrong with option 0?
>> My recollection of the thread is that there were no issues raised that
>> really "stuck". Meaning there was speculation and concern that turned
>> out to be fairly manageable and a non-issue. So what is wrong with
>> the current design approach?
>>
>
> What about your own answers to points (2) and (3)?
Not sure the question. I think we answered Kevin's original concerns.
Namely that:
1) This is an optional feature that does not come into play unless you
pack the repository.
2) If you do pack the repository, the SQLite databases are only
written to by someone that edits a revprop.
So it is still fairly manageable to get a good backup.
> I'm not married to changing the format. But if there are good reasons
> to do so --- whether they be "Easier for administrators" (Kevin's
> original point) or "Easier for extension" (a property of Peter's
> suggestion) --- now is the best time to do so, before we have to support
> it, forever, while never blocking concurrent readers.
>
>> My answers:
>>
>> 1) I always thought packing was pointless without packing revprops.
>> You get the biggest win from the revprops.
>>
>> 2) I think this would be a terrible idea. Hot backup becomes nearly impossible.
>>
>> 3) As long as we had a good design, this would have been my
>> preference.
>
> Okay. Hyrum and I raised a few designs late on Friday, but I don't
> recall discussion on which one was better.
>
>> Mainly because I think adding SQLite adds some unknowns.
>> However, given that rep sharing is there, I am not sure it matters at
>> this point.
>>
>
> rep-cache.db isn't authoritative; it is consulted while writing the rev
> files but never afterwards. revprops.db is authoritative (removing it
> is comparable to deleting a rev file).
That is mainly only a backup question though. A single packed
revision file would also be a critical file that has to be preserved.
Given that the current packing design is relatively easy to backup, a
new design would only be slightly better in this area and that it
would be easier still.
I am not against a new design that does not use SQLite. I am against
expanding the SQLite usage. My main objection to a new design is that
it feels too late. I do not want to wait for it to be agreed upon and
coded.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Mark Phippard wrote on Wed, Jul 06, 2011 at 13:01:58 -0400:
> On Wed, Jul 6, 2011 at 12:47 PM, Daniel Shahaf <da...@elego.de> wrote:
> > This thread is now the only non-FSFS release blocker (filed as #3944).
> > Last I checked there were at least three solutions suggested, but no
> > consensus on which solution to implement.
> >
> > Some suggestions were
> >
> >
> > 0. Leave things as they are
> >
> > 1. Allow packing revisions without packing revprops.
> > (revprops/ remains as in 1.6/f4)
> >
> > 2. Have all revprops in the DB all the time, never in plain files.
> >
> > 3. Swap the DB for some other "A thousand revision's revprops in one
> > file" solution. [several suggestions as to that file's format]
>
> Should there be a new discussion about what is wrong with option 0?
> My recollection of the thread is that there were no issues raised that
> really "stuck". Meaning there was speculation and concern that turned
> out to be fairly manageable and a non-issue. So what is wrong with
> the current design approach?
>
What about your own answers to points (2) and (3)?
I'm not married to changing the format. But if there are good reasons
to do so --- whether they be "Easier for administrators" (Kevin's
original point) or "Easier for extension" (a property of Peter's
suggestion) --- now is the best time to do so, before we have to support
it, forever, while never blocking concurrent readers.
> My answers:
>
> 1) I always thought packing was pointless without packing revprops.
> You get the biggest win from the revprops.
>
> 2) I think this would be a terrible idea. Hot backup becomes nearly impossible.
>
> 3) As long as we had a good design, this would have been my
> preference.
Okay. Hyrum and I raised a few designs late on Friday, but I don't
recall discussion on which one was better.
> Mainly because I think adding SQLite adds some unknowns.
> However, given that rep sharing is there, I am not sure it matters at
> this point.
>
rep-cache.db isn't authoritative; it is consulted while writing the rev
files but never afterwards. revprops.db is authoritative (removing it
is comparable to deleting a rev file).
Apples and oranges.
>
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Mark Phippard <ma...@gmail.com>.
On Wed, Jul 6, 2011 at 12:47 PM, Daniel Shahaf <da...@elego.de> wrote:
> This thread is now the only non-FSFS release blocker (filed as #3944).
> Last I checked there were at least three solutions suggested, but no
> consensus on which solution to implement.
>
> Some suggestions were
>
>
> 0. Leave things as they are
>
> 1. Allow packing revisions without packing revprops.
> (revprops/ remains as in 1.6/f4)
>
> 2. Have all revprops in the DB all the time, never in plain files.
>
> 3. Swap the DB for some other "A thousand revision's revprops in one
> file" solution. [several suggestions as to that file's format]
Should there be a new discussion about what is wrong with option 0?
My recollection of the thread is that there were no issues raised that
really "stuck". Meaning there was speculation and concern that turned
out to be fairly manageable and a non-issue. So what is wrong with
the current design approach?
My answers:
1) I always thought packing was pointless without packing revprops.
You get the biggest win from the revprops.
2) I think this would be a terrible idea. Hot backup becomes nearly impossible.
3) As long as we had a good design, this would have been my
preference. Mainly because I think adding SQLite adds some unknowns.
However, given that rep sharing is there, I am not sure it matters at
this point.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Wed, Jul 06, 2011 at 19:47:23 +0300:
> This thread is now the only non-FSFS release blocker (filed as #3944).
s/non-FSFS/non-Serf/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Peter Samuelson <pe...@p12n.org>.
[Hyrum K Wright]
> In addition, the revprop packfile manifest information won't be
> cached, since the manifest may change. We don't anticipate this to
> be a problem, since it only adds an extra seek() to the revprop
> lookup process (rather than the open() + seek() in the rev packing
> world).
Eh ... if editing revprops is accomplished by writing a whole new shard
and then renaming, then you most definitely _do_ need an open() in
there, if you think there may be changes. (Or, at the very least,
stat() and check the st_ino field to see if it has changed.)
Also, as I understand it, you can't easily delete a file, on Windows,
that someone else has open, even just for reading. So that might make
the whole 'atomic rename' business a bit more exciting.
Just sayin',
Peter
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer
allow usage of traditional backup software?
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/07/2011 10:13 AM, Daniel Shahaf wrote:
> It means that all the '% ffd->max_files_per_dir' computations need to
> get an off-by-one iff they are for revprops. Special-casing r0 and the
> first shard's manifest seems to me, too, to be preferable.
Boo! Layer knowledge violation. Ignoring, oh, the tiny percentage of
Subversion code that does something special with r0 props, why would we
special-case this?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Philip Martin wrote on Thu, Jul 07, 2011 at 14:54:10 +0100:
> Daniel Shahaf <da...@elego.de> writes:
>
> > Hyrum K Wright wrote on Thu, Jul 07, 2011 at 08:29:31 -0500:
> >> On Thu, Jul 7, 2011 at 2:36 AM, Philip Martin
> >> <ph...@wandisco.com> wrote:
> >> >
> >> > r0 revprops are a concern, they can have different access patterns. For
> >> > example a master/slave setup running svnsync once per revision (a common
> >> > setup) will write the r0 pack file several times per revision. We don't
> >> > want the pack file to become the dominant IO.
> >> >
> >> > I wonder if we could offset the shard boundaries, so that r0 is the last
> >> > revision in the first shard and r1-rN is the second shard. Then r0
> >> > would be a shard on its own and the r0 pack file would be much smaller.
> >> > We would have to repack the repository on upgrade but the code changes
> >> > for this could be small, just +/-1 in a few places.
> >>
> >> Changing the offsets for all the shard boundaries is just asking for
> >> trouble, as it would mean that revision shards and revprop shards
> >> wouldn't match up. That seems like a pretty big "gotcha".
>
> I assumed we would "repack the repository on upgrade" and change the
> revision shards as well. Maybe that's even worse :)
>
It means that all the '% ffd->max_files_per_dir' computations need to
get an off-by-one iff they are for revprops. Special-casing r0 and the
first shard's manifest seems to me, too, to be preferable.
> >> Instead, we could just special-case r0 revprops so that they never get
> >> packed, and that the 0th shard includes r1:rN, instead of r0:rN. That
> >> would only impact the 0th shard, rather than every revprop shard.
> >
> > +1
>
> If that's easy it would be even better.
>
> Subversion log messages average about 400 characters, so if we take that
> as standard a pack file starts at about half a MB. Something like
Actually it's 499931 bytes (for the last full shard from our history).
Nice estimate :-)
> 'svnsync sync' would do about 2.5MB write IO per invocation just setting
> r0 revprops.
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer allow usage of traditional backup software?
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <da...@elego.de> writes:
> Hyrum K Wright wrote on Thu, Jul 07, 2011 at 08:29:31 -0500:
>> On Thu, Jul 7, 2011 at 2:36 AM, Philip Martin
>> <ph...@wandisco.com> wrote:
>> >
>> > r0 revprops are a concern, they can have different access patterns. For
>> > example a master/slave setup running svnsync once per revision (a common
>> > setup) will write the r0 pack file several times per revision. We don't
>> > want the pack file to become the dominant IO.
>> >
>> > I wonder if we could offset the shard boundaries, so that r0 is the last
>> > revision in the first shard and r1-rN is the second shard. Then r0
>> > would be a shard on its own and the r0 pack file would be much smaller.
>> > We would have to repack the repository on upgrade but the code changes
>> > for this could be small, just +/-1 in a few places.
>>
>> Changing the offsets for all the shard boundaries is just asking for
>> trouble, as it would mean that revision shards and revprop shards
>> wouldn't match up. That seems like a pretty big "gotcha".
I assumed we would "repack the repository on upgrade" and change the
revision shards as well. Maybe that's even worse :)
>> Instead, we could just special-case r0 revprops so that they never get
>> packed, and that the 0th shard includes r1:rN, instead of r0:rN. That
>> would only impact the 0th shard, rather than every revprop shard.
>
> +1
If that's easy it would be even better.
Subversion log messages average about 400 characters, so if we take that
as standard a pack file starts at about half a MB. Something like
'svnsync sync' would do about 2.5MB write IO per invocation just setting
r0 revprops.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Hyrum K Wright wrote on Thu, Jul 07, 2011 at 08:29:31 -0500:
> On Thu, Jul 7, 2011 at 2:36 AM, Philip Martin
> <ph...@wandisco.com> wrote:
> >
> > r0 revprops are a concern, they can have different access patterns. For
> > example a master/slave setup running svnsync once per revision (a common
> > setup) will write the r0 pack file several times per revision. We don't
> > want the pack file to become the dominant IO.
> >
> > I wonder if we could offset the shard boundaries, so that r0 is the last
> > revision in the first shard and r1-rN is the second shard. Then r0
> > would be a shard on its own and the r0 pack file would be much smaller.
> > We would have to repack the repository on upgrade but the code changes
> > for this could be small, just +/-1 in a few places.
>
> Changing the offsets for all the shard boundaries is just asking for
> trouble, as it would mean that revision shards and revprop shards
> wouldn't match up. That seems like a pretty big "gotcha".
>
> Instead, we could just special-case r0 revprops so that they never get
> packed, and that the 0th shard includes r1:rN, instead of r0:rN. That
> would only impact the 0th shard, rather than every revprop shard.
>
+1
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Jul 7, 2011 at 8:39 AM, Daniel Shahaf <da...@elego.de> wrote:
> Philip Martin wrote on Thu, Jul 07, 2011 at 08:36:06 +0100:
>> Daniel Shahaf <da...@elego.de> writes:
>>
>> > The process to edit a revprop that had been packed would be:
>> >
>> > * grab write lock
>> > * prepare a new pack-file-with-inline-manifest
>> > * move-into-place, atomically replacing the previous pack file
>> > * ungrab write lock
>> >
>> > That is what guarantees cp(1) consistency that Hyrum mentions.
>>
>> Atomic replace is going to involve a retry loop on Windows, and so could
>> potentially take a long time. Virus scanners on repository disks?
>>
>
> Yes, this is going to be a problem.
Virus scanners on repo disks on Windows are *always* going to be a
problem. There isn't a whole lot we can do about it (aside from the
retry loop).
I'll again stress that I expect prop editing of packed revprops to be
a rare operation (save for r0, which I've addresses elsewhere).
Please don't let theoretical worst-case behavior drive the
implementation.
-Hyrum
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Philip Martin wrote on Thu, Jul 07, 2011 at 08:36:06 +0100:
> Daniel Shahaf <da...@elego.de> writes:
>
> > The process to edit a revprop that had been packed would be:
> >
> > * grab write lock
> > * prepare a new pack-file-with-inline-manifest
> > * move-into-place, atomically replacing the previous pack file
> > * ungrab write lock
> >
> > That is what guarantees cp(1) consistency that Hyrum mentions.
>
> Atomic replace is going to involve a retry loop on Windows, and so could
> potentially take a long time. Virus scanners on repository disks?
>
Yes, this is going to be a problem.
> Holding the write lock will block readers so for better concurrency we
> want to minimise the time that a write lock is held, or have more locks.
>
> Is there one lock per pack file or one lock per repository? Having
> multiple locks means that a write only blocks a proportion of the reads,
> but also means an operation like log has to acquire multiple locks (in
> series not in parallel).
>
It will block writers, not readers.
But having more fine-grained locks seems a good idea.
> We can move the prepare/write outside the write lock by using a sequence
> number in the pack file. Then the algorithm could be:
>
> * prepare a new pack file with incremented sequence number
> * grab write lock
> * check old sequence number
> * if changed: drop lock and start again
> * otherwise: atomic replace pack file
> * drop write lock
>
> That doesn't help with the retry problem on Windows, but may reduce the
> time the write lock is held, particularly if the pack file is large. It
> shoud work well if writes are much less common than reads. If writes
> are common then the simple write lock serialisation may be better.
>
Good idea.
> The two algorithms should interoperate so, even if we use a simple write
> lock today, implementing the sequence number would give us options in
> the future.
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Jul 7, 2011 at 2:36 AM, Philip Martin
<ph...@wandisco.com> wrote:
>
> r0 revprops are a concern, they can have different access patterns. For
> example a master/slave setup running svnsync once per revision (a common
> setup) will write the r0 pack file several times per revision. We don't
> want the pack file to become the dominant IO.
>
> I wonder if we could offset the shard boundaries, so that r0 is the last
> revision in the first shard and r1-rN is the second shard. Then r0
> would be a shard on its own and the r0 pack file would be much smaller.
> We would have to repack the repository on upgrade but the code changes
> for this could be small, just +/-1 in a few places.
Changing the offsets for all the shard boundaries is just asking for
trouble, as it would mean that revision shards and revprop shards
wouldn't match up. That seems like a pretty big "gotcha".
Instead, we could just special-case r0 revprops so that they never get
packed, and that the 0th shard includes r1:rN, instead of r0:rN. That
would only impact the 0th shard, rather than every revprop shard.
-Hyrum
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer allow usage of traditional backup software?
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Holding the write lock will block readers so for better concurrency we
> want to minimise the time that a write lock is held, or have more locks.
Daniel pointed out that readers don't block, they simply read the pack
file and get either the old or atomically replaced new file. Thus there
is no need to minimise lock time for readers, and we can design the the
locks to minimise write IO. So we want the lock held for the whole
read/write/replace operation so that concurrent writers don't do IO that
has to be thrown away.
One lock per pack file will allow concurrent writes to different shards
and so would help with write concurrency if that is important.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer allow usage of traditional backup software?
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <da...@elego.de> writes:
> The process to edit a revprop that had been packed would be:
>
> * grab write lock
> * prepare a new pack-file-with-inline-manifest
> * move-into-place, atomically replacing the previous pack file
> * ungrab write lock
>
> That is what guarantees cp(1) consistency that Hyrum mentions.
Atomic replace is going to involve a retry loop on Windows, and so could
potentially take a long time. Virus scanners on repository disks?
Holding the write lock will block readers so for better concurrency we
want to minimise the time that a write lock is held, or have more locks.
Is there one lock per pack file or one lock per repository? Having
multiple locks means that a write only blocks a proportion of the reads,
but also means an operation like log has to acquire multiple locks (in
series not in parallel).
We can move the prepare/write outside the write lock by using a sequence
number in the pack file. Then the algorithm could be:
* prepare a new pack file with incremented sequence number
* grab write lock
* check old sequence number
* if changed: drop lock and start again
* otherwise: atomic replace pack file
* drop write lock
That doesn't help with the retry problem on Windows, but may reduce the
time the write lock is held, particularly if the pack file is large. It
shoud work well if writes are much less common than reads. If writes
are common then the simple write lock serialisation may be better.
The two algorithms should interoperate so, even if we use a simple write
lock today, implementing the sequence number would give us options in
the future.
> This also implies that propediting a revprop-that-had-been-packed will
> have to rewrite a packfile containing a thousand revision's properties.
> We expect that to be a reasonable cost given that revprop files are
> small and historical revprops are rarely edited.
r0 revprops are a concern, they can have different access patterns. For
example a master/slave setup running svnsync once per revision (a common
setup) will write the r0 pack file several times per revision. We don't
want the pack file to become the dominant IO.
I wonder if we could offset the shard boundaries, so that r0 is the last
revision in the first shard and r1-rN is the second shard. Then r0
would be a shard on its own and the r0 pack file would be much smaller.
We would have to repack the repository on upgrade but the code changes
for this could be small, just +/-1 in a few places.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Hyrum K Wright wrote on Wed, Jul 06, 2011 at 14:30:15 -0500:
> On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <da...@elego.de> wrote:
> > This thread is now the only non-FSFS release blocker (filed as #3944).
> > Last I checked there were at least three solutions suggested, but no
> > consensus on which solution to implement.
> >
> > Some suggestions were
> >
> >
> > 0. Leave things as they are
> >
> > 1. Allow packing revisions without packing revprops.
> > (revprops/ remains as in 1.6/f4)
> >
> > 2. Have all revprops in the DB all the time, never in plain files.
> >
> > 3. Swap the DB for some other "A thousand revision's revprops in one
> > file" solution. [several suggestions as to that file's format]
> >
> >
> > Can we decide on what to do here?
> >
> > Thanks,
> >
> > Daniel
> >
> >
> > ---------
> >
> > My opinion:
> >
> > * (1) is orthogonal to the others, but may be a good idea if we refactor
> > the FS so shortly before the release
> >
> > * (2) simplifies things, doesn't solve the problems with having an
> > SQLite db authoritative for parts of the FS storage
> > (read: cp(1) unsafe)
> >
> > * (3) has my +1, assuming it's sufficiently performant and the concrete
> > design is reasonable
> >
> > * (0) would mean that if we refactor revprop storage later, 1.8 servers
> > will have to try and read revprops from *three* places; and lots of
> > headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
> > So, if f5 should be improved, I'd rather do that /before/ it's
> > released (and has to be indefinitely supported).
>
> After a bit of thinking and discussion, Daniel and I have come up with
> what we think is an acceptable solution, and I'm posting it here for
> validation. (Daniel, please correct me if I've gotten something
> wrong.)
>
Yes, that is what we discussed. I'll add some comments below.
> Revision properties will *not* be packed in an sqlite database, but
> will instead be packed in a single packfile, much like revision are to
> today. The key difference is that instead of having a separate
> manifest file, the manifest will be prepended to the packfile, meaning
> the two can be atomically replaced in the case of a propedit.
The manifest would consist of fixed-width records, to facilitate seeking
for rN's manifest entry: seek(record_size * (N % shard_size)).
> This solution has at least of couple of advantages:
> * No need to check a separate "edited" file before reading the packfile
> * The repo maintains consistency in the case of a filesystem copy
> (helpful for backups)
>
The process to edit a revprop that had been packed would be:
* grab write lock
* prepare a new pack-file-with-inline-manifest
* move-into-place, atomically replacing the previous pack file
* ungrab write lock
That is what guarantees cp(1) consistency that Hyrum mentions.
This also implies that propediting a revprop-that-had-been-packed will
have to rewrite a packfile containing a thousand revision's properties.
We expect that to be a reasonable cost given that revprop files are
small and historical revprops are rarely edited.
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem. In addition, the revprop
> packfile manifest information won't be cached, since the manifest may
> change. We don't anticipate this to be a problem, since it only adds
> an extra seek() to the revprop lookup process (rather than the open()
> + seek() in the rev packing world).
>
Indeed. The svn_fs_revision_proplist() would do:
* when revision is packed:
+ open pack file
+ seek to manifest entry
+ seek to revision's hunk
+ svn_hash_read2() that
* when revision is not packed:
+ open the sharded file
- if it doesn't exist, refresh ffd->min_unpacked_revprop and retry
+ svn_hash_read2() that
We trade a cache lookup for a seek() within a file that we have to open
anyway.
> Comments?
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, Jul 6, 2011 at 2:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Wed, Jul 6, 2011 at 23:30, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>> On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <da...@elego.de> wrote:
>>> This thread is now the only non-FSFS release blocker (filed as #3944).
>>> Last I checked there were at least three solutions suggested, but no
>>> consensus on which solution to implement.
>>>
>>> Some suggestions were
>>>
>>>
>>> 0. Leave things as they are
>>>
>>> 1. Allow packing revisions without packing revprops.
>>> (revprops/ remains as in 1.6/f4)
>>>
>>> 2. Have all revprops in the DB all the time, never in plain files.
>>>
>>> 3. Swap the DB for some other "A thousand revision's revprops in one
>>> file" solution. [several suggestions as to that file's format]
>>>
>>>
>>> Can we decide on what to do here?
>>>
>>> Thanks,
>>>
>>> Daniel
>>>
>>>
>>> ---------
>>>
>>> My opinion:
>>>
>>> * (1) is orthogonal to the others, but may be a good idea if we refactor
>>> the FS so shortly before the release
>>>
>>> * (2) simplifies things, doesn't solve the problems with having an
>>> SQLite db authoritative for parts of the FS storage
>>> (read: cp(1) unsafe)
>>>
>>> * (3) has my +1, assuming it's sufficiently performant and the concrete
>>> design is reasonable
>>>
>>> * (0) would mean that if we refactor revprop storage later, 1.8 servers
>>> will have to try and read revprops from *three* places; and lots of
>>> headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
>>> So, if f5 should be improved, I'd rather do that /before/ it's
>>> released (and has to be indefinitely supported).
>>
>> After a bit of thinking and discussion, Daniel and I have come up with
>> what we think is an acceptable solution, and I'm posting it here for
>> validation. (Daniel, please correct me if I've gotten something
>> wrong.)
>>
>> Revision properties will *not* be packed in an sqlite database, but
>> will instead be packed in a single packfile, much like revision are to
>> today.
> Are you suggesting to create single packfile for whole repository or
> packfile for each shard? Per shard packfile looks more reasonable for
> me.
Packfile-per-shard.
>> The key difference is that instead of having a separate
>> manifest file, the manifest will be prepended to the packfile, meaning
>> the two can be atomically replaced in the case of a propedit.
>> This solution has at least of couple of advantages:
>> * No need to check a separate "edited" file before reading the packfile
>> * The repo maintains consistency in the case of a filesystem copy
>> (helpful for backups)
>>
>> Revprops wouldn't be packed until explicitly asked to do so by
>> 'svnadmin pack' which means the frequent post-commit revprop editing
>> wouldn't pose a performance problem. In addition, the revprop
>> packfile manifest information won't be cached, since the manifest may
>> change. We don't anticipate this to be a problem, since it only adds
>> an extra seek() to the revprop lookup process (rather than the open()
>> + seek() in the rev packing world).
>>
>> Comments?
>>
>
>
> --
> Ivan Zhakov
>
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, Jul 6, 2011 at 23:30, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <da...@elego.de> wrote:
>> This thread is now the only non-FSFS release blocker (filed as #3944).
>> Last I checked there were at least three solutions suggested, but no
>> consensus on which solution to implement.
>>
>> Some suggestions were
>>
>>
>> 0. Leave things as they are
>>
>> 1. Allow packing revisions without packing revprops.
>> (revprops/ remains as in 1.6/f4)
>>
>> 2. Have all revprops in the DB all the time, never in plain files.
>>
>> 3. Swap the DB for some other "A thousand revision's revprops in one
>> file" solution. [several suggestions as to that file's format]
>>
>>
>> Can we decide on what to do here?
>>
>> Thanks,
>>
>> Daniel
>>
>>
>> ---------
>>
>> My opinion:
>>
>> * (1) is orthogonal to the others, but may be a good idea if we refactor
>> the FS so shortly before the release
>>
>> * (2) simplifies things, doesn't solve the problems with having an
>> SQLite db authoritative for parts of the FS storage
>> (read: cp(1) unsafe)
>>
>> * (3) has my +1, assuming it's sufficiently performant and the concrete
>> design is reasonable
>>
>> * (0) would mean that if we refactor revprop storage later, 1.8 servers
>> will have to try and read revprops from *three* places; and lots of
>> headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
>> So, if f5 should be improved, I'd rather do that /before/ it's
>> released (and has to be indefinitely supported).
>
> After a bit of thinking and discussion, Daniel and I have come up with
> what we think is an acceptable solution, and I'm posting it here for
> validation. (Daniel, please correct me if I've gotten something
> wrong.)
>
> Revision properties will *not* be packed in an sqlite database, but
> will instead be packed in a single packfile, much like revision are to
> today.
Are you suggesting to create single packfile for whole repository or
packfile for each shard? Per shard packfile looks more reasonable for
me.
> The key difference is that instead of having a separate
> manifest file, the manifest will be prepended to the packfile, meaning
> the two can be atomically replaced in the case of a propedit.
> This solution has at least of couple of advantages:
> * No need to check a separate "edited" file before reading the packfile
> * The repo maintains consistency in the case of a filesystem copy
> (helpful for backups)
>
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem. In addition, the revprop
> packfile manifest information won't be cached, since the manifest may
> change. We don't anticipate this to be a problem, since it only adds
> an extra seek() to the revprop lookup process (rather than the open()
> + seek() in the rev packing world).
>
> Comments?
>
--
Ivan Zhakov
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Wed, Jul 06, 2011 at 21:40:00 +0200:
> On Wed, Jul 06, 2011 at 02:30:15PM -0500, Hyrum K Wright wrote:
> > After a bit of thinking and discussion, Daniel and I have come up with
> > what we think is an acceptable solution, and I'm posting it here for
> > validation. (Daniel, please correct me if I've gotten something
> > wrong.)
> >
> > Revision properties will *not* be packed in an sqlite database, but
> > will instead be packed in a single packfile, much like revision are to
> > today. The key difference is that instead of having a separate
> > manifest file, the manifest will be prepended to the packfile, meaning
> > the two can be atomically replaced in the case of a propedit.
> > This solution has at least of couple of advantages:
> > * No need to check a separate "edited" file before reading the packfile
> > * The repo maintains consistency in the case of a filesystem copy
> > (helpful for backups)
> >
> > Revprops wouldn't be packed until explicitly asked to do so by
> > 'svnadmin pack' which means the frequent post-commit revprop editing
> > wouldn't pose a performance problem. In addition, the revprop
> > packfile manifest information won't be cached, since the manifest may
> > change. We don't anticipate this to be a problem, since it only adds
> > an extra seek() to the revprop lookup process (rather than the open()
> > + seek() in the rev packing world).
> >
> > Comments?
>
> Sounds good to me, if this is the last non-trivial change we squeeze
> in before release... IIRC this code was initially written within
> a couple of days anyway during one of the hackathons in Munich.
>
Are you thinking of rep-sharing? Revprop packing is a pquerna patch.
> I hope we have good regression tests for revprop packing (I haven't
> checked)?
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jul 06, 2011 at 02:30:15PM -0500, Hyrum K Wright wrote:
> After a bit of thinking and discussion, Daniel and I have come up with
> what we think is an acceptable solution, and I'm posting it here for
> validation. (Daniel, please correct me if I've gotten something
> wrong.)
>
> Revision properties will *not* be packed in an sqlite database, but
> will instead be packed in a single packfile, much like revision are to
> today. The key difference is that instead of having a separate
> manifest file, the manifest will be prepended to the packfile, meaning
> the two can be atomically replaced in the case of a propedit.
> This solution has at least of couple of advantages:
> * No need to check a separate "edited" file before reading the packfile
> * The repo maintains consistency in the case of a filesystem copy
> (helpful for backups)
>
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem. In addition, the revprop
> packfile manifest information won't be cached, since the manifest may
> change. We don't anticipate this to be a problem, since it only adds
> an extra seek() to the revprop lookup process (rather than the open()
> + seek() in the rev packing world).
>
> Comments?
Sounds good to me, if this is the last non-trivial change we squeeze
in before release... IIRC this code was initially written within
a couple of days anyway during one of the hackathons in Munich.
I hope we have good regression tests for revprop packing (I haven't
checked)?
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <da...@elego.de>.
Some discussion on IRC concerning editing manifest records in-place,
rather than via move-into-place of a tempfile, boiled down to "manifest
records should not cross OS page boundaries", and therefore "manifest
record length (i.e., the number of bytes for one revision's manifest
entry) should be a power of 2".
This came up in context of a proposal involving in-place editing of
manifests which hopefully one of us will bring to this list on
a separate email if it's fruitable.
[[[
00:54:59 @danielsh | when you say "I/O reordering", do you mean
00:55:11 @danielsh | "write()s by any one process are (not) done in chronological order"?
00:55:23 @stefan2 | exactly
00:55:32 @danielsh | ok
00:55:36 @danielsh | so, yes, that's one point I had in mind
00:55:41 @danielsh | today we only rely on move-into-place
00:55:52 @danielsh | any [pg]-esque suggestion means we require something new
00:56:05 @stefan2 | but they should be come visible in chronological order between processes - at least within the same
| address page
00:56:06 @danielsh | in this case, correct handling of overwriting of bytes in a file
00:56:34 @danielsh | re what you just said
00:56:40 @danielsh | why? also between threads of the same process
00:56:53 @danielsh | [ and I don't follow how/why address spaces factor in ]
00:57:24 @stefan2 | file caches use memory?
00:57:56 @danielsh | ah.
00:58:01 @stefan2 | the OS might reorder stuff for different pages but not for the same
00:58:15 @stefan2 | (no idea how relevant / likely that actually is)
00:58:18 @danielsh | so you said should == 'will probably be' rather than 'should == need to be'
00:58:38 @stefan2 | yes
00:59:02 @danielsh | so. if the manifest record crosses a page boundary we might have a VERY edge case bug?
00:59:42 @danielsh | have to admit I wouldn't have considered that... I would have stopped at the file level not at the
| page-that-file-is-cached-in level
01:01:16 @stefan2 | I have no idea how an OS makes shared file content visible to the respective processes. the edge
| case might happen only if a "record" crosses the page boundary
01:02:16 @danielsh | ack
01:02:49 @danielsh | you're saying the OS might not be ACID'ing the view of the file. (since it presents a view that
| never was present on disk)
01:02:59 @danielsh | who knows, that may be a risk we'll take
01:03:25 @stefan2 | there is copy-on-write semantics for (some forms of) memory mapped files under windows. Depending on
| when the respective page is mapped it may see the old or new content (the old page got copied while
| the new one didn't need to back then)
01:03:45 @peterS | danielsh: if the manifest record crosses a page boundary, the reason this might matter is in case of
| an OS crash. one block is written, the next not
01:04:29 @stefan2 | danielsh: yes. but I have no actual facts / pointers to support that suspicion.
01:04:34 @peterS | but if we're using 16-byte records and aligning to 16 byte boundaries, it would take a very strange
| disk block size to make this possible
01:05:34 @peterS | if we think it's unreasonable to assume 1000 revprop blobs will always average less than 256 GB per
| blob, though, then we'll need more than 16 bytes. or a binary representation as I've argued for
| before
01:05:34 @danielsh | stefan2, peterS: so between the two of you you're arguing that records should not cross page
| boundaries
01:05:56 @danielsh | fine, and I'll raise that point on dev@ for posterity
01:06:18 @peterS | I think that's reasonable. and we don't even know the disk or filesystem block size, i.e., we can't
| _really_ assume it's greater than, say, 512 bytes
01:06:31 @peterS | so we really want to align on a power-of-two
01:06:33 @stefan2 | peterS: any 2^N size should work
01:06:42 @peterS | ...stefan2 agreed (:
01:06:48 @stefan2 | indeed ;)
01:07:09 @danielsh | +1
]]]
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Peter Samuelson <pe...@p12n.org>.
[Hyrum K Wright]
> I've created the revprop-packing branch to do the implementation. It
> currently has a skeleton BRANCH-README, which Daniel will be
> augmenting shortly.
This might open another small can o' worms, but "while we're already
bumping the format and rewriting the content...": each revprops blob
should include its rev number, length, and checksum, prepended to the
blob itself. (Or if it's awkward to prepend a checksum, put those
things at the end, and omit the length as it is redundant by then.)
This gives us just that much more for 'svnadmin verify' to verify, and
that's a Good Thing. It is also insurance of a sort, to make it easier
to implement things like rewriting a revprops entry _in situ_ when the
new is not larger than the old (including, perhaps, a bit of padding).
A reader that encounters a bad checksum could just pause, retry, and
only throw an error if the wrong checksum persists too long. Also,
this removes the write barrier requirements between the content and the
manifest, even if we only append edited entries.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, Jul 6, 2011 at 2:30 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <da...@elego.de> wrote:
>> This thread is now the only non-FSFS release blocker (filed as #3944).
>> Last I checked there were at least three solutions suggested, but no
>> consensus on which solution to implement.
>>
>> Some suggestions were
>>
>>
>> 0. Leave things as they are
>>
>> 1. Allow packing revisions without packing revprops.
>> (revprops/ remains as in 1.6/f4)
>>
>> 2. Have all revprops in the DB all the time, never in plain files.
>>
>> 3. Swap the DB for some other "A thousand revision's revprops in one
>> file" solution. [several suggestions as to that file's format]
>>
>>
>> Can we decide on what to do here?
>>
>> Thanks,
>>
>> Daniel
>>
>>
>> ---------
>>
>> My opinion:
>>
>> * (1) is orthogonal to the others, but may be a good idea if we refactor
>> the FS so shortly before the release
>>
>> * (2) simplifies things, doesn't solve the problems with having an
>> SQLite db authoritative for parts of the FS storage
>> (read: cp(1) unsafe)
>>
>> * (3) has my +1, assuming it's sufficiently performant and the concrete
>> design is reasonable
>>
>> * (0) would mean that if we refactor revprop storage later, 1.8 servers
>> will have to try and read revprops from *three* places; and lots of
>> headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
>> So, if f5 should be improved, I'd rather do that /before/ it's
>> released (and has to be indefinitely supported).
>
> After a bit of thinking and discussion, Daniel and I have come up with
> what we think is an acceptable solution, and I'm posting it here for
> validation. (Daniel, please correct me if I've gotten something
> wrong.)
>
> Revision properties will *not* be packed in an sqlite database, but
> will instead be packed in a single packfile, much like revision are to
> today. The key difference is that instead of having a separate
> manifest file, the manifest will be prepended to the packfile, meaning
> the two can be atomically replaced in the case of a propedit.
> This solution has at least of couple of advantages:
> * No need to check a separate "edited" file before reading the packfile
> * The repo maintains consistency in the case of a filesystem copy
> (helpful for backups)
>
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem. In addition, the revprop
> packfile manifest information won't be cached, since the manifest may
> change. We don't anticipate this to be a problem, since it only adds
> an extra seek() to the revprop lookup process (rather than the open()
> + seek() in the rev packing world).
I've created the revprop-packing branch to do the implementation. It
currently has a skeleton BRANCH-README, which Daniel will be
augmenting shortly.
-Hyrum
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Peter Samuelson <pe...@p12n.org>.
> Peter Samuelson wrote on Wed, Jul 06, 2011 at 15:10:15 -0500:
> > I still think it'd be possible, even practical, to create packfiles on
> > the fly, instead of just explicitly via svnadmin pack. This requires
[Daniel Shahaf]
> What do you gain by that?
The main thing you gain is a single code path for reading revprops.
This may come with some efficiency for reads, as well. (If there were
no efficiency gain for packed revprops, why would we have done it at
all?)
Of course, the single codepath is already not possible if we need to
support _optional_ revprop packing, and of course prior fsfs formats.
> > Then we have the possibly common case of editing the HEAD revprops
> > immediately after commit ... say, from post-commit hook. This can
> > actually be special-cased, _because_ it is the HEAD rev:
> >
> > 0) Definitions: R0 = existing revprops blob, RN = new revprops blob
> > 1) Rewrite R0 at offset R0 + max(len(R0), len(RN))
> > 2) Rewrite R0 manifest entry
>
> How do you rewrite the manifest entry atomically?
Whatever C99 or POSIX say, I don't see why write() of just a few bytes
would ever not be atomic. Especially if we choose 16 bytes, as that
will never cross a filesystem block boundary.
> > 3) Write RN at offset R0
> > 4) Rewrite RN manifest entry
> > 5) Truncate file after RN
It occurs to me, however, that step 5 might not be safe. There could
be an unbounded amount of time a reader caches a manifest entry before
trying to read the corresponding revprops entry. This might shoot down
my whole 5-step optimized rewrite of HEAD revprops in a packfile idea.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Peter Samuelson wrote on Wed, Jul 06, 2011 at 15:10:15 -0500:
>
> [Hyrum K Wright]
> > Revprops wouldn't be packed until explicitly asked to do so by
> > 'svnadmin pack' which means the frequent post-commit revprop editing
> > wouldn't pose a performance problem.
>
> I still think it'd be possible, even practical, to create packfiles on
> the fly, instead of just explicitly via svnadmin pack. This requires
What do you gain by that?
> preallocating the 'manifest' region at the top of the file, to
> accommodate a full shard. And that in turn is easiest if manifest
> entries are fixed-width, say, 16 bytes, space-padded. (This limits us
> to 48-bit offsets. At 1000 revs per shard, you can average 256 GB of
> revprops per revision. Which is some distance into the realm of the
> ridiculous.)
>
> There is the question of atomicity of updates. But with the HEAD
> revision, note that nobody should be trying to read it until the commit
> is official! Therefore, assuming you hold a write lock, it should be
> perfectly safe to append the revprops to the pack file, then update the
> manifest entry, then release the write lock.
>
> Then we have the possibly common case of editing the HEAD revprops
> immediately after commit ... say, from post-commit hook. This can
> actually be special-cased, _because_ it is the HEAD rev:
>
> 0) Definitions: R0 = existing revprops blob, RN = new revprops blob
> 1) Rewrite R0 at offset R0 + max(len(R0), len(RN))
> 2) Rewrite R0 manifest entry
How do you rewrite the manifest entry atomically?
> 3) Write RN at offset R0
> 4) Rewrite RN manifest entry
> 5) Truncate file after RN
>
> This may seem to be a bit of work, but I bet it's not really
> noticeable. Plus, post-commit hook performance isn't nearly so
> important as performance of the rest of the system.
>
> Peter
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Peter Samuelson <pe...@p12n.org>.
[Hyrum K Wright]
> Revprops wouldn't be packed until explicitly asked to do so by
> 'svnadmin pack' which means the frequent post-commit revprop editing
> wouldn't pose a performance problem.
I still think it'd be possible, even practical, to create packfiles on
the fly, instead of just explicitly via svnadmin pack. This requires
preallocating the 'manifest' region at the top of the file, to
accommodate a full shard. And that in turn is easiest if manifest
entries are fixed-width, say, 16 bytes, space-padded. (This limits us
to 48-bit offsets. At 1000 revs per shard, you can average 256 GB of
revprops per revision. Which is some distance into the realm of the
ridiculous.)
There is the question of atomicity of updates. But with the HEAD
revision, note that nobody should be trying to read it until the commit
is official! Therefore, assuming you hold a write lock, it should be
perfectly safe to append the revprops to the pack file, then update the
manifest entry, then release the write lock.
Then we have the possibly common case of editing the HEAD revprops
immediately after commit ... say, from post-commit hook. This can
actually be special-cased, _because_ it is the HEAD rev:
0) Definitions: R0 = existing revprops blob, RN = new revprops blob
1) Rewrite R0 at offset R0 + max(len(R0), len(RN))
2) Rewrite R0 manifest entry
3) Write RN at offset R0
4) Rewrite RN manifest entry
5) Truncate file after RN
This may seem to be a bit of work, but I bet it's not really
noticeable. Plus, post-commit hook performance isn't nearly so
important as performance of the rest of the system.
Peter
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, Jul 6, 2011 at 11:47 AM, Daniel Shahaf <da...@elego.de> wrote:
> This thread is now the only non-FSFS release blocker (filed as #3944).
> Last I checked there were at least three solutions suggested, but no
> consensus on which solution to implement.
>
> Some suggestions were
>
>
> 0. Leave things as they are
>
> 1. Allow packing revisions without packing revprops.
> (revprops/ remains as in 1.6/f4)
>
> 2. Have all revprops in the DB all the time, never in plain files.
>
> 3. Swap the DB for some other "A thousand revision's revprops in one
> file" solution. [several suggestions as to that file's format]
>
>
> Can we decide on what to do here?
>
> Thanks,
>
> Daniel
>
>
> ---------
>
> My opinion:
>
> * (1) is orthogonal to the others, but may be a good idea if we refactor
> the FS so shortly before the release
>
> * (2) simplifies things, doesn't solve the problems with having an
> SQLite db authoritative for parts of the FS storage
> (read: cp(1) unsafe)
>
> * (3) has my +1, assuming it's sufficiently performant and the concrete
> design is reasonable
>
> * (0) would mean that if we refactor revprop storage later, 1.8 servers
> will have to try and read revprops from *three* places; and lots of
> headache in the upgrade (and read-from-a-being-upgraded-FS) codepaths.
> So, if f5 should be improved, I'd rather do that /before/ it's
> released (and has to be indefinitely supported).
After a bit of thinking and discussion, Daniel and I have come up with
what we think is an acceptable solution, and I'm posting it here for
validation. (Daniel, please correct me if I've gotten something
wrong.)
Revision properties will *not* be packed in an sqlite database, but
will instead be packed in a single packfile, much like revision are to
today. The key difference is that instead of having a separate
manifest file, the manifest will be prepended to the packfile, meaning
the two can be atomically replaced in the case of a propedit.
This solution has at least of couple of advantages:
* No need to check a separate "edited" file before reading the packfile
* The repo maintains consistency in the case of a filesystem copy
(helpful for backups)
Revprops wouldn't be packed until explicitly asked to do so by
'svnadmin pack' which means the frequent post-commit revprop editing
wouldn't pose a performance problem. In addition, the revprop
packfile manifest information won't be cached, since the manifest may
change. We don't anticipate this to be a problem, since it only adds
an extra seek() to the revprop lookup process (rather than the open()
+ seek() in the rev packing world).
Comments?
-Hyrum
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no longer
allow usage of traditional backup software?
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/07/2011 10:56 AM, Mark Phippard wrote:
> How about a new option?
>
> 4. Remove the f5 feature, release 1.7 without it and revisit when we
> can settle on a new design?
>
> IMO, it is too late to be considering a brand new design. If we are
> not comfortable with the current implementation let's pull it out and
> come back to it later.
+1. While we're at it (post-1.7), we might consider a design that allows
admins to keep old values of properties, too, for auditability. I would
imagine (perhaps naively) that if were only ever appending to packed files
and updating fixed-location/fixed-width manifests, for example, much of this
I/O worry would go away.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: fsfs revprop packing in f5 Re: Does fsfs revprop packing no
longer allow usage of traditional backup software?
Posted by Mark Phippard <ma...@gmail.com>.
On Wed, Jul 6, 2011 at 12:47 PM, Daniel Shahaf <da...@elego.de> wrote:
> This thread is now the only non-FSFS release blocker (filed as #3944).
> Last I checked there were at least three solutions suggested, but no
> consensus on which solution to implement.
>
> Some suggestions were
>
>
> 0. Leave things as they are
>
> 1. Allow packing revisions without packing revprops.
> (revprops/ remains as in 1.6/f4)
>
> 2. Have all revprops in the DB all the time, never in plain files.
>
> 3. Swap the DB for some other "A thousand revision's revprops in one
> file" solution. [several suggestions as to that file's format]
>
>
> Can we decide on what to do here?
How about a new option?
4. Remove the f5 feature, release 1.7 without it and revisit when we
can settle on a new design?
IMO, it is too late to be considering a brand new design. If we are
not comfortable with the current implementation let's pull it out and
come back to it later.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/