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

[Jeff Layton <jlayton@kernel.org>]

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>