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

allow storing multiple mounts for the same rootid in the mount cache #32877

Merged
merged 2 commits into from
May 2, 2023

Conversation

icewind1991
Copy link
Member

currently [$userId, $rootId] is used as the unique key for storing mounts in the mount cache,
however there are cases where the same rootid is mounted in multiple places for a user which currently leads to not all of those mounts being added to the cache.

Previously this didn't matter as the mount cache was only used to list users with access to a specific file, so a user having access to the file multiple times didn' change anything.

With 24 the mount cache is used for more cases and multiple mounts for the same id becomes relevant.
While I think there isn't a real negative effect atm besides missing the optimized path we should ensure that the mounts are properly listed

Signed-off-by: Robin Appelman robin@icewind.nl

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Jun 14, 2022
@icewind1991 icewind1991 added this to the Nextcloud 25 milestone Jun 14, 2022
@icewind1991 icewind1991 requested review from a team, PVince81, ArtificialOwl and come-nc and removed request for a team June 14, 2022 13:59
@icewind1991
Copy link
Member Author

Motivation for fixing this is the virtualfolder app which makes it easy to have the same rootid mounted multiple times

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the tests on files_sharing are failing with:
OCP\Share\Exceptions\GenericShareException: Could not get proper share mount for 1228. Failing since else the next calls are called with null

@come-nc
Copy link
Contributor

come-nc commented Jun 28, 2022

The autoloaders are also not up to date.

@PVince81
Copy link
Member

@icewind1991 can you clean up the PR ?

This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
@icewind1991 icewind1991 force-pushed the mount-cache-unique-mountpoint branch 2 times, most recently from 594a7ee to f15414c Compare August 31, 2022 14:23
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@icewind1991 icewind1991 force-pushed the mount-cache-unique-mountpoint branch 3 times, most recently from df47c4a to 05b40a3 Compare April 5, 2023 12:28
@icewind1991 icewind1991 force-pushed the mount-cache-unique-mountpoint branch from 05b40a3 to 5896bdb Compare April 13, 2023 15:19
@icewind1991
Copy link
Member Author

switched adding the new index to "add missing indexes" instead of the db migration to prevent slow db migration.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

currently `[$userId, $rootId]` is used as the unique key for storing mounts in the mount cache,
however there are cases where the same rootid is mounted in multiple places for a user which currently leads to not all of those mounts being added to the cache.

Previously this didn't matter as the mount cache was only used to list users with access to a specific file, so a user having access to the file multiple times didn' change anything.

With 24 the mount cache is used for more cases and multiple mounts for the same id becomes relevant.
While I think there isn't a real negative effect atm besides missing the optimized path we should ensure that the mounts are properly listed

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the mount-cache-unique-mountpoint branch from 5896bdb to db43d90 Compare April 28, 2023 13:08
@icewind1991 icewind1991 merged commit 8f1e711 into master May 2, 2023
@icewind1991 icewind1991 deleted the mount-cache-unique-mountpoint branch May 2, 2023 13:40
@icewind1991
Copy link
Member Author

/backport to stable26

@icewind1991
Copy link
Member Author

/backport to stable25

@szaimen
Copy link
Contributor

szaimen commented May 2, 2023

Does this fix #37473 ?

@icewind1991
Copy link
Member Author

I don't think so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants