[v2,2/2] erofs: boost negative xattr lookup with bloom filter
Commit Message
The bit value for the bloom filter map has a reverse semantics for
compatibility. That is, the bit value of 0 indicates existence, while
the bit value of 1 indicates the absence of corresponding xattr.
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
fs/erofs/internal.h | 2 ++
fs/erofs/xattr.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
Comments
On Wed, Jul 5, 2023 at 9:04 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> The bit value for the bloom filter map has a reverse semantics for
> compatibility. That is, the bit value of 0 indicates existence, while
> the bit value of 1 indicates the absence of corresponding xattr.
>
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
> fs/erofs/internal.h | 2 ++
> fs/erofs/xattr.c | 12 ++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 36e32fa542f0..7e447b48a46b 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -251,6 +251,7 @@ EROFS_FEATURE_FUNCS(fragments, incompat, INCOMPAT_FRAGMENTS)
> EROFS_FEATURE_FUNCS(dedupe, incompat, INCOMPAT_DEDUPE)
> EROFS_FEATURE_FUNCS(xattr_prefixes, incompat, INCOMPAT_XATTR_PREFIXES)
> EROFS_FEATURE_FUNCS(sb_chksum, compat, COMPAT_SB_CHKSUM)
> +EROFS_FEATURE_FUNCS(xattr_filter, compat, COMPAT_XATTR_FILTER)
>
> /* atomic flag definitions */
> #define EROFS_I_EA_INITED_BIT 0
> @@ -270,6 +271,7 @@ struct erofs_inode {
> unsigned char inode_isize;
> unsigned int xattr_isize;
>
> + unsigned long xattr_name_filter;
> unsigned int xattr_shared_count;
> unsigned int *xattr_shared_xattrs;
>
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 40178b6e0688..1137723303d3 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2021-2022, Alibaba Cloud
> */
> #include <linux/security.h>
> +#include <linux/xxhash.h>
> #include "xattr.h"
>
> struct erofs_xattr_iter {
> @@ -87,6 +88,7 @@ static int erofs_init_inode_xattrs(struct inode *inode)
> }
>
> ih = it.kaddr + erofs_blkoff(sb, it.pos);
> + vi->xattr_name_filter = le32_to_cpu(ih->h_name_filter);
> vi->xattr_shared_count = ih->h_shared_count;
> vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
> sizeof(uint), GFP_KERNEL);
> @@ -392,7 +394,10 @@ int erofs_getxattr(struct inode *inode, int index, const char *name,
> void *buffer, size_t buffer_size)
> {
> int ret;
> + uint32_t bit;
> struct erofs_xattr_iter it;
> + struct erofs_inode *vi = EROFS_I(inode);
> + struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
>
> if (!name)
> return -EINVAL;
> @@ -401,6 +406,13 @@ int erofs_getxattr(struct inode *inode, int index, const char *name,
> if (ret)
> return ret;
>
> + if (erofs_sb_has_xattr_filter(sbi)) {
As I said in my other mail. I would really like this to just always do
the filter check. It should be safe as older fs:es have zero in place
here, and doing this allows me to create composefs images with the
bloom filters that also work with older kernels.
> + bit = xxh32(name, strlen(name), EROFS_XATTR_FILTER_SEED + index);
> + bit &= EROFS_XATTR_FILTER_MASK;
> + if (test_bit(bit, &vi->xattr_name_filter))
> + return -ENOATTR;
> + }
> +
> it.index = index;
> it.name = (struct qstr)QSTR_INIT(name, strlen(name));
> if (it.name.len > EROFS_NAME_LEN)
> --
> 2.19.1.6.gb485710b
>
On 2023/7/5 15:44, Alexander Larsson wrote:
> On Wed, Jul 5, 2023 at 9:04 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
...
>> + if (erofs_sb_has_xattr_filter(sbi)) {
>
> As I said in my other mail. I would really like this to just always do
> the filter check. It should be safe as older fs:es have zero in place
> here, and doing this allows me to create composefs images with the
> bloom filters that also work with older kernels.
As my previous email, this flag is on-disk compatible which means old
unsupported kernels will just ignore this and go on mounting.
But this flag indicates a new on-disk feature in the image anyway,
users could know if an image uses the new feature rather than seek to
individual inodes.
Does it sound reasonable or some other consideration?
Thanks,
Gao Xiang
@@ -251,6 +251,7 @@ EROFS_FEATURE_FUNCS(fragments, incompat, INCOMPAT_FRAGMENTS)
EROFS_FEATURE_FUNCS(dedupe, incompat, INCOMPAT_DEDUPE)
EROFS_FEATURE_FUNCS(xattr_prefixes, incompat, INCOMPAT_XATTR_PREFIXES)
EROFS_FEATURE_FUNCS(sb_chksum, compat, COMPAT_SB_CHKSUM)
+EROFS_FEATURE_FUNCS(xattr_filter, compat, COMPAT_XATTR_FILTER)
/* atomic flag definitions */
#define EROFS_I_EA_INITED_BIT 0
@@ -270,6 +271,7 @@ struct erofs_inode {
unsigned char inode_isize;
unsigned int xattr_isize;
+ unsigned long xattr_name_filter;
unsigned int xattr_shared_count;
unsigned int *xattr_shared_xattrs;
@@ -5,6 +5,7 @@
* Copyright (C) 2021-2022, Alibaba Cloud
*/
#include <linux/security.h>
+#include <linux/xxhash.h>
#include "xattr.h"
struct erofs_xattr_iter {
@@ -87,6 +88,7 @@ static int erofs_init_inode_xattrs(struct inode *inode)
}
ih = it.kaddr + erofs_blkoff(sb, it.pos);
+ vi->xattr_name_filter = le32_to_cpu(ih->h_name_filter);
vi->xattr_shared_count = ih->h_shared_count;
vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
sizeof(uint), GFP_KERNEL);
@@ -392,7 +394,10 @@ int erofs_getxattr(struct inode *inode, int index, const char *name,
void *buffer, size_t buffer_size)
{
int ret;
+ uint32_t bit;
struct erofs_xattr_iter it;
+ struct erofs_inode *vi = EROFS_I(inode);
+ struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
if (!name)
return -EINVAL;
@@ -401,6 +406,13 @@ int erofs_getxattr(struct inode *inode, int index, const char *name,
if (ret)
return ret;
+ if (erofs_sb_has_xattr_filter(sbi)) {
+ bit = xxh32(name, strlen(name), EROFS_XATTR_FILTER_SEED + index);
+ bit &= EROFS_XATTR_FILTER_MASK;
+ if (test_bit(bit, &vi->xattr_name_filter))
+ return -ENOATTR;
+ }
+
it.index = index;
it.name = (struct qstr)QSTR_INIT(name, strlen(name));
if (it.name.len > EROFS_NAME_LEN)