Julian Foad wrote on Tue, Dec 07, 2010 at 15:27:05 +0000:
> (Quoting and replying to two emails at once.)
>
> First: I'm assuming that no process will ever do packing without getting
> the exclusive write lock. Can you confirm that for me? If that's
> false, all my reasoning would be bogus.
>
Why should I confirm that? You have the code too. :-)
Seriously: as of the last time I looked, svn_fs_pack() calls ... which
takes the write lock and calls pack_body(), which calls pack_shard(),
which does
cat db/revs/$shard/* > db/revs/$shard.pack/pack
echo '($shard + 1) * 1000' > db/min-unpacked-revprop
rm -rf db/revs/$shard/
while still holding the write lock.
>
> On Tue, 2010-12-07 at 12:13 +0200, Daniel Shahaf wrote:
> > Philip Martin wrote on Tue, Dec 07, 2010 at 09:49:40 +0000:
> > > Julian Foad <julian.f...@wandisco.com> writes:
> > >
> > > > On Mon, 2010-12-06 at 18:44 +0000, Philip Martin wrote:
> > > >> Julian Foad <julian.f...@wandisco.com> writes:
> > > >>
> > > >> > I'm going to drop this "Remove the re-try logic from
> > > >> > svn_fs_fs__path_rev_absolute()" follow-up patch, as I don't want to
> > > >> > get
> > > >> > into checking or messing with the txn-correctness of FSFS now. If
> > > >> > Daniel or anyone else wants to pursue it, I'd be glad to help.
> > > >>
> > > >> I thought the patch was an improvement. The existing retry in
> > > >> svn_fs_fs__path_rev_absolute doesn't fix the race, it simply makes the
> > > >> window smaller. As the patch makes the window larger we are more
> > > >> likely
> > > >> to see and fix any places where the caller doesn't handle the race.
> > > >
> > > > I *think* the problem is that a caller may use this function within a
> > > > transaction and depend on this internal re-try logic simply to update a
> > > > stale cached min-unpacked-foo value. "Stale" in the sense that *this*
> > > > transaction has changed the real value and hasn't updated the cached
> > > > value.
> >
> > Yes, it's conceivable that a caller might do:
> >
> > for (int i = 0; i < 2; i++)
> > {
> > ...
> > SVN_ERR(svn_fs_fs__path_rev_absolute());
> > ...
> > if (i == 0)
> > /* other process runs svn_fs_pack(); */;
> > [...]
>
> No, that's not what I mean. I mean a caller might do:
>
> with a write lock:
> {
> svn_fs_pack();
> SVN_ERR(svn_fs_fs__path_rev_absolute(&path, ...));
> foo(path);
> }
>
> where neither this code nor foo() re-tries. With the current version of
> svn_fs_fs__path_rev_absolute(), foo() will get the correct path,
Unless a commit occurs after svn_fs_fs__path_rev_absolute() returns but
before foo() is called. (This is the condition Stefan2 fixed.)
> whereas if we remove the internal re-try then foo() will get the wrong
> path.
>
> > If there are any
> > > such cases then your patch should expose them and we will fix them. Any
> > > such errors are easy to handle, compared to the difficulty of fixing
> > > races.
> > >
> > > The real problem with the existing code is that it partially hides
> > > races, by making them harder to trigger, but doesn't fix the race. If
> > > we want to fix the race properly making it easier to trigger is the
> > > right thing to do.
>
> Agreed.
>
> What I meant to indicate is that, before making this change, I would
> like to see clearly that a caller with the write lock cannot use an
> out-of-date value. One way to ensure that would be:
>
> update the cached value whenever we get the write lock (which we do);
> update the cached value whenever we update the value on disk (which we
> don't);
> anything not using a write lock must accept that the cached value may
> be stale, and re-try if necessary.
>
> Another way would be:
>
> don't update the cached value when we get the write lock;
> don't update the cached value when we update the value on disk;
> every time we use the cached value within a write lock, either update
> before using it or try and re-try if necessary;
> every time we use the cached value without a write lock, try and
> re-try if necessary.
>
> I don't have a strong preference between those, but the current
> inconsistency between updating it when we get the write lock and not
> updating it when we update the value on disk seems bad.
>
What is the cost of this inconsistency?
The stale cache will result in a failed stat() call on the path where
a revision file existed before it was packed. Then, either we will
update the cache and retry (meaning the only cost was a stat() call), or
due to a bug we will forget to retry and, instead, send a bogus
error up the stack.
So, we pay one stat(), but in exchange we hope that, by letting the
cache *actually* become stale, concurrency bugs that reproduce only
when the cache is stale will be unearthed earlier.
In fact, how about this patch?
[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1042961)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -7553,7 +7553,7 @@ write_revnum_file(const char *fs_path,
remove the pack file and start again. */
static svn_error_t *
pack_shard(const char *revs_dir,
- const char *fs_path,
+ svn_fs_t *fs,
apr_int64_t shard,
int max_files_per_dir,
svn_fs_pack_notify_t notify_func,
@@ -7562,6 +7562,7 @@ pack_shard(const char *revs_dir,
void *cancel_baton,
apr_pool_t *pool)
{
+ const char *fs_path = fs->path;
const char *pack_file_path, *manifest_file_path, *shard_path;
const char *pack_file_dir;
svn_stream_t *pack_stream, *manifest_stream;
@@ -7638,6 +7639,9 @@ pack_shard(const char *revs_dir,
SVN_ERR(write_revnum_file(fs_path, PATH_MIN_UNPACKED_REV,
(svn_revnum_t)((shard + 1) * max_files_per_dir),
iterpool));
+#ifndef SVN_DEBUG
+ SVN_ERR(update_min_unpacked_rev(fs, iterpool));
+#endif
svn_pool_destroy(iterpool);
/* Finally, remove the existing shard directory. */
@@ -7701,6 +7705,9 @@ pack_revprop_shard(svn_fs_t *fs,
SVN_ERR(write_revnum_file(fs_path, PATH_MIN_UNPACKED_REVPROP,
(svn_revnum_t)((shard + 1) * max_files_per_dir),
iterpool));
+#ifndef SVN_DEBUG
+ SVN_ERR(update_min_unpacked_revprop(fs, iterpool));
+#endif
svn_pool_destroy(iterpool);
/* Finally, remove the existing shard directory. */
@@ -7786,7 +7793,7 @@ pack_body(void *baton,
if (pb->cancel_func)
SVN_ERR(pb->cancel_func(pb->cancel_baton));
- SVN_ERR(pack_shard(data_path, pb->fs->path, i, max_files_per_dir,
+ SVN_ERR(pack_shard(data_path, pb->fs, i, max_files_per_dir,
pb->notify_func, pb->notify_baton,
pb->cancel_func, pb->cancel_baton, iterpool));
}
]]]
(still need to add comments explaining why the #ifndef guard)
> This doesn't mean we should keep the re-try loop that we're talking
> about removing. I agree it should go. It's just that I'd like to
> resolve this inconsistency, or at least agree about resolving it, before
> feeling comfortable enough to commit this myself.
>
>
> > > > Daniel, you wrote:
> > > >> But is it strictly *necessary* to do so? I think not: by not calling
> > > >> update_min_unpacked_rev(), we cause ffd->min_unpacked_rev to become
> > > >> stale --- a situation which the code has to account for anyway.
> > > >
> > > > That was true, then, and it meant that every place that used this value
> > > > had to account for it possibly being stale. But now we have realized
> > > > that the code shouldn't need to account for the possibility of the value
> > > > being stale if it is running within a transaction (with the write lock).
> > > >
> > > > Now, I think it would be better to change this. Whenever the code
> > > > updates the min-foo on disk, it should also update the cached value.
> > > > That style of coding would be easier to reason about (to use an in-vogue
> > > > phrase), and then we would be able to remove the re-try in
> > > > svn_fs_fs__path_rev_absolute() without a concern.
>
> > Making ffd->min_unpacked_rev stale less often does not mean that we will
> > have less places to account for it *possibly* being stale:
>
> I think it *does*, because every piece of code that knows it has a write
> lock would not have to bother about that possibility.
>
Code that has the write lock does not have to bother about that
possibility anyway, because with_some_lock() updates the ffd->min_*
caches after acquiring the lock and before invoking the under-the-lock
callback.
> > it could
> > become stale due to other processes who write to the repository.
> > (except for "pack runs under the write lock" considerations)
>
> It could become stale due to another process writing to the repository,
> except if *this* code is running under a write lock. If *this* code has
> a write lock, it doesn't matter whether it's doing packing or not;
> another process can't make this process's cache get stale because
> another process can't get the write lock.
>
Right: if we have the write lock, then the caches are not stale.
> > To borrow Philip's point: making ffd->min_unpacked_rev stale less often
> > might hide bugs.
>
> That is one way of looking at it, when thinking about the asynchronous
> (no write lock) cases, but another way of looking at the same thing is
> it can help to *totally avoid* potential bugs in synchronous cases.
>
Agreed, that's the flip side of the coin. The #ifndef patch above tries
to find a middle ground between hiding and unearthing bugs here.
> - Julian
>
>
