[Ocfs2-devel] [PATCH V2] ocfs2: Take inode cluster lock before moving reflinked inode from orphan dir

[Joseph Qi]

On 18/4/12 03:31, Ashish Samant wrote:
> While reflinking an inode, we create a new inode in orphan directory, then
> take EX lock on it, reflink the original inode to orphan inode and release
> EX lock. Once the lock is released another node could request it in EX mode
> from ocfs2_recover_orphans() which causes downconvert of the lock, on this
> node, to NL mode.
> 
> Later we attempt to initialize security acl for the orphan inode and move
> it to the reflink destination. However, while doing this we dont take EX
> lock on the inode. This could potentially cause problems because we could
> be starting transaction, accessing journal and modifying metadata of the
> inode while holding NL lock and with another node holding EX lock on the
> inode.
> 
> Fix this by taking orphan inode cluster lock in EX mode before
> initializing security and moving orphan inode to reflink destination.
> Use the __tracker variant while taking inode lock to avoid recursive
> locking in the ocfs2_init_security_and_acl() call chain.
> 
> Signed-off-by: Ashish Samant <ashish.samant at oracle.com>
> 
Reviewed-by: Joseph Qi <jiangqi903 at gmail.com>

> V1->V2:
> Modify commit message to better reflect the problem in upstream kernel.
> ---
>  fs/ocfs2/refcounttree.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index ab156e3..1b1283f 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -4250,10 +4250,11 @@ static int __ocfs2_reflink(struct dentry *old_dentry,
>  static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
>  			 struct dentry *new_dentry, bool preserve)
>  {
> -	int error;
> +	int error, had_lock;
>  	struct inode *inode = d_inode(old_dentry);
>  	struct buffer_head *old_bh = NULL;
>  	struct inode *new_orphan_inode = NULL;
> +	struct ocfs2_lock_holder oh;
>  
>  	if (!ocfs2_refcount_tree(OCFS2_SB(inode->i_sb)))
>  		return -EOPNOTSUPP;
> @@ -4295,6 +4296,14 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
>  		goto out;
>  	}
>  
> +	had_lock = ocfs2_inode_lock_tracker(new_orphan_inode, NULL, 1,
> +					    &oh);
> +	if (had_lock < 0) {
> +		error = had_lock;
> +		mlog_errno(error);
> +		goto out;
> +	}
> +
>  	/* If the security isn't preserved, we need to re-initialize them. */
>  	if (!preserve) {
>  		error = ocfs2_init_security_and_acl(dir, new_orphan_inode,
> @@ -4302,14 +4311,15 @@ static int ocfs2_reflink(struct dentry *old_dentry, struct inode *dir,
>  		if (error)
>  			mlog_errno(error);
>  	}
> -out:
>  	if (!error) {
>  		error = ocfs2_mv_orphaned_inode_to_new(dir, new_orphan_inode,
>  						       new_dentry);
>  		if (error)
>  			mlog_errno(error);
>  	}
> +	ocfs2_inode_unlock_tracker(new_orphan_inode, 1, &oh, had_lock);
>  
> +out:
>  	if (new_orphan_inode) {
>  		/*
>  		 * We need to open_unlock the inode no matter whether we
>