Re: svn commit: r1611379 - /subversion/trunk/subversion/libsvn_subr/named_atomic.c

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;

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane_at_wandisco.com