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.
>
> That's not really a race, it's more of a logic error. 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.
>
As I said, I think all current callers are safe. However, I agree that
applying the patch will make it easier to detect the mistake if in the
future we add callers to svn_fs_fs__path_rev_absolute() that do not
retry as they should.
>
> > 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.
> >
> > Does that make sense?
> >
> > - Julian
> >
> >
> >
>
> --
> Philip
Received on 2010-12-07 11:16:06 CET