Philip Martin wrote on Mon, Mar 05, 2012 at 11:51:05 +0000:
> Daniel Shahaf <danie...@elego.de> writes:
>
> > Philip Martin wrote on Mon, Mar 05, 2012 at 11:21:21 +0000:
> >> Daniel Shahaf <danie...@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
