[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [patch] fsfs revprop packing paranoia

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 3 Aug 2010 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: fs_fs.c
> ===================================================================
> --- fs_fs.c (revision 981659)
> +++ fs_fs.c (working copy)
> @@ -6094,6 +6094,7 @@ commit_body(void *baton, apr_pool_t *pool)
> SVN_ERR(svn_fs_fs__change_txn_prop(cb->txn, SVN_PROP_REVISION_DATE,
> &date, pool));
>
> + SVN_ERR(update_min_unpacked_revprop(fs, pool));
> if (ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
> new_rev >= ffd->min_unpacked_revprop)
> {
> ]]]
>

Okay, sorry. I should have added that call to set_revision_proplist() (which
is run under the write lock *during a revprop edit*), not where I did.

That said, my question still stands whether the following is necessary:

[[[
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,
]]]
Received on 2010-08-03 18:01:49 CEST

This is an archived mail posted to the Subversion Dev mailing list.