Re: [f2fs-dev] f2fs-tools: Check and fix inline xattr inplace

[Sheng Yong]

On 2024/12/4 20:23, wangzijie wrote:
> When we check inode which just has inline xattr data, we copy
> inline xattr data from inode, check it(maybe fix it) and copy
> it again to inode. We can check and fix xattr inplace for this
> kind of inode to reduce memcpy times.
>
> Signed-off-by: wangzijie <wangzijie1@xxxxxxxxx>
> ---
>   fsck/fsck.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index aa3fb97..fd8b082 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -840,11 +840,18 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>   	struct f2fs_xattr_entry *ent;
>   	__u32 xattr_size = XATTR_SIZE(&inode->i);
>   	bool need_fix = false;
> +	bool has_xattr_node = false;
> +	nid_t xnid = le32_to_cpu(inode->i.i_xattr_nid);
>   
>   	if (xattr_size == 0)
>   		return 0;
>   
> -	xattr = read_all_xattrs(sbi, inode, false);
> +	if (xattr_size <= inline_xattr_size(&inode->i) && !xnid)
Hi, zijie,

I propose to change the behavors of read_all_xattrs and write_all_xattrs, and to add a
new free_xattrs.

* read_all_xattrs checks whether xnid exist. If it's not, return inline_xattr directly
  without alloc and memcpy.
* write_all_xattrs checks whether inline_xattr and new txattr_addr have the same address
  to avoid copying back.
* free_xattrs checks whether inline_xattr and new txattr_addr have the same address to
  free xattr buffer properly.

In this way, all instances where {read|write}_all_xattrs are called can avoid unnecessary
memory alloc and copy. free_xattrs(xattrs) should be used instead of free(xattrs).

thanks,
shengyong

> +		xattr = inline_xattr_addr(&inode->i);
> +	else {
> +		xattr = read_all_xattrs(sbi, inode, false);
> +		has_xattr_node = true;
> +	}
>   	ASSERT(xattr);
>   
>   	last_base_addr = (void *)xattr + xattr_size;
> @@ -867,12 +874,15 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>   	}
>   	if (need_fix && c.fix_on) {
>   		memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
> -		write_all_xattrs(sbi, inode, xattr_size, xattr);
> +		if (has_xattr_node) {
> +			write_all_xattrs(sbi, inode, xattr_size, xattr);
> +			free(xattr);
> +		}
>   		FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
> -		free(xattr);
>   		return 1;
>   	}
> -	free(xattr);
> +	if (has_xattr_node)
> +		free(xattr);
>   	return 0;
>   }
>