On Tue, 2010-12-07, Daniel Shahaf wrote:
> 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. :-)
Heh. I meant: "I am assuming that's how FSFS is *intended* to work; is
that your understanding too?"
- Julian
(I have not read the rest of this email yet.)
> 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
> >
> >
