Use X-Sendfile when possible for reduced resource usage

[gstrauss]

Re-opening #13082 Use X-Accel-Redirect or X-Sendfile when possible for reduced resource usage

@rkkoszewski wrote in #13082 (comment)
From what I could observe, Nextcloud uses PHP to send the stored files to the client, which can easily time out for huge files, use a lot of RAM and is generally slower especially on low powered or older servers.

It would be great to have an option to use X-Accel-Redirect (Nginx) or X-Sendfile (Apache) when possible to take advantage of the web server's optimised native capabilities.

It might not be possible to use it for encrypted storages or remote storages, but it would greatly reduce the stress on the server for local files.

---

@gstrauss (me) wrote in #13082 (comment)

@MorrisJobke wrote

We discussed this quite intensively in the past and the biggest problem was on how to determine where are file is located and how it interferes with other storage and the web server. In the end it turned out that it was a major amount of work to get all the possible data storages covered and not break them completely.

Would you please share a little bit more detail about some of the reasons why this was deemed so difficult?

It still this sounds like a nice feature, but the requests for this are quite low. Currently there are no plans to implement such a feature. Thus I will close this ticket for now. This does not mean we don't want this feature at all, but it is simply not on our roadmap for the near future. If somebody wants to implement this feature nevertheless we are happy to assist, discuss and help out.

What about an improvement where X-Sendfile is used when safe to do so, or else continue with existing behavior? Why bother trying to get all the possible data storages covered instead of disabling the new feature by default, and enabling the new feature where it is known to be safe to do so?

How about a new NextCloud config option which, if present, indicates the name of the X-Sendfile header (X-Sendfile or X-Accel-Redirect) to be used on eligible responses.

If this new X-Sendfile option were enabled in the NextCloud config, then FileDisplayResponse() could use X-Sendfile if the target file is not a temporary file, i.e. !$file->isEncrypted() && $file->getStorage()->isLocal() . FileDisplayResponse() might also optionally check size and use X-Sendfile only if the file exceeds a certain minimum size, e.g. $file->getSize() > 32KB . A future improvement could be to provide a mechanism to determine if the temporary file is actually ephemeral and about to be removed, or if it has a non-zero lifetime (in a cache) which is long enough for the webserver to reasonably read the header response from NextCloud and to open the file indicated by the X-Sendfile response header. FileDisplayResponse() might need an extra bit of information to indicate if the object is eligible for X-Sendfile. That could be an optional attribute set on the FileDisplayResponse object by caller, or could be an optional argument to the constructor, or could even be a new method on $file->isXSendfileEligible() if arbitrary logic is needed to determine eligibility. [Edit: similar to $file->getContent(), there could be a $file->getXSendfile() which returns a string to a file path iff the response is eligible for X-Sendfile]

As an aside, some use of DataDisplayResponse() appears at first glance to be able to use FileDisplayResponse(), unless the temporary file is to be cleaned up imminently (and therefore must be read into memory).

@manwegit wrote

I had X-Sendfile working around the Owncloud Nextcloud fork.
Ubuntu system, apache + php-fpm. Had to modify some core files to fix filesending problems. And apache config was tightly coupled with nextcloud setup.

If I have the time, I'll try to see if it's still feasible with limited support after the filesystem things revamp.

@manwegit might you have anything to add from your experience getting this working?

---

@gstrauss wrote in #13082 (comment)

FYI: In the past, ownCloud supported X-Sendfile since ownCloud 5.
https://doc.owncloud.com/server/8.1/admin_manual/configuration_files/serving_static_files_configuration.html

However, it looks like that support was removed in 2015 in owncloud/core@8479702 with commit message:

Remove XSendFile support
Required to ensure proper locking

locking is, of course, an issue that must be acknowledged. Still, I think there must be some low-hanging fruit -- such as large images, audio, video, and other media -- for which the admin would like to enable X-Sendfile.

---

@gstrauss wrote in #13082 (comment)

I am willing to put together some patches (as demonstrated above).

File locking matters when the file might be modified in a non-atomic manner, e.g. via partial PUT with range, implemented as direct file modification rather than copy temp, modify temp, and atomic rename into place. The lighttpd web server mod_webdav implements atomic file updates, and provides an option (disabled by default) to enable unsafe direct file modification for partial PUT, should the admin desire. I think that there should be a similar option for admins to enable X-Sendfile in Nextcloud, should the admin attest that the partial PUT scenario is not one that affects the admin's system, and the admin instead wants the drastic performance increase available by using X-Sendfile.

@skjnldsv ? Who might provide some guidance? Thank you.

---

How to use GitHub

• Please use the 👍 reaction to show that you are interested into the same feature.
• Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
• Subscribe to receive notifications on status change and new comments.

[DeepDiver1975]

hint why it was removed - owncloud/core#16997

[gstrauss]

Thanks for the link. Reading through those threads at owncloud/core#16997, my assessment remains that X-Sendfile is safe for use for a given directory (webdav collection) if partial PUT is disabled for that location.

Aside, partial PUT could be implemented by copying the original file to a temporary file, modifying the temporary file, and then atomically renaming the temporary file to the original name. webdav locking would prevent multiple modifications at the same time. Alternatively, if the backing system supports version control, e.g. git, then X-Sendfile is safe when targetting a given revision, as modifications are made to a new revision.

In short, I believe that global support of partial PUT is extremely detrimental, imposing unnecessary requirements and performance overhead when downloading from directories containing large files which do not need to be edited in-place, and could instead use X-Sendfile for file downloads.

[BotoX]

Here's a quick patch that works for nginx:

diff --git a/lib/public/AppFramework/Http/FileDisplayResponse.php b/lib/public/AppFramework/Http/FileDisplayResponse.php
index 90c18e85..8e395124 100644
--- a/lib/public/AppFramework/Http/FileDisplayResponse.php
+++ b/lib/public/AppFramework/Http/FileDisplayResponse.php
@@ -65,7 +65,11 @@ class FileDisplayResponse extends Response implements ICallbackResponse {
        public function callback(IOutput $output) {
                if ($output->getHttpResponseCode() !== Http::STATUS_NOT_MODIFIED) {
                        $output->setHeader('Content-Length: ' . $this->file->getSize());
-                       $output->setOutput($this->file->getContent());
+                       if ($this->file instanceof \OCP\Files\File) {
+                               $output->setHeader('X-Accel-Redirect: /xaccel' . $this->file->getPath());
+                       } else {
+                               $output->setOutput($this->file->getContent());
+                       }
                }
        }
 }

    location ^~ /xaccel {
        internal;
        alias /home/nextcloud;
    }

[DeepDiver1975]

Here's a quick patch that works for nginx:

This should honestly not be done this way - reasons have been discussed in depth already. This can cause data loss!

[BotoX]

Here's a quick patch that works for nginx:

This should honestly not be done this way - reasons have been discussed in depth already. This can cause data loss!

I'd love to see how downloading (reading) a file can cause data loss?
Do you have a testcase to back that claim up? Web servers serving files is pretty much the standard everywhere...

Just now I've tried watching a video (and hitting F5 every time the buffer was full) while also uploading/replacing the same video.
The video was being played back fine the entire time and the upload has also completed without issues.
Then I've deleted the video in nextcloud while it was still buffering/playing, I was able to watch it until the end.
The file was successfully deleted from the DB and disk.

This is on linux with ZFS btw.
If your filesystem/OS will blow up on basic file operations maybe that isn't something to be "fixed" in nextcloud/PHP?

[yevon]

Here's a quick patch that works for nginx:

This should honestly not be done this way - reasons have been discussed in depth already. This can cause data loss!

If sendfile support cannot be enabled due to the filelocking mechanism, the logic of the filelocking should be revised. What I don't really understand is the file-lock mechanism, files should be locked during edits so no more than 1 edit is allowed concurrently, but reads shouldn't be blocked in any way. If a file is replaced during a file download, just redownload it if a download error occurs, and there is no corruption anywhere. Or just verify if there is no other procecess using the file (downloaded by nginx or apache), and wait for replacing it so no edit conflicts or redownloads would be needed, and no temporary copies of the edited file would be needed.

Anyways, having sendfile support should be an option for everybody, as for now is impossible to get full download speeds in nexctloud. With just an optional setting in the docs it would cover many people needs, as not everybody use external storages and having interrupted downloads when somehody edits a file may not be an issue for many.

[yevon]

/lib/public/AppFramework/Http

I'm trying to comment the line $output->setOutput($this->file->getContent()); to see if downloads stop working to see if my changes do effect, but my downloads and video preview do continue working. Maybe is there a php cache or something similar? Ho I can test the patch? thanks. It only works with the nginx docker image? Or it also applies to the apache image?

[BotoX]

/lib/public/AppFramework/Http

I'm trying to comment the line $output->setOutput($this->file->getContent()); to see if downloads stop working to see if my changes do effect, but my downloads and video preview do continue working. Maybe is there a php cache or something similar? Ho I can test the patch? thanks. It only works with the nginx docker image? Or it also applies to the apache image?

server/core/Controller/PreviewController.php

Lines 162 to 167 in 8606f16

	try {
	$f = $this->preview->getPreview($node, $x, $y, !$a, $mode);
	$response = new FileDisplayResponse($f, Http::STATUS_OK, [
	'Content-Type' => $f->getMimeType(),
	]);
	$response->cacheFor(3600 * 24, false, true);

There's a $response->cacheFor here.

My nextcloud has a few more mods that I forgot to mention here, diff that I apply manually attached since I've mentioned it: nextcloud.diff.txt
However this doesn't really matter here, it just changes previews to one fixed size and serves the original image for public previews, etc.

I've added the following to the nginx config:

    location ^~ /xaccel {
        internal;
        add_header X-Accelerated "TRUE" always;
        alias /home/nextcloud;
    }

$ curl -I https://cloud.botox.bz/s/Yj8oYGtZ4RPw7FY/preview
[...]
x-accelerated: TRUE
[...]

So it does work for public previews, which is what I had an issue with anyways.
Public previews don't support partial loads/range request or so, videos would have to be fully downloaded in order to watch them.

$ curl -I "https://user:password@cloud.botox.bz/remote.php/dav/files/....
... no x-accelerated: TRUE here ...

No x-accelerated when access your own files in the nextcloud ui /apps/files/...

[gstrauss]

owncloud and nextcloud use SabreDAV under the hood to for WebDAV support.

If you're using owncloud or nextcloud specifically to serve files, and like me you're frustrated that owncloud and nextcloud have not implemented X-Sendfile, then you might consider giving lighttpd another look. lighttpd mod_webdav is a fully-featured WebDAV file server (Class 1, 2, 3). Since lighttpd 1.4.65, lighttpd mod_webdav serves files very efficiently (e.g. using OS sendfile()) along with supporting partial PUT with webdav.opts += ("partial-put-copy-modify" => "enable"), if you need partial PUT.

Some modern filesystems support efficiently cloning files, making it less expensive to copy to a tempfile, modify, and atomically rename the modified tempfile to replace the original. lighttpd mod_webdav uses OS-specific APIs to take advantage of such filesystem features. lighttpd mod_webdav webdav.opts += ("partial-put-copy-modify" => "enable") is safe to set when using an extent-based filesystem, e.g. ext4, or other filesystems drivers in the linux kernel which implement copy acceleration for Linux copy_file_range(), or Mac OS clonefile().