Julian Foad wrote on Tue, Dec 07, 2010 at 09:40:07 +0000:
> 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(); */;
...
err = foo();
if (err)
{
svn_error_clear(err);
continue;
}
...
}
and rely on the call to svn_fs_fs__path_rev_absolute() in the second
iteration to update ffd->min_unpacked_rev.
I do not know that any of the current callers behave this way ---
I think that other than open_pack_or_rev_file(), all callers always have
the write lock --- but it's conceivable. And if we ever add such
a caller, we will have to make them call update_min_unpacked_rev() by
themselves.
. o O [It should be fun to build with
printf("-DSVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR=%d -DPACK_AFTER_EVERY_COMMIT",
1).]
> 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: it could
become stale due to other processes who write to the repository.
(except for "pack runs under the write lock" considerations)
To borrow Philip's point: making ffd->min_unpacked_rev stale less often
might hide bugs.
> Does that make sense?
>
> - Julian
>
>
Daniel
(ffd->min_unpacked_rev could be renamed ffd->min_unpacked_rev_cache...)
