From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751525AbdAPCqi (ORCPT ); Sun, 15 Jan 2017 21:46:38 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:25853 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751246AbdAPCqh (ORCPT ); Sun, 15 Jan 2017 21:46:37 -0500 Subject: Re: [PATCH 2/2] ocfs2: fix deadlocks when taking inode lock at vfs entry points To: Eric Ren , ocfs2-devel@oss.oracle.com References: <1483630262-22227-1-git-send-email-zren@suse.com> <1483630262-22227-3-git-send-email-zren@suse.com> <31cc0413-6aa2-2493-fce6-f8f8ab9aaa16@suse.com> Cc: akpm@linux-foundation.org, mfasheh@versity.com, jlbec@evilplan.org, ghe@suse.com, jiangqi903@gmail.com, linux-kernel@vger.kernel.org From: Junxiao Bi Message-ID: <6b3f4c7c-9276-321b-4e75-33c87a855757@oracle.com> Date: Mon, 16 Jan 2017 10:46:11 +0800 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <31cc0413-6aa2-2493-fce6-f8f8ab9aaa16@suse.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/13/2017 02:19 PM, Eric Ren wrote: > Hi! > > On 01/13/2017 12:22 PM, Junxiao Bi wrote: >> On 01/05/2017 11:31 PM, Eric Ren wrote: >>> Commit 743b5f1434f5 ("ocfs2: take inode lock in >>> ocfs2_iop_set/get_acl()") >>> results in a deadlock, as the author "Tariq Saeed" realized shortly >>> after the patch was merged. The discussion happened here >>> (https://oss.oracle.com/pipermail/ocfs2-devel/2015-September/011085.html). >>> >>> >>> The reason why taking cluster inode lock at vfs entry points opens up >>> a self deadlock window, is explained in the previous patch of this >>> series. >>> >>> So far, we have seen two different code paths that have this issue. >>> 1. do_sys_open >>> may_open >>> inode_permission >>> ocfs2_permission >>> ocfs2_inode_lock() <=== take PR >>> generic_permission >>> get_acl >>> ocfs2_iop_get_acl >>> ocfs2_inode_lock() <=== take PR >>> 2. fchmod|fchmodat >>> chmod_common >>> notify_change >>> ocfs2_setattr <=== take EX >>> posix_acl_chmod >>> get_acl >>> ocfs2_iop_get_acl <=== take PR >>> ocfs2_iop_set_acl <=== take EX >>> >>> Fixes them by adding the tracking logic (in the previous patch) for >>> these funcs above, ocfs2_permission(), ocfs2_iop_[set|get]_acl(), >>> ocfs2_setattr(). >>> >>> Signed-off-by: Eric Ren >>> --- >>> fs/ocfs2/acl.c | 39 ++++++++++++++++++++++++++++++++++----- >>> fs/ocfs2/file.c | 44 ++++++++++++++++++++++++++++++++++---------- >>> 2 files changed, 68 insertions(+), 15 deletions(-) >>> >>> diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c >>> index bed1fcb..c539890 100644 >>> --- a/fs/ocfs2/acl.c >>> +++ b/fs/ocfs2/acl.c >>> @@ -284,16 +284,31 @@ int ocfs2_iop_set_acl(struct inode *inode, >>> struct posix_acl *acl, int type) >>> { >>> struct buffer_head *bh = NULL; >>> int status = 0; >>> - >>> - status = ocfs2_inode_lock(inode, &bh, 1); >>> + int arg_flags = 0, has_locked; >>> + struct ocfs2_holder oh; >>> + struct ocfs2_lock_res *lockres; >>> + >>> + lockres = &OCFS2_I(inode)->ip_inode_lockres; >>> + has_locked = (ocfs2_is_locked_by_me(lockres) != NULL); >>> + if (has_locked) >>> + arg_flags = OCFS2_META_LOCK_GETBH; >>> + status = ocfs2_inode_lock_full(inode, &bh, 1, arg_flags); >>> if (status < 0) { >>> if (status != -ENOENT) >>> mlog_errno(status); >>> return status; >>> } >>> + if (!has_locked) >>> + ocfs2_add_holder(lockres, &oh); >>> + >>> status = ocfs2_set_acl(NULL, inode, bh, type, acl, NULL, NULL); >>> - ocfs2_inode_unlock(inode, 1); >>> + >>> + if (!has_locked) { >>> + ocfs2_remove_holder(lockres, &oh); >>> + ocfs2_inode_unlock(inode, 1); >>> + } >>> brelse(bh); >>> + >>> return status; >>> } >>> @@ -303,21 +318,35 @@ struct posix_acl *ocfs2_iop_get_acl(struct >>> inode *inode, int type) >>> struct buffer_head *di_bh = NULL; >>> struct posix_acl *acl; >>> int ret; >>> + int arg_flags = 0, has_locked; >>> + struct ocfs2_holder oh; >>> + struct ocfs2_lock_res *lockres; >>> osb = OCFS2_SB(inode->i_sb); >>> if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL)) >>> return NULL; >>> - ret = ocfs2_inode_lock(inode, &di_bh, 0); >>> + >>> + lockres = &OCFS2_I(inode)->ip_inode_lockres; >>> + has_locked = (ocfs2_is_locked_by_me(lockres) != NULL); >>> + if (has_locked) >>> + arg_flags = OCFS2_META_LOCK_GETBH; >>> + ret = ocfs2_inode_lock_full(inode, &di_bh, 0, arg_flags); >>> if (ret < 0) { >>> if (ret != -ENOENT) >>> mlog_errno(ret); >>> return ERR_PTR(ret); >>> } >>> + if (!has_locked) >>> + ocfs2_add_holder(lockres, &oh); >>> acl = ocfs2_get_acl_nolock(inode, type, di_bh); >>> - ocfs2_inode_unlock(inode, 0); >>> + if (!has_locked) { >>> + ocfs2_remove_holder(lockres, &oh); >>> + ocfs2_inode_unlock(inode, 0); >>> + } >>> brelse(di_bh); >>> + >>> return acl; >>> } >>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>> index c488965..62be75d 100644 >>> --- a/fs/ocfs2/file.c >>> +++ b/fs/ocfs2/file.c >>> @@ -1138,6 +1138,9 @@ int ocfs2_setattr(struct dentry *dentry, struct >>> iattr *attr) >>> handle_t *handle = NULL; >>> struct dquot *transfer_to[MAXQUOTAS] = { }; >>> int qtype; >>> + int arg_flags = 0, had_lock; >>> + struct ocfs2_holder oh; >>> + struct ocfs2_lock_res *lockres; >>> trace_ocfs2_setattr(inode, dentry, >>> (unsigned long long)OCFS2_I(inode)->ip_blkno, >>> @@ -1173,13 +1176,20 @@ int ocfs2_setattr(struct dentry *dentry, >>> struct iattr *attr) >>> } >>> } >>> - status = ocfs2_inode_lock(inode, &bh, 1); >>> + lockres = &OCFS2_I(inode)->ip_inode_lockres; >>> + had_lock = (ocfs2_is_locked_by_me(lockres) != NULL); >> If had_lock==true, it is a bug? I think we should BUG_ON for it, that >> can help us catch bug at the first time. > > Good idea! But I'm not sure if "ocfs2_setattr" is always the first one > who takes the cluster lock. > It's harder for me to name all the possible paths;-/ The BUG_ON() can help catch the path where ocfs2_setattr is not the first one. Thanks, Junxiao. > >> >> >>> + if (had_lock) >>> + arg_flags = OCFS2_META_LOCK_GETBH; >>> + status = ocfs2_inode_lock_full(inode, &bh, 1, arg_flags); >>> if (status < 0) { >>> if (status != -ENOENT) >>> mlog_errno(status); >>> goto bail_unlock_rw; >>> } >>> - inode_locked = 1; >>> + if (!had_lock) { >>> + ocfs2_add_holder(lockres, &oh); >>> + inode_locked = 1; >>> + } >>> if (size_change) { >>> status = inode_newsize_ok(inode, attr->ia_size); >>> @@ -1260,7 +1270,8 @@ int ocfs2_setattr(struct dentry *dentry, struct >>> iattr *attr) >>> bail_commit: >>> ocfs2_commit_trans(osb, handle); >>> bail_unlock: >>> - if (status) { >>> + if (status && inode_locked) { >>> + ocfs2_remove_holder(lockres, &oh); >>> ocfs2_inode_unlock(inode, 1); >>> inode_locked = 0; >>> } >>> @@ -1278,8 +1289,10 @@ int ocfs2_setattr(struct dentry *dentry, >>> struct iattr *attr) >>> if (status < 0) >>> mlog_errno(status); >>> } >>> - if (inode_locked) >>> + if (inode_locked) { >>> + ocfs2_remove_holder(lockres, &oh); >>> ocfs2_inode_unlock(inode, 1); >>> + } >>> brelse(bh); >>> return status; >>> @@ -1321,20 +1334,31 @@ int ocfs2_getattr(struct vfsmount *mnt, >>> int ocfs2_permission(struct inode *inode, int mask) >>> { >>> int ret; >>> + int has_locked; >>> + struct ocfs2_holder oh; >>> + struct ocfs2_lock_res *lockres; >>> if (mask & MAY_NOT_BLOCK) >>> return -ECHILD; >>> - ret = ocfs2_inode_lock(inode, NULL, 0); >>> - if (ret) { >>> - if (ret != -ENOENT) >>> - mlog_errno(ret); >>> - goto out; >>> + lockres = &OCFS2_I(inode)->ip_inode_lockres; >>> + has_locked = (ocfs2_is_locked_by_me(lockres) != NULL); >> The same thing as ocfs2_setattr. > OK. I will think over your suggestions! > > Thanks, > Eric > >> >> Thanks, >> Junxiao. >>> + if (!has_locked) { >>> + ret = ocfs2_inode_lock(inode, NULL, 0); >>> + if (ret) { >>> + if (ret != -ENOENT) >>> + mlog_errno(ret); >>> + goto out; >>> + } >>> + ocfs2_add_holder(lockres, &oh); >>> } >>> ret = generic_permission(inode, mask); >>> - ocfs2_inode_unlock(inode, 0); >>> + if (!has_locked) { >>> + ocfs2_remove_holder(lockres, &oh); >>> + ocfs2_inode_unlock(inode, 0); >>> + } >>> out: >>> return ret; >>> } >>> >> >