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
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
Received on 2014-03-12 09:50:53 CET