Junxiao Bi
2017-Jan-16 02:46 UTC
[Ocfs2-devel] [PATCH 2/2] ocfs2: fix deadlocks when taking inode lock at vfs entry points
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 <zren at suse.com> >>> --- >>> 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; >>> } >>> >> >
Eric Ren
2017-Jan-16 03:06 UTC
[Ocfs2-devel] [PATCH 2/2] ocfs2: fix deadlocks when taking inode lock at vfs entry points
Hi Junxiao, On 01/16/2017 10:46 AM, Junxiao Bi wrote:>>> 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.Yes, I understand. But, the problem is that the vfs entries calling order is out of our control. I don't want to place an assertion where I'm not 100% sure it's absolutely right;-) Thanks, Eric> > 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; >>>> } >>>> >