[Ocfs2-devel] [PATCH 07/15] ocfs2: reserve inline space for extended attribute v2

[Tiger Yang <tiger.yang@oracle.com>]

hi, Mark,
Thank you for your patient and your very detailed explanation.
I am very clear about this now.

Best regards,
tiger

Mark Fasheh wrote:
> Thanks for breaking this patch up even smaller.
> 
> On Fri, Jun 27, 2008 at 03:27:34PM +0800, Tiger Yang wrote:
>> This patch reserve some space in inode block for extended attribute.
>> That space used to store extent list or inline data.
>>
>> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
>> ---
>>  fs/ocfs2/alloc.c    |   31 +++++++++++++++++++++++--------
>>  fs/ocfs2/ocfs2.h    |    3 +++
>>  fs/ocfs2/ocfs2_fs.h |   36 ++++++++++++++++++++++++++++++++++--
>>  fs/ocfs2/super.c    |    2 ++
>>  4 files changed, 62 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>> index 3a06271..2df5a7f 100644
>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -6417,26 +6417,40 @@ out:
>>  	return ret;
>>  }
>>  
>> -static void ocfs2_zero_dinode_id2(struct inode *inode, struct ocfs2_dinode *di)
>> +static void ocfs2_zero_dinode_id2_with_xattr(struct super_block *sb,
>> +					     struct ocfs2_dinode *di,
>> +					     int xattrsize)
>>  {
>> -	unsigned int blocksize = 1 << inode->i_sb->s_blocksize_bits;
>> -
>> -	memset(&di->id2, 0, blocksize - offsetof(struct ocfs2_dinode, id2));
>> +	if (le16_to_cpu(di->i_dyn_features) & OCFS2_INLINE_XATTR_FL)
>> +		memset(&di->id2, 0, sb->s_blocksize -
>> +				    offsetof(struct ocfs2_dinode, id2) -
>> +				    xattrsize);
>> +	else
>> +		memset(&di->id2, 0, sb->s_blocksize -
>> +				    offsetof(struct ocfs2_dinode, id2));
>>  }
> 
> Wasn't the plan to also store inline xattr size on the inode, when it has
> OCFS2_INLINE_XATTR_FL set and only use s_xattr_inline_size when figuring out
> if a non-inline-xattr inode has enough space?
> 
> struct ocfs2_dinode {
> 	...
> 	__u16 i_xattr_inline_size;
> 	...
> };
> 
> 
> This way ocfs2_zero_dinode_id2_with_xattr becomes:
> 
> static void ocfs2_zero_dinode_id2_with_xattr(struct super_block *sb,
> 					     struct ocfs2_dinode *di)
> {
> 	unsigned int xattrsize = le16_to_cpu(di->i_xattr_inline_size);
> 
> 	if (le16_to_cpu(di->i_dyn_features) & OCFS2_INLINE_XATTR_FL)
> 		memset(&di->id2, 0, sb->s_blocksize -
> 				    offsetof(struct ocfs2_dinode, id2) -
> 				    xattrsize);
> 	else
> 		memset(&di->id2, 0, sb->s_blocksize -
> 				    offsetof(struct ocfs2_dinode, id2));
> }
> 
> 
> Think of this in terms of l_count on the in-inode extent list. We could have
> just stored that on the superblock, because lets face it - l_count hasn't
> changed for a particular block size, ever. But if we did that, then making
> space for inline xattrs wouldn't be as simple as just shrinking l_count
> because tons of code which iterates the extent list would assume "oh, every
> inode has exactly this many inline extents" instead of just _looking at the
> inode_ to see how many inline extents it contains. And really, I think it's
> just good practice to have our structures be self-referencial.
> 
> It's not enough to just change ocfs2_zero_dinode_id2_with_xattr() by the way
> - you need to get this correct for all functions which take inline xattr
> size into account. Luckily, your code is written well enough that this
> shouldn't be a big deal.
> 
> 
> Functions like ocfs2_xattr_has_space_inline() change too, but in a slightly
> different way. Those functions deal an inode that does _not_ have inline
> xattrs, but instead try to figure out whether they'll fit. In those cases,
> you use s_xattr_inline_size as a guideline:
> 
> 
> static int ocfs2_xattr_has_space_inline(struct inode *inode,
> 					struct ocfs2_dinode *di)
> {
> 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> 	int xattrsize = OCFS2_SB(inode->i_sb)->s_xattr_inline_size;
> 	int free;
> 
> 	if (xattrsize < OCFS2_MIN_XATTR_INLINE_SIZE)
> 		return 0;
> 
> 	if (oi->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
> 		struct ocfs2_inline_data *idata = &di->id2.i_data;
> 		free = le16_to_cpu(idata->id_count) - le64_to_cpu(di->i_size);
> 	} else {
> 		struct ocfs2_extent_list *el = &di->id2.i_list;
> 		free = (le16_to_cpu(el->l_count) -
> 			le16_to_cpu(el->l_next_free_rec)) *
> 			sizeof(struct ocfs2_extent_rec);
> 	}
> 	if (free >= xattrsize)
> 		return 1;
> 
> 	return 0;
> }
> 
> 
> And then, of course when you're *setting* inline xattrs for the first time
> (generally, this just means "when you set the INLINE_XATTR_FL"), you need to
> populate i_xattr_inline_size with something meaningful. I _think_ this
> happens in ocfs2_xattr_set_entry() - I haven't gotten to that patch yet:
> 
> 
> 	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) &&
> 	    (flag & OCFS2_INLINE_XATTR_FL)) {
> 		/*
> 		 * Adjust extent record count or inline data size
> 		 * to reserve space for extended attribute.
> 		 */
> 		if (oi->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
> 			struct ocfs2_inline_data *idata = &di->id2.i_data;
> 			le16_add_cpu(&idata->id_count, -xattrsize);
> 		} else {
> 			struct ocfs2_extent_list *el = &di->id2.i_list;
> 			le16_add_cpu(&el->l_count, -(xattrsize /
> 				     sizeof(struct ocfs2_extent_rec)));
> 		}
> 		di->i_xattr_inline_size = cpu_to_le16(osb->s_xattr_inline_size);
> 	}
> 
> 
> The test you should use, to figure out what some code should do is ask
> yourself "will this break, if the admin changes xattr_inline_size on a
> NON-empty file system?" If the answer is "no, i_xattr_inline_size on an
> inode with OCFS2_INLINE_XATTR_FL set is independent of the current value of
> s_xattr_inline_size" then you got it correct.
> 
> Let me know if any of this is unclear!
> 	--Mark
> 
> 
> --
> Mark Fasheh