Philip Martin wrote on Tue, Dec 07, 2010 at 09:49:40 +0000: > Julian Foad <julian.f...@wandisco.com> writes: > > > 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. > > 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
