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

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

[wangzijie]

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@honor.com>
---
 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)
+		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;
 }
 
-- 
2.25.1

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@honor.com>
> ---
>   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;
>   }
>   

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

[wangzijie]

>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@honor.com>
>> ---
>>   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

Hi, yong
Thanks for sharing what you proposed to do. By the way, I noticed that 
when setattr, read_all_xattr will set xattr header's magic and refcount,
but it seems we don't check these values in header(kernel/fsck). Should we 
add check logic for it?

>> +		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;
>>   }
>>   
>
>
>
>_______________________________________________
>Linux-f2fs-devel mailing list
>Linux-f2fs-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Sheng Yong]

On 2024/12/5 11:40, wangzijie wrote:
>> On 2024/12/4 20:23, wangzijie wrote:
> 
[...]
> 
> Hi, yong
> 
> Thanks for sharing what you proposed to do. By the way, I noticed that
> when setattr, read_all_xattr will set xattr header's magic and refcount,
> but it seems we don't check these values in header(kernel/fsck). Should we
> add check logic for it?
> 
Right, but magic number and refcount are not really used, I think we could
leave it as it is.

thanks,
shengyong
> 
[...]
> 
>