You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2014/03/11 13:23:23 UTC
Re: svn commit: r1575418 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs.h fs_fs.c fs_fs.h pack.c structure
On 8 March 2014 01:35, <st...@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.
> + const char *lock_path = svn_dirent_join(fs->path, PATH_PACK_LOCK_FILE,
> + subpool);
> + err = svn_fs_fs__get_lock_on_filesystem(lock_path, subpool);
> + if (!err)
> + err = pack_body(&pb, subpool);
> +
> + svn_pool_destroy(subpool);
> + }
> + else
> + {
> + /* Use the global write lock for older repos. */
> + err = svn_fs_fs__with_write_lock(fs, pack_body, &pb, pool);
> + }
> +
> + return svn_error_trace(err);
> }
>
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1575418 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs.h fs_fs.c fs_fs.h pack.c structure
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Mar 12, 2014 at 9:49 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 11 March 2014 21:56, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Tue, Mar 11, 2014 at 1:23 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 8 March 2014 01:35, <st...@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.
Re: svn commit: r1575418 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs.h fs_fs.c fs_fs.h pack.c structure
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 11 March 2014 21:56, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Tue, Mar 11, 2014 at 1:23 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 8 March 2014 01:35, <st...@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
Re: svn commit: r1575418 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs.h fs_fs.c fs_fs.h pack.c structure
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Mar 11, 2014 at 1:23 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 8 March 2014 01:35, <st...@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.
> > + const char *lock_path = svn_dirent_join(fs->path,
> PATH_PACK_LOCK_FILE,
> > + subpool);
> > + err = svn_fs_fs__get_lock_on_filesystem(lock_path, subpool);
> > + if (!err)
> > + err = pack_body(&pb, subpool);
> > +
> > + svn_pool_destroy(subpool);
> > + }
> > + else
> > + {
> > + /* Use the global write lock for older repos. */
> > + err = svn_fs_fs__with_write_lock(fs, pack_body, &pb, pool);
> > + }
> > +
> > + return svn_error_trace(err);
> > }
>
Thanks for the feedback!
-- Stefan^2.