[Ocfs2-devel] [PATCH 07/15] ocfs2: reserve inline space for extended attribute v2
Tiger Yang
tiger.yang at oracle.com
Fri Jul 11 01:55:55 PDT 2008
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 at 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
More information about the Ocfs2-devel
mailing list