| Message ID | 20130615020510.GA4487@shrek.cartoons (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2013/6/15 10:05, Goldwyn Rodrigues 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. Why 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. why it can 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. > Please send the testcase . thanks. > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c > =================================================================== > --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.c 2013-06-14 06:47:29.322506695 -0500 > +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c 2013-06-14 09:19:48.651924037 -0500 > @@ -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 > =================================================================== > --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/inode.c 2013-06-13 22:54:32.527606012 -0500 > +++ linux-3.0-SLE11-SP3/fs/ocfs2/inode.c 2013-06-14 07:33:53.960196648 -0500 > @@ -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 > =================================================================== > --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.h 2013-06-10 09:45:20.787386504 -0500 > +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.h 2013-06-14 07:38:49.861576515 -0500 > @@ -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 > =================================================================== > --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/namei.c 2013-06-13 22:54:32.527606012 -0500 > +++ linux-3.0-SLE11-SP3/fs/ocfs2/namei.c 2013-06-14 07:40:06.623785914 -0500 > @@ -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@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel > > . >
On Mon, Jun 17, 2013 at 8:30 PM, shencanquan <shencanquan@huawei.com> wrote: > On 2013/6/15 10:05, Goldwyn Rodrigues 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. > > > Why a PR->EX convert takes longer than a simple EX grant. This is best answered by the DLM. In short, a direct EX grant goes to the lock master and if no previous lock found, is granted. In the case of PR->EX, it first goes to the lock master, the lock master checks the holders, request each holder to unlock. Once it gets a response from all PR holders only then grants the lock. > >> >> 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. The locking sequence in inode.c:ocfs2_read_locked_inode is open_lock and then meta locks (ocfs2_inode_lock). The ocfs2_try_open_lock was a non-blocking sequence and was always called with meta locks held. However, if we use a blocking open_lock, it will deadlock. OTOH, if we already posses the PR lock, it means that the lock sequence already has happened in open_lock->meta_lock and hence this is just an upgrade from PR->EX hence we can wait. > >> >> 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. Attached. You will have to modify the name of the other nodes in the script. In my case, it was "sles11a" and "sles11c". Also remember to setup .ssh/authorized_keys for automatic logins. You may get delays in later calls because ocfs2cmt thread wakes up and has a big backlog, but it is still much better than frequent checkpoints. Create a directory, and copy the script there and execute it. Make sure the ocfs2 partition is mounted at the same directory in all the machines. Hope that clarifies your doubts. Let me know if you need further clarifications. > >> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> >> >> Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c >> =================================================================== >> --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.c 2013-06-14 06:47:29.322506695 -0500 >> +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c 2013-06-14 09:19:48.651924037 -0500 >> @@ -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 >> =================================================================== >> --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/inode.c 2013-06-13 22:54:32.527606012 -0500 >> +++ linux-3.0-SLE11-SP3/fs/ocfs2/inode.c 2013-06-14 07:33:53.960196648 -0500 >> @@ -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 >> =================================================================== >> --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.h 2013-06-10 09:45:20.787386504 -0500 >> +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.h 2013-06-14 07:38:49.861576515 -0500 >> @@ -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 >> =================================================================== >> --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/namei.c 2013-06-13 22:54:32.527606012 -0500 >> +++ linux-3.0-SLE11-SP3/fs/ocfs2/namei.c 2013-06-14 07:40:06.623785914 -0500 >> @@ -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@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> >> . >> > > > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel -- Goldwyn
On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <rgoldwyn@gmail.com> 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. > > 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.
On 06/29/2013 07:12 AM, Andrew Morton wrote: > On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <rgoldwyn@gmail.com> 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. >> >> 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? 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 > After your patch, this code does > know whether the attempt was for a write. Yes, with this change, it need a fix indeed. > > 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. Thanks, -Jeff
On Sun, Jun 30, 2013 at 1:44 AM, Jeff Liu <jeff.liu@oracle.com> wrote: > On 06/29/2013 07:12 AM, Andrew Morton wrote: > >> On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <rgoldwyn@gmail.com> 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. >>> >>> 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. >> >> 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? Yes, it should. > > 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 > >> After your patch, this code does >> know whether the attempt was for a write. > > > > Yes, with this change, it need a fix indeed. > >> >> 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. No problem. > > 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. > > 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. > """ 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? > > 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. -- Goldwyn
On 07/02/2013 12:44 AM, Goldwyn Rodrigues wrote: > On Sun, Jun 30, 2013 at 1:44 AM, Jeff Liu <jeff.liu@oracle.com> wrote: >> On 06/29/2013 07:12 AM, Andrew Morton wrote: >> >>> On Fri, 14 Jun 2013 21:05:10 -0500 Goldwyn Rodrigues <rgoldwyn@gmail.com> 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. >>>> >>>> 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. > >>> >>> 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? > > Yes, it should. > > >> >> 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 >> >>> After your patch, this code does >>> know whether the attempt was for a write. >> >> >> >> Yes, with this change, it need a fix indeed. >> >>> >>> 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. > > No problem. > >> >> 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. > >> >> 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. >> """ > > 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? > >> >> 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. https://oss.oracle.com/pipermail/ocfs2-devel/2012-August/008709.html Thanks, -Jeff
Index: linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c =================================================================== --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.c 2013-06-14 06:47:29.322506695 -0500 +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.c 2013-06-14 09:19:48.651924037 -0500 @@ -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 =================================================================== --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/inode.c 2013-06-13 22:54:32.527606012 -0500 +++ linux-3.0-SLE11-SP3/fs/ocfs2/inode.c 2013-06-14 07:33:53.960196648 -0500 @@ -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 =================================================================== --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/dlmglue.h 2013-06-10 09:45:20.787386504 -0500 +++ linux-3.0-SLE11-SP3/fs/ocfs2/dlmglue.h 2013-06-14 07:38:49.861576515 -0500 @@ -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 =================================================================== --- linux-3.0-SLE11-SP3.orig/fs/ocfs2/namei.c 2013-06-13 22:54:32.527606012 -0500 +++ linux-3.0-SLE11-SP3/fs/ocfs2/namei.c 2013-06-14 07:40:06.623785914 -0500 @@ -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);
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 <rgoldwyn@suse.com>