:p
atchew
Login
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 XXXXXXX..XXXXXXX 100644 --- a/fsck/fsck.c +++ b/fsck/fsck.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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
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 alloc 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> --- v1: https://lore.kernel.org/linux-f2fs-devel/20241204122317.3042137-1-wangzijie1@honor.com/ change since v1: - Suggestions from ShengYong to change {read|write}_all_xattrs and add free_xattrs - If we may need alloc xnid, 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 | 22 ++++++++++++++++++---- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/fsck/dump.c b/fsck/dump.c index XXXXXXX..XXXXXXX 100644 --- a/fsck/dump.c +++ b/fsck/dump.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/fsck/fsck.c +++ b/fsck/fsck.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/fsck/fsck.h +++ b/fsck/fsck.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/fsck/mount.c +++ b/fsck/mount.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/fsck/xattr.c +++ b/fsck/xattr.c @@ -XXX,XX +XXX,XX @@ #include "xattr.h" void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode, - bool sanity_check) + bool sanity_check, bool may_alloc_xnid) { struct f2fs_xattr_header *header; void *txattr_addr; @@ -XXX,XX +XXX,XX @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode, return NULL; } + if (!xnid && !may_alloc_xnid) + return inline_xattr_addr(&inode->i); + txattr_addr = calloc(inline_size + F2FS_BLKSIZE, 1); ASSERT(txattr_addr); @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ free_xattr_node: ASSERT(ret >= 0); } +/* + * Different address 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) { @@ -XXX,XX +XXX,XX @@ 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); @@ -XXX,XX +XXX,XX @@ 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