You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2005/02/09 20:54:10 UTC
bug in recording fs 'changes'
Hey fsfs experts -- cmpilato fears that fsfs may have this same logic
bug, regarding the recording of changes. Maybe you can take a look.
On Feb 9, 2005, at 2:44 PM, sussman@tigris.org wrote:
> Author: sussman
> Date: Wed Feb 9 14:44:47 2005
> New Revision: 12956
>
> Modified:
> branches/locking/subversion/libsvn_fs_base/tree.c
> Log:
> Locking branch: obscure BDB changes-table bugfix, done with cmpilato.
>
> Normal clients don't stimulate this bug, but the new lock-checking
> behavior in svn_fs_commit_txn() stimulated it in changes-test #5. The
> locking branch should now pass regression tests again.
>
> In particular, changes-test #5 was deleting a child of a dir, then
> deleting the parent dir, all in one transaction. When
> svn_fs_commit_txn() started looping over over the list of txn changes
> (to enforce any locks), it was getting back a mutable-id for the
> deleted parent dir... a mutable-id of a node that was now gone!
>
> * subversion/libsvn_fs_base/tree.c
> (txn_body_delete): don't write a mutable node-id into the changes
> list.
> Instead, write the immutable predecessor id.
>
>
> Modified: branches/locking/subversion/libsvn_fs_base/tree.c
> Url:
> http://svn.collab.net/viewcvs/svn/branches/locking/subversion/
> libsvn_fs_base/tree.c?view=diff&rev=12956&p1=branches/locking/
> subversion/libsvn_fs_base/tree.c&r1=12955&p2=branches/locking/
> subversion/libsvn_fs_base/tree.c&r2=12956
> =======================================================================
> =======
> --- branches/locking/subversion/libsvn_fs_base/tree.c (original)
> +++ branches/locking/subversion/libsvn_fs_base/tree.c Wed Feb 9
> 14:44:47 2005
> @@ -2926,6 +2926,7 @@
> const char *path = args->path;
> parent_path_t *parent_path;
> const char *txn_id = root->txn;
> + const svn_fs_id_t *change_id;
>
> if (! root->is_txn_root)
> return not_txn (root);
> @@ -2956,9 +2957,26 @@
> txn_id, trail));
>
> /* Make a record of this modification in the changes table. */
> - SVN_ERR (add_change (root->fs, txn_id, path,
> - svn_fs_base__dag_get_id (parent_path->node),
> - svn_fs_path_change_delete, 0, 0, trail));
> +
> + /* The goal: avoid writing a 'changes' entry of a mutable node-id.
> + If it's mutable, we try to use its predecessor-id instead, which
> + is guaranteed to be immutable. In the unusual case that there is
> + no predecessor id (e.g. we created and deleted the object all in
> + the same transaction), then we don't want to record any
> + 'changes' information at all -- so we send a 'reset' command. */
> +
> + if (svn_fs_base__dag_check_mutable (parent_path->node, txn_id))
> + SVN_ERR (svn_fs_base__dag_get_predecessor_id (&change_id,
> + parent_path->node,
> + trail));
> + else
> + change_id = svn_fs_base__dag_get_id (parent_path->node);
> +
> + SVN_ERR (add_change (root->fs, txn_id, path, change_id,
> + change_id ? svn_fs_path_change_delete :
> + svn_fs_path_change_reset,
> + 0, 0, trail));
> +
>
> return SVN_NO_ERROR;
> }
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 9 Feb 2005, Ben Collins-Sussman wrote:
> Hey fsfs experts -- cmpilato fears that fsfs may have this same logic
> bug, regarding the recording of changes. Maybe you can take a look.
>
I'm not caliming to be an fsfs expert, but I've taken a look and it indeed
has this bug. I'll see if I can just port the change.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> Philip is right, though. The following scenario is possible:
>
> 1. svn client: deletes a file in a txn
> 2. svn client: adds a directory by the same name
> 3. non-svn DAV client: locks ("reserves") an imaginary path under the
> dir
> 4. svn client: tries to commit
>
> svn_fs_commit_txn() needs to do a recursive lock-check below the added
> directory. Which I believe it does.
It makes no difference whether the added object is a file or a
directory, either will cause the same problem to the client with a
lock on an imaginary path under the added object. Thus added files
need to do the same recursive lock-check as added directories.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by Ben Collins-Sussman <su...@collab.net>.
On Feb 11, 2005, at 5:59 AM, Philip Martin wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
>
>> If the object deleted was a directory, then it
>> wants to check recursively for locks underneath the pathname in
>> question; if the object deleted was a file, it wants to check only for
>> a lock at the specific pathname.
>
> I'm not sure that's correct.
>
> A recent thread confirmed that it's possible to lock schedule add
> files before the path to the file is present in the repository.
> Suppose a schedule delete file were replaced by a schedule add
> directory, then there could be schedule add files within that
> directory. Is it possible to lock such file? If so then there may be
> locks below any given repository object whether that object is a file
> or a directory.
>
> The current command line client cannot create such a working copy, but
> I think that's just a libsvn_wc limitation.
>
Sorry to be slow. I need to go revisit the locking fs code to verify
what's going on.
Philip is right, though. The following scenario is possible:
1. svn client: deletes a file in a txn
2. svn client: adds a directory by the same name
3. non-svn DAV client: locks ("reserves") an imaginary path under the
dir
4. svn client: tries to commit
svn_fs_commit_txn() needs to do a recursive lock-check below the added
directory. Which I believe it does.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:
> If the object deleted was a directory, then it
> wants to check recursively for locks underneath the pathname in
> question; if the object deleted was a file, it wants to check only for
> a lock at the specific pathname.
I'm not sure that's correct.
A recent thread confirmed that it's possible to lock schedule add
files before the path to the file is present in the repository.
Suppose a schedule delete file were replaced by a schedule add
directory, then there could be schedule add files within that
directory. Is it possible to lock such file? If so then there may be
locks below any given repository object whether that object is a file
or a directory.
The current command line client cannot create such a working copy, but
I think that's just a libsvn_wc limitation.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by Greg Hudson <gh...@MIT.EDU>.
I wrote:
> I feel like the locking code should not need to know the type of
> object which was deleted in order to work correctly, and that the
> designers have wandered down the wrong path somewhere. But I don't
> have my head wrapped around the locking code completely, so I can't
> say exactly where I think the wrong turn was.
Okay, I have more thoughts on this. As I understand it, the locking
code wants to know what was deleted so it knows what kind of lock
check to perform. If the object deleted was a directory, then it
wants to check recursively for locks underneath the pathname in
question; if the object deleted was a file, it wants to check only for
a lock at the specific pathname.
However, the same logic applies if a node is replaced. If a directory
is replaced (with a directory or with a file), then the locking code
wants to check recursively; if a file is replaced (again, either with
a directory or with a file), then the locking code wants to check
singly.
But the changes table contains no information about what used to be
present when a node is replaced; it only says what new thing was put
there.
So, no amount of tweaking of what ID is recorded in "delete"
operations will satisfy the locking code; we would need to extend the
changes table to include both old and new IDs for "replace" operations
as well. (And if we do that, perhaps we want old and new IDs for
modifications for consistency, although the locking code doesn't care
because modifications can't change the type of a node.)
Short of making this data format change, the locking code must behave
conservatively, and perform a recursive check whenever it sees a
pathname replaced. Which means it may as well behave that way
whenever it sees a pathname deleted.
As an aside, CMike and I worked out a strategy for correctly recording
the old ID in deletes. It goes like this, for both FSFS and BDB:
* Reintroduce Ben's change on the branch (or keep it, if it hasn't
been reverted), so that a delete of a mutable node-revision
records the ID of the immutable predecessor of that node-revision.
* In txn_body_copy, record replacement operations as a delete + add,
not as a replace. The folding code will canonicalize this into a
replace under normal circumstances.
* When the folding code sees the first delete operation for a
pathname, make it remember what the ID was there. If the final
outcome for the pathname is a delete, use the ID from the first
delete operation, not the ID from the last one.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in recording fs 'changes'
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2005-02-09 at 15:54, Ben Collins-Sussman wrote:
> Hey fsfs experts -- cmpilato fears that fsfs may have this same logic
> bug, regarding the recording of changes. Maybe you can take a look.
FSFS has the same issue.
Unfortunately, r12956 does not produce the correct answer in all
circumstances. There is an even screwier case, identified (mostly) by
Max, which is to do the following, all within a single transaction:
delete a/b (a/b was a directory)
copy some/other/path a/b (some/other/path is a file)
delete a/b
r12956 will record the node-ID of some/other/file in the delete
operation. So we now have a transaction whose effect is to delete a
directory at a/b, but whose changes table says it deleted the node-ID of
some file. To determine the node ID we want to record, we would have to
do something find ugly, like find the old changes entry where a/b was
first deleted, or read the directory information from the predecessor of
the parent directory.
On IRC, I suggested that maybe the locking code should do a path lookup
at the base-rev of the transaction, but Mike pointed out that that isn't
so easy, since some parent directory of a/b may itself have been a copy.
I feel like the locking code should not need to know the type of object
which was deleted in order to work correctly, and that the designers
have wandered down the wrong path somewhere. But I don't have my head
wrapped around the locking code completely, so I can't say exactly where
I think the wrong turn was.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org