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

Store extended attributes in files in stead of in extended attributes #3649

Merged
merged 39 commits into from
Feb 21, 2023

Conversation

aduffeck
Copy link
Contributor

@aduffeck aduffeck commented Feb 9, 2023

We added a new metadata backend for the decomposed storage driver that uses an additional .ini file to store file metadata. This allows scaling beyond some filesystem specific xattr limitations.

@update-docs
Copy link

update-docs bot commented Feb 9, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@aduffeck aduffeck changed the base branch from master to edge February 9, 2023 14:22
@aduffeck aduffeck force-pushed the materialized-xattrs branch 5 times, most recently from b03f553 to f4a1bbe Compare February 13, 2023 10:19
@butonic butonic marked this pull request as ready for review February 16, 2023 16:51
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

Yuck, the missing abstraction of a node causes a lot of noise and making the linter happy causes even more noise. The prefix usage is sprinkled over all decomposedfs files ... we really need to move that to the node as in 87a0d23 to truly decouple the implementation. The current xattr backend interface only ties the code further together by introducing methods like IsMetaFile(path string) bool, UsesExternalMetadataFile() bool and especially MetadataPath(path string) string.

I'm ok with merging it as is, as it introduces a new experimental backend for metadata which frees us from xattr limitations in some corner cases. But before touching this again we should refactor this mess ASAP.

@butonic butonic changed the title [WIP] Store extended attributes in files in stead of in extended attributes Store extended attributes in files in stead of in extended attributes Feb 16, 2023
@butonic butonic mentioned this pull request Feb 16, 2023
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

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

LGTM. Just one small nitpick

pkg/storage/utils/decomposedfs/tree/tree.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

LGTM 👍 But can we run a [full-ci] pipeline with this changes on ocis before merging? If you did already please ignore the comment.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Contributor

butonic commented Feb 21, 2023

LGTM 👍 But can we run a [full-ci] pipeline with this changes on ocis before merging? If you did already please ignore the comment.

running in https://drone.owncloud.com/owncloud/ocis/19330

@butonic
Copy link
Contributor

butonic commented Feb 21, 2023

ocis full-ci green, too https://drone.owncloud.com/owncloud/ocis/19330

@butonic butonic merged commit 573ae44 into cs3org:edge Feb 21, 2023
@labkode
Copy link
Member

labkode commented Feb 22, 2023

Hi, I just want to give my 2 cents on this.

Keeping a metadata file in sync with the file is referring to is very hard task. Atomicity is paramount to avoid race conditions.

  • How are you preventing two concurrent updates to the metadata of the file?
  • The use of flock is process-dependant. The storage layer dealing with files is usually one single process . How do you flock.
  • The use of flock it's has always a pain in the ass on top of NFS (what enterprises usually want). fnctl should be used in these cases.

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

Successfully merging this pull request may close these issues.

5 participants