On 31.10.2010 17:08, Daniel Shahaf wrote:
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).
The retry logic in svn_fs_fs__path_rev_absolute() seems
to be gone by now. With r1037470 and assuming proper
locking of FSFS repositories during requests, the retry
loop in open_pack_or_rev_file() will only be needed if the
repo gets packed within the same repository session.
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).
Done in r1037586.
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.
Done in r1037552. The real issue is not the error
leak but the potential infinite loop if there is some
unexpected error after the first iteration.
}
while (err&& retry);
return svn_error_return(err);
}
-- Stefan^2.
