subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <eq...@web.de>
Subject Re: svn commit: r1040832 - Port a fix for a FSFS packing race
Date Sun, 05 Dec 2010 22:45:50 GMT
On 01.12.2010 14:16, Daniel Shahaf wrote:
> Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +0000:
>> On Wed, 2010-12-01, stefan2@apache.org wrote:
>>> Port (not merge) a fix for a FSFS packing race condition from the
>>> performance branch to /trunk: There is a slight time window
>>> between finding the name of a rev file and actually open it. If
>>> the revision in question gets packed just within this window,
>>> we will find the new (and final) file name with just one retry.
>>>
>>> * subversion/libsvn_fs_fs/fs_fs.c
>>>    (open_pack_or_rev_file): retry once upon "missing file" error
>> Hi Stefan.
>>
>> (1) I think the doc string of svn_fs_fs__path_rev_absolute() should note
>> that the returned path may no longer be valid by the time you come to
>> use it, and the reason for that.  Does my patch below sound right?
>>
> Well, yes, and Stefan already added such a comment at the definition at
> my suggestion.  But +1 to move that to the declaration.
>

>> Patch for (1) above:
>> [[[
>> Index: subversion/libsvn_fs_fs/fs_fs.h
>> ===================================================================
>> --- subversion/libsvn_fs_fs/fs_fs.h	(revision 1040662)
>> +++ subversion/libsvn_fs_fs/fs_fs.h	(working copy)
>> @@ -404,13 +404,16 @@ svn_error_t *svn_fs_fs__txn_changes_fetc
>>      PERMS_REFERENCE.  Temporary allocations are from POOL. */
>>   svn_error_t *svn_fs_fs__move_into_place(const char *old_filename,
>>                                           const char *new_filename,
>>                                           const char *perms_reference,
>>                                           apr_pool_t *pool);
>>
>> -/* Sets *PATH to the path of REV in FS, whether in a pack file or not.
>> +/* Set *PATH to the path of REV in FS, whether in a pack file or not.
>> +   The path is not guaranteed to remain correct after the function returns,
>> +   because it is possible that the revision will become packed just after
>> +   this call, so the caller should re-try once if the path is not found.
>>      Allocate in POOL. */
> Sounds right.
>
> Bordering on bikeshed, I would suggest some changes:
>
> * Separate describing what the function does ("Sets *PATH to FOO and
>    allocates in POOL") from describing the conditions the caller should
>    beware of / check for ("sometimes the return value will be out-of-date
>    by the time you receive it").
>
> * Mention that the non-guarantee is not in effect if the caller has the
>    write lock.
>
I took the comment from the performance branch and added the
info about guarantees in presence of a write lock in r1042479.

-- Stefan^2.

Mime
View raw message