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. > > --This line, and those below, will be ignored-- > > > > Index: subversion/libsvn_fs_fs/fs_fs.c > > =================================================================== > > --- subversion/libsvn_fs_fs/fs_fs.c (revision 1041339) > > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy) > > @@ -246,8 +246,6 @@ path_rev(svn_fs_t *fs, svn_revnum_t rev, > > apr_psprintf(pool, "%ld", rev), NULL); > > } > > > > -/* Returns the path of REV in FS, whether in a pack file or not. > > - Allocate in POOL. */ > > svn_error_t * > > svn_fs_fs__path_rev_absolute(const char **path, > > svn_fs_t *fs, > > @@ -256,45 +254,16 @@ svn_fs_fs__path_rev_absolute(const char > > { > > fs_fs_data_t *ffd = fs->fsap_data; > > > > - if (ffd->format < SVN_FS_FS__MIN_PACKED_FORMAT) > > + if (ffd->format < SVN_FS_FS__MIN_PACKED_FORMAT > > + || ! is_packed_rev(fs, rev)) > > { > > *path = path_rev(fs, rev, pool); > > - return SVN_NO_ERROR; > > } > > - > > - if (! is_packed_rev(fs, rev)) > > + else > > { > > - svn_node_kind_t kind; > > - > > - /* Initialize the return variable. */ > > - *path = path_rev(fs, rev, pool); > > - > > - SVN_ERR(svn_io_check_path(*path, &kind, pool)); > > - if (kind == svn_node_file) > > - { > > - /* *path is already set correctly. */ > > - return SVN_NO_ERROR; > > - } > > - else > > - { > > - /* Someone must have run 'svnadmin pack' while this fs object > > - * was open. */ > > - > > - SVN_ERR(update_min_unpacked_rev(fs, pool)); > > - > > - /* The rev really should be present now. */ > > - if (! is_packed_rev(fs, rev)) > > - return svn_error_createf(APR_ENOENT, NULL, > > - _("Revision file '%s' does not exist, > > " > > - "and r%ld is not packed"), > > - svn_dirent_local_style(*path, pool), > > - rev); > > - /* Fall through. */ > > - } > > + *path = path_rev_packed(fs, rev, "pack", pool); > > } > > > > - *path = path_rev_packed(fs, rev, "pack", pool); > > - > > return SVN_NO_ERROR; > > } > > > > This part looks good. I'm more concerned about a bug in the "All > callers use the write lock, ..." analysis than about a bug in the > above hunk.
Right. > 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? > (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. Do you have an idea whether something should be done differently? I don't, not without studying it further. 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. 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. > > 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. - Julian > > svn_error_t * > > svn_fs_fs__path_rev_absolute(const char **path, > > svn_fs_t *fs, >
