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.
> 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 10:50:20 CET