You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2009/11/19 19:09:08 UTC
svn commit: r882232 - in /subversion/trunk/subversion:
libsvn_client/revert.c tests/cmdline/schedule_tests.py
Author: hwright
Date: Thu Nov 19 18:09:08 2009
New Revision: 882232
URL: http://svn.apache.org/viewvc?rev=882232&view=rev
Log:
Replace another use of access batons with the working copy locking APIs.
This has the side effect of allowing behavior that would otherwise error, so
update a test to expect the (improved) behavior.
* subversion/tests/cmdline/schedule_tests.py
(revert_inside_newly_added_dir): Update docs and expectation.
* subversion/libsvn_client/revert.c
(revert): Use the locking APIs in place of access batons.
Modified:
subversion/trunk/subversion/libsvn_client/revert.c
subversion/trunk/subversion/tests/cmdline/schedule_tests.py
Modified: subversion/trunk/subversion/libsvn_client/revert.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/revert.c?rev=882232&r1=882231&r2=882232&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/revert.c (original)
+++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08 2009
@@ -68,19 +68,13 @@
svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
- svn_wc_adm_access_t *adm_access, *target_access;
- const char *target, *local_abspath;
+ const char *local_abspath;
svn_error_t *err;
- int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
-
- SVN_ERR(svn_wc__adm_open_anchor_in_context(
- &adm_access, &target_access, &target,
- ctx->wc_ctx, path, TRUE, adm_lock_level,
- ctx->cancel_func, ctx->cancel_baton,
- pool));
SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
+ SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
+
err = svn_wc_revert4(ctx->wc_ctx,
local_abspath,
depth,
@@ -108,7 +102,8 @@
return svn_error_return(err);
}
- return svn_wc_adm_close2(adm_access, pool);
+ return svn_error_return(
+ svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
}
Modified: subversion/trunk/subversion/tests/cmdline/schedule_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/schedule_tests.py?rev=882232&r1=882231&r2=882232&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/schedule_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/schedule_tests.py Thu Nov 19 18:09:08 2009
@@ -497,6 +497,8 @@
#----------------------------------------------------------------------
# Regression test for issue #854:
# Revert . inside an svn added empty directory should generate an error.
+# Not anymore! wc-ng uses absolute paths for everything, which means we
+# can handle this case without too much trouble.
def revert_inside_newly_added_dir(sbox):
"revert inside a newly added dir"
@@ -511,9 +513,9 @@
svntest.main.run_svn(None, 'add', 'foo')
# Now change into the newly added directory, revert and make sure
- # an error is output.
+ # no error is output.
os.chdir('foo')
- svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
+ svntest.actions.run_and_verify_svn(None, None, [],
'revert', '.')
#----------------------------------------------------------------------
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Nov 23, 2009, at 11:07 AM, Philip Martin wrote:
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> Thanks! Some of my questions may not have been very clear, there may
> have been confusion between "Subversion locks" and "SQLite locks", but
> your answers were good enough.
Yes; thanks, Bert. My explanations can be a bit muddled at times.
>> An atomic transaction which checks the existance of a write lock inside a
>> sqlite transaction can't be interrupted by another process taking the write
>> lock.. As that would require breaking the sqlite transaction.
>
> The transaction ensures there is no other write lock, or it has its
> own write lock or whatever, and the transaction only completes if it's
> true. That sounds plausible. It's not how the current write lock
> checking occurs is it? At present the code checks a boolean flag in
> the PDH struct.
We've gradually been moving code from the access baton into the wc_db, which internally uses the PDH struct, and from on-disk files to the sqlite database. An entry in the WC_LOCK table answers the question "has this directory been locked for writing?" and the LOCKED member of the PDH answers the question "does the write lock belong to this process?"
This is the same paradigm that access batons have been using, and since we've moved it into the wc_db, it's translated pretty well using our per-directory sqlite databases. However, that scheme will shortly disappear, so things have been a bit, shakey while we feel our way around the new locking scheme implementation (especially since we don't yet have centralized metadata).
>> I agree that we should be able to take some shortcuts in the transition
>> period towards WC-NG, because keeping all guarantees up (around loggy as
>> poor mans transaction, etc.) takes an insane amount of extra work that can
>> be /dev/null-ed a few weeks later, when the working queue items and pristine
>> handling replace most of the loggy stuff.
>
> Are the shortcuts listed anywhere? How close are we to centralised
> metadata? I'm still not sure what I should do about code that tries
> to open an sqlite database that no longer exists. Would it be
> sensible to make that "not an error" on the basis that it is one of
> the shortcuts?
Centralized metadata is basically waiting on a few prerequisites:
* Get rid of all calls to svn_wc__entry_modify2()
* Move properties into the sqlite database
* Implement proper pristine handling (a sha-1 indexed datastore)
* Write the upgrade code to centralize the metadata
The properties migration is all but complete. I've got properties-in-sqlite in a working copy locally, but the upgrade process is borked and needs to be fixed before we move the properties. It's been on my stack for the past couple of weeks, but has been repeatedly preempted. :(
As was mentioned earlier, there's been some discussion, and a general understanding that some of the concurrency protections will be loosened on trunk while we do the transition. In the end, we expect to use sqlite transactions for much of our concurrency needs, but there will still be times when we need to have explicit write locks on the database. We may need to go and audit the code once we are finished, but I'm hoping the new (and much simpified) locking scheme makes that easier.
-Hyrum
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
Thanks! Some of my questions may not have been very clear, there may
have been confusion between "Subversion locks" and "SQLite locks", but
your answers were good enough.
> An atomic transaction which checks the existance of a write lock inside a
> sqlite transaction can't be interrupted by another process taking the write
> lock.. As that would require breaking the sqlite transaction.
The transaction ensures there is no other write lock, or it has its
own write lock or whatever, and the transaction only completes if it's
true. That sounds plausible. It's not how the current write lock
checking occurs is it? At present the code checks a boolean flag in
the PDH struct.
> I agree that we should be able to take some shortcuts in the transition
> period towards WC-NG, because keeping all guarantees up (around loggy as
> poor mans transaction, etc.) takes an insane amount of extra work that can
> be /dev/null-ed a few weeks later, when the working queue items and pristine
> handling replace most of the loggy stuff.
Are the shortcuts listed anywhere? How close are we to centralised
metadata? I'm still not sure what I should do about code that tries
to open an sqlite database that no longer exists. Would it be
sensible to make that "not an error" on the basis that it is one of
the shortcuts?
--
Philip
RE: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: maandag 23 november 2009 4:07
> To: Bert Huijben
> Cc: 'Hyrum K. Wright'; dev@subversion.apache.org
> Subject: Re: svn commit: r882232 - in /subversion/trunk/subversion:
> libsvn_client/revert.c tests/cmdline/schedule_tests.py
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> > * The atomic operations. WC-NG operations that can operate without
> outside
> > knowledge learned before the operation.
> >
> > These functions that are just one sqlite transaction by itself, just
> need to
> > make sure nobody else has a write lock. Having a write lock is not
> required
> > for operations like just changing the actual properties on a node. Of
> course
> > nobody else can own a write lock, or it might change the properties
> after
> > sending the commit data, but before moving the data to the base_node
> table.
>
> It's not just commits. WC-NG is supposed to be backwards compatible
> with 1.6 which means that once a process takes a 1.6 write lock no
> other process can modify the wc; it makes no difference whether that
> modification is atomic or not.
>
> You also seem to be implying that some operations don't need to create
> write locks, that atomic operations merely need to ensure no write
> locks exist. At what point does this check occur?
>
> - process A: start atomic transaction
> - process B: take a 1.6 write lock
> - process A: continue atomic transaction
> - process B: use 1.6 write lock
> - process A: continue atomic transaction
> - process B: remove 1.6 write lock
> - process A: complete/commit/finalise atomic transaction
An atomic transaction which checks the existance of a write lock inside a
sqlite transaction can't be interrupted by another process taking the write
lock.. As that would require breaking the sqlite transaction.
> Does the atomic transaction in one process block the 1.6 write lock
> creation in another process? That's just like having the transaction
> take a write lock.
Just checking it inside the transaction is enough. Values that are read
can't change before committing.
Writing it and removing it in the same transaction doesn't add anything.
> Does the atomic transaction check all the way through for a write
> lock? That doesn't even seem possible.
When everything is in a single DB we only have to store the roots of locks
in the database.. So checking if a lock exists for a specific path is
walking up all parents, which is pretty cheap in my eyes. (Not walking all
descendants like pre 1.7)
> Does the transaction do some sort of write lock counting to make sure
> that none have been created during the transaction? I don't see
> anything like that in the code.
See the Sqlite locking documentation
> > Once we have a central DB it will be really easy to perform this
> nobody else
> > has a write lock check. (Checking if we ourselves have a write lock
> ourself
> > is just a memory lookup of course).
>
> So the plan is that locking remains broken until centralised metadata?
Ask Hyrum (who removed the write locks) or Greg (who designed the new meta
data system)...
I objected against removing the 1.6 style write locks until this new system
is in place and well tested. Previous discussion came to the conclusion that
it was OK for trunk to leave out some guarantees for the transition period.
Personally I think we have bigger problems now than this specific locking
issue that can be resolved by introducing these checks in wc_db.c.
I agree that we should be able to take some shortcuts in the transition
period towards WC-NG, because keeping all guarantees up (around loggy as
poor mans transaction, etc.) takes an insane amount of extra work that can
be /dev/null-ed a few weeks later, when the working queue items and pristine
handling replace most of the loggy stuff.
But we have to be in a guaranteed stable/lock safe state at release time.
(And personally I would have preferred to keep the old locks until we are
sure we can remove them in the final state...)
Bert
>
> > * Partial operations
> >
> > These operations rely on data read before the wc_db operation and
> only work
> > correctly if the data didn't change since reading. All entry based
> > operations are in this category and the WC-NG work tries to redesign
> several
> > of these operation to the first class of operations.
> >
> > These operations really need a write lock and we need far more checks
> here.
>
> --
> Philip
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> * The atomic operations. WC-NG operations that can operate without outside
> knowledge learned before the operation.
>
> These functions that are just one sqlite transaction by itself, just need to
> make sure nobody else has a write lock. Having a write lock is not required
> for operations like just changing the actual properties on a node. Of course
> nobody else can own a write lock, or it might change the properties after
> sending the commit data, but before moving the data to the base_node table.
It's not just commits. WC-NG is supposed to be backwards compatible
with 1.6 which means that once a process takes a 1.6 write lock no
other process can modify the wc; it makes no difference whether that
modification is atomic or not.
You also seem to be implying that some operations don't need to create
write locks, that atomic operations merely need to ensure no write
locks exist. At what point does this check occur?
- process A: start atomic transaction
- process B: take a 1.6 write lock
- process A: continue atomic transaction
- process B: use 1.6 write lock
- process A: continue atomic transaction
- process B: remove 1.6 write lock
- process A: complete/commit/finalise atomic transaction
Does the atomic transaction in one process block the 1.6 write lock
creation in another process? That's just like having the transaction
take a write lock.
Does the atomic transaction check all the way through for a write
lock? That doesn't even seem possible.
Does the transaction do some sort of write lock counting to make sure
that none have been created during the transaction? I don't see
anything like that in the code.
> Once we have a central DB it will be really easy to perform this nobody else
> has a write lock check. (Checking if we ourselves have a write lock ourself
> is just a memory lookup of course).
So the plan is that locking remains broken until centralised metadata?
> * Partial operations
>
> These operations rely on data read before the wc_db operation and only work
> correctly if the data didn't change since reading. All entry based
> operations are in this category and the WC-NG work tries to redesign several
> of these operation to the first class of operations.
>
> These operations really need a write lock and we need far more checks here.
--
Philip
RE: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: maandag 23 november 2009 11:09
> To: Hyrum K. Wright
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r882232 - in /subversion/trunk/subversion:
> libsvn_client/revert.c tests/cmdline/schedule_tests.py
>
> Philip Martin <ph...@wandisco.com> writes:
>
> > The current locking appears to be broken, there are paths that modify
> > the metadata without verifying that a lock is held. I don't know how
> > widespread this is
>
> I scattered calls to svn_wc__write_check through libsvn_wc/wc_db.c and
> I get over a hundred regression test failures. I'm tempted to commit
> it despite the failures.
Note that there are two kinds of operations in wc_db.c that need a different
kind of write check:
* The atomic operations. WC-NG operations that can operate without outside
knowledge learned before the operation.
These functions that are just one sqlite transaction by itself, just need to
make sure nobody else has a write lock. Having a write lock is not required
for operations like just changing the actual properties on a node. Of course
nobody else can own a write lock, or it might change the properties after
sending the commit data, but before moving the data to the base_node table.
Once we have a central DB it will be really easy to perform this nobody else
has a write lock check. (Checking if we ourselves have a write lock ourself
is just a memory lookup of course).
* Partial operations
These operations rely on data read before the wc_db operation and only work
correctly if the data didn't change since reading. All entry based
operations are in this category and the WC-NG work tries to redesign several
of these operation to the first class of operations.
These operations really need a write lock and we need far more checks here.
Bert
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> The current locking appears to be broken, there are paths that modify
> the metadata without verifying that a lock is held. I don't know how
> widespread this is
I scattered calls to svn_wc__write_check through libsvn_wc/wc_db.c and
I get over a hundred regression test failures. I'm tempted to commit
it despite the failures.
--
Philip
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
"Hyrum K. Wright" <hy...@mail.utexas.edu> writes:
> [ sorry it's taken a few days to followup with this ]
>
> On Nov 20, 2009, at 1:00 PM, Philip Martin wrote:
>> How do we fix it? I suppose we could simply make it not an error to
>> release locks from directories that don't exist. That would have
>> the drawback that some errors such as releasing the wrong lock
>> wouldn't get caught. We could attempt to second-guess which
>> directories get removed and so restrict which lock releases we allow
>> to fail. That doesn't sound very good either, one bit of code is
>> second-guessing another. Also what happens if something creates a
>> new directory with a name matching the removed one in the gap
>> between the directory removal and the attempted lock release? We
>> need to be very careful not to release a lock we didn't acquire.
>>
>> Another solution would be to track the directory removal and record
>> the "missing" lock until it gets released. We could have
>> svn_wc__db_temp_forget_directory leave an in-memory pdh structure
>> that exists until the lock is released. I think this is a better
>> solution, it also seems closer to the single DB per WC case where it
>> will be possible to hold onto the locks until released.
>>
>> Do you agree there is a problem? Do any of these solutions look
>> like the right thing to do?
>
> I do agree that it is a problem. The long-term (read:
> post-centralized-metadata) solution is to have a lock at any level
> imply an infinitely recursive lock for levels beneath that. Since we
> have not yet centralized metadata, wc_db doesn't understand this
> notion quite yet. svn_wc__acquire_write_lock() and
> svn_wc__release_write_lock() attempt to emulate it with (apparent)
> limited success.
With 1.6 a non-recursive write-lock on a directory and doesn't stop
some other process taking a write-lock on a directory underneath the
locked directory. Are we going to stop supporting that?
>
> A possible solution would be to make the svn_wc__db_* functions
> smarter in that when a lock for a given location is requested, they
> would then apply the lock to that location, and when queried about an
> lock, it would look up the tree to determine if any of the parents had
> a lock. With our SQL schema, that turns out to be pretty easy in the
> centralized environment; with the current paradigm, not so much.
>
> Does this make sense?
It doesn't really answer my question about what should happen when
attempting to release a lock in a db that no longer exists. As I
described in my earlier mail above, we either allow such errors
indiscriminately or we track the names of the directories that have
been removed.
The current locking appears to be broken, there are paths that modify
the metadata without verifying that a lock is held. I don't know how
widespread this is but the first case I looked at is reverting a
schedule add directory: the parent doesn't get locked and yet the
shedule add child gets removed via
revert_entry
svn_wc__internal_remove_from_revision_control
svn_wc__entry_remove
svn_wc__db_temp_op_remove_entry
--
Philip
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
[ sorry it's taken a few days to followup with this ]
On Nov 20, 2009, at 1:00 PM, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> hwright@apache.org writes:
>>>
>>>> Author: hwright
>>>> Date: Thu Nov 19 18:09:08 2009
>>>> New Revision: 882232
>>>
>>>> --- subversion/trunk/subversion/libsvn_client/revert.c (original)
>>>> +++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08 2009
>>>> @@ -68,19 +68,13 @@
>>>> svn_client_ctx_t *ctx,
>>>> apr_pool_t *pool)
>>>> {
>>>> - svn_wc_adm_access_t *adm_access, *target_access;
>>>> - const char *target, *local_abspath;
>>>> + const char *local_abspath;
>>>> svn_error_t *err;
>>>> - int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
>>>> -
>>>> - SVN_ERR(svn_wc__adm_open_anchor_in_context(
>>>> - &adm_access, &target_access, &target,
>>>> - ctx->wc_ctx, path, TRUE, adm_lock_level,
>>>> - ctx->cancel_func, ctx->cancel_baton,
>>>> - pool));
>>>>
>>>> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>>>>
>>>> + SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
>>>> +
>>>> err = svn_wc_revert4(ctx->wc_ctx,
>>>> local_abspath,
>>>> depth,
>>>> @@ -108,7 +102,8 @@
>>>> return svn_error_return(err);
>>>> }
>>>>
>>>> - return svn_wc_adm_close2(adm_access, pool);
>>>> + return svn_error_return(
>>>> + svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
>>>> }
>>>
>>> Your change conflicts with my change shown below. The problem is
>>> shown by depth_tests 33 which reverts a schedule add directory. Your
>>> code above assumes after calling svn_wc_revert4 on such a directory it
>>> is still possible to call svn_wc__release_write_lock. Is that valid
>>> given that revert makes the whole directory vanish?
Probably not.
>> So there might be a problem in my code, but I still don't understand
>> your code. The problem is when local_abspath is a schedule add
>> directory. Your code acquires a write lock on local_abspath and then
>> runs revert, which removes local_abspath from revision control. Then
>> you release the lock on local_abspath which is no longer a versioned
>> directory.
>
> I'm still stuck on this bit above. Your code only works because
> r881265 means that we don't remove .svn directories when reverting
> added directories. That allows you to release the lock because the
> directory still exists; that's not going to work when the directory is
> correctly removed.
>
> How do we fix it? I suppose we could simply make it not an error to
> release locks from directories that don't exist. That would have the
> drawback that some errors such as releasing the wrong lock wouldn't
> get caught. We could attempt to second-guess which directories get
> removed and so restrict which lock releases we allow to fail. That
> doesn't sound very good either, one bit of code is second-guessing
> another. Also what happens if something creates a new directory with
> a name matching the removed one in the gap between the directory
> removal and the attempted lock release? We need to be very careful
> not to release a lock we didn't acquire.
>
> Another solution would be to track the directory removal and record
> the "missing" lock until it gets released. We could have
> svn_wc__db_temp_forget_directory leave an in-memory pdh structure that
> exists until the lock is released. I think this is a better solution,
> it also seems closer to the single DB per WC case where it will be
> possible to hold onto the locks until released.
>
> Do you agree there is a problem? Do any of these solutions look like
> the right thing to do?
I do agree that it is a problem. The long-term (read: post-centralized-metadata) solution is to have a lock at any level imply an infinitely recursive lock for levels beneath that. Since we have not yet centralized metadata, wc_db doesn't understand this notion quite yet. svn_wc__acquire_write_lock() and svn_wc__release_write_lock() attempt to emulate it with (apparent) limited success.
A possible solution would be to make the svn_wc__db_* functions smarter in that when a lock for a given location is requested, they would then apply the lock to that location, and when queried about an lock, it would look up the tree to determine if any of the parents had a lock. With our SQL schema, that turns out to be pretty easy in the centralized environment; with the current paradigm, not so much.
Does this make sense?
-Hyrum
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> hwright@apache.org writes:
>>
>>> Author: hwright
>>> Date: Thu Nov 19 18:09:08 2009
>>> New Revision: 882232
>>
>>> --- subversion/trunk/subversion/libsvn_client/revert.c (original)
>>> +++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08 2009
>>> @@ -68,19 +68,13 @@
>>> svn_client_ctx_t *ctx,
>>> apr_pool_t *pool)
>>> {
>>> - svn_wc_adm_access_t *adm_access, *target_access;
>>> - const char *target, *local_abspath;
>>> + const char *local_abspath;
>>> svn_error_t *err;
>>> - int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
>>> -
>>> - SVN_ERR(svn_wc__adm_open_anchor_in_context(
>>> - &adm_access, &target_access, &target,
>>> - ctx->wc_ctx, path, TRUE, adm_lock_level,
>>> - ctx->cancel_func, ctx->cancel_baton,
>>> - pool));
>>>
>>> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>>>
>>> + SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
>>> +
>>> err = svn_wc_revert4(ctx->wc_ctx,
>>> local_abspath,
>>> depth,
>>> @@ -108,7 +102,8 @@
>>> return svn_error_return(err);
>>> }
>>>
>>> - return svn_wc_adm_close2(adm_access, pool);
>>> + return svn_error_return(
>>> + svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
>>> }
>>
>> Your change conflicts with my change shown below. The problem is
>> shown by depth_tests 33 which reverts a schedule add directory. Your
>> code above assumes after calling svn_wc_revert4 on such a directory it
>> is still possible to call svn_wc__release_write_lock. Is that valid
>> given that revert makes the whole directory vanish?
>
> So there might be a problem in my code, but I still don't understand
> your code. The problem is when local_abspath is a schedule add
> directory. Your code acquires a write lock on local_abspath and then
> runs revert, which removes local_abspath from revision control. Then
> you release the lock on local_abspath which is no longer a versioned
> directory.
I'm still stuck on this bit above. Your code only works because
r881265 means that we don't remove .svn directories when reverting
added directories. That allows you to release the lock because the
directory still exists; that's not going to work when the directory is
correctly removed.
How do we fix it? I suppose we could simply make it not an error to
release locks from directories that don't exist. That would have the
drawback that some errors such as releasing the wrong lock wouldn't
get caught. We could attempt to second-guess which directories get
removed and so restrict which lock releases we allow to fail. That
doesn't sound very good either, one bit of code is second-guessing
another. Also what happens if something creates a new directory with
a name matching the removed one in the gap between the directory
removal and the attempted lock release? We need to be very careful
not to release a lock we didn't acquire.
Another solution would be to track the directory removal and record
the "missing" lock until it gets released. We could have
svn_wc__db_temp_forget_directory leave an in-memory pdh structure that
exists until the lock is released. I think this is a better solution,
it also seems closer to the single DB per WC case where it will be
possible to hold onto the locks until released.
Do you agree there is a problem? Do any of these solutions look like
the right thing to do?
> Inside svn_wc__release_write_lock the first check on kind
> gets svn_wc__db_kind_unknown so the code then gets the parent and
> releases locks from there. The recently reverted directory is no
> longer a child of the parent, isn't locked and so doesn't get a lock
> released. In effect the release call is releasing a completely
> different set of locks to those acquired earlier in the function.
> That's not the right thing to do is it?
That bit was my mistake, I had a patch applied.
--
Philip
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> hwright@apache.org writes:
>
>> Author: hwright
>> Date: Thu Nov 19 18:09:08 2009
>> New Revision: 882232
>
>> --- subversion/trunk/subversion/libsvn_client/revert.c (original)
>> +++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08 2009
>> @@ -68,19 +68,13 @@
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool)
>> {
>> - svn_wc_adm_access_t *adm_access, *target_access;
>> - const char *target, *local_abspath;
>> + const char *local_abspath;
>> svn_error_t *err;
>> - int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
>> -
>> - SVN_ERR(svn_wc__adm_open_anchor_in_context(
>> - &adm_access, &target_access, &target,
>> - ctx->wc_ctx, path, TRUE, adm_lock_level,
>> - ctx->cancel_func, ctx->cancel_baton,
>> - pool));
>>
>> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>>
>> + SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
>> +
>> err = svn_wc_revert4(ctx->wc_ctx,
>> local_abspath,
>> depth,
>> @@ -108,7 +102,8 @@
>> return svn_error_return(err);
>> }
>>
>> - return svn_wc_adm_close2(adm_access, pool);
>> + return svn_error_return(
>> + svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
>> }
>
> Your change conflicts with my change shown below. The problem is
> shown by depth_tests 33 which reverts a schedule add directory. Your
> code above assumes after calling svn_wc_revert4 on such a directory it
> is still possible to call svn_wc__release_write_lock. Is that valid
> given that revert makes the whole directory vanish?
So there might be a problem in my code, but I still don't understand
your code. The problem is when local_abspath is a schedule add
directory. Your code acquires a write lock on local_abspath and then
runs revert, which removes local_abspath from revision control. Then
you release the lock on local_abspath which is no longer a versioned
directory. Inside svn_wc__release_write_lock the first check on kind
gets svn_wc__db_kind_unknown so the code then gets the parent and
releases locks from there. The recently reverted directory is no
longer a child of the parent, isn't locked and so doesn't get a lock
released. In effect the release call is releasing a completely
different set of locks to those acquired earlier in the function.
That's not the right thing to do is it?
--
Philip
Re: svn commit: r882232 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/schedule_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
hwright@apache.org writes:
> Author: hwright
> Date: Thu Nov 19 18:09:08 2009
> New Revision: 882232
> --- subversion/trunk/subversion/libsvn_client/revert.c (original)
> +++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08 2009
> @@ -68,19 +68,13 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - svn_wc_adm_access_t *adm_access, *target_access;
> - const char *target, *local_abspath;
> + const char *local_abspath;
> svn_error_t *err;
> - int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> -
> - SVN_ERR(svn_wc__adm_open_anchor_in_context(
> - &adm_access, &target_access, &target,
> - ctx->wc_ctx, path, TRUE, adm_lock_level,
> - ctx->cancel_func, ctx->cancel_baton,
> - pool));
>
> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>
> + SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
> +
> err = svn_wc_revert4(ctx->wc_ctx,
> local_abspath,
> depth,
> @@ -108,7 +102,8 @@
> return svn_error_return(err);
> }
>
> - return svn_wc_adm_close2(adm_access, pool);
> + return svn_error_return(
> + svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
> }
Your change conflicts with my change shown below. The problem is
shown by depth_tests 33 which reverts a schedule add directory. Your
code above assumes after calling svn_wc_revert4 on such a directory it
is still possible to call svn_wc__release_write_lock. Is that valid
given that revert makes the whole directory vanish?
Note that in the 1.6 code it was possible to close access batons more
than once, and that r881265 introduced what is in my view an erroneous
skip of a null access baton.
My change:
A better fix for the SEGV fixed in r881265: replace another use
of access batons.
* subversion/libsvn_wc/adm_files.h
(svn_wc__adm_destroy): Pass svn_wc__db_t and abspath rather than
svn_wc_adm_access_t.
* subversion/libsvn_wc/adm_files.c
(svn_wc__adm_destroy): Use svn_wc__db_t directly.
* subversion/libsvn_wc/adm_ops.c
(svn_wc__internal_remove_from_revision_control): Revert r881265: no
need to check for null access baton.
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 882277)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -2528,14 +2528,8 @@
/* Remove the entire administrative .svn area, thereby removing
_this_ dir from revision control too. */
- {
- svn_wc_adm_access_t *adm_access =
- svn_wc__adm_retrieve_internal2(db, local_abspath, iterpool);
+ SVN_ERR(svn_wc__adm_destroy(db, local_abspath, iterpool));
- if (adm_access)
- SVN_ERR(svn_wc__adm_destroy(adm_access, iterpool));
- }
-
/* If caller wants us to recursively nuke everything on disk, go
ahead, provided that there are no dangling local-mod files
below */
Index: subversion/libsvn_wc/adm_files.c
===================================================================
--- subversion/libsvn_wc/adm_files.c (revision 882277)
+++ subversion/libsvn_wc/adm_files.c (working copy)
@@ -611,26 +611,21 @@
}
svn_error_t *
-svn_wc__adm_destroy(svn_wc_adm_access_t *adm_access,
+svn_wc__adm_destroy(svn_wc__db_t *db,
+ const char *dir_abspath,
apr_pool_t *scratch_pool)
{
- const char *path, *local_abspath;
- svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
+ const char *fullpath;
- SVN_ERR(svn_wc__write_check(db, svn_wc__adm_access_abspath(adm_access),
- scratch_pool));
+ SVN_ERR(svn_wc__write_check(db, dir_abspath, scratch_pool));
/* Well, the coast is clear for blowing away the administrative
- directory, which also removes the lock file */
- path = svn_wc__adm_child(svn_wc_adm_access_path(adm_access), NULL,
- scratch_pool);
- local_abspath = svn_wc__adm_access_abspath(adm_access);
+ directory, which also removes the lock */
+ SVN_ERR(svn_wc__db_temp_forget_directory(db, dir_abspath, scratch_pool));
- SVN_ERR(svn_wc_adm_close2(adm_access, scratch_pool));
- SVN_ERR(svn_wc__db_temp_forget_directory(db, local_abspath, scratch_pool));
+ fullpath = extend_with_adm_name(dir_abspath, NULL, FALSE, scratch_pool, NULL);
+ SVN_ERR(svn_io_remove_dir2(fullpath, FALSE, NULL, NULL, scratch_pool));
- SVN_ERR(svn_io_remove_dir2(path, FALSE, NULL, NULL, scratch_pool));
-
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/adm_files.h
===================================================================
--- subversion/libsvn_wc/adm_files.h (revision 882277)
+++ subversion/libsvn_wc/adm_files.h (working copy)
@@ -130,10 +130,9 @@
apr_pool_t *scratch_pool);
-/* Blow away the admistrative directory associated with the access baton
- ADM_ACCESS. This closes ADM_ACCESS, but it is safe to close ADM_ACCESS
- again, after calling this function. */
-svn_error_t *svn_wc__adm_destroy(svn_wc_adm_access_t *adm_access,
+/* Blow away the admistrative directory associated with DIR_ABSPATH */
+svn_error_t *svn_wc__adm_destroy(svn_wc__db_t *db,
+ const char *dir_abspath,
apr_pool_t *scratch_pool);
--
Philip