On Wed, Mar 12, 2014 at 9:49 AM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 11 March 2014 21:56, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
> wrote:
> > On Tue, Mar 11, 2014 at 1:23 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> >>
> >> On 8 March 2014 01:35, <stefan2_at_apache.org> wrote:
> >> > Author: stefan2
> >> > Date: Fri Mar 7 21:35:54 2014
> >> > New Revision: 1575418
> >> >
> >> > URL: http://svn.apache.org/r1575418
> >> > Log:
> >> > Enable FSFS format 7 repositories to be packed without completely
> >> > blocking commits. We simply introduce a separate lock file for
> >> > 'svnadmin pack' and take out the global write lock for packing
> >> > revprops and switching the shard to "packed" state.
> >> >
> >> > Most of the run time is spent building the revision pack file
> >> > and does not require any synchronization with writers.
> >> >
> >> [...]
> >>
> >> > @@ -1987,10 +2005,33 @@ svn_fs_fs__pack(svn_fs_t *fs,
> >> > apr_pool_t *pool)
> >> > {
> >> > struct pack_baton pb = { 0 };
> >> > + fs_fs_data_t *ffd = fs->fsap_data;
> >> > + svn_error_t *err;
> >> > +
> >> > pb.fs = fs;
> >> > pb.notify_func = notify_func;
> >> > pb.notify_baton = notify_baton;
> >> > pb.cancel_func = cancel_func;
> >> > pb.cancel_baton = cancel_baton;
> >> > - return svn_fs_fs__with_write_lock(fs, pack_body, &pb, pool);
> >> > +
> >> > + if (ffd->format >= SVN_FS_FS__MIN_PACK_LOCK_FORMAT)
> >> > + {
> >> > + /* Newer repositories provide a pack operation specific lock.
> >> > + Acquire it to prevent concurrent packs. */
> >> > + apr_pool_t *subpool = svn_pool_create(pool);
> >> What is the reason for using subpool here? Could you please document
> >> it if any, otherwise it looks very confusing.
> >
> >
> > Done in r1576427.
> >
> Thanks! It's clearer now. But I have suggestions for this code:
> 1. It seems mutex is missed for POSIX platform where file lock is
> per-process, not per-thread. Like we
> fs_fs_shared_data_t->fs_write_lock
>
You are right. Although it is unlikely for an application
to do attempt multi-threaded packs ... ;)
> 2. I think the code will be clearer with explicit
> svn_fs_fs__with_pack_lock() function. In this case mutexes and subpool
> will be abstracted from pack logic, which is good imho.
>
Both fixed in r1578176.
-- Stefan^2.
Received on 2014-03-17 00:20:00 CET