On 2018/4/12 3: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.sam...@oracle.com>
Acked-by: Jun Piao <piao...@huawei.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
>
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
