You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/10/31 16:22:31 UTC
svn commit: r1029381 -
/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c
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
{
svn_error_t *err;
const char *path;
+ svn_boolean_t retry = FALSE;
+ fs_fs_data_t *ffd = fs->fsap_data;
- /* make sure file has a defined state */
- *file = NULL;
- err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
-
- if (! err)
+ do
{
- /* open the revision file in buffered r/o mode */
- fs_fs_data_t *ffd = fs->fsap_data;
- err = svn_file_handle_cache__open(file,
- ffd->file_handle_cache,
- path,
- APR_READ | APR_BUFFERED,
- APR_OS_DEFAULT,
- offset,
- cookie,
- pool);
+ /* make sure file has a defined state */
+ *file = NULL;
+ err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
- /* if that succeeded, there must be an underlying APR file */
- assert(err || svn_file_handle_cache__get_apr_handle(*file));
- }
+ 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 (err && APR_STATUS_IS_ENOENT(err->apr_err))
- {
- svn_error_clear(err);
- return svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, NULL,
- _("No such revision %ld"), rev);
+ /* 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;
+ }
}
+ while (err && retry);
return svn_error_return(err);
}
@@ -2953,9 +2973,9 @@ svn_fs_fs__rev_get_root(svn_fs_id_t **ro
SVN_ERR(svn_cache__get((void **) root_id_p, &is_cached,
ffd->rev_root_id_cache, &rev, pool));
- if (is_cached &&
- is_packed_rev(fs, rev) == svn_fs_fs__is_packed(*root_id_p))
- return SVN_NO_ERROR;
+// if (is_cached &&
+// is_packed_rev(fs, rev) == svn_fs_fs__is_packed(*root_id_p))
+// return SVN_NO_ERROR;
/* we don't care about the file pointer position */
SVN_ERR(open_pack_or_rev_file(&revision_file, fs, rev, -1,
Re: svn commit: r1029381 - /subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c
Posted by 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.
Re: svn commit: r1029381 -
/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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).
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);
> }