subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@elego.de>
Subject Re: svn commit: r1296604 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
Date Mon, 05 Mar 2012 12:12:39 GMT
Philip Martin wrote on Mon, Mar 05, 2012 at 11:51:05 +0000:
> Daniel Shahaf <danielsh@elego.de> writes:
> 
> > Philip Martin wrote on Mon, Mar 05, 2012 at 11:21:21 +0000:
> >> Daniel Shahaf <danielsh@elego.de> writes:
> >> 
> >> >> What about atomic revprop changes?  I don't see what ensures that the
> >> >> old value read by change_rev_prop_body is the most up-to-date value.
> >> >> 
> >> >
> >> > ffd->revprop_generation is used as part of the cache key, the file is
> >> > written before the write-lock is released and read again as soon as the
> >> > write-lock is taken.  Do you see a problem?
> >> 
> >> I'm worried about the case where the FS passed to
> >> svn_fs_fs__change_rev_prop has a cache that is already populated.  The
> >> values in the cache might be out-of-date because some other thread with
> >> another FS has written newer values.  It looks like change_rev_prop_body
> >> will examine the out-of-date cached value when doing the "atomic"
> >> update.
> >
> > The values will be in a cache under a key of the form (N,g), but the
> > cache lookup will use a key (N,g'), where $g' > g$, and will therefore
> > fail.  Here, $N$ is a revnum and $g$ is a revprop generation.
> 
> Suppose the cache gets populated in the atomic change process with
> revprop generation g.  Some other process writes to the repository
> creating revprop generation g' making the cache in the atomic process
> out-of-date.  Now the atomic process takes the write lock and does the
> "atomic" revprop change using the out-of-date cache.

You're right, I misread the code: I mistakenly thought line 2867 will
always re-read the revprop-gen file from disk.  How about:

Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c	(revision 1297002)
+++ subversion/libsvn_fs_fs/fs_fs.c	(working copy)
@@ -583,8 +583,9 @@ with_some_lock_file(svn_fs_t *fs,
       fs_fs_data_t *ffd = fs->fsap_data;
       if (ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
         SVN_ERR(update_min_unpacked_rev(fs, pool));
       SVN_ERR(get_youngest(&ffd->youngest_rev_cache, fs->path,
                            pool));
+      SVN_ERR(read_revprop_generation(fs, pool));
       err = body(baton, subpool);
     }
 

(And as an aside, perhaps the return statement in
check_revprop_generation() should have been written more clearly.)

> 
> Without the cache change_rev_prop_body would return
> SVN_ERR_FS_PROP_BASEVALUE_MISMATCH if the current value doesn't match
> the supplied value. The cache means the current value might be
> out-of-date.
> 
> -- 
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

Mime
View raw message