[PATCH] erofs-utils: mkfs: don't clean cached xattr items prematurely

[Gao Xiang]

On 2023/7/5 13:39, Jingbo Xu wrote:
> 
> 
> On 7/5/23 10:21 AM, Gao Xiang wrote:
>>
>>
>> On 2023/7/5 10:10, Jingbo Xu wrote:
>>> Extended attributes read from file are cached in a hash table when
>>> building shared extended attributes.  However the cached xattr items
>>> for inline xattrs are cleaned up from the hash table when the processing
>>> for shared extended attributes has finished, while later the hash table
>>> is reconstructed from scratch when building the inode tree (see
>>> erofs_mkfs_build_tree_from_path()).
>>>
>>> Don't clean up the xattr hash table halfway until mkfs exits.  Also move
>>> the logic of cleaning long xattr name prefixes into erofs_cleanxattrs().
>>>
>>> Signed-off-by: Jingbo Xu <jefflexu at linux.alibaba.com>
>>> ---
>>>    include/erofs/xattr.h |  2 +-
>>>    lib/xattr.c           | 30 ++++++++----------------------
>>>    mkfs/main.c           |  2 +-
>>>    3 files changed, 10 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/include/erofs/xattr.h b/include/erofs/xattr.h
>>> index 14fc081..b202f78 100644
>>> --- a/include/erofs/xattr.h
>>> +++ b/include/erofs/xattr.h
>>> @@ -75,9 +75,9 @@ static inline unsigned int
>>> xattrblock_offset(unsigned int xattr_id)
>>>    int erofs_prepare_xattr_ibody(struct erofs_inode *inode);
>>>    char *erofs_export_xattr_ibody(struct list_head *ixattrs, unsigned
>>> int size);
>>>    int erofs_build_shared_xattrs_from_path(const char *path);
>>> +void erofs_cleanxattrs(void);
>>>      int erofs_xattr_insert_name_prefix(const char *prefix);
>>> -void erofs_xattr_cleanup_name_prefixes(void);
>>>    int erofs_xattr_write_name_prefixes(FILE *f);
>>>      #ifdef __cplusplus
>>> diff --git a/lib/xattr.c b/lib/xattr.c
>>> index 7d7dc54..8d0079f 100644
>>> --- a/lib/xattr.c
>>> +++ b/lib/xattr.c
>>> @@ -547,24 +547,23 @@ fail:
>>>        return ret;
>>>    }
>>>    -static void erofs_cleanxattrs(bool sharedxattrs)
>>> +void erofs_cleanxattrs(void)
>>>    {
>>>        unsigned int i;
>>>        struct xattr_item *item;
>>>        struct hlist_node *tmp;
>>> +    struct ea_type_node *tnode, *n;
>>>          hash_for_each_safe(ea_hashtable, i, tmp, item, node) {
>>> -        if (sharedxattrs && item->shared_xattr_id >= 0)
>>> -            continue;
>>
>> I'm not sure it's the expected behavior. Previously we will remove
>> all non-shared xattrs which are below xattr threshold in the shared
>> xattr generation process.
> 
> What's the purpose of removing these non-shared xattrs while leaving
> shared xattrs there?

shared xattrs will be used for later tree walking. non shared xattrs
are not.

> 
>>
>> But now, such non-shared xattrs will be left in memory.  What
>> the benefits of this?  In addition, I'm not sure if we need to add
>> non-shared (inline) xattrs to hash table as well.
> 
> Later erofs_mkfs_build_tree_from_path() needs to reconstruct these
> non-shared xattrs, which have been previously removed.
> 
> erofs_mkfs_build_tree_from_path
>    erofs_mkfs_build_tree
>      erofs_prepare_xattr_ibody
>        read_xattrs_from_file

That is true, but in principle these are seperate two stuffs:

  - erofs_build_shared_xattrs_from_path  generates shared xattrs
                                         from a tree.
  - erofs_mkfs_build_tree_from_path generate trees with the
                                    previous shared xattrs.

These two paths can even be different if we'd like to build
a wide shared xattr array.

For example, you could build shared xattrs from "/usr" but only
pack "/usr/lib" then.

Thanks,
Gao Xiang

> 
>