Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Refactor shared storage #12086

Closed
wants to merge 21 commits into from
Closed

Conversation

icewind1991
Copy link
Contributor

Since the removal of the Shared folder in oc7 the logic of the shared storage and cache can be greatly simplified.

This replaces the shared storage with a combination of 2 generic storage and cache wrappers

  • \OC\Files\Storage\Wrapper\Jail: limits all access of the storage to a specific folder
  • \OC\Files\Storage\Wrapper\PermissionsMask: applies a permissions mask on top of a storage

Together they form 99% of the logic behind a share (a sub directory of storage with limited permissions) and also allow dropping the ReadOnlyWrapper previously in place.

Overall this drastically reduces the amount of logic for shared storages which results in a nice performance increase.
On my local test instance this reduces the time needed for a WebDAV upload to a shared folder with ~120ms and drops the number of database queries by 103

TODO:

  • Adjust unit tests
  • Mount any storage inside a shared folder for the share recipient

cc @DeepDiver1975 @PVince81 @schiesbn

@PVince81
Copy link
Contributor

Wow, incredible.

Not sure if Jail is the right name as it still feels a bit confusing, but I understand your intention.

@PVince81 PVince81 added this to the 2014-sprint-07-current milestone Nov 10, 2014
@karlitschek
Copy link
Contributor

Awesome!!!

@schiessle
Copy link
Contributor

This sounds like a awesome improvement!

I just did a short test, re-sharing seems to be broken, I get "Sharing welcome (2).txt failed, because the file does not exist". Another interesting effect: I have a folder-re-share in my share table from before I switched to your branch. If I navigate into the re-shared folder I get the content of the data/ folder.

I'm sure you will see this issues while working on the failing unit test... Just wanted to write down what I noticed so that it doesn't get lost.

@craigpg craigpg modified the milestones: 2014-sprint-08-current, 2014-sprint-07 Nov 10, 2014
@icewind1991 icewind1991 force-pushed the shared-storage-use-wrappers branch 2 times, most recently from f690fd3 to 9e6b8e3 Compare November 13, 2014 13:14
@DeepDiver1975
Copy link
Member

bloody jenkins plugin - @icewind1991 please rebase - thx

@icewind1991
Copy link
Contributor Author

I cant reproduce any of the failing tests locally...

@icewind1991 icewind1991 force-pushed the shared-storage-use-wrappers branch 2 times, most recently from 694918a to ee12a38 Compare December 9, 2014 09:16
@icewind1991
Copy link
Contributor Author

Mount points inside shared folders are now correctly moved over

@icewind1991 icewind1991 force-pushed the shared-storage-use-wrappers branch from 318c958 to 9bef2a9 Compare December 9, 2014 16:30
@icewind1991 icewind1991 force-pushed the shared-storage-use-wrappers branch from 9bef2a9 to 9a6e2f1 Compare December 10, 2014 08:36
@icewind1991 icewind1991 changed the title [WIP] Refactor shared storage Refactor shared storage Dec 10, 2014
@icewind1991 icewind1991 force-pushed the shared-storage-use-wrappers branch from 9a6e2f1 to 50185c3 Compare December 10, 2014 09:14
@PVince81
Copy link
Contributor

  • BUG: (with files_locking + encryption enabled) move file into subdir as non-owner
  1. User1 shares non-empty dir "test" to user2
  2. User2 creates "test/subdir"
  3. User2 moves a file "test/test.txt" to "test/subdir"

Result: "Could not obtain lock type 2" (locking app).
Maybe some file is opened twice in a row.

@PVince81
Copy link
Contributor

  • BUG: move a shared folder into another must show an error message (worked on master). On this branch the ajax call doesn't return an error, but the folder also isn't physicially moved even though the UI shows so

@PVince81
Copy link
Contributor

  • BUG: WebDAV move between two shared folders gives 500 internal server error
  1. user1 shares folder "abc" with user3
  2. user2 shares folder "def" with user3
  3. user3 moves file "def/test.txt" to "abc/test.txt" with WebDAV

@jospoortvliet
Copy link

We're past feature freeze, right, this should target 8.2?

@DeepDiver1975 DeepDiver1975 modified the milestones: 8.2-next, 8.1-current Apr 9, 2015
@jospoortvliet
Copy link

Darth Vader is VERY angry ;-)

@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2015

@icewind1991 reminder to try and have this early in 8.2 😄

@PVince81
Copy link
Contributor

PVince81 commented Aug 4, 2015

@icewind1991 another reminder to continue this so we can have early in 8.2

@MorrisJobke
Copy link
Contributor

@icewind1991 another reminder to continue this so we can have early in 8.2

Ping? Defer to 9.0?

@icewind1991
Copy link
Contributor Author

Defer

@icewind1991 icewind1991 modified the milestones: 9.0-next, 8.2-current Sep 1, 2015
@MorrisJobke
Copy link
Contributor

@icewind1991 Can we get this in? @rullzer is maybe also happy about this ;)

@icewind1991
Copy link
Contributor Author

Waiting for @rullzer sharing work before continuing with this

@MorrisJobke
Copy link
Contributor

Waiting for @rullzer sharing work before continuing with this

Ah okay. If this is the better approach I'm fine with it :)

@jospoortvliet
Copy link

Isn't the sharing work by @rullzer in? The basics, at least...

@rullzer
Copy link
Contributor

rullzer commented Jan 6, 2016

@jospoortvliet baby steps ;)

@PVince81
Copy link
Contributor

Moving this major refactoring to 9.1, if it still makes sense @icewind1991 ?

CC @cmonteroluque

@PVince81 PVince81 modified the milestones: 9.1-next, 9.0-current Feb 11, 2016
@rullzer rullzer mentioned this pull request Mar 31, 2016
19 tasks
@rullzer
Copy link
Contributor

rullzer commented Mar 31, 2016

Closing this for now.
But keeping a reminding in #22209

@rullzer rullzer closed this Mar 31, 2016
@rullzer rullzer deleted the shared-storage-use-wrappers branch March 31, 2016 17:42
@lock
Copy link

lock bot commented Aug 6, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants