On Mon, Jul 21, 2014 at 4:14 PM, Branko Čibej <br...@wandisco.com> wrote: > On 17.07.2014 16:50, stef...@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.
