On Wed, 2010-12-01, stefan2_at_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?
(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, 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. */
svn_error_t *
svn_fs_fs__path_rev_absolute(const char **path,
svn_fs_t *fs,
svn_revnum_t rev,
apr_pool_t *pool);
]]]
- Julian
> Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1040832&r1=1040831&r2=1040832&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Dec 1 00:15:11 2010
> @@ -1903,21 +1903,42 @@ open_pack_or_rev_file(apr_file_t **file,
> {
> svn_error_t *err;
> const char *path;
> svn_boolean_t retry = FALSE;
>
> do
> {
> err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
>
> /* open the revision file in buffered r/o mode */
> if (! err)
> err = svn_io_file_open(file, path,
> APR_READ | APR_BUFFERED, APR_OS_DEFAULT, pool);
>
> if (err && APR_STATUS_IS_ENOENT(err->apr_err))
> {
> /* Could not open the file. This may happen if the
> * file once existed but got packed later. */
> svn_error_clear(err);
>
> /* if that was our 2nd attempt, leave it at that. */
> if (retry)
> return svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, NULL,
> _("No such revision %ld"), rev);
>
> /* we failed for the first time. Refresh cache & retry. */
> SVN_ERR(update_min_unpacked_rev(fs, pool));
>
> retry = TRUE;
> }
> else
> {
> /* the file exists but something prevented us from opnening it */
> return svn_error_return(err);
> }
> }
> while (err);
>
> return SVN_NO_ERROR;
> }
Received on 2010-12-01 13:33:27 CET