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.
- Julian On Thu, 2010-12-02, Daniel Shahaf wrote: > Julian Foad wrote on Thu, Dec 02, 2010 at 14:33:19 +0000: > > On Thu, 2010-12-02 at 14:56 +0200, Daniel Shahaf wrote: > > > Julian Foad wrote on Thu, Dec 02, 2010 at 12:15:19 +0000: > > > > Remove the re-try logic from svn_fs_fs__path_rev_absolute(). Since > > > > r1040832, all its callers correctly account for the possibility of an > > > > out-of-date result due to a concurrent packing operation. > > > > > > > > The re-try logic was introduced in r875097 and reduced but did not > > > > eliminate > > > > the window of opportunity for the caller to use an out-of-date result. > > > > > > > > See the email thread > > > > <http://svn.haxx.se/dev/archive-2010-12/0019.shtml>, > > > > subject "Re: svn commit: r1040832 - Port a fix for a FSFS packing race". > > > > > > > > * subversion/libsvn_fs_fs/fs_fs.c > > > > (svn_fs_fs__path_rev_absolute): Remove the re-try logic. > > > > > > > > * subversion/libsvn_fs_fs/fs_fs.h > > > > (svn_fs_fs__path_rev_absolute): Update the doc string accordingly. > > > > > > > > > > In fact, doesn't the correctness of this change depend on the fact that > > > svn_fs_fs__with_write_lock() calls update_min_unpacked_rev() before > > > executing the body() callback? > > > > Yes, it does. Do you think it shouldn't? > > > > I'm not sure whether it "should" or "shouldn't" depend. The change IS > correct, but it is only correct because the ffd->min_unpacked_rev is > known to be up-to-date whenever the write lock is held. > > That's a bit of a spaghetti effect there. Now, is there another such > effect that means the patch is wrong? (I haven't come up with one yet, > but that doesn't mean there isn't any.) > > > > (Otherwise we don't know whether there > > > is a "concurrent" packer, or just ffd->min_unpacked_rev is stale.) > > > > You know, I'm not too clear on the design intention here. I would have > > assumed it would be the job of all the fs_fs.c code to ensure that > > ffd->min_unpacked_rev is never stale (due to its own actions) at any > > point where it matters. But the way it does that seems to be to re-read > > the file in several places, instead of simply updating > > ffd->min_unpacked_rev when it writes the file. Maybe the idea was that > > that method would pick up both internal and concurrent (external) > > changes in the same way - I don't know. Seems odd. > > > > Yes, that was the idea. If two processes access the filesystem at the > same time, and one of them calls svn_fs_pack(), then the other's > ffd->min_unpacked_rev would be stale. > > It's the same story with ffd->youngest_rev, by the way; compare > ensure_revision_exists(), which I believe predates the packing logic. > > > Do you have an idea whether something should be done differently? I > > don't, not without studying it further. > > > > I'm not sure. Ultimately, ffd->min_unpacked_rev is just a cache, so we > have to update it when we're about to do something that depends on its > value. > > In the meantime, let me at least attempt to define the "something" which > perhaps should be done differently: > > Given the way packing works today, it's possible for a revision file to > stop existing where you expected it to exist. Today the code informs > itself of that situation by re-reading min-packed-rev and retrying. > (Further up the stack, the code calculating the offset to seek to also > needs to know whether it's seeking a pack file or a revision file.) > > > I note that the following comment in pack_shard() is not quite right: > > > > /* Update the min-unpacked-rev file to reflect our newly packed shard. > > * (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().) > > > > That may or may not be true, but it doesn't seem like this function has > > any right to make that assertion. > > > > We could remove the comment and have that function call > update_min_unpacked_rev(). > (This would also save us a failed disk access later.) > > 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. > > > And in pack_revprop_shard(), it's the same, verbatim: > > > > /* Update the min-unpacked-rev file to reflect our newly packed shard. > > > > should say 'the min-unpacked-revprop file'; > > > > * (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().) > > > > and that second line looks completely bogus in this context. > > > > Yes, whoever did the copy-paste forgot to update some places. > > > > > > > Index: subversion/libsvn_fs_fs/fs_fs.h > > > > =================================================================== > > > > --- subversion/libsvn_fs_fs/fs_fs.h (revision 1041339) > > > > +++ subversion/libsvn_fs_fs/fs_fs.h (working copy) > > > > @@ -411,8 +411,10 @@ svn_error_t *svn_fs_fs__move_into_place( > > > > Allocate *PATH in POOL. > > > > > > > > Note: If the caller does not have the write lock on FS, then the > > > > path is > > > > - not guaranteed to remain correct after the function returns, > > > > because the > > > > - revision might become packed just after this call. */ > > > > + not guaranteed to be correct or to remain correct after the function > > > > + returns, because the revision might become packed before or after > > > > this > > > > + call. If a file exists at that path, then it is correct; if not, > > > > then > > > > + the caller should call update_min_unpacked_rev() and re-try once. */ > > > > > > +1 semantically. However, update_min_unpacked_rev() is private to > > > fs_fs.c, so technically you aren't supposed to mention it in this > > > context. > > > > Good point. Really that means any external caller of this API has to > > hold the write lock. Calling update_min_unpacked_rev() and re-trying is > > only an option for internal callers (within fs_fs.c). I wonder if this > > should give us a clue about the questions above. > > > > First, I still think having the update_min_unpacked_rev() reference > there is useful. > > Would it be a layering violation for code in libsvn_fs_fs outside of > fs_fs.c to call update_min_unpacked_rev()? I'm not sure. > > > - Julian > > > > > > > > svn_error_t * > > > > svn_fs_fs__path_rev_absolute(const char **path, > > > > svn_fs_t *fs, > > > > > > >
