[PATCH v6 3/3] erofs-utils: mkfs: enable xattr name filter

[Jingbo Xu]

On 8/29/23 8:53 PM, Gao Xiang wrote:
> 
> 
> On 2023/8/29 20:41, Jingbo Xu wrote:
>> Introduce "-Exattr-name-filter" option to enable the xattr name bloom
>> filter feature.
>>
>> Signed-off-by: Jingbo Xu <jefflexu at linux.alibaba.com>
>> ---
>>   include/erofs/config.h   |  1 +
>>   include/erofs/internal.h |  1 +
>>   lib/xattr.c              | 63 ++++++++++++++++++++++++++++++++++++++++
>>   mkfs/main.c              |  7 +++++
>>   4 files changed, 72 insertions(+)
>>
>> diff --git a/include/erofs/config.h b/include/erofs/config.h
>> index 8f52d2c..c51f0cd 100644
>> --- a/include/erofs/config.h
>> +++ b/include/erofs/config.h
>> @@ -53,6 +53,7 @@ struct erofs_configure {
>>       bool c_ignore_mtime;
>>       bool c_showprogress;
>>       bool c_extra_ea_name_prefixes;
>> +    bool c_xattr_name_filter;
>>     #ifdef HAVE_LIBSELINUX
>>       struct selabel_handle *sehnd;
>> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
>> index 3e73eef..382024a 100644
>> --- a/include/erofs/internal.h
>> +++ b/include/erofs/internal.h
>> @@ -139,6 +139,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)
>>     #define EROFS_I_EA_INITED    (1 << 0)
>>   #define EROFS_I_Z_INITED    (1 << 1)
>> diff --git a/lib/xattr.c b/lib/xattr.c
>> index 46a301a..325241d 100644
>> --- a/lib/xattr.c
>> +++ b/lib/xattr.c
>> @@ -18,6 +18,7 @@
>>   #include "erofs/cache.h"
>>   #include "erofs/io.h"
>>   #include "erofs/fragments.h"
>> +#include "erofs/xxhash.h"
>>   #include "liberofs_private.h"
>>     #define EA_HASHTABLE_BITS 16
>> @@ -783,6 +784,63 @@ out:
>>       return ret;
>>   }
>>   +
>> +static int erofs_xattr_filter_hashbit(struct xattr_item *item)
>> +{
>> +    u8 prefix = item->prefix;
>> +    const char *key = item->kvbuf;
>> +    unsigned int len = item->len[0];
>> +    char *name = NULL;
>> +    uint32_t hashbit;
>> +
>> +    if (prefix & EROFS_XATTR_LONG_PREFIX) {
>> +        struct ea_type_node *tnode;
>> +        u16 prefix_len;
>> +        int ret;
>> +
>> +        list_for_each_entry(tnode, &ea_name_prefixes, list) {
>> +            if (tnode->index == item->prefix) {
>> +                ret = asprintf(&name, "%s%.*s",
>> +                           tnode->type.prefix, len, key);
>> +                if (ret < 0)
>> +                    return -ENOMEM;
>> +                break;
>> +            }
>> +        }
>> +        if (!name)
>> +            return -ENOENT;
>> +
>> +        if (!match_base_prefix(name, &prefix, &prefix_len)) {
>> +            free(name);
>> +            return -ENOENT;
>> +        }
>> +        key = name + prefix_len;
>> +        len = strlen(key);
>> +    }
>> +
>> +    hashbit = xxh32(key, len, EROFS_XATTR_FILTER_SEED + prefix) &
>> +          (EROFS_XATTR_FILTER_BITS - 1);
>> +    if (name)
>> +        free(name);
>> +    return hashbit;
>> +}
>> +
>> +static u32 erofs_xattr_filter_map(struct list_head *ixattrs)
>> +{
>> +    struct inode_xattr_node *node, *n;
>> +    u32 name_filter;
>> +    int hashbit;
>> +
>> +    name_filter = 0;
>> +    list_for_each_entry_safe(node, n, ixattrs, list) {
>> +        hashbit = erofs_xattr_filter_hashbit(node->item);
>> +        if (hashbit < 0)
> 
> I'd suggest to clear feature bit instead:
> 
> erofs_sb_clear_xattr_filter(&sbi);

Good idea.


> 
>> +            return 0;
>> +        name_filter |= (1UL << hashbit);
>> +    }
>> +    return EROFS_XATTR_FILTER_DEFAULT & ~name_filter;
>> +}
>> +
>>   char *erofs_export_xattr_ibody(struct list_head *ixattrs, unsigned
>> int size)
>>   {
>>       struct inode_xattr_node *node, *n;
>> @@ -797,6 +855,11 @@ char *erofs_export_xattr_ibody(struct list_head
>> *ixattrs, unsigned int size)
>>       header = (struct erofs_xattr_ibody_header *)buf;
>>       header->h_shared_count = 0;
>>   +    if (cfg.c_xattr_name_filter) {
>> +        u32 name_filter = erofs_xattr_filter_map(ixattrs);
> 
> Leave a blank line here.
> 
>> +        header->h_name_filter = cpu_to_le32(name_filter);
> 
> Or
>         header->h_name_filter =
>             cpu_to_le32(erofs_xattr_filter_map(ixattrs));
> 

Okay.  I would prefer the latter.

-- 
Thanks,
Jingbo