On 11 March 2014 21:56, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote: > On Tue, Mar 11, 2014 at 1:23 PM, Ivan Zhakov <i...@visualsvn.com> wrote: >> >> On 8 March 2014 01:35, <stef...@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 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.
-- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
