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 <d.s_at_daniel.shahaf.name> -----
>
>> Date: Wed, 21 Nov 2018 14:55:23 +0000
>> From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
>> To: dev_at_subversion.apache.org
>> Subject: [PATCH] Re: [PATCH] A test for "Can't get entries" error
>> Message-ID: <20181121145523.kgwfhlrhiffm5ru7_at_tarpaulin.shahaf.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.
Received on 2018-12-01 11:08:19 CET