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

[Junxiao Bi]

On 04/12/2018 03:31 AM, 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: Junxiao Bi <junxiao.bi at oracle.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