[RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Jeff Layton]

From: Jeff Layton <jlayton@redhat.com>

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 <dhowells@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ext4/xattr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 63656dbafdc4..6ea78dd367ca 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -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)
-- 
2.14.3

Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Darrick J. Wong]

On Tue, Jan 09, 2018 at 09:47:01AM -0500, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
> 
> 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

> 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 <dhowells@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/ext4/xattr.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 63656dbafdc4..6ea78dd367ca 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -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)
> -- 
> 2.14.3
> 

Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Jeff Layton]

On Tue, 2018-01-09 at 10:52 -0800, Darrick J. Wong wrote:
> On Tue, Jan 09, 2018 at 09:47:01AM -0500, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > 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 :)
> 

(cc'ing Tahsin)

Hah, ok. I was thinking this was legacy code, but I guess not! I should
have probably done some git archaeology first.

> 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
> 

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.


> > 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 <dhowells@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/ext4/xattr.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> > index 63656dbafdc4..6ea78dd367ca 100644
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -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)
> > -- 
> > 2.14.3
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Theodore Ts'o]

On Tue, Jan 09, 2018 at 02:05:31PM -0500, Jeff Layton wrote:
>
> 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.

   	     	     	       	  	- Ted

Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Jeff Layton]

On Thu, 2018-01-11 at 13:39 -0500, Theodore Ts'o wrote:
> On Tue, Jan 09, 2018 at 02:05:31PM -0500, Jeff Layton wrote:
> > 
> > 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.
> 

Oh, sorry...I may not have been clear.

I wasn't really asking about the need for xattr inodes. My question was
more about why it was decided to split the refcount in two, and store
part of it in the ctime and part in the i_version field.

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.

Cheers,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] ext4: don't split xattr inode refcounts across i_ctime and i_version fields

[Theodore Ts'o]

On Thu, Jan 11, 2018 at 03:09:03PM -0500, Jeff Layton wrote:
> 
> 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.

Yes, that part is there for compatibility for the deployed base
(Lustre Servers).

					- Ted