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

[Sheng Yong via Linux-f2fs-devel]

On 2024/12/10 17:34, wangzijie wrote:
[...]

diff --git a/fsck/xattr.c b/fsck/xattr.c
index 6373c06..3abdcd8 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 may_alloc_xnid)
  {
        struct f2fs_xattr_header *header;
        void *txattr_addr;
@@ -30,6 +30,9 @@ 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);

Hi, zijie,

It cannot return directly, because the header should be checked and set at
the end of this function.

+
        txattr_addr = calloc(inline_size + F2FS_BLKSIZE, 1);
        ASSERT(txattr_addr);

@@ -97,7 +100,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 +141,16 @@ 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)
  {
@@ -174,7 +188,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);

Oops, this is a special case. My bad for not thinking it through earlier.
Personally, I am not a fan of adding a new `may_alloc_nid' parameter (btw,
I think something like `for_create' would make more sense), but I cannot
find an appropriate way to get rid of it :-(
Otherwise, it looks good to me.

thanks,
shengyong

        ASSERT(base_addr);

last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i);

@@ -257,8 +271,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;
  }

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel