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

[Joel Becker]

On Tue, Jul 02, 2013 at 11:12:33AM +0800, Junxiao Bi wrote:
> 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);

	The i_mutex protects the VFS inode.  The ocfs2_inode_lock
protects against other nodes in the cluster.  new_oi->ip_lock protects
the ocfs2_inode_info specific stuff.
	Reading a bit out of new_oi->ip_dyn_features is probably safe;
we are protected from inline data being converted to extents.  So I
suppose this isn't terribly different than xattr_ibody_init().

Joel

> I am not sure about the orphan dir scan, can this two locks protect the
> inode?
> 
> Thanks,
> Junxiao.
> >
> > Joel
> >
> >
> 

-- 

"Get right to the heart of matters.
 It's the heart that matters more."

			http://www.jlbec.org/
			jlbec at evilplan.org