Fix the nested PR lock calling issue in ACL

PATCH Superseded Jiaju Zhang 3 participants 5 messages Jul 26, 2010 – Jul 27, 2010 OCFS2

lore

Share & tools

lore

Jiaju Zhang

author

2010-07-26 04:36 UTC #0 lore raw

PATCH v3 1/1 Fix the nested PR lock calling issue in ACL

Hi,

Thank you very much for all the review and comments so far;) In order to review the patch easily, I attached it as below, as well as some background of this bug.

It was found in OCFS2 and Samba integration using scenario, the symptom is several smbd processes will be hung under heavy workload. Finally we found out the root cause is that the nested PR lock calling issue in ocfs2 ACL leads to a deadlock.
See the following scenario:

node1 node2

              gr PR
                |
                V
 PR(EX) -->   BAST: OCFS2_LOCK_BLOCKED
                |
                V
              rq PR
                |
                V
              wait=1

After requesting the 2nd PR lock, the process "smbd" went into D state. It can only be woken up when the 1st PR's l_ro_holders = 0. There should be an ocfs2_inode_unlock in the calling path later on, but since it has been in uninterruptible sleep, so the unlock function can't be called.

The related stack trace is:
smbd D ffff8800013d0600 0 9522 5608 0x00000000
ffff88002ca7fb18 0000000000000282 ffff88002f964500 ffff88002ca7fa98 ffff8800013d0600 ffff88002ca7fae0 ffff88002f964340 ffff88002f964340 ffff88002ca7ffd8 ffff88002ca7ffd8 ffff88002f964340 ffff88002f964340

Call Trace:
[<ffffffff80350425>] schedule_timeout+0x175/0x210
[<ffffffff8034f580>] wait_for_common+0xf0/0x210
[<ffffffffa03e12b9>] __ocfs2_cluster_lock+0x3b9/0xa90 [ocfs2]
[<ffffffffa03e7665>] ocfs2_inode_lock_full_nested+0x255/0xdb0 [ocfs2]
[<ffffffffa0446019>] ocfs2_get_acl+0x69/0x120 [ocfs2]
[<ffffffffa0446368>] ocfs2_check_acl+0x28/0x80 [ocfs2]
[<ffffffff800e3507>] acl_permission_check+0x57/0xb0
[<ffffffff800e357d>] generic_permission+0x1d/0xc0
[<ffffffffa03eecea>] ocfs2_permission+0x10a/0x1d0 [ocfs2]
[<ffffffff800e3f65>] inode_permission+0x45/0x100
[<ffffffff800d86b3>] sys_chdir+0x53/0x90
[<ffffffff80007458>] system_call_fastpath+0x16/0x1b

[<00007f34a4ef6927>] 0x7f34a4ef6927

For details, please see:
https://bugzilla.novell.com/show_bug.cgi?id=614332 and
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1278

Below is the patch, review and comments are highly appreciated;)

Thanks,
Jiaju

Sign-off-by Jiaju Zhang <[email protected]>

---
 fs/ocfs2/acl.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

fs/ocfs2/acl.c +19 -4

index da70229..a1d25ca 100644
@@ -290,17 +290,32 @@ static int ocfs2_set_acl(handle_t *handle,
 
 int ocfs2_check_acl(struct inode *inode, int mask)
 {
-	struct posix_acl *acl = ocfs2_get_acl(inode, ACL_TYPE_ACCESS);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct posix_acl *acl;
+	struct buffer_head *di_bh = NULL;
+	int ret = -EAGAIN;
+
+	if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL))
+		return ret;
+
+	ret = ocfs2_read_inode_block(inode, &di_bh);
+	if (ret < 0) {
+		mlog_errno(ret);
+		return ret;
+	}
+
+	acl = ocfs2_get_acl_nolock(inode, ACL_TYPE_ACCESS, di_bh);
+
+	brelse(di_bh);
 
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
 	if (acl) {
-		int ret = posix_acl_permission(inode, acl, mask);
+		ret = posix_acl_permission(inode, acl, mask);
 		posix_acl_release(acl);
-		return ret;
 	}
 
-	return -EAGAIN;
+	return ret;
 }
 
 int ocfs2_acl_chmod(struct inode *inode)

Jiaju Zhang

author

2010-07-26 10:36 UTC #1 lore raw

PATCH v3 1/1 Fix the nested PR lock calling issue in ACL

Oh, this is a slight change of the patch I just posted, since the original one may have wrongly processing of error return code. So please review this one instead;)

Thanks a lot,
Jiaju

Signed-off-by Jiaju Zhang <[email protected]>

---
 fs/ocfs2/acl.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

fs/ocfs2/acl.c +18 -2

index da70229..28a25b8 100644
@@ -290,12 +290,28 @@ static int ocfs2_set_acl(handle_t *handle,
 
 int ocfs2_check_acl(struct inode *inode, int mask)
 {
-	struct posix_acl *acl = ocfs2_get_acl(inode, ACL_TYPE_ACCESS);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct buffer_head *di_bh = NULL;
+	struct posix_acl *acl;
+	int ret = -EAGAIN;
+
+	if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL))
+		return ret;
+
+	ret = ocfs2_read_inode_block(inode, &di_bh);
+	if (ret < 0) {
+		mlog_errno(ret);
+		return ret;
+	}
+
+	acl = ocfs2_get_acl_nolock(inode, ACL_TYPE_ACCESS, di_bh);
+
+	brelse(di_bh);
 
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
 	if (acl) {
-		int ret = posix_acl_permission(inode, acl, mask);
+		ret = posix_acl_permission(inode, acl, mask);
 		posix_acl_release(acl);
 		return ret;
 	}

Wengang Wang

2010-07-26 12:08 UTC #2 lore raw

PATCH v3 1/1 Fix the nested PR lock calling issue in ACL

Quoted 30 lines · Jiaju Zhang

Oh, this is a slight change of the patch I just posted, since the original one may have wrongly processing of error return code. So please review this one instead;)

Thanks a lot,
Jiaju

Signed-off-by Jiaju Zhang <[email protected]>

---
 fs/ocfs2/acl.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

fs/ocfs2/acl.c +9 -1

index da70229..28a25b8 100644
@@ -290,12 +290,28 @@ static int ocfs2_set_acl(handle_t *handle,
 
 int ocfs2_check_acl(struct inode *inode, int mask)
 {
-	struct posix_acl *acl = ocfs2_get_acl(inode, ACL_TYPE_ACCESS);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct buffer_head *di_bh = NULL;
+	struct posix_acl *acl;
+	int ret = -EAGAIN;
+
+	if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL))
+		return ret;
+
+	ret = ocfs2_read_inode_block(inode, &di_bh);

fs/ocfs2/acl.c static int ocfs2_set_acl(handle_t *handle, · line 311

Quoted 10 lines

fs/ocfs2/acl.c +8 -0

+	if (ret < 0) {
+		mlog_errno(ret);
+		return ret;
+	}
+
+	acl = ocfs2_get_acl_nolock(inode, ACL_TYPE_ACCESS, di_bh);
+
+	brelse(di_bh);
 
 	if (IS_ERR(acl))

Could you log this error? though it's not part of the fix. Otherwise, ack.

regards,
wengang.

Quoted 7 lines

fs/ocfs2/acl.c +1 -1

 		return PTR_ERR(acl);
 	if (acl) {
-		int ret = posix_acl_permission(inode, acl, mask);
+		ret = posix_acl_permission(inode, acl, mask);
 		posix_acl_release(acl);
 		return ret;
 	}

Jiaju Zhang

author

2010-07-27 03:16 UTC #3 lore raw

PATCH v3 1/1 Fix the nested PR lock calling issue in ACL

Quoted 40 lines · Wengang Wang

Quoted 39 lines · Jiaju Zhang

Oh, this is a slight change of the patch I just posted, since the original one may have wrongly processing of error return code. So please review this one instead;)

Thanks a lot,
Jiaju

Signed-off-by Jiaju Zhang <[email protected]>

---
 fs/ocfs2/acl.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

fs/ocfs2/acl.c +17 -1

index da70229..28a25b8 100644
@@ -290,12 +290,28 @@ static int ocfs2_set_acl(handle_t *handle,
 
 int ocfs2_check_acl(struct inode *inode, int mask)
 {
-	struct posix_acl *acl = ocfs2_get_acl(inode, ACL_TYPE_ACCESS);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct buffer_head *di_bh = NULL;
+	struct posix_acl *acl;
+	int ret = -EAGAIN;
+
+	if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL))
+		return ret;
+
+	ret = ocfs2_read_inode_block(inode, &di_bh);
+	if (ret < 0) {
+		mlog_errno(ret);
+		return ret;
+	}
+
+	acl = ocfs2_get_acl_nolock(inode, ACL_TYPE_ACCESS, di_bh);
+
+	brelse(di_bh);

Quoted 4 lines

fs/ocfs2/acl.c static int ocfs2_set_acl(handle_t *handle, · line 311
Quoted 1 line
fs/ocfs2/acl.c +0 -0
 	if (IS_ERR(acl))
Could you log this error? though it's not part of the fix. Otherwise, ack.

Hi Wengang, many thanks;)

Yes, I can add a log like

+ mlog_errno(PTR_ERR(acl));

But I'm wondering if we really need to add the log? Since there are many places calling like

        if (IS_ERR(acl))
                return PTR_ERR(acl);

and some other filesystems such as ext3, ext4, btrfs didn't log this error at this point as well.

Thanks,
Jiaju

Quoted 10 lines

regards,
wengang.

Quoted 7 lines

fs/ocfs2/acl.c +1 -1

 		return PTR_ERR(acl);
 	if (acl) {
-		int ret = posix_acl_permission(inode, acl, mask);
+		ret = posix_acl_permission(inode, acl, mask);
 		posix_acl_release(acl);
 		return ret;
 	}

Mark Fasheh

2010-07-27 20:27 UTC #4 lore raw

PATCH v3 1/1 Fix the nested PR lock calling issue in ACL

In reply to Jiaju Zhang

Quoted 8 lines · Jiaju Zhang

Oh, this is a slight change of the patch I just posted, since the original one may have wrongly processing of error return code. So please review this one instead;)
Thanks a lot,
Jiaju
Signed-off-by Jiaju Zhang <[email protected]>

Acked-by Mark Fasheh <[email protected]>

Thanks for catching this.

You'll want to send with the original message from the first patch btw. Also I guess you can then include Wengang's requested mlog_errno().

	--Mark

Quoted 40 lines

---
 fs/ocfs2/acl.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

fs/ocfs2/acl.c +18 -2

index da70229..28a25b8 100644
@@ -290,12 +290,28 @@ static int ocfs2_set_acl(handle_t *handle,
 
 int ocfs2_check_acl(struct inode *inode, int mask)
 {
-	struct posix_acl *acl = ocfs2_get_acl(inode, ACL_TYPE_ACCESS);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct buffer_head *di_bh = NULL;
+	struct posix_acl *acl;
+	int ret = -EAGAIN;
+
+	if (!(osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL))
+		return ret;
+
+	ret = ocfs2_read_inode_block(inode, &di_bh);
+	if (ret < 0) {
+		mlog_errno(ret);
+		return ret;
+	}
+
+	acl = ocfs2_get_acl_nolock(inode, ACL_TYPE_ACCESS, di_bh);
+
+	brelse(di_bh);
 
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
 	if (acl) {
-		int ret = posix_acl_permission(inode, acl, mask);
+		ret = posix_acl_permission(inode, acl, mask);
 		posix_acl_release(acl);
 		return ret;
 	}

Quoted 4 lines

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

--
Mark Fasheh