I figure this patch can't do any harm (except cost another file-read when a
write-lock or txn-current-lock is being acquired), and it might as well
help, so I committed it in r984990.
Daniel Shahaf wrote on Tue, Aug 03, 2010 at 18:59:33 +0300:
> Daniel Shahaf wrote on Mon, Aug 02, 2010 at 23:44:10 +0300:
> > Is this necessary to avoid some race conditions around a revprop becoming
> > packed just before commit_body()'s write-lock had been acquired?
>
> [[[
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 981921)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -2781,6 +2789,7 @@ set_revision_proplist(svn_fs_t *fs,
>
> SVN_ERR(ensure_revision_exists(fs, rev, pool));
>
> + SVN_ERR(update_min_unpacked_revprop(fs, pool));
> if (ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
> rev >= ffd->min_unpacked_revprop)
> {
> ]]]
>
>
> And, to be honest, I think I'd rather fix this once and for all by updating
> the ffd->min_unpacked_* caches whenever a write-lock is obtained anywhere:
>
> [[[
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 981921)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -150,6 +150,9 @@ read_min_unpacked_rev(svn_revnum_t *min_unpacked_r
> static svn_error_t *
> update_min_unpacked_rev(svn_fs_t *fs, apr_pool_t *pool);
>
> +static svn_error_t *
> +update_min_unpacked_revprop(svn_fs_t *fs, apr_pool_t *pool);
> +
> /* Pathname helper functions */
>
> /* Return TRUE is REV is packed in FS, FALSE otherwise. */
> @@ -567,7 +570,8 @@ get_lock_on_filesystem(const char *lock_filename,
> BATON and that subpool, destroy the subpool (releasing the write
> lock) and return what BODY returned. */
> static svn_error_t *
> -with_some_lock(svn_error_t *(*body)(void *baton,
> +with_some_lock(svn_fs_t *fs,
> + svn_error_t *(*body)(void *baton,
> apr_pool_t *pool),
> void *baton,
> const char *lock_filename,
> @@ -594,7 +598,11 @@ static svn_error_t *
> err = get_lock_on_filesystem(lock_filename, subpool);
>
> if (!err)
> - err = body(baton, subpool);
> + {
> + SVN_ERR(update_min_unpacked_rev(fs, pool));
> + SVN_ERR(update_min_unpacked_revprop(fs, pool));
> + err = body(baton, subpool);
> + }
>
> svn_pool_destroy(subpool);
>
> @@ -622,7 +630,7 @@ svn_fs_fs__with_write_lock(svn_fs_t *fs,
> apr_thread_mutex_t *mutex = ffsd->fs_write_lock;
> #endif
>
> - return with_some_lock(body, baton,
> + return with_some_lock(fs, body, baton,
> path_lock(fs, pool),
> #if SVN_FS_FS__USE_LOCK_MUTEX
> mutex,
> @@ -645,7 +653,7 @@ with_txn_current_lock(svn_fs_t *fs,
> apr_thread_mutex_t *mutex = ffsd->txn_current_lock;
> #endif
>
> - return with_some_lock(body, baton,
> + return with_some_lock(fs, body, baton,
> path_txn_current_lock(fs, pool),
> #if SVN_FS_FS__USE_LOCK_MUTEX
> mutex,
> ]]]
