Re: svn commit: r1029381 - /subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c

[Stefan Fuhrmann <st...@alice-dsl.de>]

On 31.10.2010 17:08, Daniel Shahaf wrote:
> stefan2@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.