[Cluster-devel] GFS2: Don't use trylocks when converting dlm locks

[Cluster-devel] GFS2: Don't use trylocks when converting dlm locks

[Steven Whitehouse]

There is no need to use a try lock when up converting a dlm lock because
the dlm will return -EDEADLK if the conversion would otherwise result in
a deadlock. GFS2 can already handle the -EDEADLK return on conversion.

This should make GFS2 a bit more efficient when it comes to locking
during contention situations, although tests haven't shown any
measurable improvement so far. Also this is a clean up of the code.


Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>


diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8777885..41fea85 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -590,10 +590,6 @@ __acquires(&gl->gl_spin)
 	clear_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags);
 
 	gfs2_glock_hold(gl);
-	if (target != LM_ST_UNLOCKED && (gl->gl_state == LM_ST_SHARED ||
-	    gl->gl_state == LM_ST_DEFERRED) &&
-	    !(lck_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
-		lck_flags |= LM_FLAG_TRY_1CB;
 	ret = gfs2_lm_lock(sdp, gl, target, lck_flags);
 
 	if (!(ret & LM_OUT_ASYNC)) {

[Cluster-devel] GFS2: Don't use trylocks when converting dlm locks

[Bob Peterson]

----- "Steven Whitehouse" <swhiteho@redhat.com> wrote:
| There is no need to use a try lock when up converting a dlm lock
| because
| the dlm will return -EDEADLK if the conversion would otherwise result
| in
| a deadlock. GFS2 can already handle the -EDEADLK return on
| conversion.
| 
| This should make GFS2 a bit more efficient when it comes to locking
| during contention situations, although tests haven't shown any
| measurable improvement so far. Also this is a clean up of the code.
| 
| 
| Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
| 
| 
| diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
| index 8777885..41fea85 100644
| --- a/fs/gfs2/glock.c
| +++ b/fs/gfs2/glock.c
| @@ -590,10 +590,6 @@ __acquires(&gl->gl_spin)
|  	clear_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags);
|  
|  	gfs2_glock_hold(gl);
| -	if (target != LM_ST_UNLOCKED && (gl->gl_state == LM_ST_SHARED ||
| -	    gl->gl_state == LM_ST_DEFERRED) &&
| -	    !(lck_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
| -		lck_flags |= LM_FLAG_TRY_1CB;
|  	ret = gfs2_lm_lock(sdp, gl, target, lck_flags);
|  
|  	if (!(ret & LM_OUT_ASYNC)) {

This patch looks good in theory, but for some unknown reason,
causes a problem.  It seems to open a timing window with
gfs2_rename's unlink code:

GFS2: fsid=hlab_cluster:svr_home.0: fatal: filesystem consistency error
GFS2: fsid=hlab_cluster:svr_home.0:   RG = 1376275
GFS2: fsid=hlab_cluster:svr_home.0:   function = gfs2_setbit, file = /root/bob/gfs2/rgrp.c, line = 104
GFS2: fsid=hlab_cluster:svr_home.0: about to withdraw this file system
GFS2: fsid=hlab_cluster:svr_home.0: telling LM to withdraw
PWLPOST.EXE[9371]: segfault at 00000000ff2c3e78 rip 00000000002a6bf3 rsp 00000000ff2c3dd8 error 6
GFS2: fsid=hlab_cluster:svr_home.0: withdrawn

Call Trace:
 [<ffffffff885dc64a>] :gfs2:gfs2_lm_withdraw+0xd1/0xfe
 [<ffffffff80013b31>] find_lock_page+0x26/0xa2
 [<ffffffff80025c15>] find_or_create_page+0x22/0x72
 [<ffffffff885de5b9>] :gfs2:gfs2_pin+0xb4/0xbc
 [<ffffffff885efd0f>] :gfs2:gfs2_consist_rgrpd_i+0x34/0x39
 [<ffffffff885ec719>] :gfs2:rgblk_free+0x13a/0x15b
 [<ffffffff885ec9ba>] :gfs2:gfs2_unlink_di+0x25/0x60
 [<ffffffff885dac21>] :gfs2:gfs2_change_nlink+0xf8/0x102
 [<ffffffff885e6cc9>] :gfs2:gfs2_rename+0x48e/0x67d
 [<ffffffff885e6957>] :gfs2:gfs2_rename+0x11c/0x67d
 [<ffffffff885e699d>] :gfs2:gfs2_rename+0x162/0x67d
 [<ffffffff885e69c2>] :gfs2:gfs2_rename+0x187/0x67d
 [<ffffffff885e69f5>] :gfs2:gfs2_rename+0x1ba/0x67d
 [<ffffffff885d7e96>] :gfs2:gfs2_glock_schedule_for_reclaim+0x5d/0x9a
 [<ffffffff885d8833>] :gfs2:gfs2_glock_put+0xf9/0x111
 [<ffffffff885ec26e>] :gfs2:gfs2_rindex_hold+0x32/0x12b
 [<ffffffff80030c87>] d_splice_alias+0xdc/0xfb
 [<ffffffff8000da09>] permission+0x81/0xc8
 [<ffffffff8002a9b9>] vfs_rename+0x2f4/0x471
 [<ffffffff80036ce9>] sys_renameat+0x180/0x1eb
 [<ffffffff800b95c8>] audit_syscall_entry+0x1a4/0x1cf
 [<ffffffff8006149d>] sysenter_do_call+0x1e/0x76

Instrumentation reveals:
cur_state = 0, new_state = 2 block = 0x1506ea i_unlinked: 0x0

So apparently the reclaiming of unlinked dinodes has nothing
to do with it.  It was trying to change a block from "free"
to "unlinked" which is invalid.

I'm investigating the problem now.

Regards,

Bob Peterson
Red Hat File Systems