Julian Foad wrote on Tue, Dec 07, 2010 at 18:13:09 +0000:
> 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?"
>
I'm not sure how it was "intended" to work. I know that at some point
after packing was added I pointed out that two concurrent 'svnadmin pack'
operations will cause data loss if not serialized, and in response
glasser said they should run under the write lock:
http://thread.gmane.org/gmane.comp.version-control.subversion.devel/108040/focus=108048
Daniel
(who, at the time, didn't fully appreciate the importance of using the
same write lock as other FSFS operations)
> - 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.foad_at_wandisco.com> writes:
> > > > >
> > > > > > On Mon, 2010-12-06 at 18:44 +0000, Philip Martin wrote:
> > > > > >> Julian Foad <julian.foad_at_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
> > >
> > >
>
>
Received on 2010-12-08 22:58:17 CET