From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Becker Date: Tue, 2 Jul 2013 08:40:42 -0700 Subject: [Ocfs2-devel] [PATCH V3] ocfs2: xattr: fix inlined xattr reflink In-Reply-To: <51D24521.4080202@oracle.com> References: <1372388183-6378-1-git-send-email-junxiao.bi@oracle.com> <20130701225746.GC965@localhost> <51D22814.7050500@oracle.com> <20130702015040.GJ965@localhost> <51D24521.4080202@oracle.com> Message-ID: <20130702154040.GD26307@localhost> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ocfs2-devel@oss.oracle.com 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. > > > >>>> 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