[Ocfs2-devel] [PATCH V2] ocfs2: xattr: fix inlined xattr reflink

[Junxiao Bi]

Hi Andrew,

On 06/28/2013 07:08 AM, Andrew Morton wrote:
> On Fri, 21 Jun 2013 09:27:24 +0800 Junxiao Bi <junxiao.bi at oracle.com> wrote:
>
>> Inlined xattr shared free space of inode block with inlined data
>> or data extent record, so the size of the later two should be
>> adjusted when inlined xattr is enabled. See ocfs2_xattr_ibody_init().
>> But this isn't done well when reflink. For inode with inlined data,
>> its max inlined data size is adjusted in ocfs2_duplicate_inline_data(),
>> no problem. But for inode with data extent record, its record count isn't
>> adjusted. Fix it, or data extent record and inlined xattr may overwrite
>> each other, then cause data corruption or xattr failure.
>>
>> One panic caused by this bug in our test environment is the following:
>>
>> kernel BUG at fs/ocfs2/xattr.c:1435!
>>
>> ...
>>
>> @@ -6499,6 +6499,16 @@ static int ocfs2_reflink_xattr_inline(struct ocfs2_xattr_reflink *args)
>>  	}
>>  
>>  	new_oi = OCFS2_I(args->new_inode);
>> +	/*
>> +	 * Adjust extent record count to reserve space for extended attribute.
>> +	 * Inline data count had been adjusted in ocfs2_duplicate_inline_data().
>> +	 */
>> +	if (!(new_oi->ip_dyn_features & OCFS2_INLINE_DATA_FL) &&
>> +	    !(ocfs2_inode_is_fast_symlink(new_inode))) {
>> +		struct ocfs2_extent_list *el = &new_di->id2.i_list;
>> +		le16_add_cpu(&el->l_count, -(inline_size /
>> +					sizeof(struct ocfs2_extent_rec)));
>> +	}
> This is badly screwed up.  There is no local variable `new_inode' -
> new_inode() is a global function!
>
> I assume you meant args->new_inode, but this patch clearly wasn't
> tested (or wasn't what you _did_ test).
Sorry, I made a bad mistake, it should be args->new_inode,
I just test the case where the inode is not fast symlink.
I will fix this and test all the case and send again.

Thanks,
Junxiao.

>
> I'll drop it.  Please fix, retest, resend.