Shares Plugin really slow when listing lots of files in folder with high php-fpm load

[slade87]

Steps to reproduce

1. Enable Shares Plugin
2. Create a folder with with more than 1000 files
3. Go into folder to list the files
4. I made a blackfire.io log with just a small folder : https://blackfire.io/profiles/5829bcdd-a0b0-443b-97bb-832bcac8c0d8/graph for analysis

Expected behaviour

Tell us what should happen
Site should show files in the folders almost immediately (maybe 1 - 2 seconds) (sql query of entire oc_filecache takes less than 0.01 seconds when testing)

Actual behaviour

Tell us what happens instead
In my case it takes from 10 seconds upwards for less than 1000 files
for more than 1000 files it takes more than 15 seconds to just display the list of files. Only the "loading" icon is showing.

Server configuration

Operating system:
Linux ubuntu 16.04
Web server:
nginx
Database:
mariadb
PHP version:
7.0.8
ownCloud version: (see ownCloud admin page)
9.0.2
Updated from an older ownCloud or fresh install:
fresh or updated same effect
Where did you install ownCloud from:
ubuntu package repository

Login as admin user into your ownCloud and access
No errors have been found.

List of activated apps:
Music, Documents, SharesPlugin, ExternalStorage, Gallery

The content of config/config.php:
"system": {
"updatechecker": false,
"instanceid": "ocqqq4thcgry",
"passwordsalt": "_REMOVED SENSITIVE VALUE",
"secret": "_REMOVED SENSITIVE VALUE",
"trusted_domains": [
"xxxxx",
"xxxxx",
"xxxxx"
],
"datadirectory": "/var/www/owncloud/data",
"overwrite.cli.url": "xxxxxxxxxx",
"dbtype": "mysql",
"version": "9.0.2.2",
"dbname": "owncloud",
"dbhost": "localhost:/var/run/mysqld/mysqld.sock",
"dbtableprefix": "oc_",
"dbuser": "_REMOVED SENSITIVE VALUE",
"dbpassword": "_REMOVED SENSITIVE VALUE",
"installed": true,
"maintenance": false,
"theme": "",
"filelocking.enabled": "true",
"memcache.local": "\OC\Memcache\APCu",
"memcache.locking": "\OC\Memcache\Redis",
"filesystem_check_changes": 0,
"cron_log": true,
"log_rotate_size": 10485760,
"debug": false,
"redis": {
"host": "/var/run/redis/redis.sock",
"port": 0,
"timeout": 0,
"dbindex": 0
},
"trashbin_retention_obligation": "auto",
"logtimezone": "UTC"

Are you using external storage, if yes which one: local/smb/sftp/...
Yes smb

Are you using encryption: yes/no
no
Are you using an external user-backend, if yes which one: LDAP/ActiveDirectory/Webdav/...
no

Blackfire.io performance log
https://blackfire.io/profiles/5829bcdd-a0b0-443b-97bb-832bcac8c0d8/graph

[slade87]

I have been able to workaround this slowdown by editing SharesPlugin.php:148
handleGetProperties()
{
$shares = [];
return new SharesTypeList( $shares );
...
}

and now listing files in folders is back to normal (about 1-2 seconds for my 2800 files folder) and sharing still seems to work without any issues?!?!
Any ideas on how to improve this?

[PVince81]

@slade87 if I understand your change, it seems you discarded the sharing information completely, so the web UI will not show the sharing indicators at all.

The share manager needs to implement a new method that leverages the database better, something like select * from oc_share where item_source in (select fileid from oc_filecache where parent = $parentFolderId) or using a join, where $parentFolderId is the fileid of the parent folder in which to retrieve the shares.

@owncloud/sharing

[PVince81]

hmm and even that might not work 100% in case of mount points, but could be used as a base to pre-retrieve the share statuses. And then a second query might be needed to make sure to also cover reshares and mount points.

[slade87]

@PVince81
Yes my change bypasses and returns immediately out of shares Plugin. However shares still seem to work without any issues for me which seems very strange. If you have any change i can help testing. Thanks alot for looking into the issue.

[PVince81]

Yes, shares will still work. The purpose of this part is only for the user experience, so that a user can see the sharing indicators on the file list.

Whenever the user opens the share panel, the share details for that file will be loaded again (indicator is only minimal info)

[PVince81]

without the share indicators a user might need to click on "Share" on every file to find out which one was actually shared

[rullzer]

I agree we need fancier methods. But keep in mind that we want to operate on an abstract level. E.g. You pass in an \OCP\Files\Folder to a function, then that should have a method to get all the id's for the children etc.

Joining is basically not done if we want to provide a modular setup.

[PVince81]

Hmm, right. But this only applies if we consider that the Folder interface can be a different implementation. I think this was relevant in the discussions about custom storages which might store the file ids somewhere else than the database.

So if we'd want to use the join anyhow, it would need to check that we're dealing with a default implementation.

But as you say, maybe the solution that asks for a list of ids on the Folder object would already improve performance.

[rullzer]

Yeah it is not ideal. But I prefer to keep the sharing stuff simple and clean. It is already a bit to cluttered in some scenarios (and needs to be cleaned up more). But I think this would already be a step in the right direction.

Also here applies I'd rather have 1 path then a check if we have the default implementation etc. Duplication will lead to bugs eventually or untested stuff.

Adding a getShareStatus function that takes a folder (and some other args) should be a good start I think

[PVince81]

@VicDeo please have a look into this. Might need adjusting the share manager / provider APIs but that's ok. Might need a new v2 interface. We'll see.

This is important for the clients to not have a performance drop when doing a PROPFIND of "oc:share-types" when syncing.

[PVince81]

@mrow4a FYI

[mrow4a]

@PVince81 Will try to rebuild it and levarage sth like
public function getSharesBy($userId, $shareType, $path = null, $reshares = false, $limit = 50, $offset = 0); with pageing instead one query per file. Probably we cannot just use the function above

https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/SharesPlugin.php#L113

[PVince81]

Sounds good. You could go even further and take an array of fileids and plug it directly into the query with a select ... where file_source in (...)

[mrow4a]

Dont spoil, I am having fun here :>

[DeepDiver1975]

Sounds good. You could go even further and take an array of fileids and plug it directly into the query with a select ... where file_source in (...)

but chunk it in blocks of 1000 - dbs have limits in IN conditions

[PVince81]

@DeepDiver1975 you heard the man, don't spoil it. 😉
I was reserving this as a surprise for later in his PR.

Since we're already there, here is an example how to chunk https://github.com/owncloud/core/blob/master/lib/private/Repair/CleanTags.php#L190

[mrow4a]

Can we decouple file_target in oc_shares into folder_target and file_target? Then we can index on folder and share owner? We dont even need IN() there and to know the files, we just need to know the parent folder, thats it. IN() on varchar might not be a too good idea because it is long (EDIT: ok, digging more into the code I see that I can do it also with ids having getDirectoryListing). We would need to limit to small batches like 10-100. Or at least provide parent_hash column (hash of parent folder path) on which we could index. Is it also viable option?

[mrow4a]

The other thing we might look at after decoupling is how we can link it. Imagine that you have 15 shares in the folder, and you move the folder. What you need to do is update 1 record instead of 15, because you just update parent varchar :>

[PVince81]

Can we decouple file_target in oc_shares into folder_target and file_target

I suggest that in the future we rather use oc_mounts for that. (together with #26190). Then we can discuss to decouple there. "file_target" in oc_share is anyway an abberation. Try the following:

1. Create three users "user1", "user2", "user3"
2. Add these users in group "group1"
3. Login as admin
4. Share a folder "test" with group "group1"
5. Login as "user1"
6. Rename "test" to "test_user1"
7. Look at oc_share

You'll see that a new special entry (share type 2) was inserted just to be able to track the specific "file_target" for "user1" due to the rename. Additionally it adds redundant info. We need to use oc_mounts instead for tracking mount point names specific to users instead.

[PVince81]

What you need to do is update 1 record instead of 15, because you just update parent varchar :>

That could happen in oc_mounts later. But now thinking of it, if we had the mount point directly in oc_filecache as suggested in #14396, the "move and change all parents" logic already exists in the oc_filecache specific logic.

[PVince81]

Fixed through #27284

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.