Work directly on the storage when uploading over webdav #12006

icewind1991 · 2014-11-06T15:54:23Z

Not going trough the entire filesystem stack for the part files during uploading saves a good amount of work and database queries.

In my local test instance this saves 24 queries for a regular upload and 89 queries when uploading to a shared folder.

cc @DeepDiver1975 @PVince81

PVince81 · 2014-11-06T15:55:34Z

Wow, sounds promising 😄

icewind1991 · 2014-11-06T17:57:57Z

Hmm, thinking about it this currently bypasses encryption, either need to add some special case for it (yuk) or finally refactor encryption as a storage wrapper

DeepDiver1975 · 2014-11-07T10:22:34Z

 Test_OC_Connector_Sabre_File.testSimplePutFails
 Stacktrace

Test_OC_Connector_Sabre_File::testSimplePutFails
Failed asserting that exception of type "PHPUnit_Framework_Error_Warning" matches expected exception "\Sabre\DAV\Exception". Message was: "stream_copy_to_stream() expects parameter 1 to be resource, string given".

 Test_OC_Connector_Sabre_File.testSimplePutFailsOnRename
 Stacktrace

Test_OC_Connector_Sabre_File::testSimplePutFailsOnRename
Failed asserting that exception of type "PHPUnit_Framework_Error_Warning" matches expected exception "\Sabre\DAV\Exception". Message was: "stream_copy_to_stream() expects parameter 1 to be resource, string given".

 Test_OC_Connector_Sabre_File.testUploadAbort
 Stacktrace

Test_OC_Connector_Sabre_File::testUploadAbort
Failed asserting that exception of type "PHPUnit_Framework_Error_Warning" matches expected exception "\Sabre\DAV\Exception\BadRequest". Message was: "stream_copy_to_stream() expects parameter 1 to be resource, string given".

 Test_Encryption_Webdav.testWebdavPUT
 Stacktrace

Test_Encryption_Webdav::testWebdavPUT
Failed asserting that false is true.

/var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/vm-slave-02/apps/files_encryption/tests/webdav.php:140

DeepDiver1975 · 2014-11-12T11:04:53Z

Hmm, thinking about it this currently bypasses encryption, either need to add some special case for it (yuk) or finally refactor encryption as a storage wrapper

I'd vote against a special case - what would be the effort to implement encryption as a storage wrapper?
I assume we can remove much more special cases as of today in the code - right?
And further more having a look at https://github.com/owncloud/core/pull/12086/files I assume we can even get a performance gain.

@schiesbn what's your opinion on the encryption storage wrapper?

@karlitschek @craigpg we should definitly invest some time to follow this path as this has a direct impact on the sync performance and opens up a path to solve chunked upload (in general and for external storage maybe - see #4997 (comment))

PVince81 · 2014-11-13T13:15:48Z

Note: moving encryption to a storage wrapper is likely to break a lot of stuff, as we need to make sure that all the numerous places that relied on hooks still works with the storage wrapper. This will need some research. @schiesbn what do you think ?

We've struggled at lot to make encryption stable and avoid lots of bugs in special situations, so need to make sure these will still be there after the move. If the unit tests coverage we have now for encryption is broad enough, then it might help ensure this.

PVince81 · 2014-11-13T13:16:54Z

@icewind1991 @schiesbn maybe raise a ticket to do the necessary research, but I feel like this is out of scope for OC8.

DeepDiver1975 · 2014-11-13T13:30:55Z

@icewind1991 @schiesbn maybe raise a ticket to do the necessary research, but I feel like this is out of scope for OC8.

increasing performance (including chunked upload performance and related issue in the area of the desktop client) are a goal for OC8 - this approach here looks really promissing and - as far as I can tell - is the basis for improving chunked upload - see #4997 (comment)

But @PVince81 is right - this is a critical decision regarding encryption and we need @schiesbn and @icewind1991 collaborating on this

schiessle · 2014-11-17T10:51:22Z

I didn't had a detailed look at the storage wrapper yet. As far as I understand it it would replace the proxies. I think it depends how large the changes to the encryption code are. I don't want to risk any instability if it isn't absolutely necessary. Failures in encryption often results in data lose so I don't want to throw out code which matured over the last two releases airy.

I'm also not sure if it is possible to write the chunks directly to the file. This would mean that the chunks needs to be encrypted on the fly and placed at the right spot. But encrypted files have a different size which is not linear across the file. So the uploaded chunks would need to match the chunk size we need for encryption or need to be a multiple of it.

All probably doable, but I'm skeptical that we can do it for OC8 with the current time frame and the other stuff we have on the table.

DeepDiver1975 · 2014-12-22T13:59:17Z

Requires adoption of encryption -> OC8.1

jknockaert · 2015-01-21T08:34:46Z

@schiesbn I think that the code I developed for #12909 has support for writing chunks to existing encrypted streams. The stream_write I wrote can definitely handle that; stream_open would have to be updated. Not sure how it would fit in with the storage wrapper suggested here.

DeepDiver1975 · 2015-04-08T12:08:08Z

@icewind1991 @schiesbn rebased ... let's see

DeepDiver1975 · 2015-04-08T14:08:39Z

@icewind1991 4 failung unit tests:

There were 4 errors:

1) Test\Connector\Sabre\File::testSimplePutFails
stream_copy_to_stream() expects parameter 1 to be resource, string given

/home/deepdiver/Development/ownCloud/core-autotest/lib/private/connector/sabre/file.php:124
/home/deepdiver/Development/ownCloud/core-autotest/tests/lib/connector/sabre/file.php:34

2) Test\Connector\Sabre\File::testPutSingleFileShare
fopen(/dev/shm/data-autotest/): failed to open stream: Is a directory

/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/local.php:256
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/wrapper/wrapper.php:284
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/wrapper/wrapper.php:284
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/wrapper/wrapper.php:284
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/wrapper/wrapper.php:284
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/files/storage/wrapper/wrapper.php:284
/home/deepdiver/Development/ownCloud/core-autotest/lib/private/connector/sabre/file.php:117
/home/deepdiver/Development/ownCloud/core-autotest/tests/lib/connector/sabre/file.php:59

3) Test\Connector\Sabre\File::testSimplePutFailsOnRename
stream_copy_to_stream() expects parameter 1 to be resource, string given

/home/deepdiver/Development/ownCloud/core-autotest/lib/private/connector/sabre/file.php:124
/home/deepdiver/Development/ownCloud/core-autotest/tests/lib/connector/sabre/file.php:94

4) Test\Connector\Sabre\File::testUploadAbort
stream_copy_to_stream() expects parameter 1 to be resource, string given

/home/deepdiver/Development/ownCloud/core-autotest/lib/private/connector/sabre/file.php:124
/home/deepdiver/Development/ownCloud/core-autotest/tests/lib/connector/sabre/file.php:171

icewind1991 · 2015-04-09T13:16:16Z

Jenkins: #15514

karlitschek · 2015-04-09T13:24:43Z

This would be super sweet to get in for 8.1 :-)

MorrisJobke · 2015-04-09T17:27:15Z

@th3fallen @schiesbn @DeepDiver1975 So this now works with encryption 2.0?

MorrisJobke · 2015-04-09T17:27:40Z

@icewind1991 Is this still WIP? Otherwise change the title and add the correct label ;)

DeepDiver1975 · 2015-04-09T21:31:02Z

@th3fallen @schiesbn @DeepDiver1975 So this now works with encryption 2.0?

yes

PVince81 · 2015-04-13T14:47:27Z

Let me know when you're done so I can retest and rereview. Thanks.

icewind1991 · 2015-04-14T13:28:57Z

Should be done given jenkins isn't sick today

PVince81 · 2015-04-14T13:31:30Z

@icewind1991 give him his pills

PVince81 · 2015-04-14T14:10:14Z

lib/private/connector/sabre/file.php

@@ -71,7 +71,7 @@ class File extends Node implements IFile {
 	 * different object on a subsequent GET you are strongly recommended to not
 	 * return an ETag, and just return null.
 	 *
-	 * @param resource $data
+	 * @param resource|string $data


you forgot to remove the comment

PVince81 · 2015-04-14T14:26:05Z

test without encryption: works well
test with encryption (needed to run add migration script from old encryption to new one #15445 first)

Uploads with cadaver, pyocclient test suite and sync client upload worked.
Note: pyocclient test suite fails due to encryption bugs, will rerun later on master once all encryption PRs are merged.

👍

scrutinizer-notifier · 2015-04-14T14:26:09Z

A new inspection was created.

icewind1991 · 2015-04-14T14:33:38Z

@MorrisJobke @DeepDiver1975 please review

ghost · 2015-04-14T15:12:46Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11514/
🚀 Test PASSed.🚀

MorrisJobke · 2015-04-15T01:08:50Z

👍

Work directly on the storage when uploading over webdav

PVince81 · 2015-04-15T15:09:18Z

Raised #15647 to do the same in chunking mode.

This PR only affects files uploaded without chunking.

PVince81 · 2015-07-31T09:30:27Z

Regression: folder size propagation doesn't happen any more: #18006

PVince81 · 2015-07-31T09:31:57Z

This this change improve performance significantly ?

DeepDiver1975 · 2015-07-31T09:41:09Z

In my local test instance this saves 24 queries for a regular upload and 89 queries when uploading to a shared folder.

^ should make a difference - afaik we had a blackfire graph ...

PVince81 · 2015-08-03T08:57:31Z

Was the benchmark done before or after fixing the regressions ?
I'm pretty sure that if we fix the last regression (correctFolder #18006) it will increase the number of SQL queries again.

Maybe the correctFolder logic should be done asynchronously (and debounced) eventually.

DeepDiver1975 mentioned this pull request Nov 13, 2014

OC_FileChunking on external file systems #4997

Closed

PVince81 added the performance label Nov 13, 2014

DeepDiver1975 mentioned this pull request Dec 8, 2014

Storage changes overview #12487

Closed

22 tasks

MorrisJobke added the 1 - To develop label Dec 10, 2014

DeepDiver1975 added this to the 8.1-next milestone Dec 22, 2014

jknockaert mentioned this pull request Jan 21, 2015

make encryption stream wrapper seekable #12909

Closed

LukasReschke changed the title ~~Work directly on the storage when uploading over webdav~~ [WIP] Work directly on the storage when uploading over webdav Feb 24, 2015

Work directly on the storage when uploading over webdav

6a59502

DeepDiver1975 force-pushed the dav-put-storage branch from a48357a to 6a59502 Compare April 8, 2015 12:07

update tests

cbcee34

DeepDiver1975 changed the title ~~[WIP] Work directly on the storage when uploading over webdav~~ Work directly on the storage when uploading over webdav Apr 9, 2015

DeepDiver1975 added 3 - To Review and removed 1 - To develop labels Apr 9, 2015

MorrisJobke added 2 - Developing and removed 3 - To Review labels Apr 13, 2015

pass a stream to the tests

308af8b

icewind1991 added 3 - To Review and removed 2 - Developing labels Apr 14, 2015

PVince81 reviewed Apr 14, 2015
View reviewed changes

Keep phpdoc updated.

eeecca0

MorrisJobke added a commit that referenced this pull request Apr 15, 2015

Merge pull request #12006 from owncloud/dav-put-storage

e33e5b4

Work directly on the storage when uploading over webdav

MorrisJobke merged commit e33e5b4 into master Apr 15, 2015

MorrisJobke deleted the dav-put-storage branch April 15, 2015 01:08

PVince81 mentioned this pull request Apr 15, 2015

Work directly on storage in chunk upload mode #15647

Closed

This was referenced Apr 21, 2015

Smashbox tests running against master+encryption #15781

Closed

WebDAV PUT on file share fails with 500 #15786

Closed

PVince81 mentioned this pull request May 12, 2015

Versions not created when updated via webdav #16130

Closed

PVince81 mentioned this pull request Jul 31, 2015

Folder size propagation not triggered by Webdav upload #18006

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work directly on the storage when uploading over webdav #12006

Work directly on the storage when uploading over webdav #12006

icewind1991 commented Nov 6, 2014

PVince81 commented Nov 6, 2014

icewind1991 commented Nov 6, 2014

DeepDiver1975 commented Nov 7, 2014

DeepDiver1975 commented Nov 12, 2014

PVince81 commented Nov 13, 2014

PVince81 commented Nov 13, 2014

DeepDiver1975 commented Nov 13, 2014

schiessle commented Nov 17, 2014

DeepDiver1975 commented Dec 22, 2014

jknockaert commented Jan 21, 2015

DeepDiver1975 commented Apr 8, 2015

DeepDiver1975 commented Apr 8, 2015

icewind1991 commented Apr 9, 2015

karlitschek commented Apr 9, 2015

MorrisJobke commented Apr 9, 2015

MorrisJobke commented Apr 9, 2015

DeepDiver1975 commented Apr 9, 2015

PVince81 commented Apr 13, 2015

icewind1991 commented Apr 14, 2015

PVince81 commented Apr 14, 2015

PVince81 Apr 14, 2015

icewind1991 Apr 14, 2015

PVince81 commented Apr 14, 2015

scrutinizer-notifier commented Apr 14, 2015

icewind1991 commented Apr 14, 2015

ghost commented Apr 14, 2015

MorrisJobke commented Apr 15, 2015

PVince81 commented Apr 15, 2015

PVince81 commented Jul 31, 2015

PVince81 commented Jul 31, 2015

DeepDiver1975 commented Jul 31, 2015

PVince81 commented Aug 3, 2015

Work directly on the storage when uploading over webdav #12006

Work directly on the storage when uploading over webdav #12006

Conversation

icewind1991 commented Nov 6, 2014

PVince81 commented Nov 6, 2014

icewind1991 commented Nov 6, 2014

DeepDiver1975 commented Nov 7, 2014

DeepDiver1975 commented Nov 12, 2014

PVince81 commented Nov 13, 2014

PVince81 commented Nov 13, 2014

DeepDiver1975 commented Nov 13, 2014

schiessle commented Nov 17, 2014

DeepDiver1975 commented Dec 22, 2014

jknockaert commented Jan 21, 2015

DeepDiver1975 commented Apr 8, 2015

DeepDiver1975 commented Apr 8, 2015

icewind1991 commented Apr 9, 2015

karlitschek commented Apr 9, 2015

MorrisJobke commented Apr 9, 2015

MorrisJobke commented Apr 9, 2015

DeepDiver1975 commented Apr 9, 2015

PVince81 commented Apr 13, 2015

icewind1991 commented Apr 14, 2015

PVince81 commented Apr 14, 2015

PVince81 Apr 14, 2015

Choose a reason for hiding this comment

icewind1991 Apr 14, 2015

Choose a reason for hiding this comment

PVince81 commented Apr 14, 2015

scrutinizer-notifier commented Apr 14, 2015

icewind1991 commented Apr 14, 2015

ghost commented Apr 14, 2015

MorrisJobke commented Apr 15, 2015

PVince81 commented Apr 15, 2015

PVince81 commented Jul 31, 2015

PVince81 commented Jul 31, 2015

DeepDiver1975 commented Jul 31, 2015

PVince81 commented Aug 3, 2015