1. Patchew
  2. linux
  3. View series

[PATCH v3] f2fs-tools: reduce memory alloc and copy for xattr

wangzijie posted 1 patch 1 year, 4 months ago 
fsck/dump.c  |  4 ++--
fsck/fsck.c  |  6 +++---
fsck/fsck.h  |  3 ++-
fsck/mount.c |  4 ++--
fsck/xattr.c | 25 +++++++++++++++++++++----
5 files changed, 30 insertions(+), 12 deletions(-)
[PATCH v3] f2fs-tools: reduce memory alloc and copy for xattr
Posted by wangzijie 1 year, 4 months ago 
Currently when we check xattr of inode which dosen't have xnid, we make unnecessary
memory alloc and copy. Followed by ShengYong's suggestion[1], change the behaviors of
read_all_xattrs and write_all_xattrs, and add a new function free_xattrs.

* read_all_xattrs: If xnid dosen't exist and there's no possibility to change xattr(which
   may enlarge xattr's size and leads to alloc new xnid) in next steps, 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.

After that, instances(except setxattr) where {read|write}_all_xattrs are called can avoid unnecessary
memory alloc and copy.

Use free_xattrs(xattrs) instead of free(xattrs) to free buffer properly.

[1] https://lore.kernel.org/linux-f2fs-devel/502ae396-ae82-44d6-b08d-617e9e9c4092@oppo.com/

Signed-off-by: wangzijie <wangzijie1@honor.com>
---
v2 -> v3:
 - fix read_all_xattrs logic to check and set xattr header
 - change parameter name for read_all_xattrs
 - change subject
v1 -> v2:
 - Suggestions from ShengYong to change {read|write}_all_xattrs and add free_xattrs
 - If we may need change xattr, still alloc xattr buffer in read_all_xattrs
---
---
 fsck/dump.c  |  4 ++--
 fsck/fsck.c  |  6 +++---
 fsck/fsck.h  |  3 ++-
 fsck/mount.c |  4 ++--
 fsck/xattr.c | 25 +++++++++++++++++++++----
 5 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/fsck/dump.c b/fsck/dump.c
index dc3c199..cc89909 100644
--- a/fsck/dump.c
+++ b/fsck/dump.c
@@ -399,7 +399,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
 	char xattr_name[F2FS_NAME_LEN] = {0};
 	int ret;
 
-	xattr = read_all_xattrs(sbi, node_blk, true);
+	xattr = read_all_xattrs(sbi, node_blk, true, false);
 	if (!xattr)
 		return;
 
@@ -478,7 +478,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
 		free(name);
 	}
 
-	free(xattr);
+	free_xattrs(node_blk, xattr);
 }
 #else
 static void dump_xattr(struct f2fs_sb_info *UNUSED(sbi),
diff --git a/fsck/fsck.c b/fsck/fsck.c
index aa3fb97..982defb 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -844,7 +844,7 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
 	if (xattr_size == 0)
 		return 0;
 
-	xattr = read_all_xattrs(sbi, inode, false);
+	xattr = read_all_xattrs(sbi, inode, false, false);
 	ASSERT(xattr);
 
 	last_base_addr = (void *)xattr + xattr_size;
@@ -869,10 +869,10 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
 		memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
 		write_all_xattrs(sbi, inode, xattr_size, xattr);
 		FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
-		free(xattr);
+		free_xattrs(inode, xattr);
 		return 1;
 	}
-	free(xattr);
+	free_xattrs(inode, xattr);
 	return 0;
 }
 
diff --git a/fsck/fsck.h b/fsck/fsck.h
index b581d3e..2897a5e 100644
--- a/fsck/fsck.h
+++ b/fsck/fsck.h
@@ -341,9 +341,10 @@ struct hardlink_cache_entry *f2fs_search_hardlink(struct f2fs_sb_info *sbi,
 						struct dentry *de);
 
 /* xattr.c */
-void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool);
+void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool, bool);
 void write_all_xattrs(struct f2fs_sb_info *sbi,
 		struct f2fs_node *inode, __u32 hsize, void *txattr_addr);
+void free_xattrs(struct f2fs_node *inode, void *txattr_addr);
 
 /* dir.c */
 int convert_inline_dentry(struct f2fs_sb_info *sbi, struct f2fs_node *node,
diff --git a/fsck/mount.c b/fsck/mount.c
index a189ba7..f6085e9 100644
--- a/fsck/mount.c
+++ b/fsck/mount.c
@@ -370,7 +370,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
 	DISP_u32(F2FS_INODE_NIDS(inode), i_nid[3]);	/* indirect */
 	DISP_u32(F2FS_INODE_NIDS(inode), i_nid[4]);	/* double indirect */
 
-	xattr_addr = read_all_xattrs(sbi, node, true);
+	xattr_addr = read_all_xattrs(sbi, node, true, false);
 	if (!xattr_addr)
 		goto out;
 
@@ -384,7 +384,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
 		}
 		print_xattr_entry(ent);
 	}
-	free(xattr_addr);
+	free_xattrs(node, xattr_addr);
 
 out:
 	printf("\n");
diff --git a/fsck/xattr.c b/fsck/xattr.c
index 6373c06..413cf73 100644
--- a/fsck/xattr.c
+++ b/fsck/xattr.c
@@ -18,7 +18,7 @@
 #include "xattr.h"
 
 void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
-			bool sanity_check)
+			bool sanity_check, bool for_change)
 {
 	struct f2fs_xattr_header *header;
 	void *txattr_addr;
@@ -30,6 +30,11 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
 			return NULL;
 	}
 
+	if (!xnid && !for_change) {
+		txattr_addr = inline_xattr_addr(&inode->i);
+		goto check_header;
+	}
+
 	txattr_addr = calloc(inline_size + F2FS_BLKSIZE, 1);
 	ASSERT(txattr_addr);
 
@@ -49,6 +54,7 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
 				sizeof(struct node_footer));
 	}
 
+check_header:
 	header = XATTR_HDR(txattr_addr);
 
 	/* Never been allocated xattrs */
@@ -97,7 +103,8 @@ void write_all_xattrs(struct f2fs_sb_info *sbi,
 	bool xattrblk_alloced = false;
 	struct seg_entry *se;
 
-	memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
+	if (inline_xattr_addr(&inode->i) != txattr_addr)
+		memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
 
 	if (hsize <= inline_size)
 		return;
@@ -137,6 +144,16 @@ free_xattr_node:
 	ASSERT(ret >= 0);
 }
 
+/*
+ * Different addresses between inline_xattr and txattr_addr means
+ * we newly allocate xattr buffer in read_all_xattrs, free it
+ */
+void free_xattrs(struct f2fs_node *inode, void *txattr_addr)
+{
+	if (inline_xattr_addr(&inode->i) != txattr_addr)
+		free(txattr_addr);
+}
+
 int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *name,
 		const void *value, size_t size, int flags)
 {
@@ -174,7 +191,7 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
 	ret = dev_read_block(inode, ni.blk_addr);
 	ASSERT(ret >= 0);
 
-	base_addr = read_all_xattrs(sbi, inode, true);
+	base_addr = read_all_xattrs(sbi, inode, true, true);
 	ASSERT(base_addr);
 
 	last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i);
@@ -257,8 +274,8 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
 	/* inode need update */
 	ASSERT(update_inode(sbi, inode, &ni.blk_addr) >= 0);
 exit:
+	free_xattrs(inode, base_addr);
 	free(inode);
-	free(base_addr);
 	return error;
 }
 
-- 
2.25.1
Re: [PATCH v3] f2fs-tools: reduce memory alloc and copy for xattr
Posted by Sheng Yong 1 year, 4 months ago 
On 2024/12/30 21:23, wangzijie wrote:
> Currently when we check xattr of inode which dosen't have xnid, we make unnecessary
> memory alloc and copy. Followed by ShengYong's suggestion[1], change the behaviors of
> read_all_xattrs and write_all_xattrs, and add a new function free_xattrs.
> 
> * read_all_xattrs: If xnid dosen't exist and there's no possibility to change xattr(which
>     may enlarge xattr's size and leads to alloc new xnid) in next steps, 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.
> 
> After that, instances(except setxattr) where {read|write}_all_xattrs are called can avoid unnecessary
> memory alloc and copy.
> 
> Use free_xattrs(xattrs) instead of free(xattrs) to free buffer properly.
> 
> [1] https://lore.kernel.org/linux-f2fs-devel/502ae396-ae82-44d6-b08d-617e9e9c4092@oppo.com/
> 
> Signed-off-by: wangzijie <wangzijie1@honor.com>

It looks good to me.

Reviewed-by: Sheng Yong <shengyong@oppo.com>

thanks,
shengyong
> ---
> v2 -> v3:
>   - fix read_all_xattrs logic to check and set xattr header
>   - change parameter name for read_all_xattrs
>   - change subject
> v1 -> v2:
>   - Suggestions from ShengYong to change {read|write}_all_xattrs and add free_xattrs
>   - If we may need change xattr, still alloc xattr buffer in read_all_xattrs
> ---
> ---
>   fsck/dump.c  |  4 ++--
>   fsck/fsck.c  |  6 +++---
>   fsck/fsck.h  |  3 ++-
>   fsck/mount.c |  4 ++--
>   fsck/xattr.c | 25 +++++++++++++++++++++----
>   5 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/fsck/dump.c b/fsck/dump.c
> index dc3c199..cc89909 100644
> --- a/fsck/dump.c
> +++ b/fsck/dump.c
> @@ -399,7 +399,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
>   	char xattr_name[F2FS_NAME_LEN] = {0};
>   	int ret;
>   
> -	xattr = read_all_xattrs(sbi, node_blk, true);
> +	xattr = read_all_xattrs(sbi, node_blk, true, false);
>   	if (!xattr)
>   		return;
>   
> @@ -478,7 +478,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
>   		free(name);
>   	}
>   
> -	free(xattr);
> +	free_xattrs(node_blk, xattr);
>   }
>   #else
>   static void dump_xattr(struct f2fs_sb_info *UNUSED(sbi),
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index aa3fb97..982defb 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -844,7 +844,7 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>   	if (xattr_size == 0)
>   		return 0;
>   
> -	xattr = read_all_xattrs(sbi, inode, false);
> +	xattr = read_all_xattrs(sbi, inode, false, false);
>   	ASSERT(xattr);
>   
>   	last_base_addr = (void *)xattr + xattr_size;
> @@ -869,10 +869,10 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>   		memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
>   		write_all_xattrs(sbi, inode, xattr_size, xattr);
>   		FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
> -		free(xattr);
> +		free_xattrs(inode, xattr);
>   		return 1;
>   	}
> -	free(xattr);
> +	free_xattrs(inode, xattr);
>   	return 0;
>   }
>   
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index b581d3e..2897a5e 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -341,9 +341,10 @@ struct hardlink_cache_entry *f2fs_search_hardlink(struct f2fs_sb_info *sbi,
>   						struct dentry *de);
>   
>   /* xattr.c */
> -void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool);
> +void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool, bool);
>   void write_all_xattrs(struct f2fs_sb_info *sbi,
>   		struct f2fs_node *inode, __u32 hsize, void *txattr_addr);
> +void free_xattrs(struct f2fs_node *inode, void *txattr_addr);
>   
>   /* dir.c */
>   int convert_inline_dentry(struct f2fs_sb_info *sbi, struct f2fs_node *node,
> diff --git a/fsck/mount.c b/fsck/mount.c
> index a189ba7..f6085e9 100644
> --- a/fsck/mount.c
> +++ b/fsck/mount.c
> @@ -370,7 +370,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
>   	DISP_u32(F2FS_INODE_NIDS(inode), i_nid[3]);	/* indirect */
>   	DISP_u32(F2FS_INODE_NIDS(inode), i_nid[4]);	/* double indirect */
>   
> -	xattr_addr = read_all_xattrs(sbi, node, true);
> +	xattr_addr = read_all_xattrs(sbi, node, true, false);
>   	if (!xattr_addr)
>   		goto out;
>   
> @@ -384,7 +384,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
>   		}
>   		print_xattr_entry(ent);
>   	}
> -	free(xattr_addr);
> +	free_xattrs(node, xattr_addr);
>   
>   out:
>   	printf("\n");
> diff --git a/fsck/xattr.c b/fsck/xattr.c
> index 6373c06..413cf73 100644
> --- a/fsck/xattr.c
> +++ b/fsck/xattr.c
> @@ -18,7 +18,7 @@
>   #include "xattr.h"
>   
>   void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
> -			bool sanity_check)
> +			bool sanity_check, bool for_change)
>   {
>   	struct f2fs_xattr_header *header;
>   	void *txattr_addr;
> @@ -30,6 +30,11 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
>   			return NULL;
>   	}
>   
> +	if (!xnid && !for_change) {
> +		txattr_addr = inline_xattr_addr(&inode->i);
> +		goto check_header;
> +	}
> +
>   	txattr_addr = calloc(inline_size + F2FS_BLKSIZE, 1);
>   	ASSERT(txattr_addr);
>   
> @@ -49,6 +54,7 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
>   				sizeof(struct node_footer));
>   	}
>   
> +check_header:
>   	header = XATTR_HDR(txattr_addr);
>   
>   	/* Never been allocated xattrs */
> @@ -97,7 +103,8 @@ void write_all_xattrs(struct f2fs_sb_info *sbi,
>   	bool xattrblk_alloced = false;
>   	struct seg_entry *se;
>   
> -	memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
> +	if (inline_xattr_addr(&inode->i) != txattr_addr)
> +		memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
>   
>   	if (hsize <= inline_size)
>   		return;
> @@ -137,6 +144,16 @@ free_xattr_node:
>   	ASSERT(ret >= 0);
>   }
>   
> +/*
> + * Different addresses between inline_xattr and txattr_addr means
> + * we newly allocate xattr buffer in read_all_xattrs, free it
> + */
> +void free_xattrs(struct f2fs_node *inode, void *txattr_addr)
> +{
> +	if (inline_xattr_addr(&inode->i) != txattr_addr)
> +		free(txattr_addr);
> +}
> +
>   int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *name,
>   		const void *value, size_t size, int flags)
>   {
> @@ -174,7 +191,7 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
>   	ret = dev_read_block(inode, ni.blk_addr);
>   	ASSERT(ret >= 0);
>   
> -	base_addr = read_all_xattrs(sbi, inode, true);
> +	base_addr = read_all_xattrs(sbi, inode, true, true);
>   	ASSERT(base_addr);
>   
>   	last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i);
> @@ -257,8 +274,8 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
>   	/* inode need update */
>   	ASSERT(update_inode(sbi, inode, &ni.blk_addr) >= 0);
>   exit:
> +	free_xattrs(inode, base_addr);
>   	free(inode);
> -	free(base_addr);
>   	return error;
>   }
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.