Eric Ren
2017-Jan-13 06:19 UTC
[Ocfs2-devel] [PATCH 2/2] ocfs2: fix deadlocks when taking inode lock at vfs entry points
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;-/> > >> + 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; >> } >> >
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; >>> } >>> >> >