[Ext storage] Now using SFTP stream wrapper from phpseclib

[PVince81]

• Upgraded phpseclib to master version (post 0.3.5)
• Now using fopen() on sftp URL for both read and write
• Fixes 5.0.8 - External storage SFTP incredible slow #4063

Please make sure to test with a directory full of pics that are at least 2 MB big.
Preview generation should be faster than before.

[PVince81]

I've tried before and after this fix:
Scanning directory with 65 pictures of sizes between 500 KB and 4MB
Before: takes at least 4 mins, I gave up waiting...
After: 7 seconds !!!
This was probably the mime type scanning. Thumbnails loading also seems to be a bit faster.

I've looked at the phpseclib code and it seems to be due to the fact that it's possible to tell it to only read the first X bytes of a file.
Internally, it is still downloading the file... so downloading a big file still takes twice as long because it will first download the file into a variable (which is worse than the temp file we had).

Uploading seemed to be better though. It took 2min34 to upload a 58min file, but the double time to download.
It seems the SFTP stream is more suitable for uploading.

CC @bantu

[PVince81]

I'm going to debug a bit. It seems there another place in the OC code that will download to a temp file first...
the phpseclib code doesn't download the whole file but just the number of bytes needed.

[PVince81]

For some reason, getLocalFile() is called here in lib/private/files.php:135

            list($storage) = \OC\Files\Filesystem::resolvePath($filename);
            if ($storage instanceof \OC\Files\Storage\Local) {
                self::addSendfileHeader(\OC\Files\Filesystem::getLocalFile($filename));
            }

Even though the $filename is on the mount point, resolve still gives the local storage and trigger the getLocalFile() call. This seems to be a hidden bug. Maybe the mountpoints aren't inited ?

CC @icewind1991

I'll comment this out for my tests.

[PVince81]

Hurray! After commenting out the buggy block the file starts downloading directly, streaming from SFTP -> OC -> browser directly. Only a bit more than 2 min !

[PVince81]

@schiesbn encryption seems to work. The SFTP stream implements fseek(). Nice!

[PVince81]

Uploading 58 MB file after this fix: 2min 34
Before the fix: already 7min... stopping it now...

[karlitschek]

Wow. Awesome! You rock! But let's kill the mimetype scanning anyways. :-)

[ghost]

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2039/

[PVince81]

I've submit this PR #5961 to fix the "addSendfileheader()" condition issue I mentionned in #5954 (comment)

[PVince81]

This PR is still valid as it will improve upload/download performance.
#5965 improves scanning.

[ghost]

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2053/

[bantu]

@PVince81 FYI: The phpseclib update only should give you a bit of a speed improvement already.

[PVince81]

@bantu good to know. I'm still wondering why copying a 58 MB file to localhost through SFTP takes 2min30 but when I do it with the scp command it takes less than a second.
Maybe all these buffer copying and PHP layers do.

Anyway, the new phpseclib and its stream are already a great improvement.

I just wanted to do a bit more testing before I remove the WIP tag.
Feel free to join me 😉

[bantu]

@PVince81 Feel free to send me your test script and I'll try to find out why it is so slow.

[PVince81]

It's a bit late for that one and I don't want to risk causing regressions.
Setting to OC7.

[ghost]

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2743/

[PVince81]

Rebased.
Tested with the video player and download: both start loading the file directly (streaming) instead of waiting for the file to first be downloaded to the temp dir. This is definitely a speed improvement.

Will test encryption next.

[PVince81]

Works well with encryption, the file is streamed through the encryption code.

Please review @bantu @icewind1991 @karlitschek @DeepDiver1975 @schiesbn

Note: this seems like a big change but actually simply imports the new phpseclib version that supports streaming and switches sftp.php to streaming (similar to ftp.php)

This removes the need for temp files for upload/download which significantly accelerates the transfer.

[karlitschek]

can't test at the moment but looks good. 👍

[ghost]

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2745/

[icewind1991]

tested 👍