On Thu, 24 Mar 2005 fitz@tigris.org wrote:
> Author: fitz
> Date: Thu Mar 24 15:01:33 2005
> New Revision: 13648
> Modified: trunk/subversion/libsvn_fs_fs/lock.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_fs_fs/lock.c?view=diff&rev=13648&p1=trunk/subversion/libsvn_fs_fs/lock.c&r1=13647&p2=trunk/subversion/libsvn_fs_fs/lock.c&r2=13648
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/lock.c (original)
> +++ trunk/subversion/libsvn_fs_fs/lock.c Thu Mar 24 15:01:33 2005
> @@ -476,11 +476,15 @@
> }
>
>
> -/* Set *LOCK_P to the lock for PATH in FS. Use POOL for allocations. */
> +/* Set *LOCK_P to the lock for PATH in FS. HAVE_WRITE_LOCK should be
> + TRUE if the caller (or one of its callers) has taken out the
> + repository-wide write lock, FALSE otherwise. Use POOL for
> + allocations. */
> static svn_error_t *
> get_lock (svn_lock_t **lock_p,
> svn_fs_t *fs,
> const char *path,
> + svn_boolean_t have_write_lock,
> apr_pool_t *pool)
> {
> svn_lock_t *lock;
> @@ -493,11 +497,24 @@
> /* Possibly auto-expire the lock. */
> if (lock->expiration_date && (apr_time_now() > lock->expiration_date))
> {
> - /* ### TODO: We need the write lock here, but some callers
> - ### already has it. Also, we need to reread the lock to
> - ### avoid a race. We don't want the write lock grabbed just
> - ### to get a lock. */
> - SVN_ERR (delete_lock (fs, lock, pool));
> + if (have_write_lock)
> + SVN_ERR (delete_lock (fs, lock, pool));
> + else
> + {
> + /* Grab the fs write lock. */
> + apr_pool_t *subpool = svn_pool_create (pool);
> + SVN_ERR (svn_fs_fs__get_write_lock (fs, subpool));
> +
The reason I added an objection to this to the old TODO file was that this
will be serialized with the finalization of a commit. So a get_lock of an
expired lock may appear to have hang for quite some time. I don't think
the comparison to BDB is valid, since it has more fine-graned locking.
OTOH, it is correct. We can fix it later if it becomes a problem. So, no
big deal if you think we might be leaving lots of stray locks around.
> + /* Reread the lock to avoid a race. */
> + SVN_ERR (read_digest_file (NULL, &lock, fs, digest_path, pool));
> + if (! lock)
> + return svn_fs_fs__err_no_such_lock (fs, path);
> +
It is good to destroy the subpoo9l here.
> + SVN_ERR (delete_lock (fs, lock, pool));
> +
The important race is still there. You may delete a lock that was created
after the expiration time check. You need to verify that it is the same
lock you read while the write lock is held.
> + /* Destroy our subpool and release the fs write lock. */
> + svn_pool_destroy (subpool);
> + }
> *lock_p = NULL;
> return svn_fs_fs__err_lock_expired (fs, lock->token);
> }
> @@ -507,17 +524,21 @@
> }
>
>
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 29 11:31:51 2005