On 31.10.2010 17:08, Daniel Shahaf wrote:
> stefan2_at_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.
Received on 2010-11-22 02:40:11 CET