lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Mon, 30 Oct 2023 06:49:32 -0700
From:   JunChao Sun <sunjunchao2870@...il.com>
To:     linux-ext4@...r.kernel.org
Cc:     tytso@....edu, adilger@...ger.ca,
        JunChao Sun <sunjunchao2870@...il.com>
Subject: [PATCH v3] ext4: Replace value of xattrs in place

When replacing the value of an xattr found in an ea_inode, currently
ext4 will evict the ea_inode that stores the old value, recreate an
ea_inode, and then write the new value into the new ea_inode.
This can be optimized by writing the new value into the old
ea_inode directly.

The logic for replacing value of xattrs without this patch
is as follows:
ext4_xattr_set_entry()
    ->ext4_xattr_inode_iget(&old_ea_inode)
    ->ext4_xattr_inode_lookup_create(&new_ea_inode)
    ->ext4_xattr_inode_dec_ref(old_ea_inode)
    ->iput(old_ea_inode)
        ->ext4_destroy_inode()
        ->ext4_evict_inode()
        ->ext4_free_inode()
    ->iput(new_ea_inode)

The logic with this patch is:
ext4_xattr_set_entry()
    ->ext4_xattr_inode_iget(&old_ea_inode)
    ->ext4_xattr_inode_write(old_ea_inode, new_value)
    ->iput(old_ea_inode)

This patch reduces the time it takes to replace xattrs in the ext4.
Without this patch, replacing the value of an xattr two million times takes
about 45 seconds on Intel(R) Xeon(R) CPU E5-2620 v3 platform.
With this patch, the same operation takes only 6 seconds.

  [root@...ent01 sjc]# ./mount.sh
  /dev/sdb1 contains a ext4 file system
      last mounted on /mnt/ext4 on Mon May  8 17:05:38 2023
  [root@...ent01 sjc]# touch /mnt/ext4/file1
  [root@...ent01 sjc]# gcc test.c
  [root@...ent01 sjc]# time ./a.out

  real    0m45.248s
  user    0m0.513s
  sys 0m39.231s

  [root@...ent01 sjc]# ./mount.sh
  /dev/sdb1 contains a ext4 file system
      last mounted on /mnt/ext4 on Mon May  8 17:08:20 2023
  [root@...ent01 sjc]# touch /mnt/ext4/file1
  [root@...ent01 sjc]# time ./a.out

  real    0m5.977s
  user    0m0.316s
  sys 0m5.659s

The test.c and mount.sh are in [1].
This patch passed the tests with xfstests 'check -g quick'.

[1] https://gist.github.com/sjc2870/c923d7fa627d10ab65d6c305afb02cdb

Signed-off-by: JunChao Sun <sunjunchao2870@...il.com>
---

Change in v3:
  - Fix resources leakage when xattr size is shrinking
  - Link to v2: https://lore.kernel.org/linux-ext4/20230510001409.14522-1-sunjunchao2870@gmail.com/

Change in v2:
  - Fix a problem when ref of an ea_inode not equal to 1
  - Link to v1: https://lore.kernel.org/linux-ext4/20230509011042.11781-1-sunjunchao2870@gmail.com/

 fs/ext4/xattr.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 92ba28cebac6..ed5c34352e40 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1695,6 +1695,59 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 		}
 	}
 
+	if (!s->not_found && i->value && here->e_value_inum && i->in_inode) {
+		/* Replace xattr value in ea_inode in place */
+		int size_diff = i->value_len - le32_to_cpu(here->e_value_size);
+		u32 hash;
+		int err = 0;
+
+		ret = ext4_xattr_inode_iget(inode,
+						le32_to_cpu(here->e_value_inum),
+						le32_to_cpu(here->e_hash),
+						&old_ea_inode);
+		if (ret) {
+			old_ea_inode = NULL;
+			goto out;
+		}
+		if (ext4_xattr_inode_get_ref(old_ea_inode) == 1) {
+			if (size_diff > 0)
+				ret = ext4_xattr_inode_alloc_quota(inode, size_diff);
+			else if (size_diff < 0)
+				ext4_xattr_inode_free_quota(inode, NULL, -size_diff);
+			if (ret)
+				goto out;
+
+			ret = ext4_xattr_inode_write(handle, old_ea_inode, i->value, i->value_len);
+			if (ret) {
+				if (size_diff > 0)
+					ext4_xattr_inode_free_quota(inode, NULL, size_diff);
+				else if (size_diff < 0)
+					ret = ext4_xattr_inode_alloc_quota(inode, -size_diff);
+				goto out;
+			}
+			here->e_value_size = cpu_to_le32(i->value_len);
+			new_ea_inode = old_ea_inode;
+			old_ea_inode = NULL;
+			hash = ext4_xattr_inode_hash(EXT4_SB(inode->i_sb), i->value, i->value_len);
+			ext4_xattr_inode_set_hash(new_ea_inode, hash);
+			if (size_diff < 0) {
+				inode_lock(new_ea_inode);
+				ext4_truncate(new_ea_inode);
+				inode_unlock(new_ea_inode);
+				/*
+				 * ext4_truncate() changed c_time, so ref_count which depends on c_time was changed also.
+				 * Let's restore it.
+				 */
+				ext4_xattr_inode_set_ref(new_ea_inode, 1);
+				err = ext4_mark_inode_dirty(handle, new_ea_inode);
+				if (err)
+					EXT4_ERROR_INODE(new_ea_inode, "mark ea inode dirty(error %d)", err);
+			}
+			goto update_hash;
+		} else
+			iput(old_ea_inode);
+	}
+
 	/*
 	 * Getting access to old and new ea inodes is subject to failures.
 	 * Finish that work before doing any modifications to the xattr data.
-- 
2.34.1

Powered by blists - more mailing lists