Uploaded image for project: 'Subversion'
  1. Subversion
  2. SVN-4531

server-side copy (over dav) is slow and uses too much memory

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: 1.7.19, 1.8.14
    • Component/s: mod_dav_svn
    • Labels:
      None

      Description

      Copying a large and deep directory tree via URL->URL copy over DAV uses too much
      memory. In the wild, I've seen servers abort or crash while servicing copy
      operations of top-level directories in repositories totalling about 40GB in size.
      
      To reproduce, create a directory tree with the attached script (writes about
      400MB of data to /var/tmp/gentreetest/) and import this tree into a fresh
      repository under /trunk. I tested with trunk, FSFS format 7.
      
      Then try to copy the trunk directory over HTTP. httpd's quickly reaches up to 1
      or 2 GB in my observations. Even if the copy succeeds it takes some noticable
      amount of time to complete.
      
      This problem seems to be specific to mod_dav or mod_dav_svn.
      
      A workaround is to use file:// URLs. The copy operation completes instantly over
      file:// with no apparent memory usage problem.
      

      1. 1_gentree.py
        5 kB
        Stefan Sperling
      2. 2_mod-dav-walk-scratch-pool.diff
        3 kB
        Stefan Sperling

        Activity

        Hide
        stsp Stefan Sperling added a comment -

        Created an attachment (id=1311)
        Script to generate sample repository content (written by wklust@elego)
        
        

        Show
        stsp Stefan Sperling added a comment - Created an attachment (id=1311) Script to generate sample repository content (written by wklust@elego)
        Hide
        stsp Stefan Sperling added a comment -

        Attachment 1_gentree.py has been added with description: Script to generate sample repository content (written by wklust@elego)

        Show
        stsp Stefan Sperling added a comment - Attachment 1_gentree.py has been added with description: Script to generate sample repository content (written by wklust@elego)
        Hide
        stsp Stefan Sperling added a comment -

        Setting milestone to 1.8.x.
        

        Show
        stsp Stefan Sperling added a comment - Setting milestone to 1.8.x.
        Hide
        stsp Stefan Sperling added a comment -

        Tweak subject and subcomponent.
        
        The problem also happens with svnserve, not just httpd.
        
        Pool debugging indicates the per-request pool is littered with allocations
        related to FSFS caching. Isn't there a cache size limit that is supposed to
        prevent unbound growth of the cache? Is the default cache configuration too
        large for this use case?
        
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261356/  33320184/  52386762)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383>
        (295970/295970/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261367/  33320195/  52386773)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106>
        (295971/295971/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261447/  33320275/  52386853)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295972/295972/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261471/  33320300/  52386878)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383>
        (295973/295973/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261482/  33320311/  52386889)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106>
        (295974/295974/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261562/  33320391/  52386969)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295975/295975/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261586/  33320414/  52386992)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383>
        (295976/295976/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261597/  33320425/  52387003)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106>
        (295977/295977/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261677/  33320505/  52387083)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295978/295978/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261701/  33320530/  52387108)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383>
        (295979/295979/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261712/  33320541/  52387119)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106>
        (295980/295980/0) 
        POOL DEBUG: [23565/2060423960288] PCALLOC (  33261792/  33320621/  52387199)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295981/295981/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261848/  33320546/  52387124)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/temp_serializer.c:221>
        (295982/295982/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261876/  33320574/  52387152)
        0x1dfd1453900 "request" <subversion/libsvn_fs_fs/temp_serializer.c:222>
        (295983/295983/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33261908/  33320606/  52387184)
        0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:115>
        (295984/295984/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33262852/  33321550/  52388128)
        0x1dfd1453900 "request" <subversion/libsvn_subr/string.c:59> (295985/295985/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33262876/  33321574/  52388152)
        0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:123>
        (295986/295986/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33262900/  33321598/  52388176)
        0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:240>
        (295987/295987/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33262924/  33321622/  52388200)
        0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:240>
        (295988/295988/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33262964/  33321662/  52388240)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313>
        (295989/295989/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263004/  33321702/  52388280)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313>
        (295990/295990/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263044/  33321742/  52388320)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313>
        (295991/295991/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263084/  33321782/  52388360)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313>
        (295992/295992/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263116/  33321814/  52388392)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:144>
        (295993/295993/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263324/  33322022/  52388600)
        0x1dfd1453900 "request" <subversion/libsvn_subr/string.c:59> (295994/295994/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263496/  33322194/  52388772)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:118>
        (295995/295995/0) 
        POOL DEBUG: [23565/2060423960288]  PALLOC (  33263924/  33322622/  52389200)
        0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106>
        (295996/295996/0) 
        
        
        

        Show
        stsp Stefan Sperling added a comment - Tweak subject and subcomponent. The problem also happens with svnserve, not just httpd. Pool debugging indicates the per-request pool is littered with allocations related to FSFS caching. Isn't there a cache size limit that is supposed to prevent unbound growth of the cache? Is the default cache configuration too large for this use case? POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261356/ 33320184/ 52386762) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383> (295970/295970/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261367/ 33320195/ 52386773) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106> (295971/295971/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261447/ 33320275/ 52386853) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295972/295972/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261471/ 33320300/ 52386878) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383> (295973/295973/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261482/ 33320311/ 52386889) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106> (295974/295974/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261562/ 33320391/ 52386969) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295975/295975/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261586/ 33320414/ 52386992) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383> (295976/295976/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261597/ 33320425/ 52387003) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106> (295977/295977/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261677/ 33320505/ 52387083) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295978/295978/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261701/ 33320530/ 52387108) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/cached_data.c:2383> (295979/295979/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261712/ 33320541/ 52387119) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106> (295980/295980/0) POOL DEBUG: [23565/2060423960288] PCALLOC ( 33261792/ 33320621/ 52387199) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/id.c:507> (295981/295981/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261848/ 33320546/ 52387124) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/temp_serializer.c:221> (295982/295982/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261876/ 33320574/ 52387152) 0x1dfd1453900 "request" <subversion/libsvn_fs_fs/temp_serializer.c:222> (295983/295983/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33261908/ 33320606/ 52387184) 0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:115> (295984/295984/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33262852/ 33321550/ 52388128) 0x1dfd1453900 "request" <subversion/libsvn_subr/string.c:59> (295985/295985/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33262876/ 33321574/ 52388152) 0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:123> (295986/295986/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33262900/ 33321598/ 52388176) 0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:240> (295987/295987/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33262924/ 33321622/ 52388200) 0x1dfd1453900 "request" <subversion/libsvn_subr/temp_serializer.c:240> (295988/295988/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33262964/ 33321662/ 52388240) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313> (295989/295989/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263004/ 33321702/ 52388280) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313> (295990/295990/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263044/ 33321742/ 52388320) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313> (295991/295991/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263084/ 33321782/ 52388360) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:313> (295992/295992/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263116/ 33321814/ 52388392) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/tables/apr_hash.c:144> (295993/295993/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263324/ 33322022/ 52388600) 0x1dfd1453900 "request" <subversion/libsvn_subr/string.c:59> (295994/295994/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263496/ 33322194/ 52388772) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:118> (295995/295995/0) POOL DEBUG: [23565/2060423960288] PALLOC ( 33263924/ 33322622/ 52389200) 0x1dfd1453900 "request" </home/stsp/svn/src/apr-1.5.1/strings/apr_strings.c:106> (295996/295996/0)
        Hide
        rhuijben Bert Huijben added a comment -

        Do you have any idea if this is authz and or lock walking related?
        
        I could imagine cases where mod_dav walks trees itself without using our 
        optimized code... but then you added that it is reproducible with svn:// ?
        
        Theoretically copy shouldn't be affected by the size of what is copied (bytes) 
        nor the number of nodes, but authz and lock are possible exceptions. Locks are 
        implemented by mod_dav independently of the (other) ra layers.
        

        Show
        rhuijben Bert Huijben added a comment - Do you have any idea if this is authz and or lock walking related? I could imagine cases where mod_dav walks trees itself without using our optimized code... but then you added that it is reproducible with svn:// ? Theoretically copy shouldn't be affected by the size of what is copied (bytes) nor the number of nodes, but authz and lock are possible exceptions. Locks are implemented by mod_dav independently of the (other) ra layers.
        Hide
        stsp Stefan Sperling added a comment -

        Correction: svnserve is not affected after all (I misjudged the situation while
        trying to reproduce with a pool-debug svnserve binary). svn:// works just as
        well as file:// does.
        

        Show
        stsp Stefan Sperling added a comment - Correction: svnserve is not affected after all (I misjudged the situation while trying to reproduce with a pool-debug svnserve binary). svn:// works just as well as file:// does.
        Hide
        stsp Stefan Sperling added a comment -

        It could be related to the lock walk on the copy source by mod_dav, yes.
        

        Show
        stsp Stefan Sperling added a comment - It could be related to the lock walk on the copy source by mod_dav, yes.
        Hide
        stsp Stefan Sperling added a comment -

        Attachment 2_mod-dav-walk-scratch-pool.diff has been added with description: This patch adds use of a scratch pool during the lock walk.

        Show
        stsp Stefan Sperling added a comment - Attachment 2_mod-dav-walk-scratch-pool.diff has been added with description: This patch adds use of a scratch pool during the lock walk.
        Hide
        stsp Stefan Sperling added a comment -

        Created an attachment (id=1312)
        This patch adds use of a scratch pool during the lock walk.
        
        

        Show
        stsp Stefan Sperling added a comment - Created an attachment (id=1312) This patch adds use of a scratch pool during the lock walk.
        Hide
        stsp Stefan Sperling added a comment -

        With the above patch memory use doesn't exceed 500MB in my testing.
        
        500MB is still a huge threshold. I hope we can find other temporary state that
        can be split off into sub pools and cleaned up earlier.
        

        Show
        stsp Stefan Sperling added a comment - With the above patch memory use doesn't exceed 500MB in my testing. 500MB is still a huge threshold. I hope we can find other temporary state that can be split off into sub pools and cleaned up earlier.
        Hide
        stsp Stefan Sperling added a comment -

        Problem mitigated in r1643801 by Bert and me, which is now also nominated for
        backport to 1.7.x and 1.8.x.
        
        I'm now seeing 350MB or so of memory use with trunk as of r1643816.
        This is still quite huge but much better than before.
        

        Show
        stsp Stefan Sperling added a comment - Problem mitigated in r1643801 by Bert and me, which is now also nominated for backport to 1.7.x and 1.8.x. I'm now seeing 350MB or so of memory use with trunk as of r1643816. This is still quite huge but much better than before.
        Hide
        ivan.z Ivan Zhakov added a comment -

        Stefan,
        
        The issue may be also related to r1647887 fix if you have SVNPathAuthz
        short_circuit option enabled on your server.
        

        Show
        ivan.z Ivan Zhakov added a comment - Stefan, The issue may be also related to r1647887 fix if you have SVNPathAuthz short_circuit option enabled on your server.
        Hide
        philipm Philip Martin added a comment -

        This issue is related to a change in mod_dav in 2.2.25 to fix PR54610 which
        added a walk over the copy source looking for lock tokens. The walk involves
        reading the type of every node in the tree and the contents of every directory,
        so to a certain extent it means copy is no longer an O(1) operation. Apache
        knows in advance that the walk is redundant in cases such as Subversion's
        URL-to-URL copy but Subversion cannot avoid the read access. We should attempt
        to fix mod_dav to avoid the walk where possible.
        

        Show
        philipm Philip Martin added a comment - This issue is related to a change in mod_dav in 2.2.25 to fix PR54610 which added a walk over the copy source looking for lock tokens. The walk involves reading the type of every node in the tree and the contents of every directory, so to a certain extent it means copy is no longer an O(1) operation. Apache knows in advance that the walk is redundant in cases such as Subversion's URL-to-URL copy but Subversion cannot avoid the read access. We should attempt to fix mod_dav to avoid the walk where possible.
        Hide
        jcorvel Johan Corveleyn added a comment -

        [ Edited subject to indicate the twofold nature of this issue: branching /
        tagging over dav with httpd 2.2.25+ or 2.4.5+ is both slow, and uses too much
        memory. The memory issue has been fixed in SVN 1.8.11 and 1.7.19, but the copy
        is still O(size of tree), so the performance issue remains. ]
        
        See http://svn.haxx.se/users/archive-2015-03/0174.shtml for a report
        specifically pointing out "slow branching". This was seen with 1.8.11, where the
        memory usage issue is fixed (but clearly not the performance issue). It's quite
        severe: 6-8 minutes for creating a branch is IMHO unacceptable.
        

        Show
        jcorvel Johan Corveleyn added a comment - [ Edited subject to indicate the twofold nature of this issue: branching / tagging over dav with httpd 2.2.25+ or 2.4.5+ is both slow, and uses too much memory. The memory issue has been fixed in SVN 1.8.11 and 1.7.19, but the copy is still O(size of tree), so the performance issue remains. ] See http://svn.haxx.se/users/archive-2015-03/0174.shtml for a report specifically pointing out "slow branching". This was seen with 1.8.11, where the memory usage issue is fixed (but clearly not the performance issue). It's quite severe: 6-8 minutes for creating a branch is IMHO unacceptable.
        Hide
        jcorvel Johan Corveleyn added a comment -

        As context, the 6-8 minutes branching time is when copying a trunk with:
        37681  folders
        186244 files
        
        Admittedly that's pretty big (it's a collection of java and other projects which
        are always branched together). But there must surely be other deployments out
        there with similar sizes of "trees-to-copy".
        
        Until our recent upgrade this never posed a problem ... we clearly counted on
        SVN's O(1) branching performance in this regard.
        

        Show
        jcorvel Johan Corveleyn added a comment - As context, the 6-8 minutes branching time is when copying a trunk with: 37681 folders 186244 files Admittedly that's pretty big (it's a collection of java and other projects which are always branched together). But there must surely be other deployments out there with similar sizes of "trees-to-copy". Until our recent upgrade this never posed a problem ... we clearly counted on SVN's O(1) branching performance in this regard.
        Hide
        jcorvel Johan Corveleyn added a comment -

        There is a workaround (see
        http://mail-archives.apache.org/mod_mbox/subversion-users/201503.mbox/%3CCAB84uBU6+mAqLmHgaNa=_RwVAN4vybMWk2dw5nTDXZwy=owBbw@mail.gmail.com%3E):
        
        Add these directives to your Apache configuration (preferably only in the
        Location sections that serve SVN):
        [[[
            SetEnvIf Request_Method COPY method_is_copy
            RequestHeader set Depth 0 env=method_is_copy
        ]]]
        
        This adds a request header "Depth" with value 0 to each COPY request. This makes
        mod_dav avoid the crawl of the tree being copied (yet still lets Subversion
        perform a normal recursive copy).
        
        The issue (and workaround) has also been documented on our FAQ:
        http://subversion.apache.org/faq.html#dav-slow-copy
        

        Show
        jcorvel Johan Corveleyn added a comment - There is a workaround (see http://mail-archives.apache.org/mod_mbox/subversion-users/201503.mbox/%3CCAB84uBU6+mAqLmHgaNa=_RwVAN4vybMWk2dw5nTDXZwy=owBbw@mail.gmail.com%3E): Add these directives to your Apache configuration (preferably only in the Location sections that serve SVN): [[[ SetEnvIf Request_Method COPY method_is_copy RequestHeader set Depth 0 env=method_is_copy ]]] This adds a request header "Depth" with value 0 to each COPY request. This makes mod_dav avoid the crawl of the tree being copied (yet still lets Subversion perform a normal recursive copy). The issue (and workaround) has also been documented on our FAQ: http://subversion.apache.org/faq.html#dav-slow-copy
        Hide
        gstein Greg Stein added a comment -

        This was fixed on trunk (future 1.10) in r1674627, and nominated for backport to 1.9.x and 1.8.x.
        
        Johan's workaround of setting the Depth:0 header was ported into code within mod_dav_svn.
        
        mod_dav's decision to walk the tree *is* correct behavior for WebDAV operations. We want to skip that 
        behavior for our specific scenario, yet the APIs between mod_dav and its backends (eg. mod_dav_svn) 
        do not provide enough hook points / semantics / whatever to effect the behavior we want. Should 
        mod_dav's backend API ever get bumped/extended, then we'll want to ensure this kind of decision 
        capability is in the new API.
        
        

        Show
        gstein Greg Stein added a comment - This was fixed on trunk (future 1.10) in r1674627, and nominated for backport to 1.9.x and 1.8.x. Johan's workaround of setting the Depth:0 header was ported into code within mod_dav_svn. mod_dav's decision to walk the tree *is* correct behavior for WebDAV operations. We want to skip that behavior for our specific scenario, yet the APIs between mod_dav and its backends (eg. mod_dav_svn) do not provide enough hook points / semantics / whatever to effect the behavior we want. Should mod_dav's backend API ever get bumped/extended, then we'll want to ensure this kind of decision capability is in the new API.
        Hide
        breser Ben Reser added a comment -

        This fix is a hack at best and depends on an implementation detail (that Depth:
        0 is handled the way it currently is).  While I tend to agree that it's highly
        unlikely that this implementation detail will change, I strongly disagree that
        mod_dav's current behavior is correct.
        
        This issue is exercised by the walk of the tree of the source of a copy. 
        mod_dav does this walk because the code to validate lock tokens is intertwined
        with the code that handles pre-conditions (currently limited to ETag checking).
         Prior to recently mod_dav would not validate pre-conditions within the COPY
        source tree.  As I've pointed out previously, this has a certain amount of logic
        since both of these behaviors are specified in an If header.  However, they
        require very different behaviors.  Verifying a pre-condition requires checking
        that a specific resource has the exact ETag provided.  Verifying lock tokens
        means insuring that all lock tokens in the effected tree are represented by lock
        tokesn provided by the client.
        
        However, there is absolutely no reason to validate lock tokens on a COPY source
        because the source is not being modified.  DAV specifically says you should not
        do this.  mod_dav was fixed to ignore the lock checks on the COPY source, but it
        still does the tree walk because it validates pre-conditions as it walks the tree.
        
        The tree walk is utterly unnecessary in order to validate pre-conditions.  In
        fact pre-conditions should only be validated once per method not on the source
        and destination trees.  In my opinion they should also be validated regardless
        of if they fall within the either of the trees being operated on by the method.
         I don't think it's up to the server to second guess why a client is requesting
        a per-condition outside the scope of the action it's asking the server to take.  
        
        The tree walk within the scope of the current mod_dav API is necessary for
        locks.  In my opinion it'd be more ideal for mod_dav to provide an API where it
        requested the provider provide it all lock tokens for locks under a given path.
         Rather than walking the tree and asking for lock tokens on a resource by
        resource basis.  It may be that the DAV provider chooses to walk the tree to
        implement this.  Or it may be the DAV provider chooses to have a storage
        mechanism that allows it to trivially answer this question.
        
        This fix only restores the situation prior to the fix in mod_dav to handle
        preconditions on the COPY source.  It does not resolve other situations where a
        COPY can be slow due to the walk.  Specifically, if the destination of the COPY
        already exists, we need to check the locks.  Which we have to do because we
        support locks.
        
        There is also nothing special at all about what SVN needs and other DAV
        providers need in this case.  Presumably all of them want to avoid walking the
        tree of the source of a COPY.  This issue does show up in SVN more clearly
        because we do copy on write (i.e. O(1) copies).  Other DAV providers are being
        slowed as much as SVN, it's just that they're already slow in doing a COPY.
        
        The concern I have with this fix is who is going to remember to come back and
        remove it if mod_dav's behavior of "Depth: 0" changes?  Are we sure our test
        suite would detect such a change?  Frankly this is a potential pitfall disguised
        as a fix.
        
        I'm not going to stand in the way of this fix, but in my decision we're making a
        poor choice here just to resolve something in the most expedient way.
        

        Show
        breser Ben Reser added a comment - This fix is a hack at best and depends on an implementation detail (that Depth: 0 is handled the way it currently is). While I tend to agree that it's highly unlikely that this implementation detail will change, I strongly disagree that mod_dav's current behavior is correct. This issue is exercised by the walk of the tree of the source of a copy. mod_dav does this walk because the code to validate lock tokens is intertwined with the code that handles pre-conditions (currently limited to ETag checking). Prior to recently mod_dav would not validate pre-conditions within the COPY source tree. As I've pointed out previously, this has a certain amount of logic since both of these behaviors are specified in an If header. However, they require very different behaviors. Verifying a pre-condition requires checking that a specific resource has the exact ETag provided. Verifying lock tokens means insuring that all lock tokens in the effected tree are represented by lock tokesn provided by the client. However, there is absolutely no reason to validate lock tokens on a COPY source because the source is not being modified. DAV specifically says you should not do this. mod_dav was fixed to ignore the lock checks on the COPY source, but it still does the tree walk because it validates pre-conditions as it walks the tree. The tree walk is utterly unnecessary in order to validate pre-conditions. In fact pre-conditions should only be validated once per method not on the source and destination trees. In my opinion they should also be validated regardless of if they fall within the either of the trees being operated on by the method. I don't think it's up to the server to second guess why a client is requesting a per-condition outside the scope of the action it's asking the server to take. The tree walk within the scope of the current mod_dav API is necessary for locks. In my opinion it'd be more ideal for mod_dav to provide an API where it requested the provider provide it all lock tokens for locks under a given path. Rather than walking the tree and asking for lock tokens on a resource by resource basis. It may be that the DAV provider chooses to walk the tree to implement this. Or it may be the DAV provider chooses to have a storage mechanism that allows it to trivially answer this question. This fix only restores the situation prior to the fix in mod_dav to handle preconditions on the COPY source. It does not resolve other situations where a COPY can be slow due to the walk. Specifically, if the destination of the COPY already exists, we need to check the locks. Which we have to do because we support locks. There is also nothing special at all about what SVN needs and other DAV providers need in this case. Presumably all of them want to avoid walking the tree of the source of a COPY. This issue does show up in SVN more clearly because we do copy on write (i.e. O(1) copies). Other DAV providers are being slowed as much as SVN, it's just that they're already slow in doing a COPY. The concern I have with this fix is who is going to remember to come back and remove it if mod_dav's behavior of "Depth: 0" changes? Are we sure our test suite would detect such a change? Frankly this is a potential pitfall disguised as a fix. I'm not going to stand in the way of this fix, but in my decision we're making a poor choice here just to resolve something in the most expedient way.
        Hide
        gstein Greg Stein added a comment -

        "However, there is absolutely no reason to validate lock tokens on a COPY source because the source is 
        not being modified."
        
        Actually, there is a *very* good reason to do so. Consider the scenario: I lock A/B. Then I copy A to C, 
        presenting the lock token for A/B. The semantics of this operation is, "make the copy, conditioned 
        upon the fact that $token is found." Or to put another way, "nobody has modified A/B, which is critical 
        when I copy it to C." ... there is a valid scenario where after the lock of A/B, another client 
        administratively unlocks it, and modifies it. Thus, jeopardizing what gets copied to C/B.
        
        To put another way, "preconditions" include *both* ETags and lock tokens. Operations can be 
        conditioned upon either datum.
        
        "DAV specifically says you should not do this."
        
        Citation? I don't recall this within the DAV spec.
        
        ...
        
        I concur that this situation could be improved with an API change between mod_dav and its backend. 
        We felt that was out of scope, and would require an update of both httpd *and* svn to acquire a fix for 
        this issue. ... And yes, it is true that we might not remember to fix this, if mod_dav decides to change 
        the behavior (which I doubt).
        
        ...
        
        "if the destination of the COPY already exists, we need to check the locks."
        
        Hmm? We don't allow overwrites. The destination should never exist (the FS will throw an error if it 
        does). mod_dav *does* attempt to check for a lock on the parent dir (the list of members is being 
        modified), but we do not have locks on directories, so that's a null operation.
        
        

        Show
        gstein Greg Stein added a comment - "However, there is absolutely no reason to validate lock tokens on a COPY source because the source is not being modified." Actually, there is a *very* good reason to do so. Consider the scenario: I lock A/B. Then I copy A to C, presenting the lock token for A/B. The semantics of this operation is, "make the copy, conditioned upon the fact that $token is found." Or to put another way, "nobody has modified A/B, which is critical when I copy it to C." ... there is a valid scenario where after the lock of A/B, another client administratively unlocks it, and modifies it. Thus, jeopardizing what gets copied to C/B. To put another way, "preconditions" include *both* ETags and lock tokens. Operations can be conditioned upon either datum. "DAV specifically says you should not do this." Citation? I don't recall this within the DAV spec. ... I concur that this situation could be improved with an API change between mod_dav and its backend. We felt that was out of scope, and would require an update of both httpd *and* svn to acquire a fix for this issue. ... And yes, it is true that we might not remember to fix this, if mod_dav decides to change the behavior (which I doubt). ... "if the destination of the COPY already exists, we need to check the locks." Hmm? We don't allow overwrites. The destination should never exist (the FS will throw an error if it does). mod_dav *does* attempt to check for a lock on the parent dir (the list of members is being modified), but we do not have locks on directories, so that's a null operation.
        Hide
        breser Ben Reser added a comment -

        I recall doing some testing in the past that showed that mod_dav wasn't actually
        enforcing lock tokens as preconditions.  This surprised me at the time (thinking
        that I had found a bug) and eventually I concluded that the spec didn't require it.
        
        Looking at the behavior today and the spec it appears that it is doing this and
        it is required.  So I guess I was wrong on this.  Apparently I'd found a bug in
        the past and wrongly concluded it wasn't (either in mod_dav or my testing).
        
        The spec does say that you shouldn't enforce locks when the locked resource
        isn't being modified.
        
        However, this doesn't really help this solution either.  Because it means that
        you're deliberately breaking the compliance with the DAV spec.  This means that
        If headers won't work right for an Autoversioning client.  Which incidentally is
        the client most likely to use these since it doesn't know about versioning.
        
        Granted this has been broken on the COPY source for a long time so most of these
        clients have had to work without this functionality anyway.  But just rolling
        back the fix to the COPY source for SVN seems like a wrong solution to me. 
        Though I'll admit if you do actually have to enforce locks as preconditions that
        I don't see a better solution.
        
        Maybe we should at least limit this to when we're talking with an SVN client or
        if we're not talking to an SVN client and there are no if headers.
        
        This does also mean that the fix I proposed in this mail to mod_dav actually is
        probably the ideal optimization:
        http://mail-archives.apache.org/mod_mbox/subversion-dev/201504.mbox/%3C551EDAA5.90102%40reser.org%3E
        

        Show
        breser Ben Reser added a comment - I recall doing some testing in the past that showed that mod_dav wasn't actually enforcing lock tokens as preconditions. This surprised me at the time (thinking that I had found a bug) and eventually I concluded that the spec didn't require it. Looking at the behavior today and the spec it appears that it is doing this and it is required. So I guess I was wrong on this. Apparently I'd found a bug in the past and wrongly concluded it wasn't (either in mod_dav or my testing). The spec does say that you shouldn't enforce locks when the locked resource isn't being modified. However, this doesn't really help this solution either. Because it means that you're deliberately breaking the compliance with the DAV spec. This means that If headers won't work right for an Autoversioning client. Which incidentally is the client most likely to use these since it doesn't know about versioning. Granted this has been broken on the COPY source for a long time so most of these clients have had to work without this functionality anyway. But just rolling back the fix to the COPY source for SVN seems like a wrong solution to me. Though I'll admit if you do actually have to enforce locks as preconditions that I don't see a better solution. Maybe we should at least limit this to when we're talking with an SVN client or if we're not talking to an SVN client and there are no if headers. This does also mean that the fix I proposed in this mail to mod_dav actually is probably the ideal optimization: http://mail-archives.apache.org/mod_mbox/subversion-dev/201504.mbox/%3C551EDAA5.90102%40reser.org%3E

          People

          • Assignee:
            Unassigned
            Reporter:
            stsp Stefan Sperling
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development