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.
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
Received on 2010-12-07 10:40:50 CET