unlink performance: wait for open lock in case of dirs

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

Signed-off-by Goldwyn Rodrigues <[email protected]>

Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c
===================================================================

fs/ocfs2/dlmglue.c +18 -36

@@ -1681,9 +1681,9 @@ void ocfs2_rw_unlock(struct inode *inode
 /*
  * ocfs2_open_lock always get PR mode lock.
  */
-int ocfs2_open_lock(struct inode *inode)
+int ocfs2_open_lock(struct inode *inode, int write, int wait)
 {
-	int status = 0;
+	int status = 0, level, flags;
 	struct ocfs2_lock_res *lockres;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 

@@ -1696,43 +1696,25 @@ int ocfs2_open_lock(struct inode *inode)
 		goto out;
 
 	lockres = &OCFS2_I(inode)->ip_open_lockres;
-
-	status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
-				    DLM_LOCK_PR, 0, 0);
-	if (status < 0)
-		mlog_errno(status);
-
-out:
-	return status;
-}
-
-int ocfs2_try_open_lock(struct inode *inode, int write)
-{
-	int status = 0, level;
-	struct ocfs2_lock_res *lockres;
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-
-	BUG_ON(!inode);
-
-	mlog(0, "inode %llu try to take %s open lock\n",
-	     (unsigned long long)OCFS2_I(inode)->ip_blkno,
-	     write ? "EXMODE" : "PRMODE");
-
-	if (ocfs2_mount_local(osb))
-		goto out;
-
-	lockres = &OCFS2_I(inode)->ip_open_lockres;
-
 	level = write ? DLM_LOCK_EX : DLM_LOCK_PR;
+	if (wait) {
+		flags = 0;
+		/* If we don't already have the lock in PR mode,
+		 * don't wait.
+		 *
+		 * This should avoid ABBA locking.
+		 */
+		if ((lockres->l_level != DLM_LOCK_PR) && write)
+			flags = DLM_LKF_NOQUEUE;
+
+	} else
+		flags = DLM_LKF_NOQUEUE;
 
-	/*
-	 * The file system may already holding a PRMODE/EXMODE open lock.
-	 * Since we pass DLM_LKF_NOQUEUE, the request won't block waiting on
-	 * other nodes and the -EAGAIN will indicate to the caller that
-	 * this inode is still in use.
-	 */
 	status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
-				    level, DLM_LKF_NOQUEUE, 0);
+				    level, flags, 0);
+
+	if ((status < 0) && (flags && (status != -EAGAIN)))
+		mlog_errno(status);
 
 out:
 	return status;

Index: linux-3.0-SLE11-SP3/fs/ocfs2/inode.c
===================================================================

fs/ocfs2/inode.c +4 -3

@@ -455,7 +455,7 @@ static int ocfs2_read_locked_inode(struc
 				  0, inode);
 
 	if (can_lock) {
-		status = ocfs2_open_lock(inode);
+		status = ocfs2_open_lock(inode, 0, 1);
 		if (status) {
 			make_bad_inode(inode);
 			mlog_errno(status);

@@ -470,7 +470,7 @@ static int ocfs2_read_locked_inode(struc
 	}
 
 	if (args->fi_flags & OCFS2_FI_FLAG_ORPHAN_RECOVERY) {
-		status = ocfs2_try_open_lock(inode, 0);
+		status = ocfs2_open_lock(inode, 0, 0);
 		if (status) {
 			make_bad_inode(inode);
 			return status;

@@ -923,7 +923,8 @@ static int ocfs2_query_inode_wipe(struct
 	 * Though we call this with the meta data lock held, the
 	 * trylock keeps us from ABBA deadlock.
 	 */
-	status = ocfs2_try_open_lock(inode, 1);
+	status = ocfs2_open_lock(inode, 1, S_ISDIR(inode->i_mode));
+
 	if (status == -EAGAIN) {
 		status = 0;
 		reason = 3;

Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.h
===================================================================

fs/ocfs2/dlmglue.h +1 -2

@@ -110,8 +110,7 @@ int ocfs2_create_new_inode_locks(struct
 int ocfs2_drop_inode_locks(struct inode *inode);
 int ocfs2_rw_lock(struct inode *inode, int write);
 void ocfs2_rw_unlock(struct inode *inode, int write);
-int ocfs2_open_lock(struct inode *inode);
-int ocfs2_try_open_lock(struct inode *inode, int write);
+int ocfs2_open_lock(struct inode *inode, int write, int wait);
 void ocfs2_open_unlock(struct inode *inode);
 int ocfs2_inode_lock_atime(struct inode *inode,
 			  struct vfsmount *vfsmnt,

Index: linux-3.0-SLE11-SP3/fs/ocfs2/namei.c
===================================================================

fs/ocfs2/namei.c +1 -1

@@ -2302,7 +2302,7 @@ int ocfs2_create_inode_in_orphan(struct
 	}
 
 	/* get open lock so that only nodes can't remove it from orphan dir. */
-	status = ocfs2_open_lock(inode);
+	status = ocfs2_open_lock(inode, 0, 1);
 	if (status < 0)
 		mlog_errno(status);
 

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

.

Quoted 6 lines · Goldwyn Rodrigues

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

Quoted 5 lines

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

Why a PR->EX convert takes longer than a simple EX grant.

Quoted 9 lines

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

why it can avoid the ABBA locking.

Quoted 9 lines

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

Please send the testcase . thanks.

Quoted 144 lines

Signed-off-by Goldwyn Rodrigues <[email protected]>

Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c
===================================================================

fs/ocfs2/dlmglue.c 2013-06-14 09:19:48.651924037 -0500 renamed from fs/ocfs2/dlmglue.c 2013-06-14 06:47:29.322506695 -0500 +18 -36

@@ -1681,9 +1681,9 @@ void ocfs2_rw_unlock(struct inode *inode
 /*
  * ocfs2_open_lock always get PR mode lock.
  */
-int ocfs2_open_lock(struct inode *inode)
+int ocfs2_open_lock(struct inode *inode, int write, int wait)
 {
-     int status = 0;
+     int status = 0, level, flags;
      struct ocfs2_lock_res *lockres;
      struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 

@@ -1696,43 +1696,25 @@ int ocfs2_open_lock(struct inode *inode)
              goto out;
 
      lockres = &OCFS2_I(inode)->ip_open_lockres;
-
-     status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
-                                 DLM_LOCK_PR, 0, 0);
-     if (status < 0)
-             mlog_errno(status);
-
-out:
-     return status;
-}
-
-int ocfs2_try_open_lock(struct inode *inode, int write)
-{
-     int status = 0, level;
-     struct ocfs2_lock_res *lockres;
-     struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-
-     BUG_ON(!inode);
-
-     mlog(0, "inode %llu try to take %s open lock\n",
-          (unsigned long long)OCFS2_I(inode)->ip_blkno,
-          write ? "EXMODE" : "PRMODE");
-
-     if (ocfs2_mount_local(osb))
-             goto out;
-
-     lockres = &OCFS2_I(inode)->ip_open_lockres;
-
      level = write ? DLM_LOCK_EX : DLM_LOCK_PR;
+     if (wait) {
+             flags = 0;
+             /* If we don't already have the lock in PR mode,
+              * don't wait.
+              *
+              * This should avoid ABBA locking.
+              */
+             if ((lockres->l_level != DLM_LOCK_PR) && write)
+                     flags = DLM_LKF_NOQUEUE;
+
+     } else
+             flags = DLM_LKF_NOQUEUE;
 
-     /*
-      * The file system may already holding a PRMODE/EXMODE open lock.
-      * Since we pass DLM_LKF_NOQUEUE, the request won't block waiting on
-      * other nodes and the -EAGAIN will indicate to the caller that
-      * this inode is still in use.
-      */
      status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
-                                 level, DLM_LKF_NOQUEUE, 0);
+                                 level, flags, 0);
+
+     if ((status < 0) && (flags && (status != -EAGAIN)))
+             mlog_errno(status);
 
 out:
      return status;

Index: linux-3.0-SLE11-SP3/fs/ocfs2/inode.c
===================================================================

fs/ocfs2/inode.c 2013-06-14 07:33:53.960196648 -0500 renamed from fs/ocfs2/inode.c 2013-06-13 22:54:32.527606012 -0500 +4 -3

@@ -455,7 +455,7 @@ static int ocfs2_read_locked_inode(struc
                                0, inode);
 
      if (can_lock) {
-             status = ocfs2_open_lock(inode);
+             status = ocfs2_open_lock(inode, 0, 1);
              if (status) {
                      make_bad_inode(inode);
                      mlog_errno(status);

@@ -470,7 +470,7 @@ static int ocfs2_read_locked_inode(struc
      }
 
      if (args->fi_flags & OCFS2_FI_FLAG_ORPHAN_RECOVERY) {
-             status = ocfs2_try_open_lock(inode, 0);
+             status = ocfs2_open_lock(inode, 0, 0);
              if (status) {
                      make_bad_inode(inode);
                      return status;

@@ -923,7 +923,8 @@ static int ocfs2_query_inode_wipe(struct
       * Though we call this with the meta data lock held, the
       * trylock keeps us from ABBA deadlock.
       */
-     status = ocfs2_try_open_lock(inode, 1);
+     status = ocfs2_open_lock(inode, 1, S_ISDIR(inode->i_mode));
+
      if (status == -EAGAIN) {
              status = 0;
              reason = 3;

Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.h
===================================================================

fs/ocfs2/dlmglue.h 2013-06-14 07:38:49.861576515 -0500 renamed from fs/ocfs2/dlmglue.h 2013-06-10 09:45:20.787386504 -0500 +1 -2

@@ -110,8 +110,7 @@ int ocfs2_create_new_inode_locks(struct
 int ocfs2_drop_inode_locks(struct inode *inode);
 int ocfs2_rw_lock(struct inode *inode, int write);
 void ocfs2_rw_unlock(struct inode *inode, int write);
-int ocfs2_open_lock(struct inode *inode);
-int ocfs2_try_open_lock(struct inode *inode, int write);
+int ocfs2_open_lock(struct inode *inode, int write, int wait);
 void ocfs2_open_unlock(struct inode *inode);
 int ocfs2_inode_lock_atime(struct inode *inode,
                        struct vfsmount *vfsmnt,

Index: linux-3.0-SLE11-SP3/fs/ocfs2/namei.c
===================================================================

fs/ocfs2/namei.c 2013-06-14 07:40:06.623785914 -0500 renamed from fs/ocfs2/namei.c 2013-06-13 22:54:32.527606012 -0500 +1 -1

@@ -2302,7 +2302,7 @@ int ocfs2_create_inode_in_orphan(struct
      }
 
      /* get open lock so that only nodes can't remove it from orphan dir. */
-     status = ocfs2_open_lock(inode);
+     status = ocfs2_open_lock(inode, 0, 1);
      if (status < 0)
              mlog_errno(status);
 

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

Quoted 2 lines

.

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

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

Quoted 20 lines · On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <[email protected]> wrote:

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

Quoted 6 lines

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

We need some more review and test of this patch, please.

The patch doesn't apply to current kernels - please redo, retest and resend it.

In particular, the kernel you're patching doesn't have the ocfs2_is_hard_readonly() tests in ocfs2_open_lock() and ocfs2_try_open_lock().

And looking at those tests, I wonder about ocfs2_open_lock().

: int ocfs2_open_lock(struct inode *inode)
: {
: int status = 0;
: struct ocfs2_lock_res *lockres;
: struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
:
: BUG_ON(!inode);
:
: mlog(0, "inode %llu take PRMODE open lock\n",
: (unsigned long long)OCFS2_I(inode)->ip_blkno);
:
: if (ocfs2_is_hard_readonly(osb) || ocfs2_mount_local(osb))
: goto out;
:
: lockres = &OCFS2_I(inode)->ip_open_lockres;
:
: status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
: DLM_LOCK_PR, 0, 0);
: if (status < 0)
: mlog_errno(status);
:
: out:
: return status;
: }

It will return zero if ocfs2_is_hard_readonly(). Is that correct?

Should it return -EROFS for writes?

After your patch, this code does
know whether the attempt was for a write.

Please check all that.

Quoted 25 lines · Andrew Morton

Quoted 23 lines · On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <[email protected]> wrote:

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Quoted 8 lines

Quoted 3 lines

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

We need some more review and test of this patch, please.

The patch doesn't apply to current kernels - please redo, retest and resend it.

Quoted 32 lines

In particular, the kernel you're patching doesn't have the ocfs2_is_hard_readonly() tests in ocfs2_open_lock() and ocfs2_try_open_lock().

And looking at those tests, I wonder about ocfs2_open_lock().

: int ocfs2_open_lock(struct inode *inode)
: {

:     int status = 0;
:     struct ocfs2_lock_res *lockres;
:     struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);

:
: BUG_ON(!inode);
:
: mlog(0, "inode %llu take PRMODE open lock\n",
: (unsigned long long)OCFS2_I(inode)->ip_blkno);
:
: if (ocfs2_is_hard_readonly(osb) || ocfs2_mount_local(osb))
: goto out;
:
: lockres = &OCFS2_I(inode)->ip_open_lockres;
:
: status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
: DLM_LOCK_PR, 0, 0);
: if (status < 0)
: mlog_errno(status);
:
: out:
: return status;
: }

Quoted 3 lines

It will return zero if ocfs2_is_hard_readonly(). Is that correct?

Should it return -EROFS for writes?

Hi Andrew,

That is correct for the current design, because ocfs2_open_lock() is implemented to grant a lock in protect read mode.

The ocfs2_is_hard_readonly() has been introduced to ocfs2_open_lock() by commit: 03efed8a2a1b8e00164eb4720a82a7dd5e368a8e

ocfs2: Bugfix for hard readonly mount
ocfs2 cannot currently mount a device that is readonly at the media
("hard readonly"). Fix the broken places.
see detail: http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322

Quoted 2 lines

After your patch, this code does
know whether the attempt was for a write.

Yes, with this change, it need a fix indeed.

Quoted 2 lines

Please check all that.

Hi Goldwyn,

Sorry for the too late response as I have mentioned that I'll take a look at it, but some internal tasks really racked my brains in the past couple of weeks.

Just as Andrew's comments, this patch can not be applied properly against the upstream development tree, could you please re-base it.

If we merge ocfs2_open_lock()/ocfs2_open_try_lock() into one, looks the semantics of ocfs2_open_lock() is broke with the 'write' option is introduced, but this is not a big issue as we can tweak up the comments as well as those functions which would be affected, but we need to run ocfs2-test(at least cover all the code path which are involved in this change) to avoid any regression, it can be download from:
https://oss.oracle.com/projects/ocfs2-test/source.html
https://oss.oracle.com/osswiki/OCFS2/Ocfs2TestList.html

Another thing come to mind since this patch would affect ocfs2_evict_inode() with ocfs2_try_open_lock(), i.e, the following call chains:

ocfs2_evict_inode()
|
|-------ocfs2_delete_inode()

        |
        |-------ocfs2_query_inode_wipe()
        |       |
        |       |-------ocfs2_try_open_lock()
        |       |
        |-------ocfs2_clear_inode()
                |
                |---------ocfs2_open_unlock()

OCFS2 has a DEAD LOCK issue when creating/removing tons of files in parallel if the disk quota is enabled, please refer to:
https://oss.oracle.com/bugzilla/show_bug.cgi?id=1339

As per the old discussions, Jan Kara pointed out that there is a race in ocfs2_evict_inode() if consider the following lock sequences:

ocfs2_evict_inode()
|
|-------ocfs2_inode_lock()

        |
        |-------ocfs2_try_open_lock() [ try to get open lock in EX mode ]
                |
                |-------ocfs2_inode_unlock()
                        |
                        |-------ocfs2_open_unlock() [ drops shared open lock that is hold ]

Quotas from Jan:
"""
Now if two nodes happen to execute ocfs2_evict_inode() in parallel and ocfs2_try_open_lock() happens on both nodes before ocfs2_open_unlock() is called on any of them, ocfs2_try_open_lock() fails for both nodes...

I would think that the code should be reorganized so that shared open lock is dropped before we drop inode lock. Then the race could not happen. But I'm not sure if something else would not break.
"""

This is deserve to give a try IMHO, especially there would be kind of dependencies with your fix, and that would be wonderful if we can have your improvement as well as lock sequences adjustments that might fix the quota problems.

Quoted 27 lines · Jeff Liu

Quoted 25 lines · Andrew Morton

Quoted 23 lines · On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <[email protected]> wrote:

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3 files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs ocfs2_evict_inode() -> ocfs2_delete_inode() -> ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails with -EAGAIN. The open lock fails because on the remote node a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set on the directory inode. Now, a checkpoint interferes with the journaling of the inodes deleted in the following unlinks, in our case, directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already have an open_lock in PR mode. This way we will avoid the ABBA locking.

By waiting for the open_lock on the directory, I am getting a unlink performance improvement of a rm -Rf of 50-60% in the usual case.

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Also, folded ocfs2_open_lock and ocfs2_try_open_lock into one.

Let me know if you would like to see the test case.

We need some more review and test of this patch, please.

The patch doesn't apply to current kernels - please redo, retest and resend it.

Yes, I sent it against an older kernel. I will update it once I incorporate the other review comments. Thanks for this comment.

Quoted 32 lines

Quoted 32 lines

In particular, the kernel you're patching doesn't have the ocfs2_is_hard_readonly() tests in ocfs2_open_lock() and ocfs2_try_open_lock().

And looking at those tests, I wonder about ocfs2_open_lock().

: int ocfs2_open_lock(struct inode *inode)
: {

:     int status = 0;
:     struct ocfs2_lock_res *lockres;
:     struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);

:
: BUG_ON(!inode);
:
: mlog(0, "inode %llu take PRMODE open lock\n",
: (unsigned long long)OCFS2_I(inode)->ip_blkno);
:
: if (ocfs2_is_hard_readonly(osb) || ocfs2_mount_local(osb))
: goto out;
:
: lockres = &OCFS2_I(inode)->ip_open_lockres;
:
: status = ocfs2_cluster_lock(OCFS2_SB(inode->i_sb), lockres,
: DLM_LOCK_PR, 0, 0);
: if (status < 0)
: mlog_errno(status);
:
: out:
: return status;
: }

Quoted 3 lines

Quoted 3 lines

It will return zero if ocfs2_is_hard_readonly(). Is that correct?

Should it return -EROFS for writes?

Yes, it should.

Quoted 27 lines

Hi Andrew,

That is correct for the current design, because ocfs2_open_lock() is implemented to grant a lock in protect read mode.

The ocfs2_is_hard_readonly() has been introduced to ocfs2_open_lock() by commit: 03efed8a2a1b8e00164eb4720a82a7dd5e368a8e

ocfs2: Bugfix for hard readonly mount
ocfs2 cannot currently mount a device that is readonly at the media
("hard readonly"). Fix the broken places.
see detail: http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322

Quoted 2 lines

After your patch, this code does
know whether the attempt was for a write.

Yes, with this change, it need a fix indeed.

Quoted 2 lines

Please check all that.

Quoted 4 lines

Hi Goldwyn,

Sorry for the too late response as I have mentioned that I'll take a look at it, but some internal tasks really racked my brains in the past couple of weeks.

No problem.

Quoted 3 lines

Just as Andrew's comments, this patch can not be applied properly against the upstream development tree, could you please re-base it.

Yes, will do. I have other comments from Mark as well.

Quoted 47 lines

If we merge ocfs2_open_lock()/ocfs2_open_try_lock() into one, looks the semantics of ocfs2_open_lock() is broke with the 'write' option is introduced, but this is not a big issue as we can tweak up the comments as well as those functions which would be affected, but we need to run ocfs2-test(at least cover all the code path which are involved in this change) to avoid any regression, it can be download from:
https://oss.oracle.com/projects/ocfs2-test/source.html
https://oss.oracle.com/osswiki/OCFS2/Ocfs2TestList.html

Another thing come to mind since this patch would affect ocfs2_evict_inode() with ocfs2_try_open_lock(), i.e, the following call chains:

ocfs2_evict_inode()
|
|-------ocfs2_delete_inode()

        |
        |-------ocfs2_query_inode_wipe()
        |       |
        |       |-------ocfs2_try_open_lock()
        |       |
        |-------ocfs2_clear_inode()
                |
                |---------ocfs2_open_unlock()

OCFS2 has a DEAD LOCK issue when creating/removing tons of files in parallel if the disk quota is enabled, please refer to:
https://oss.oracle.com/bugzilla/show_bug.cgi?id=1339

As per the old discussions, Jan Kara pointed out that there is a race in ocfs2_evict_inode() if consider the following lock sequences:

ocfs2_evict_inode()
|
|-------ocfs2_inode_lock()

        |
        |-------ocfs2_try_open_lock() [ try to get open lock in EX mode ]
                |
                |-------ocfs2_inode_unlock()
                        |
                        |-------ocfs2_open_unlock() [ drops shared open lock that is hold ]

Quotas from Jan:
"""
Now if two nodes happen to execute ocfs2_evict_inode() in parallel and ocfs2_try_open_lock() happens on both nodes before ocfs2_open_unlock() is called on any of them, ocfs2_try_open_lock() fails for both nodes...

Quoted 4 lines

I would think that the code should be reorganized so that shared open lock is dropped before we drop inode lock. Then the race could not happen. But I'm not sure if something else would not break.
"""

I could not find the reference of this in the bug. Is it discussed on the ML. Do you have a link so I can look at the history?

Quoted 1 line

Quoted 4 lines

This is deserve to give a try IMHO, especially there would be kind of dependencies with your fix, and that would be wonderful if we can have your improvement as well as lock sequences adjustments that might fix the quota problems.

Yes, I understand. Please provide the references if possible so I get the bigger picture.