The code is on github: https://github.com/enovation/moodle/tree/MDL-26028 . One important bit is that I had to change:
protected function get_content_file_location()

to:
public function get_content_file_location()

Hello,
did you verify that the headers returned through X-Sendfile are 100% the same compared to the current way (preferably on all types of servers, not just apache)? If not it might create unexpected XSS issues.

I think that the benefit will be quite low because the biggest cost is our session, security checks and general overhead.

In any case thanks a lot for evaluating this potential performance improvement! I am going to make this a subtask in the existing meta issue.

Petr

Lighttpd 1.4 uses the "X-LIGHTTPD-send-file" header for this. I'll try this out later on (today hopefully).

FWIW, the headers might not be 100% the same – the web server may calculate its own ETag header (and maybe others).

The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem.

Trying with lighttpd, downloading a file from the front page (so all users have access), the headers are the same except for the Accept-Ranges: header – lighttpd sends Accept-Ranges: none, while plain Moodle sends Accept-Ranges: bytes. I'm going to do some more testing.

My updates (lighttpd support, added language strings, add support to the send_file() function, and some cleanups) are available here: http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028

Nginx's X-Accel-Redirect doesn't seem very straight forward, and I think we should get someone who uses Nginx to test that setting out (and document how Nginx needs to be configured for it to work) before we add that option.

Edit: updated link to my git browser

The files uploaded by students have intentionally borked file headers (aka forcedownload), if anything fixes them we have a XSS problem.

For lighttpd the docs (well, blog post) says: "All the response headers from the backend are forwarded, just Content-Length is added.". And when I tested it, it maintained the "Content-Type: application/x-forcedownload" and "Content-Disposition: attachment; ..." headers.

From what I can tell, the Apache module should maintain those headers too (it looks like it just modifies ETag, and drops Content-Encoding), but I don't have Apache to try it out.

I did some informal benchmarking on lighttpd 1.4 using ApacheBench, and here are the results for 'ab -n 1000 -c 100 "http://localhost/xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1"' (1000 requests 100 concurrent, downloading a file uploaded to the front page):

Using X-Sendfile:

This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        lighttpd/1.4.28
Server Hostname:        localhost
Server Port:            80

Document Path:          /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1
Document Length:        163284427 bytes

Concurrency Level:      100
Time taken for tests:   188.595 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      163284902000 bytes
HTML transferred:       163284427000 bytes
Requests per second:    5.30 [#/sec] (mean)
Time per request:       18859.505 [ms] (mean)
Time per request:       188.595 [ms] (mean, across all concurrent requests)
Transfer rate:          845504.23 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    2   1.5      2       8
Processing:  7261 18413 4044.9  17673   34347
Waiting:      266 4177 6043.5    525   19613
Total:       7263 18415 4044.9  17675   34350

Percentage of the requests served within a certain time (ms)
  50%  17675
  66%  17937
  75%  18456
  80%  20192
  90%  23707
  95%  26304
  98%  30332
  99%  31565
 100%  34350 (longest request)

Without X-Sendfile:

This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        lighttpd/1.4.28
Server Hostname:        localhost
Server Port:            80

Document Path:          /xsendfile/pluginfile.php/14/mod_resource/content/1/elisdoc.tar.gz?forcedownload=1
Document Length:        163284427 bytes

Concurrency Level:      100
Time taken for tests:   321.726 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      163284903000 bytes
HTML transferred:       163284427000 bytes
Requests per second:    3.11 [#/sec] (mean)
Time per request:       32172.641 [ms] (mean)
Time per request:       321.726 [ms] (mean, across all concurrent requests)
Transfer rate:          495632.03 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.8      0       4
Processing:  3171 30678 5574.6  31112   40680
Waiting:      462 29776 5608.2  30196   39994
Total:       3175 30679 5574.0  31113   40680

Percentage of the requests served within a certain time (ms)
  50%  31113
  66%  31372
  75%  31605
  80%  31717
  90%  37622
  95%  39641
  98%  40227
  99%  40358
 100%  40680 (longest request)

Also, when using X-Sendfile, the lighttpd process was using about 20% CPU according to top (vs around 80% CPU without X-Sendfile), and the PHP processes stayed at their normal ~80-90MB memory usage (vs jumping up to ~100-200MB without X-Sendfile)

We have conduced a benchmark to test how this could improove overall download performance and the results are attached

Thanks for the benchmarks, did you try to evaluate the results somehow?

Here is the code used to run the benchmark.

For the "optimizations" we used:

EnableMMAP On
EnableSendfile On

On apache Vhost

And for the "mod_xsendfile" modified file.php we did modified file.php near:

// ========================================
// finally send the file
// ========================================
session_write_close(); // unlock session during fileserving
$filename = $args[count($args)-1];
header('X-Sendfile: '.$pathname);
die();
send_file($pathname, $filename, $lifetime, $CFG->filteruploadedfiles, false, $forcedownload);

The difference between Hubert's and Gabriel's tests and mine is that I was downloading a file that has required user to be authorized - I made ab to pass a cookie to authenticate. In such a case the speed-up may be smaller as there is more PHP logic executed before the file starts downloading.

Anyway, it looks to me that it makes sense to add XSendfile support - the only problem I have is testing with other web servers than Apache.

Petr Škoda, we found that, for small files, file.php will performs better (on response time), but as the filesize grows X-Sendfile with optimizations starts to became very similar, and with files bigger then 10Mb it performs better (I have no idea why 100Mb benchmark stated it wrong but I can guess there was a problem collecting the data near that test).

This is some kind obvious, as it's faster, for small files, to just let the php handle it in one step, not two (like what is needed with X-Sendfile).

But the chief gain is that X-Sendfile can handle more "Hits per second" or better saying it can deliver more files at the same time then "php" version.

This can be seen by looking at "duration" graphs. (Fewer times are better)

I can test with lighttpd. The other servers that I know of that support this are Nginx and Cherokee. (I may try out Cherokee at some point.) I don't know if IIS has any similar support (whether official or not). As far as what to test, we need to test that it serves the correct file contents (obviously), and that it doesn't munge important headers (especially the headers related to forcedownload). Is there anything else?

Major benefit from sendfile support is ability to offload file sending (which may take significant amount of time because of large file size/slow client connection) to lightweight threaded webservers (lighttpd, nginx) instead of heavy php processes. It is very inefficient to use full-blown php process to simply serve a static file.

Currently I am investigating nginx + php-fpm setup for Moodle, and definitely can help with testing of this feature.

Just came back to this ticket to see if it's already accepted or not, and as Alexey said, if we consider nginx + php-fpm, apache2 + php-fpm, or even apache2 + php with fastcgi, offloading file transmission to the webserver means that your PHP process is free to serve other users while the webserver takes the responsability to sending the file.

Just imagine these two scenarios:

• A not so big file but user with really slow connection
• A big file (maybe a movie) and a user with a quite decent connection

In both situations, the PHP process will stay "blocked" throttling the download and not serving the other users. Just imagine that you spawned 10 process and you have 10 users downloading files with a really slow connection, your site became irresponsive until the users finish their downloads.

X-Sendfile is a major improvement and should really be considered as a major feature for next release.

If no one volunteers to code this feature, I will probably do it, but will need some help to not mess existing features.

Gabriel: it has already been coded. See http://vcs.uhoreg.ca/git/cgit/moodle/log/?h=MDL-26028. It just needs to be tested to make sure that:

• all file downloads are handled
• downloads are processed correctly (sends the correct file contents)
• Petr's security concerns are handled correctly

I've lightly tested it in lighttpd, and for the things that I've tested, the security things were fine, but it needs to be tested more (and it needs to be documented what has been tested an how), and on different servers.

Well thank you for that, I didn't saw your first comment with the link to the repository.
I will make a test with nginx and paste configuration instructions.

Ah, good, nginx. I'm not sure that I completely understood its X-Accel-Redirect documentation, so it's good if you can test it (and fix it if it's incorrect).

according to documentation:

Note that the following HTTP headers aren't modified by nginx:

Content-Type
Content-Disposition
Accept-Ranges
Set-Cookie
Cache-Control
Expires

source: http://wiki.nginx.org/XSendfile

I also found it difficult to apply your modification as your code is based on January 2011 codebase. Could you please update it to a more recent version? If possible also could you please do it on a public accessible git address (like on github)?

Sorry, I don't have time to update the patch right away. I'll see if I have time soon. (By the way, in case you didn't notice, there are two patches that need to be applied – one by Tomasz, and one by myself.)

It is on a public git repository; you can "git clone http://vcs.uhoreg.ca/git/moodle.git", and use the MDL-26028. If you have a recent git, you can just do "git clone -b MDL-26028 http://vcs.uhoreg.ca/git/moodle.git".

I've implemented a more "spread" use of nginx's X-Accel-Redirect against v2.2.2. You can find my initial work here
http://http://git.digitalsk.com.br/moodle.git branch xaccel

To make it work you will have to configure 2 internal locations in nginx, one for your root dir and one for moodleadta:
— snip —
location /moodledata/

location /directfile/

— snip —

In the config file you will have to setup those locations:
$CFG->dataroot_xaccelredirect = '/moodledata/';
$CFG->dirroot_xaccelredirect = '/directfile/';

The implementation should be compatible with "non nginx setups", except for one buffer flushing I had to remove in prepare_file_content_sending() [lib/filelib.php]. I'm not sure about the implications of this modification, so if you are familiar with the subject please comment.

Thanks,

Loic, I'm having problems fetching from your repository. When I try to do a git fetch, it just hangs.

Anyways, I don't think that you need the /directfile/ rule. As far as I can tell, you should be able to just do "X-Accel-Redirect: /path/to/file/relative/to/http/root" if you're sending a file that is in the Moodle web directory.

Hi,

finally I have installed xsendfile support for my test server, I am going to work on it this week, hopefully to be submitted for integration for next week (master). Thanks for all the patches and information!

Petr

notes:

• we must not give away the filepool file path, instead I guess the easiest may be to move this inside the stored_file
• I agree we can not use xsendfile when dontdie or temp file served - because we can not delete files afterserving
• I like the idea of readfile_accel() that does the selected serving
• prepare_file_content_sending() should be optionally called from readfile_accel()
• it might be better to allow configuration from config.php only because we need to have the setting available also before we load the CFG from database
• I did not know there are so many different xsendfile ways - I will definitely need testers

Hmm, there are two major changes pending in master - image preview and repository linking, I am afraid this will have to wait a bit more, hopefully the feature freeze will not block this...

Hubert,
did you do : git clone http://git.digitalsk.com.br/moodle.git ? It should work.
For the /directfile/ you might be right but I have not tested that way. I assumed I had to declare the location "internal;" to work with X-Accel-Redirect.

Hubert, I attached the patch (xaccel_v2.2.2.patch) in case you are still unable to clone my repository.

Loic,
I tried cloning again, and it still hangs. I'm not sure what's wrong. Thanks for attaching the patch, though.
The "internal" is just a suggestion, to make sure that files that shouldn't be otherwise available aren't. So it isn't needed if nginx already serves that directory.
Regarding your patch, a few comments:

• it would be better to put your readfile_accel in an existing library file instead of creating a new one
• you should follow the Moodle coding style (the thing I noticed was your indenting)
• there's no guarantee that the Moodle data directory will contain the word "moodledata", or that something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in case someone, for whatever reason, calls that function on a file that isn't in either directory.

Comparing Loic's patch against Tomasz/my patch, I would say that Loic's patch is more thorough in that it also uses acceleration for theme files. Some benchmarks show that acceleration is less important for small files, so it's debatable how important it is for theme files. The downside to doing acceleration for theme files is that the theme PHP files use ABORT_AFTER_CONFIG, which means that it must be configured in config.php instead of in the database (or, at least, if it's configured in the database, the theme PHP files won't see the configuration). IMHO, I don't see a reason not to use acceleration in the theme files if it's possible, but we also want to allow users to configure those options within Moodle. So I would suggest that we add the options in the Moodle UI, but add a note saying that if they want theme files to be accelerated, then they need to set the options in the config.php file.

BTW, I also now have access to an nginx server, so I can test both Lighttpd and nginx.

> it would be better to put your readfile_accel in an existing library file instead of creating a new one

which one do you suggest ?

> you should follow the Moodle coding style (the thing I noticed was your indenting)

Ok, my vim settings are ok now.

> there's no guarantee that the Moodle data directory will contain the word "moodledata", or that
>something in the main Moodle directory won't contain "moodledata" in its filename (e.g. if someone decides
>for whatever reason to create a plugin with "moodledata" in the name). So it would be best to compare
>against the actual contents of $CFG->dataroot. The "else" should also explicitly check against dirroot, in
>case someone, for whatever reason, calls that function on a file that isn't in either directory.

Right, I've already noticed that too.

I'm going to rewrite following the coding style (tabs/spaced are already fixed) and post it here.

> it would be better to put your readfile_accel in an existing library file instead of creating a new one
which one do you suggest ?

Sorry, I thought that I erased that comment.
Usually, I would suggest either moodlelib or weblib. But in this case, since you're calling the function from the theme PHP files, and it looks like there's a desire to keep those as light as possible, I don't know if that would work. So in this particular case, it might be fine the way you have it.

version 0.2

I've updated the git repository and the attached patch, now the coding style should be fixed.
I have 2 issues remaining:

• the buffer flushing is still commented (prepare_file_content_sending())
• the stored_file->readfile() is still unaware of the "dontdie" option

Thanks everybody for the patch, while I was refactoring the file apis a bit I realised we could use the contenthash of stored files as etag and significantly improve performance of force downloaded files such as forum attachments.

I have tested my patch with Apache X-Sendfile module only, hopefully it will work with Nginx and Lighttpd too.

Please test and make some noise to help with getting this integrated.

Petr

I assume that Petr's patch implements everything discussed above...

yes, it should

Petr,
The nginx part in readaccel is missing a way to add the prefix configured in nginx (internal or not) for a direct access to the dataroot.
The way you wrote it should work OOTB with $dirroot but not with $dataroot.

from your patch (line 61):
$filepath = substr($filepath, strlen($dataroot));
should be something like that:
$filepath = $CFG->dataroot_nginxprefix . substr($filepath, strlen($dataroot));

Thanks,

sorry, s/readaccel/xsendfilelib/

ah, the problem is that the cachedir and tempdir might be outside of $CFG->dataroot, I am afraid we will have to add more nginx prefixes

I guess there is no way to simply whitelist absolute paths in nginx, right?

I have added a new setting for the Nginx aliases (see config-dist.php), please test it.

Sorry I don't understand what you mean with whitelist.
X-Accel-Redirect is a bit more complicated than the other solutions. It does not work with file paths, only with url, so you have to configure all the locations you want to reach in your nginx config. I had'nt thought of the cases where cachedir/tempdir are outside the dataroot; in this case you will have to configure each dir separately and check in xsendfilelib in which dir the file is in to add the proper url prefix.
I'm afraid there is no "easy way" to configure it.

Thanks,

Petr, looks great now.
I'm testing

Thanks for the explanation, I think I understood that correctly, my patch allows unlimited number of directory aliases which are selected automatically based on the real file path of the served path. Could somebody please verify it works? I do not have nginx install here. Or if somebody tells me how to configure nginx from macports on OSX I could do it myself...

Petr,
It worked with this small adjustment:

diff --git a/lib/xsendfilelib.php b/lib/xsendfilelib.php
index 39106f7..6c78456 100644
— a/lib/xsendfilelib.php
+++ b/lib/xsendfilelib.php
@@ -52,9 +52,11 @@ function xsendfile($filepath) {
$aliased = false;
if (!empty($CFG->xsendfilealiases) and is_array($CFG->xsendfilealiases)) {
foreach ($CFG->xsendfilealiases as $alias=>$dir) {

• $dir = realpath($dir).PATH_SEPARATOR;
  + $ds = DIRECTORY_SEPARATOR;
  + $dir = realpath($dir).DIRECTORY_SEPARATOR;
  if (strpos($filepath, $dir) === 0) { $filepath = $alias.substr($filepath, strlen($dir)); + $filepath = str_replace($ds.$ds, $ds, $filepath); $aliased = true; break; }

—
The str_replace is just here to clean the path to be compared.

The patch

arrgh, separators again, thanks!!! Why are you replacing the separators in your patch? For me the realpath() removes all doubles and trims the trailing separator. It seems it might break only if you had '/' or "d:\" in alias, oh, but that is correct value too (but I guess not recommended) - fixed too.

please retest, I have used a bit different logic that detects invalid dirs too.

Ok, test passed in linux env nginx + php_fpm. To be extensive we should find someone with a windows setup and nginx (good luck).
On the actual code : it's much cleaner to test only the last char.

Thanks,

Alrighty, this has been integrated now, thanks for the amazing effort everyone!

Results of Etag test:-
-> Uploaded an image to forum.304 status is returned as expected on multiple page loads.
-> Forum attachment was set to force download.

Results of X-sendifle test:-

X-sendfile set to on
-> I got following strict standard errors all over the place( probably not related to above fix)

Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 225
Strict Standards: Redefining already defined constructor for class grade_export_txt in /var/www/int/master/moodle/grade/export/txt/grade_export_txt.php on line 31 
Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/grade/lib.php on line 219
Strict Standards: Creating default object from empty value in /var/www/int/master/moodle/mod/scorm/locallib.php on line 538 

Passing
Thanks

UPDATE tracker_issues
   SET status = 'Closed',
      comment = 'Thanks!'
WHEN participants = 'Did a gorgeous work'

This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).