-
Notifications
You must be signed in to change notification settings - Fork 35
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
Erofs bloom filter #176
Erofs bloom filter #176
Conversation
Signed-off-by: Alexander Larsson <[email protected]>
This is a copy of the upstream bsd-2 licences code which is also used by the kernel. Signed-off-by: Alexander Larsson <[email protected]>
9769a76
to
7031321
Compare
NOTE: This is a format-breaking change, so its also updates the test checksums. We should merge this before freezing the format. |
struct erofs_inode_chunk_info { | ||
__le16 format; /* chunk blkbits, etc. */ | ||
__le16 reserved; | ||
}; | ||
|
||
union erofs_inode_i_u { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like some of this could be split out as a prep patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. This is a prep patch. All it does is copy the latest version of this file from the kernel tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, actually, it does tweak the code to work with some new naming in the new file, but its nice to merge those to have each commit build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I was reviewing in the "files changed" UI which squashes everything. Let's just do the two prep patches as a distinct PR that can merge right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes were proposed upstream today, so I think we can merge the whole thing soon.
Sorry for a bit late reply since I'm busy in the last test for commits of the next cycle. I think the implementation won't be changed, but let's wait until the kernel side merged. |
This matches: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git/commit/?h=dev&id=fd73a4395d477ae134f319f7368a9f8a6264fd8b We set the COMPAT_XATTR_FILTER flag on the superblock, and we add to each xattr header a 32bit bloom filter which defaults to all bits set, and then we compute a hash for each key modulo 32, and unset that bit in the bloom filter. Older kernels will ignore this, but newer kernels will use it to make xattr lookup more efficient. NOTE: This is a format-breaking change, so we also update the test checksums. We should merge this before freezing the format. Signed-off-by: Alexander Larsson <[email protected]>
7031321
to
2993e34
Compare
Added the link to the kernel source of xxh32() |
upstreaming is here: https://lists.ozlabs.org/pipermail/linux-erofs/2023-August/009427.html |
it already lands upstream: https://git.kernel.org/torvalds/c/fd73a4395d47 |
The implementation looks good to me. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds the COMPAT_XATTR_FILTER feature flag, and computes h_name_filter in all the inode xattr header.
Fixes #167
This is based on and tested against the code in the dev branch of erofs.git, so this is not upstream quite yet. However, this likely will go in unchanged.
@hsiangkao wanna have a quick look?