ext4: don't split xattr inode refcounts across i_ctime and i_version fields

From: Jeff Layton <[email protected]>

This patch is based on top of the i_version rework that I'm flogging upstream. It's just a cleanup of some nastiness I noticed while in there.

This code uses the i_ctime and i_version fields to each store half of a refcount. I suspect it was done this way long ago when the i_version

field was a 32 bits, and was never changed when the field was converted to a 64 bit value.

Change the code to just store the refcount in the i_version field rather than splitting it across both fields.

Cc David Howells <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/ext4/xattr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/ext4/xattr.c +2 -4

index 63656dbafdc4..6ea78dd367ca 100644
@@ -294,14 +294,12 @@ ext4_xattr_inode_hash(struct ext4_sb_info *sbi, const void *buffer, size_t size)
 
 static u64 ext4_xattr_inode_get_ref(struct inode *ea_inode)
 {
-	return ((u64)ea_inode->i_ctime.tv_sec << 32) |
-		(u32) inode_peek_iversion_raw(ea_inode);
+	return inode_peek_iversion_raw(ea_inode);
 }
 
 static void ext4_xattr_inode_set_ref(struct inode *ea_inode, u64 ref_count)
 {
-	ea_inode->i_ctime.tv_sec = (u32)(ref_count >> 32);
-	inode_set_iversion_raw(ea_inode, ref_count & 0xffffffff);
+	inode_set_iversion_raw(ea_inode, ref_count);
 }
 
 static u32 ext4_xattr_inode_get_hash(struct inode *ea_inode)

Signature · 1 line

2.14.3

Quoted 7 lines · Jeff Layton

From: Jeff Layton <[email protected]>

This patch is based on top of the i_version rework that I'm flogging upstream. It's just a cleanup of some nastiness I noticed while in there.

This code uses the i_ctime and i_version fields to each store half of a refcount. I suspect it was done this way long ago when the i_version

Way long ago == 2017-06-22 :)

The new 64k xattr value feature in ext4 uses hidden inodes to store attr values that don't fit in a single block. Since the inode can be shared by multiple xattr keys and isn't exported via NFS (I hope...), they use a 64-bit refcount mashed into i_ctime and i_version. That's what this gobbledygook is for.

https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#Large_Extended_Attribute_Values

--D

Quoted 36 lines

field was a 32 bits, and was never changed when the field was converted to a 64 bit value.

Change the code to just store the refcount in the i_version field rather than splitting it across both fields.

Cc David Howells <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/ext4/xattr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/ext4/xattr.c +2 -4

index 63656dbafdc4..6ea78dd367ca 100644
@@ -294,14 +294,12 @@ ext4_xattr_inode_hash(struct ext4_sb_info *sbi, const void *buffer, size_t size)
 
 static u64 ext4_xattr_inode_get_ref(struct inode *ea_inode)
 {
-	return ((u64)ea_inode->i_ctime.tv_sec << 32) |
-		(u32) inode_peek_iversion_raw(ea_inode);
+	return inode_peek_iversion_raw(ea_inode);
 }
 
 static void ext4_xattr_inode_set_ref(struct inode *ea_inode, u64 ref_count)
 {
-	ea_inode->i_ctime.tv_sec = (u32)(ref_count >> 32);
-	inode_set_iversion_raw(ea_inode, ref_count & 0xffffffff);
+	inode_set_iversion_raw(ea_inode, ref_count);
 }
 
 static u32 ext4_xattr_inode_get_hash(struct inode *ea_inode)

Signature · 1 line

2.14.3

Ahh, many thanks...

My main question was: why split the refcount across fields like this? If it's necessary now for backward compatibility then so be it, but it's weird and not 100% clear why it's being done that way.

Quoted 6 lines · Jeff Layton

Ahh, many thanks...

My main question was: why split the refcount across fields like this? If it's necessary now for backward compatibility then so be it, but it's weird and not 100% clear why it's being done that way.

The main reason is that the only inode that will need it is the hidden extended attribute inode (and then only for Samba servers that are supporting enterprise domain CIFS servers where there are more than 64k files using the same windows ACL). So we didn't want to use extra bytes in the inode, since it's only going to be used in a very tiny fraction of servers.

For the right workload, though, this should allow ext4 to have significantly better performance, since if you are serving a large directory where all of the files have their own ACL or Windows security ID xattrs, without shared extended attributes, when you open a Files explorer on that directory, for each file the file system will be forced to do lots of random 4k reads to fetch the xattrs.

Wouldn't it have made more sense to just store it all in i_version field (like this patch makes it do)? If this breaks backward compatibility though, then I'm fine with just dropping the patch.