found the PDH problem

[Greg Stein <gs...@gmail.com>]

Hyrum,

I #if'd out the child/parent section at the end of parse_local_abspath
that we talked about on IRC, and confirmed the test failures that you
saw. Digging in, it turns out that we end up calling "owns lock" with
two different wcroot_t structures. IOW, the wcroot that opened/owns
the lock is attached to one directory, but another directory ends up
with its own wcroot_t which has no "owned" locks associated with it,
so things blow up because the code believes some other process is
working on that other directory.

With the old PDH code in there, we get the wcroot inserted into PDH
structures all the way up to the root. Any lookup thereafter will
share the same wcroot_t structure.

As we discussed on IRC, if a move is made to a more pure lookup of
wcroot_t ancestors, then it could solve this particular problem.

Your conversion of dir_data to wcroot pointers may run into a similar
problem. I don't know enough about your change to truly know, but be
aware of the problem. An SVN_DBG in create_wcroot could provide a lot
of information (generally, it should only ever be called once per
process).

I'm putting this on hold now because I don't want to interfere with
your work. A suggested fix would be to switch wcroot_parse to a new
algorithm that cleans out the PDH concept and more directly identifies
(and reuses!) a parent wcroot. I'd be happy to coordinate that rewrite
with/for you.

Cheers,
-g

Re: found the PDH problem

[Greg Stein <gs...@gmail.com>]

On Sun, Feb 27, 2011 at 06:40, Stefan Sperling <st...@elego.de> wrote:
> On Sun, Feb 27, 2011 at 02:34:44AM -0500, Greg Stein wrote:
>> Hyrum,
>>
>> On IRC, we also talked about trying to remove stat() calls from the
>> parse function. Upon reflection, that won't be possible. If the given
>> path is below a wcroot, then we cannot simply return that wcroot. We
>> need to see if the path is in a CHILD wcroot (think: nested wcroots in
>> a disjoint working copy). Thus, we need to examine the disk to look
>> for the closest wcroot to a given path.
>
> Greg,
>
> it would be really interesting to know your opinion on this thread,
> because it is slightly related:
> http://mail-archives.apache.org/mod_mbox/subversion-dev/201102.mbox/%3C20110223153217.GG29291@jack.stsp.name%3E
> (same message in a different archive: http://svn.haxx.se/dev/archive-2011-02/0789.shtml)
>
>> If we continue to keep a mapping of dirpath->wcroot, then we can avoid
>> future stat calls after the first (effectively, it is a cache). For a
>> file, we will always need a stat since the fname will not be in a
>> memory structure. When the file is discovered, the parent directory
>> may be in the cache and no further stats will be required.
>
> I think the current local_abspath->wcroot mapping approach is broken for
> svn:externals, as long externals have their own wc.db. There are two
> valid results in that situation.
>
> It's possible that what the code does right now is not what's intended,
> but on the surface it looks more like a gap in the current design than
> an implementation error.

Ah! This is your externals problem?

Yeah. I'll read up on the thread. Hyrum and I planned some revisions
in this area, and it sounds like I should keep more in mind.

WIll Do. Thanks.

Cheers,
-g

Re: found the PDH problem

[Stefan Sperling <st...@elego.de>]

On Sun, Feb 27, 2011 at 02:34:44AM -0500, Greg Stein wrote:
> Hyrum,
> 
> On IRC, we also talked about trying to remove stat() calls from the
> parse function. Upon reflection, that won't be possible. If the given
> path is below a wcroot, then we cannot simply return that wcroot. We
> need to see if the path is in a CHILD wcroot (think: nested wcroots in
> a disjoint working copy). Thus, we need to examine the disk to look
> for the closest wcroot to a given path.

Greg,

it would be really interesting to know your opinion on this thread,
because it is slightly related:
http://mail-archives.apache.org/mod_mbox/subversion-dev/201102.mbox/%3C20110223153217.GG29291@jack.stsp.name%3E
(same message in a different archive: http://svn.haxx.se/dev/archive-2011-02/0789.shtml)

> If we continue to keep a mapping of dirpath->wcroot, then we can avoid
> future stat calls after the first (effectively, it is a cache). For a
> file, we will always need a stat since the fname will not be in a
> memory structure. When the file is discovered, the parent directory
> may be in the cache and no further stats will be required.

I think the current local_abspath->wcroot mapping approach is broken for
svn:externals, as long externals have their own wc.db. There are two
valid results in that situation.

It's possible that what the code does right now is not what's intended,
but on the surface it looks more like a gap in the current design than
an implementation error.

Thanks,
Stefan

Re: found the PDH problem

[Greg Stein <gs...@gmail.com>]

Hyrum,

On IRC, we also talked about trying to remove stat() calls from the
parse function. Upon reflection, that won't be possible. If the given
path is below a wcroot, then we cannot simply return that wcroot. We
need to see if the path is in a CHILD wcroot (think: nested wcroots in
a disjoint working copy). Thus, we need to examine the disk to look
for the closest wcroot to a given path.

If we continue to keep a mapping of dirpath->wcroot, then we can avoid
future stat calls after the first (effectively, it is a cache). For a
file, we will always need a stat since the fname will not be in a
memory structure. When the file is discovered, the parent directory
may be in the cache and no further stats will be required.

The loop at the end of the parse function will still be desirable to
cache dirpath:wcroot mappings from the target path up to the actual
wcroot.

Cheers,
-g

On Fri, Feb 25, 2011 at 02:54, Greg Stein <gs...@gmail.com> wrote:
> Hyrum,
>
> I #if'd out the child/parent section at the end of parse_local_abspath
> that we talked about on IRC, and confirmed the test failures that you
> saw. Digging in, it turns out that we end up calling "owns lock" with
> two different wcroot_t structures. IOW, the wcroot that opened/owns
> the lock is attached to one directory, but another directory ends up
> with its own wcroot_t which has no "owned" locks associated with it,
> so things blow up because the code believes some other process is
> working on that other directory.
>
> With the old PDH code in there, we get the wcroot inserted into PDH
> structures all the way up to the root. Any lookup thereafter will
> share the same wcroot_t structure.
>
> As we discussed on IRC, if a move is made to a more pure lookup of
> wcroot_t ancestors, then it could solve this particular problem.
>
> Your conversion of dir_data to wcroot pointers may run into a similar
> problem. I don't know enough about your change to truly know, but be
> aware of the problem. An SVN_DBG in create_wcroot could provide a lot
> of information (generally, it should only ever be called once per
> process).
>
> I'm putting this on hold now because I don't want to interfere with
> your work. A suggested fix would be to switch wcroot_parse to a new
> algorithm that cleans out the PDH concept and more directly identifies
> (and reuses!) a parent wcroot. I'd be happy to coordinate that rewrite
> with/for you.
>
> Cheers,
> -g
>