stef...@apache.org wrote on Sun, Oct 31, 2010 at 15:22:31 -0000:
> Author: stefan2
> Date: Sun Oct 31 15:22:31 2010
> New Revision: 1029381
>
> URL: http://svn.apache.org/viewvc?rev=1029381&view=rev
> Log:
> Fix another packing issue. Due to concurrent access alone
> it may happen that between determining the rev file name
> and actually opening it, that very revision may get packed
> (actually, get deleted after packing).
>
> The file handle cache adds another issue: an open file handle
> may not be suitable due to different open flags and the file
> must be re-opened. While the existing handle would have
> kept the file content alive, opening a new handle to the same
> file will fail if the file was deleted before.
>
> Since there can be only one transition from non-packed to
> packed, we need to retry only once if we could not find the
> revision file in question.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (open_pack_or_rev_file): retry once because the file might have
> gotten packed.
>
> Modified:
> subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c
>
> Modified: subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c
> URL:
> http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c?rev=1029381&r1=1029380&r2=1029381&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c Sun Oct
> 31 15:22:31 2010
> @@ -1939,34 +1939,54 @@ open_pack_or_rev_file(svn_file_handle_ca
(the unidiff is unreadable, so I'll just paste the whole function)
> static svn_error_t *
> open_pack_or_rev_file(svn_file_handle_cache__handle_t **file,
> svn_fs_t *fs,
> svn_revnum_t rev,
> apr_off_t offset,
> int cookie,
> apr_pool_t *pool)
> {
> svn_error_t *err;
> const char *path;
> svn_boolean_t retry = FALSE;
> fs_fs_data_t *ffd = fs->fsap_data;
>
> do
> {
> /* make sure file has a defined state */
> *file = NULL;
> err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
>
So, now this call is inside the retry loop. But it does its own retry
loop already. Looking at svn_fs_fs__path_rev_absolute()'s other
callers, they all run under the fsfs write lock (which pack operations
also acquire) except for the one in lock.c. So, I suppose we can remove
the retry logic from svn_fs_fs__path_rev_absolute() and add it in lock.c
(where it's currently absent).
Also, svn_fs_fs__path_rev_absolute()'s docstring could warn that its
result is "unstable" (points to a path that may vanish under certain
conditions).
> if (! err)
> {
> /* open the revision file in buffered r/o mode */
> err = svn_file_handle_cache__open(file,
> ffd->file_handle_cache,
> path,
> APR_READ | APR_BUFFERED,
> APR_OS_DEFAULT,
> offset,
> cookie,
> pool);
>
> /* if that succeeded, there must be an underlying APR file */
> assert(err || svn_file_handle_cache__get_apr_handle(*file));
> }
>
> 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. Note that
> * the file handle cache may have had an open handle
> * to the old file but had to close it in the function
> * call above due to different open flags.
> */
> 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 caches & retry. */
> SVN_ERR(svn_file_handle_cache__flush(ffd->file_handle_cache));
> SVN_ERR(update_min_unpacked_rev(fs, pool));
>
> retry = TRUE;
> }
Need "else\n return svn_error_return(err);" here, to prevent leaking the
error when retry=TRUE.
> }
> while (err && retry);
>
> return svn_error_return(err);
> }
