moodle-tool_objectfs
moodle-tool_objectfs copied to clipboard
catalyst
Allow blocking deletion of local parent directories, but efficiently
Ie reverse this:
https://github.com/catalyst/moodle-tool_objectfs/pull/399/files
The issue was that deleting a parent directory did a recursive expensive dir scan, which was wrong. It only needs to look for siblings and as soon as it finds a single sibling then it can stop and exit.
I agree that function really needs optiminsting, but... I'd be against reversing that change without some other improvements.
It would need a check against deletelocal (and not do anything if deletelocal is false) or a new setting that lets the admin turn this on/off.
I'm also not convinced it even makes sense to do a cleanup during the users session - what happens if delete of directories is slow with certain file systems? and it feels like the objectfs code might even be the wrong place to be doing this at all - maybe there should be something in core that cleans up empty directories?
Hi, I wonder if it's this problem causing a performance problem in unusual cases in our system.
We have a script that deletes a moderate number of files from moodle filesystem. Without objectfs this takes about a second. With objectfs it takes about 160 seconds. Profiling shows it's because delete_empty_dirs gets called once per deleted file (65 times)... but as a result of this, is_dir & some other functions gets called 91,175 times because of the recursive calls.
On a larger filesystem (this isn't our biggest instance, that one isn't deployed on cloud yet) it would seem like even one delete could potentially add several seconds to a request.
It doesn't seem like deleting empty dirs should be a priority, this seems like something that should happen on scheduled task or something if it's going to scan everything (rather than just delete a specific one that just became empty, say).
@sammarshallou are you on the latest master? This dir cleanup was reversed out. The implementation was clearly wrong, when you delete a file it should (if done correctly) do in the worse case 2 stats and 2 rmdir's, but most of the time it should exit early after a single stat. It should be totally safe to run blocking all the time and not need to be delegated to cron.
@brendanheywood Thanks for letting us know. I just pulled the latest master and it appears to resolve the performance issue we were experiencing.
