Re: Fwd: [Daniel Shahaf: [PATCH] Re: [PATCH] A test for "Can't get entries" error]

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Fwd: [Daniel Shahaf: [PATCH] Re: [PATCH] A test for "Can't get entries" error]

Stefan Fuhrmann
On 21.11.18 16:00, Daniel Shahaf wrote:

> Good morning Stefan,
>
> Forwarding from dev@.
>
> tl;dr: False positive SVN_ERR_FS_NOT_DIRECTORY error, with test¹,
> workaround, analysis, and patch.
>
> The error doesn't happen with caches disabled so I thought you might be
> interested.
>
> Cheers,
>
> Daniel
>
> ¹ The test was posted upthread by Julian (based on Dmitry's work).
>
> ----- Forwarded message from Daniel Shahaf <[hidden email]> -----
>
>> Date: Wed, 21 Nov 2018 14:55:23 +0000
>> From: Daniel Shahaf <[hidden email]>
>> To: [hidden email]
>> Subject: [PATCH] Re: [PATCH] A test for "Can't get entries" error
>> Message-ID: <[hidden email].local2>
>>
>> Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +0000:
>>> Julian Foad wrote on Tue, 20 Nov 2018 08:38 +0000:
>>>> Daniel Shahaf wrote:
>>>>> Could you please clarify whether the bug reproduces under other backends
>>>>> (FSX and BDB)?
>>>> I found that the test passes under FSX and BDB; it only fails under FSFS.
>>> The test XPASSes with SVN_X_DOES_NOT_MARK_THE_SPOT=1 (see notes/knobs),
>>> so it's something to do with the caches.
>> So, looking at subversion/libsvn_fs_fs/tree.c:
>>
>>    1076          /* If we found a directory entry, follow it.  First, we
>>    1077             check our node cache, and, failing that, we hit the DAG
>>    1078             layer.  Don't bother to contact the cache for the last
>>    1079             element if we already know the lookup to fail for the
>>    1080             complete path. */
>>    1081          if (next || !(flags & open_path_uncached))
>>    1082            SVN_ERR(dag_node_cache_get(&cached_node, root, path_so_far->data,
>>    1083                                       pool));
>>    1084          if (cached_node)
>>    1085            child = cached_node;
>>    1086          else
>>    1087            SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>>    1088
>>    1089          /* "file not found" requires special handling.  */
>>    1090          if (child == NULL)
>>    1091            {
>>    ⋮
>>    1103              else if (flags & open_path_allow_null)
>>    1104                {
>>    1105                  parent_path = NULL;
>>    1106                  break;
>>    1107                }
>>    ⋮
>>    1114            }
>>
>> This is what happens:
>>
>> [[[
>> (lldb) breakpoint set -f tree.c -l 1087 -c 'root->rev == 2 && !(int)strcmp(path, "/B/mu/iota")'
>> (lldb) r
>> Process 17504 stopped
>> * thread #1, name = 'ra-test', stop reason = breakpoint 1.1
>>      frame #0: 0x00007ffff5b8be7a libsvn_fs_fs-1.so.0`open_path(parent_path_p=0x00007fffffffcb18, root=0x00007ffff7f6c9d8, path="/B/mu/iota", flags=12, is_txn_path=0, pool=0x00007ffff7f6c028) at tree.c:1087
>>     1084           if (cached_node)
>>     1085             child = cached_node;
>>     1086           else
>> -> 1087             SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>>     1088
>>     1089           /* "file not found" requires special handling.  */
>>     1090           if (child == NULL)
>> (lldb) p root->is_txn_root
>> (svn_boolean_t) $2 = 0
>> (lldb) p root->rev
>> (svn_revnum_t) $3 = 2
>> (lldb) p stringify_node(here, pool)
>> (const char *) $0 = 0x00007ffff7f6cc08 "2.0.r1/0"
>> (lldb) pla shell svnlook tree --show-ids --full-paths cant_get_entries_of_non_directory -r2 | fgrep 2.0.r1/0
>> A/mu <2.0.r1/0>
>> B/mu <2.0.r1/0>
>> (lldb) p entry
>> (char *) $1 = 0x00007ffff7f6cbd8 "iota"
>> (lldb) n
>> Process 2969 stopped
>> * thread #1, name = 'ra-test', stop reason = step over
>>      frame #0: 0x00007ffff5b8c2b2 libsvn_fs_fs-1.so.0`open_path(parent_path_p=0x00007fffffffcb18, root=0x00007ffff7f6c9d8, path="/B/mu/iota", flags=12, is_txn_path=0, pool=0x00007ffff7f6c028) at tree.c:1176
>>     1173   svn_pool_destroy(iterpool);
>>     1174   *parent_path_p = parent_path;
>>     1175   return SVN_NO_ERROR;
>> -> 1176 }
>>     1177
>>     1178
>>     1179 /* Make the node referred to by PARENT_PATH mutable, if it isn't
>> (lldb)
>> ]]]
>>
>> In words: svn_fs_fs__dag_open() is asked to find the child "iota" of
>> r2:/B/mu, and instead of setting 'child' to NULL as the code expects, it
>> returns an error which percolates all the way to the client.
>>
>> The reason SVN_X_DOES_NOT_MARK_THE_SPOT fixes it is that it bypasses the
>> optimization earlier in the function.  That optimization causes the the
>> very first iteration of the loop is to process "/B/mu".  With caches
>> disabled, the first iteration of the loop processes "/" and the second
>> iteration processes "/B" and exits early, here:
>>
>>    1144      /* The path isn't finished yet; we'd better be in a directory.  */
>>    1145      if (svn_fs_fs__dag_node_kind(child) != svn_node_dir)
>>    1146        {
>>    1147          const char *msg;
>>    1148
>>    1149          /* Since this is not a directory and we are looking for some
>>    1150             sub-path, that sub-path will not exist.  That will be o.k.,
>>    1151             if we are just here to check for the path's existence. */
>>    1152          if (flags & open_path_allow_null)
>>    1153            {
>>    1154              parent_path = NULL;
>>    1155              break;
>>    1156            }
>>
>> So, we just need to make the svn_fs_fs__dag_open() call set child=NULL
>> so it can fall back to the existing logic for handling FLAGS:
>>
>> [[[
>> Index: subversion/libsvn_fs_fs/tree.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/tree.c (revision 1845259)
>> +++ subversion/libsvn_fs_fs/tree.c (working copy)
>> @@ -1083,8 +1083,10 @@
>>                                          pool));
>>             if (cached_node)
>>               child = cached_node;
>> -          else
>> -            SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>> +          else if (svn_fs_fs__dag_node_kind(here) == svn_node_dir)
>> +            SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool));
>> +          else
>> +            child = NULL;
>>  
>>             /* "file not found" requires special handling.  */
>>             if (child == NULL)
>> ]]]
>>
>> Makes sense?
Yes, that does make sense. Thank you for fixing this.

My mental image was: Stop at the last parent level (re-usable
between siblings) and attempt a leaf lookup, which would then
handle the error cases. This is obviously incomplete for the
case that the parent is not a directory but has been cached.

What I do not understand, yet, is why this has not been
reported earlier and why it is not present in FSX. I plan to dive
into that code tonight and come back with the result.

-- Stefan^2.


Reply | Threaded
Open this post in threaded view
|

Re: Fwd: [Daniel Shahaf: [PATCH] Re: [PATCH] A test for "Can't get entries" error]

Daniel Shahaf-2
Stefan Fuhrmann wrote on Sat, Dec 01, 2018 at 11:08:02 +0100:

> Yes, that does make sense. Thank you for fixing this.
>
> My mental image was: Stop at the last parent level (re-usable
> between siblings) and attempt a leaf lookup, which would then
> handle the error cases. This is obviously incomplete for the
> case that the parent is not a directory but has been cached.
>
> What I do not understand, yet, is why this has not been
> reported earlier and why it is not present in FSX. I plan to dive
> into that code tonight and come back with the result.

Thanks, Stefan.

To avoid misunderstanding, please note that I haven't committed this
patch yet, and that kotkov has committed a different fix in r1847572.
I assume the patch I posted is still applicable, though, as it fixes
a (now latent) bug in the for(;;) loop.

Cheers,

Daniel