On Mon, Jul 21, 2014 at 4:14 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 17.07.2014 16:50, stefan2_at_apache.org wrote:
>
> Author: stefan2
> Date: Thu Jul 17 14:50:49 2014
> New Revision: 1611379
>
> URL: http://svn.apache.org/r1611379
> Log:
> Fix Windows redo loop oddity for named atomics. As it turns out, the redo
> loops for file creation and locking don't play very well with files that
> get / got deleted. This is the root cause for the bad ra_serf performance
> when revprop caching has been enabled.
>
> With this patch, we simply use the same lock file scheme as for e.g. lock
> files in FSFS, i.e. we auto-create an empty lock file and keep it.
>
> * subversion/libsvn_subr/named_atomic.c
> (mutex_t): Don't keep the lock file open while not holding the lock,
> i.e. the mutex is simply the file name and the pool that
> will contain the actual file lock.
> (lock): Auto-create and lock the file as we do in e.g. FSFS.
> (unlock): Simple pool cleanup now released the lock file.
> (delete_lock_file): Drop.
> (svn_atomic_namespace__create): Update mutex initialization code.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/named_atomic.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/named_atomic.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/named_atomic.c?rev=1611379&r1=1611378&r2=1611379&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/named_atomic.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/named_atomic.c Thu Jul 17
> 14:50:49 2014
> @@ -197,8 +197,8 @@ struct shared_data_t
> */
> struct mutex_t
> {
> - /* Inter-process sync. is handled by through lock file. */
> - apr_file_t *lock_file;
> + /* Inter-process sync. is handled by through a lock file. */
> + const char *lock_name;
>
> /* Pool to be used with lock / unlock functions */
> apr_pool_t *pool;
> @@ -276,10 +276,23 @@ lock(struct mutex_t *mutex)
> {
> svn_error_t *err;
>
> - /* Get lock on the filehandle. */
> + /* Intra-process lock */
> SVN_ERR(svn_mutex__lock(thread_mutex));
> - err = svn_io_lock_open_file(mutex->lock_file, TRUE, FALSE, mutex->pool);
>
> + /* Inter-process lock. */
> + err = svn_io_file_lock2(mutex->lock_name, TRUE, FALSE, mutex->pool);
> + if (err && APR_STATUS_IS_ENOENT(err->apr_err))
> + {
> + /* No lock file? No big deal; these are just empty files anyway.
> + Create it and try again. */
> + svn_error_clear(err);
> + err = NULL;
> +
> + SVN_ERR(svn_io_file_create_empty(mutex->lock_name, mutex->pool));
>
>
>
> This looks like a heisenbug waiting to happen. Some other process may have
> created the file, but not locked it yet. This code should explicitly check
> for EEXIST and attempt to lock anyway.
>
> + SVN_ERR(svn_io_file_lock2(mutex->lock_name, TRUE, FALSE,
> mutex->pool));
>
>
> Also, the inter-process lock should be released on error, regardless. I
> suspect this is a potential deadlock.
>
> + }
> +
> + /* Don't leave us in a semi-locked state ... */
> return err
> ? svn_mutex__unlock(thread_mutex, err)
> : err;
>
>
> Obfuscated condition-expression here. This is much more readable:
>
> if (err)
> return svn_mutex__unlock(thread_mutex, err);
> return SVN_NO_ERROR;
Thanks for the review!
r1612405 should handle those issues nicely now.
-- Stefan^2.
Received on 2014-07-21 23:57:31 CEST