Re: [Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-14 Thread Steven Whitehouse 
Hi,

Now pushed to the -nmw git tree. Thanks,

Steve.

On Tue, 2012-11-13 at 10:58 -0500, David Teigland wrote:
 When unmounting, gfs2 does a full dlm_unlock operation on every
 cached lock.  This can create a very large amount of work and can
 take a long time to complete.  However, the vast majority of these
 dlm unlock operations are unnecessary because after all the unlocks
 are done, gfs2 leaves the dlm lockspace, which automatically clears
 the locks of the leaving node, without unlocking each one individually.
 So, gfs2 can skip explicit dlm unlocks, and use dlm_release_lockspace to
 remove the locks implicitly.  The one exception is when the lock's lvb is
 being used.  In this case, dlm_unlock is called because it may update the
 lvb of the resource.
 
 Signed-off-by: David Teigland teigl...@redhat.com
 ---
  fs/gfs2/glock.c|1 +
  fs/gfs2/incore.h   |1 +
  fs/gfs2/lock_dlm.c |8 
  3 files changed, 10 insertions(+)
 
 diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
 index e6c2fd5..f3a5edb 100644
 --- a/fs/gfs2/glock.c
 +++ b/fs/gfs2/glock.c
 @@ -1528,6 +1528,7 @@ static void dump_glock_func(struct gfs2_glock *gl)
  
  void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)
  {
 + set_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags);
   glock_hash_walk(clear_glock, sdp);
   flush_workqueue(glock_workqueue);
   wait_event(sdp-sd_glock_wait, atomic_read(sdp-sd_glock_disposal) == 
 0);
 diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
 index 3d469d3..67a39cf 100644
 --- a/fs/gfs2/incore.h
 +++ b/fs/gfs2/incore.h
 @@ -539,6 +539,7 @@ enum {
   SDF_DEMOTE  = 5,
   SDF_NOJOURNALID = 6,
   SDF_RORECOVERY  = 7, /* read only recovery */
 + SDF_SKIP_DLM_UNLOCK = 8,
  };
  
  #define GFS2_FSNAME_LEN  256
 diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
 index 0fb6539..f6504d3 100644
 --- a/fs/gfs2/lock_dlm.c
 +++ b/fs/gfs2/lock_dlm.c
 @@ -289,6 +289,14 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
   gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT);
   gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
   gfs2_update_request_times(gl);
 +
 + /* don't want to skip dlm_unlock writing the lvb when lock is ex */
 + if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
 + gl-gl_state != LM_ST_EXCLUSIVE) {
 + gfs2_glock_free(gl);
 + return;
 + }
 +
   error = dlm_unlock(ls-ls_dlm, gl-gl_lksb.sb_lkid, DLM_LKF_VALBLK,
  NULL, gl);
   if (error) {

[Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-13 Thread David Teigland 
When unmounting, gfs2 does a full dlm_unlock operation on every
cached lock.  This can create a very large amount of work and can
take a long time to complete.  However, the vast majority of these
dlm unlock operations are unnecessary because after all the unlocks
are done, gfs2 leaves the dlm lockspace, which automatically clears
the locks of the leaving node, without unlocking each one individually.
So, gfs2 can skip explicit dlm unlocks, and use dlm_release_lockspace to
remove the locks implicitly.  The one exception is when the lock's lvb is
being used.  In this case, dlm_unlock is called because it may update the
lvb of the resource.

Signed-off-by: David Teigland teigl...@redhat.com
---
 fs/gfs2/glock.c|1 +
 fs/gfs2/incore.h   |1 +
 fs/gfs2/lock_dlm.c |8 
 3 files changed, 10 insertions(+)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index e6c2fd5..f3a5edb 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1528,6 +1528,7 @@ static void dump_glock_func(struct gfs2_glock *gl)
 
 void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)
 {
+   set_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags);
glock_hash_walk(clear_glock, sdp);
flush_workqueue(glock_workqueue);
wait_event(sdp-sd_glock_wait, atomic_read(sdp-sd_glock_disposal) == 
0);
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 3d469d3..67a39cf 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -539,6 +539,7 @@ enum {
SDF_DEMOTE  = 5,
SDF_NOJOURNALID = 6,
SDF_RORECOVERY  = 7, /* read only recovery */
+   SDF_SKIP_DLM_UNLOCK = 8,
 };
 
 #define GFS2_FSNAME_LEN256
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 0fb6539..f6504d3 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -289,6 +289,14 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT);
gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
gfs2_update_request_times(gl);
+
+   /* don't want to skip dlm_unlock writing the lvb when lock is ex */
+   if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
+   gl-gl_state != LM_ST_EXCLUSIVE) {
+   gfs2_glock_free(gl);
+   return;
+   }
+
error = dlm_unlock(ls-ls_dlm, gl-gl_lksb.sb_lkid, DLM_LKF_VALBLK,
   NULL, gl);
if (error) {
-- 
1.7.10.1.362.g242cab3

Re: [Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-12 Thread Steven Whitehouse 
Hi,

On Fri, 2012-11-09 at 10:30 -0500, David Teigland wrote:
 On Fri, Nov 09, 2012 at 09:45:17AM +, Steven Whitehouse wrote:
   + if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
   + (!gl-gl_lvb[0] || gl-gl_state != LM_ST_EXCLUSIVE)) {
  I'm still not happy with using !gl-gl_lvb[0] to determine whether the
  LVB is in use or not. I think we need a better test, or alternatively
  just test the lock state, since most locks will be NL anyway before they
  get to this point in time,
 
 Yeah, a glock flag indicating the lvb is used would be best, I'll just
 test the lock state.
 
 This actually brings up another improvement you could make.  Right now
 gfs2 enables the lvb on all locks, even though it only uses it on a small
 minority.  Limiting the lvb to locks that need it would:
 
 - save 64 bytes of memory for every local lock
   (32 in gfs2_glock, 32 in dlm_rsb)
 
 - save 96 bytes of memory for every remote lock
   (32 in gfs2_glock, 32 in local dlm_rsb, 32 in remote dlm_lkb)
 
 - save 32 bytes of network message size in many dlm messages
 
 - save a lot of memcpying of zeros
 
 - save some recovery time
 
 

Yes, although we did consider what the best thing to do was back at the
start of GFS2 development wrt LVBs. The actual overhead didn't seem too
much really. The previous implementation had the LVB hanging off the
glock from a pointer, so on 64 bit, that pointer alone was 8 bytes. We
also saved another 4 bytes (plus a further 4 for alignment) by not
requiring the atomic counter. So it seemed not unreasonable to just
inline the LVB into the glock.

Another for having it on all glocks was that if we did want to start
making use of it on different glock types in the future, we could do so
without having to worry about whether its value would be preserved or
not. Also, it removed some tests from the fast path of acquiring and
dropping locks.

Trying to reduce the size of the lock requests makes sense if that is
becoming a limiting factor in performance (is it? I'm not sure) so maybe
we should revisit this.

Steve.

Re: [Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-12 Thread David Teigland 
On Mon, Nov 12, 2012 at 10:44:36AM +, Steven Whitehouse wrote:
  - save 64 bytes of memory for every local lock
(32 in gfs2_glock, 32 in dlm_rsb)
  
  - save 96 bytes of memory for every remote lock
(32 in gfs2_glock, 32 in local dlm_rsb, 32 in remote dlm_lkb)
  
  - save 32 bytes of network message size in many dlm messages
  
  - save a lot of memcpying of zeros
  
  - save some recovery time
  
  
 
 Yes, although we did consider what the best thing to do was back at the
 start of GFS2 development wrt LVBs. The actual overhead didn't seem too
 much really. The previous implementation had the LVB hanging off the
 glock from a pointer, so on 64 bit, that pointer alone was 8 bytes. We
 also saved another 4 bytes (plus a further 4 for alignment) by not
 requiring the atomic counter. So it seemed not unreasonable to just
 inline the LVB into the glock.

I still think you'll save around 64-80 bytes per lock on average.

 Another for having it on all glocks was that if we did want to start
 making use of it on different glock types in the future, we could do so
 without having to worry about whether its value would be preserved or
 not. Also, it removed some tests from the fast path of acquiring and
 dropping locks.

Keep in mind that the dlm does not inline them, so using an lvb when it's
not needed creates extra work in the dlm.  This extra work probably
exceeds the extra work gfs2 would have to do with non-inlined lvbs.

 Trying to reduce the size of the lock requests makes sense if that is
 becoming a limiting factor in performance (is it? I'm not sure) so maybe
 we should revisit this.

I think it's worth a try, it's probably no less helpful than a lot of the
other optimizations we've added, which do add up together.

Re: [Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-09 Thread Steven Whitehouse 
Hi,

On Thu, 2012-11-08 at 16:34 -0500, David Teigland wrote:
 When unmounting, gfs2 does a full dlm_unlock operation on every
 cached lock.  This can create a very large amount of work and can
 take a long time to complete.  However, the vast majority of these
 dlm unlock operations are unnecessary because after all the unlocks
 are done, gfs2 leaves the dlm lockspace, which automatically clears
 the locks of the leaving node, without unlocking each one individually.
 So, gfs2 can skip explicit dlm unlocks, and use dlm_release_lockspace to
 remove the locks implicitly.  The one exception is when the lock's lvb is
 being used.  In this case, dlm_unlock is called because it may update the
 lvb of the resource.
 
 Signed-off-by: David Teigland teigl...@redhat.com
 ---
  fs/gfs2/glock.c|1 +
  fs/gfs2/incore.h   |1 +
  fs/gfs2/lock_dlm.c |8 
  3 files changed, 10 insertions(+)
 
 diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
 index e6c2fd5..f3a5edb 100644
 --- a/fs/gfs2/glock.c
 +++ b/fs/gfs2/glock.c
 @@ -1528,6 +1528,7 @@ static void dump_glock_func(struct gfs2_glock *gl)
  
  void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)
  {
 + set_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags);
   glock_hash_walk(clear_glock, sdp);
   flush_workqueue(glock_workqueue);
   wait_event(sdp-sd_glock_wait, atomic_read(sdp-sd_glock_disposal) == 
 0);
 diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
 index 3d469d3..67a39cf 100644
 --- a/fs/gfs2/incore.h
 +++ b/fs/gfs2/incore.h
 @@ -539,6 +539,7 @@ enum {
   SDF_DEMOTE  = 5,
   SDF_NOJOURNALID = 6,
   SDF_RORECOVERY  = 7, /* read only recovery */
 + SDF_SKIP_DLM_UNLOCK = 8,
  };
  
  #define GFS2_FSNAME_LEN  256
 diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
 index 0fb6539..806a639 100644
 --- a/fs/gfs2/lock_dlm.c
 +++ b/fs/gfs2/lock_dlm.c
 @@ -289,6 +289,14 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
   gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT);
   gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
   gfs2_update_request_times(gl);
 +
 + /* don't want to skip dlm_unlock writing the lvb when lock is ex */
 + if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
 + (!gl-gl_lvb[0] || gl-gl_state != LM_ST_EXCLUSIVE)) {
I'm still not happy with using !gl-gl_lvb[0] to determine whether the
LVB is in use or not. I think we need a better test, or alternatively
just test the lock state, since most locks will be NL anyway before they
get to this point in time,

Steve.

 + gfs2_glock_free(gl);
 + return;
 + }
 +
   error = dlm_unlock(ls-ls_dlm, gl-gl_lksb.sb_lkid, DLM_LKF_VALBLK,
  NULL, gl);
   if (error) {

Re: [Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-09 Thread David Teigland 
On Fri, Nov 09, 2012 at 09:45:17AM +, Steven Whitehouse wrote:
  +   if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
  +   (!gl-gl_lvb[0] || gl-gl_state != LM_ST_EXCLUSIVE)) {
 I'm still not happy with using !gl-gl_lvb[0] to determine whether the
 LVB is in use or not. I think we need a better test, or alternatively
 just test the lock state, since most locks will be NL anyway before they
 get to this point in time,

Yeah, a glock flag indicating the lvb is used would be best, I'll just
test the lock state.

This actually brings up another improvement you could make.  Right now
gfs2 enables the lvb on all locks, even though it only uses it on a small
minority.  Limiting the lvb to locks that need it would:

- save 64 bytes of memory for every local lock
  (32 in gfs2_glock, 32 in dlm_rsb)

- save 96 bytes of memory for every remote lock
  (32 in gfs2_glock, 32 in local dlm_rsb, 32 in remote dlm_lkb)

- save 32 bytes of network message size in many dlm messages

- save a lot of memcpying of zeros

- save some recovery time

[Cluster-devel] [PATCH] gfs2: skip dlm_unlock calls in unmount

2012-11-08 Thread David Teigland 
When unmounting, gfs2 does a full dlm_unlock operation on every
cached lock.  This can create a very large amount of work and can
take a long time to complete.  However, the vast majority of these
dlm unlock operations are unnecessary because after all the unlocks
are done, gfs2 leaves the dlm lockspace, which automatically clears
the locks of the leaving node, without unlocking each one individually.
So, gfs2 can skip explicit dlm unlocks, and use dlm_release_lockspace to
remove the locks implicitly.  The one exception is when the lock's lvb is
being used.  In this case, dlm_unlock is called because it may update the
lvb of the resource.

Signed-off-by: David Teigland teigl...@redhat.com
---
 fs/gfs2/glock.c|1 +
 fs/gfs2/incore.h   |1 +
 fs/gfs2/lock_dlm.c |8 
 3 files changed, 10 insertions(+)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index e6c2fd5..f3a5edb 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1528,6 +1528,7 @@ static void dump_glock_func(struct gfs2_glock *gl)
 
 void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)
 {
+   set_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags);
glock_hash_walk(clear_glock, sdp);
flush_workqueue(glock_workqueue);
wait_event(sdp-sd_glock_wait, atomic_read(sdp-sd_glock_disposal) == 
0);
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 3d469d3..67a39cf 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -539,6 +539,7 @@ enum {
SDF_DEMOTE  = 5,
SDF_NOJOURNALID = 6,
SDF_RORECOVERY  = 7, /* read only recovery */
+   SDF_SKIP_DLM_UNLOCK = 8,
 };
 
 #define GFS2_FSNAME_LEN256
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 0fb6539..806a639 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -289,6 +289,14 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
gfs2_glstats_inc(gl, GFS2_LKS_DCOUNT);
gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
gfs2_update_request_times(gl);
+
+   /* don't want to skip dlm_unlock writing the lvb when lock is ex */
+   if (test_bit(SDF_SKIP_DLM_UNLOCK, sdp-sd_flags) 
+   (!gl-gl_lvb[0] || gl-gl_state != LM_ST_EXCLUSIVE)) {
+   gfs2_glock_free(gl);
+   return;
+   }
+
error = dlm_unlock(ls-ls_dlm, gl-gl_lksb.sb_lkid, DLM_LKF_VALBLK,
   NULL, gl);
if (error) {
-- 
1.7.10.1.362.g242cab3