On Wed, 2010-12-01, Daniel Shahaf wrote:
> Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +0000:
> > On Wed, 2010-12-01, stef...@apache.org wrote:
> > > Port (not merge) a fix for a FSFS packing race condition from the
> > > performance branch to /trunk: There is a slight time window
> > > between finding the name of a rev file and actually open it. If
> > > the revision in question gets packed just within this window,
> > > we will find the new (and final) file name with just one retry.
> > >
> > > * subversion/libsvn_fs_fs/fs_fs.c
> > > (open_pack_or_rev_file): retry once upon "missing file" error
> >
> > Hi Stefan.
> >
> > (1) I think the doc string of svn_fs_fs__path_rev_absolute() should note
> > that the returned path may no longer be valid by the time you come to
> > use it, and the reason for that. Does my patch below sound right?
> >
>
> Well, yes, and Stefan already added such a comment at the definition at
> my suggestion. But +1 to move that to the declaration.
>
> > (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.
> [1] with the exception of set_revision_proplist(), which can be called
> either under the write lock or under write_revision_zero() --- but in
> the latter case it's called before the format file has been created.
>
> > all of them using the result as a permissions reference to set the
> > file permissions on some new file. It seems to me that those uses can
> > fail in the same way.
> >
> > The root problem is the existence of a "get the path of this file" API
> > in the presence of semantics whereby the file might be removed from that
> > path at any time.
> >
> > Perhaps your fix is the best fix for this caller, but for the other
> > callers I think creating and using a "copy file permissions from
> > rev-file of rev R" API would be a better solution than adding re-tries
> > there. That API can do the re-try internally.
> >
> > What do you think?
> >
> >
> > Patch for (1) above:
> > [[[
> > Index: subversion/libsvn_fs_fs/fs_fs.h
> > ===================================================================
> > --- subversion/libsvn_fs_fs/fs_fs.h (revision 1040662)
> > +++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
> > @@ -404,13 +404,16 @@ svn_error_t *svn_fs_fs__txn_changes_fetc
> > PERMS_REFERENCE. Temporary allocations are from POOL. */
> > svn_error_t *svn_fs_fs__move_into_place(const char *old_filename,
> > const char *new_filename,
> > const char *perms_reference,
> > apr_pool_t *pool);
> >
> > -/* 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.
> > 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;
}
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. */
svn_error_t *
svn_fs_fs__path_rev_absolute(const char **path,
svn_fs_t *fs,
