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

From: Junxiao Bi <junxiao.bi@oracle.com>
To: ocfs2-devel@oss.oracle.com
Subject: [Ocfs2-devel] [PATCH V3] ocfs2: xattr: fix inlined xattr reflink
Date: Tue, 02 Jul 2013 11:12:33 +0800	[thread overview]
Message-ID: <51D24521.4080202@oracle.com> (raw)
In-Reply-To: <20130702015040.GJ965@localhost>

Hi Joel,

On 07/02/2013 09:50 AM, Joel Becker wrote:
> On Tue, Jul 02, 2013 at 09:08:36AM +0800, Junxiao Bi wrote:
>> Hi Joel,
>>
>> On 07/02/2013 06:57 AM, Joel Becker wrote:
>>> On Fri, Jun 28, 2013 at 10:56:23AM +0800, Junxiao Bi 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.
> <snip>
>>>>  	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(args->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)));
>>>> +	}
>>> Why are you referencing the inode before taking the lock?
>> I think we didn't need take the lock here because this is in reflink,
>> it's a new inode. It can not be access from other process or node at
>> that time.
> 	You're sure about that?  The orphan_dir scan cannot find it?  
Thanks for pointing this out, I just realized that the new_inode is
added into orphan_dir first.
> If
> you are correct, why are we looking it just another line below?
> Shouldn't you remove those locks?  The inconsistency is glaring.
Indeed this code snippet is referenced from the code in
ocfs2_xattr_ibody_init(), at there, it didn't use the oi->ip_lock to
protect. In  __ocfs2_reflink(), there are following code snippet,

4244         mutex_lock_nested(&new_inode->i_mutex, I_MUTEX_CHILD);
4245         ret = ocfs2_inode_lock_nested(new_inode, &new_bh, 1,
4246                                       OI_LS_REFLINK_TARGET);

I am not sure about the orphan dir scan, can this two locks protect the
inode?

Thanks,
Junxiao.
>
> Joel
>
>