Julian Foad wrote on Thu, Dec 02, 2010 at 12:15:19 +0000:
> On Wed, 2010-12-01, Daniel Shahaf wrote:
> > Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +0000:
> > > (2) Doesn't the exact same race exist in *all* uses of
> > > svn_fs_fs__path_rev_absolute()? There are five other calls to it,
> >
> > As far as I can tell, apart from open_pack_or_rev_file(), all callers
> > always[1] run under the write lock. Since pack operation take the write
> > lock too, it's not possible for the result of svn_fs_fs__path_rev_absolute()
> > to become outdated for those callers.
>
> OK, that's good. No change needed then.
>
>
> > > -/* Sets *PATH to the path of REV in FS, whether in a pack file or not.
> > > +/* Set *PATH to the path of REV in FS, whether in a pack file or not.
> > > + The path is not guaranteed to remain correct after the function
> > > returns,
> > > + because it is possible that the revision will become packed just after
> > > + this call, so the caller should re-try once if the path is not found.
> > > Allocate in POOL. */
> >
> > Sounds right.
> >
> > Bordering on bikeshed, I would suggest some changes:
> >
> > * Separate describing what the function does ("Sets *PATH to FOO and
> > allocates in POOL") from describing the conditions the caller should
> > beware of / check for ("sometimes the return value will be out-of-date
> > by the time you receive it").
> >
> > * Mention that the non-guarantee is not in effect if the caller has the
> > write lock.
>
> OK, committed in r1041056 with these improvements.
>
+1, thanks.
> > > svn_error_t *
> > > svn_fs_fs__path_rev_absolute(const char **path,
> > > svn_fs_t *fs,
> > > svn_revnum_t rev,
> > > apr_pool_t *pool);
> >
> > Also, svn_fs_fs__path_rev_absolute() also does, internally, a single
> > repeat loop. Theoretically this isn't needed any more now (since all
> > callers either run under the write lock or retry)...
>
> Can you check the attached patch for this, please?
>
> - Julian
>
>
> 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.
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? (Otherwise we don't know whether there
is a "concurrent" packer, or just ffd->min_unpacked_rev is stale.)
> 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.
> svn_error_t *
> svn_fs_fs__path_rev_absolute(const char **path,
> svn_fs_t *fs,
