[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Wengang Wang]

This patch tries to dynamically allocate lvb for dlm_lock_resource which needs to access lvb.

Without the patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
o2dlm_lockres         42     42    256   32    2 : tunables    0    0    0 : slabdata      2      2      0

After patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
o2dlm_lockres         42     42    192   21    1 : tunables    0    0    0 : slabdata      2      2      0

#the result is taken on i686

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlm/dlmast.c    |    2 ++
 fs/ocfs2/dlm/dlmcommon.h |    3 ++-
 fs/ocfs2/dlm/dlmdebug.c  |    6 ++++--
 fs/ocfs2/dlm/dlmlock.c   |    6 ++++++
 fs/ocfs2/dlm/dlmmaster.c |   31 ++++++++++++++++++++++++++++---
 5 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmast.c b/fs/ocfs2/dlm/dlmast.c
index f449991..fe272b1 100644
--- a/fs/ocfs2/dlm/dlmast.c
+++ b/fs/ocfs2/dlm/dlmast.c
@@ -191,6 +191,8 @@ static void dlm_update_lvb(struct dlm_ctxt *dlm, struct dlm_lock_resource *res,
 			mlog(0, "getting lvb from lockres for %s node\n",
 				  lock->ml.node == dlm->node_num ? "master" :
 				  "remote");
+			mlog_bug_on_msg(!res->lvb, "lockname : %.*s\n",
+					res->lockname.len, res->lockname.name);
 			memcpy(lksb->lvb, res->lvb, DLM_LVB_LEN);
 		}
 		/* Do nothing for lvb put requests - they should be done in
diff --git a/fs/ocfs2/dlm/dlmcommon.h b/fs/ocfs2/dlm/dlmcommon.h
index 4b6ae2c..49e6492 100644
--- a/fs/ocfs2/dlm/dlmcommon.h
+++ b/fs/ocfs2/dlm/dlmcommon.h
@@ -329,7 +329,7 @@ struct dlm_lock_resource
 	wait_queue_head_t wq;
 	u8  owner;              //node which owns the lock resource, or unknown
 	u16 state;
-	char lvb[DLM_LVB_LEN];
+	char *lvb;
 	unsigned int inflight_locks;
 	unsigned long refmap[BITS_TO_LONGS(O2NM_MAX_NODES)];
 };
@@ -1033,6 +1033,7 @@ void dlm_clean_master_list(struct dlm_ctxt *dlm,
 int dlm_lock_basts_flushed(struct dlm_ctxt *dlm, struct dlm_lock *lock);
 int __dlm_lockres_has_locks(struct dlm_lock_resource *res);
 int __dlm_lockres_unused(struct dlm_lock_resource *res);
+char *dlm_alloc_lvb(char **lvb);
 
 static inline const char * dlm_lock_mode_name(int mode)
 {
diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index 5efdd37..15a4ca2 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -603,9 +603,11 @@ static int dump_lockres(struct dlm_lock_resource *res, char *buf, int len)
 
 	/* lvb */
 	out += snprintf(buf + out, len - out, "LVBX:");
-	for (i = 0; i < DLM_LVB_LEN; i++)
-		out += snprintf(buf + out, len - out,
+	if (res->lvb) {
+		for (i = 0; i < DLM_LVB_LEN; i++)
+			out += snprintf(buf + out, len - out,
 					"%02x", (unsigned char)res->lvb[i]);
+	}
 	out += snprintf(buf + out, len - out, "\n");
 
 	/* granted */
diff --git a/fs/ocfs2/dlm/dlmlock.c b/fs/ocfs2/dlm/dlmlock.c
index 69cf369..5c7ece7 100644
--- a/fs/ocfs2/dlm/dlmlock.c
+++ b/fs/ocfs2/dlm/dlmlock.c
@@ -425,6 +425,12 @@ static void dlm_init_lock(struct dlm_lock *newlock, int type,
 	kref_init(&newlock->lock_refs);
 }
 
+char *dlm_alloc_lvb(char **lvb)
+{
+	*lvb = kzalloc(DLM_LVB_LEN, GFP_NOFS);
+	return *lvb;
+}
+
 struct dlm_lock * dlm_new_lock(int type, u8 node, u64 cookie,
 			       struct dlm_lockstatus *lksb)
 {
diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index ffb4c68..77315a4 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -545,6 +545,7 @@ static void dlm_lockres_release(struct kref *kref)
 
 	kmem_cache_free(dlm_lockname_cache, (void *)res->lockname.name);
 
+	kfree(res->lvb);
 	kmem_cache_free(dlm_lockres_cache, res);
 }
 
@@ -553,7 +554,20 @@ void dlm_lockres_put(struct dlm_lock_resource *res)
 	kref_put(&res->refs, dlm_lockres_release);
 }
 
-static void dlm_init_lockres(struct dlm_ctxt *dlm,
+static int dlm_lockres_uses_lvb(struct dlm_lock_resource *res)
+{
+	switch (res->lockname.name[0]) {
+	case 'M':
+	case 'Q':
+	case 'R':
+	case 'P':
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static int dlm_init_lockres(struct dlm_ctxt *dlm,
 			     struct dlm_lock_resource *res,
 			     const char *name, unsigned int namelen)
 {
@@ -603,8 +617,16 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 	list_add_tail(&res->tracking, &dlm->tracking_list);
 	spin_unlock(&dlm->spinlock);
 
-	memset(res->lvb, 0, DLM_LVB_LEN);
+	if (dlm_lockres_uses_lvb(res)) {
+		if (!dlm_alloc_lvb(&res->lvb)) {
+			mlog(ML_ERROR, "no memory for lvb\n");
+			return -ENOMEM;
+		}
+	} else {
+		res->lvb = NULL;
+	}
 	memset(res->refmap, 0, sizeof(res->refmap));
+	return 0;
 }
 
 struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
@@ -612,6 +634,7 @@ struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
 				   unsigned int namelen)
 {
 	struct dlm_lock_resource *res = NULL;
+	int ret;
 
 	res = kmem_cache_zalloc(dlm_lockres_cache, GFP_NOFS);
 	if (!res)
@@ -621,7 +644,9 @@ struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
 	if (!res->lockname.name)
 		goto error;
 
-	dlm_init_lockres(dlm, res, name, namelen);
+	ret = dlm_init_lockres(dlm, res, name, namelen);
+	if (ret) 
+		goto error;
 	return res;
 
 error:
-- 
1.7.2.1

[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Sunil Mushran]

On 08/26/2010 06:06 AM, Wengang Wang wrote:
> This patch tries to dynamically allocate lvb for dlm_lock_resource which needs to access lvb.
>
> Without the patch applied,
> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
> o2dlm_lockres         42     42    256   32    2 : tunables    0    0    0 : slabdata      2      2      0
>
> After patch applied,
> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
> o2dlm_lockres         42     42    192   21    1 : tunables    0    0    0 : slabdata      2      2      0
>
> #the result is taken on i686
>
> Signed-off-by: Wengang Wang<wen.gang.wang@oracle.com>
> ---
>   fs/ocfs2/dlm/dlmast.c    |    2 ++
>   fs/ocfs2/dlm/dlmcommon.h |    3 ++-
>   fs/ocfs2/dlm/dlmdebug.c  |    6 ++++--
>   fs/ocfs2/dlm/dlmlock.c   |    6 ++++++
>   fs/ocfs2/dlm/dlmmaster.c |   31 ++++++++++++++++++++++++++++---
>   5 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmast.c b/fs/ocfs2/dlm/dlmast.c
> index f449991..fe272b1 100644
> --- a/fs/ocfs2/dlm/dlmast.c
> +++ b/fs/ocfs2/dlm/dlmast.c
> @@ -191,6 +191,8 @@ static void dlm_update_lvb(struct dlm_ctxt *dlm, struct dlm_lock_resource *res,
>   			mlog(0, "getting lvb from lockres for %s node\n",
>   				  lock->ml.node == dlm->node_num ? "master" :
>   				  "remote");
> +			mlog_bug_on_msg(!res->lvb, "lockname : %.*s\n",
> +					res->lockname.len, res->lockname.name);
>   			memcpy(lksb->lvb, res->lvb, DLM_LVB_LEN);
>   		}
>   		/* Do nothing for lvb put requests - they should be done in
> diff --git a/fs/ocfs2/dlm/dlmcommon.h b/fs/ocfs2/dlm/dlmcommon.h
> index 4b6ae2c..49e6492 100644
> --- a/fs/ocfs2/dlm/dlmcommon.h
> +++ b/fs/ocfs2/dlm/dlmcommon.h
> @@ -329,7 +329,7 @@ struct dlm_lock_resource
>   	wait_queue_head_t wq;
>   	u8  owner;              //node which owns the lock resource, or unknown
>   	u16 state;
> -	char lvb[DLM_LVB_LEN];
> +	char *lvb;
>   	unsigned int inflight_locks;
>   	unsigned long refmap[BITS_TO_LONGS(O2NM_MAX_NODES)];
>   };
> @@ -1033,6 +1033,7 @@ void dlm_clean_master_list(struct dlm_ctxt *dlm,
>   int dlm_lock_basts_flushed(struct dlm_ctxt *dlm, struct dlm_lock *lock);
>   int __dlm_lockres_has_locks(struct dlm_lock_resource *res);
>   int __dlm_lockres_unused(struct dlm_lock_resource *res);
> +char *dlm_alloc_lvb(char **lvb);
>
>   static inline const char * dlm_lock_mode_name(int mode)
>   {
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index 5efdd37..15a4ca2 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -603,9 +603,11 @@ static int dump_lockres(struct dlm_lock_resource *res, char *buf, int len)
>
>   	/* lvb */
>   	out += snprintf(buf + out, len - out, "LVBX:");
> -	for (i = 0; i<  DLM_LVB_LEN; i++)
> -		out += snprintf(buf + out, len - out,
> +	if (res->lvb) {
> +		for (i = 0; i<  DLM_LVB_LEN; i++)
> +			out += snprintf(buf + out, len - out,
>   					"%02x", (unsigned char)res->lvb[i]);
> +	}
>   	out += snprintf(buf + out, len - out, "\n");
>
>   	/* granted */
> diff --git a/fs/ocfs2/dlm/dlmlock.c b/fs/ocfs2/dlm/dlmlock.c
> index 69cf369..5c7ece7 100644
> --- a/fs/ocfs2/dlm/dlmlock.c
> +++ b/fs/ocfs2/dlm/dlmlock.c
> @@ -425,6 +425,12 @@ static void dlm_init_lock(struct dlm_lock *newlock, int type,
>   	kref_init(&newlock->lock_refs);
>   }
>
> +char *dlm_alloc_lvb(char **lvb)
> +{
> +	*lvb = kzalloc(DLM_LVB_LEN, GFP_NOFS);
> +	return *lvb;
> +}
> +
>    

I am not sold on this still.

>   struct dlm_lock * dlm_new_lock(int type, u8 node, u64 cookie,
>   			       struct dlm_lockstatus *lksb)
>   {
> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> index ffb4c68..77315a4 100644
> --- a/fs/ocfs2/dlm/dlmmaster.c
> +++ b/fs/ocfs2/dlm/dlmmaster.c
> @@ -545,6 +545,7 @@ static void dlm_lockres_release(struct kref *kref)
>
>   	kmem_cache_free(dlm_lockname_cache, (void *)res->lockname.name);
>
> +	kfree(res->lvb);
>   	kmem_cache_free(dlm_lockres_cache, res);
>   }
>
> @@ -553,7 +554,20 @@ void dlm_lockres_put(struct dlm_lock_resource *res)
>   	kref_put(&res->refs, dlm_lockres_release);
>   }
>
> -static void dlm_init_lockres(struct dlm_ctxt *dlm,
> +static int dlm_lockres_uses_lvb(struct dlm_lock_resource *res)
> +{
> +	switch (res->lockname.name[0]) {
> +	case 'M':
> +	case 'Q':
> +	case 'R':
> +	case 'P':
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
>    

How about just passing ptr to the lockname. That will make
patch#2 a lot smaller.

> +
> +static int dlm_init_lockres(struct dlm_ctxt *dlm,
>   			     struct dlm_lock_resource *res,
>   			     const char *name, unsigned int namelen)
>   {
> @@ -603,8 +617,16 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
>   	list_add_tail(&res->tracking,&dlm->tracking_list);
>   	spin_unlock(&dlm->spinlock);
>
> -	memset(res->lvb, 0, DLM_LVB_LEN);
> +	if (dlm_lockres_uses_lvb(res)) {
> +		if (!dlm_alloc_lvb(&res->lvb)) {
> +			mlog(ML_ERROR, "no memory for lvb\n");
> +			return -ENOMEM;
> +		}
> +	} else {
> +		res->lvb = NULL;
> +	}
>    

Move the allocation to the caller dlm_new_lockres. This way
all allocs for the lockres are done in one place.

>   	memset(res->refmap, 0, sizeof(res->refmap));
> +	return 0;
>   }
>
>   struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
> @@ -612,6 +634,7 @@ struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
>   				   unsigned int namelen)
>   {
>   	struct dlm_lock_resource *res = NULL;
> +	int ret;
>
>   	res = kmem_cache_zalloc(dlm_lockres_cache, GFP_NOFS);
>   	if (!res)
> @@ -621,7 +644,9 @@ struct dlm_lock_resource *dlm_new_lockres(struct dlm_ctxt *dlm,
>   	if (!res->lockname.name)
>   		goto error;
>
> -	dlm_init_lockres(dlm, res, name, namelen);
> +	ret = dlm_init_lockres(dlm, res, name, namelen);
> +	if (ret)
> +		goto error;
>   	return res;
>
>   error:
>    

[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Sunil Mushran]

On 08/26/2010 06:06 AM, Wengang Wang wrote:
> This patch tries to dynamically allocate lvb for dlm_lock_resource which needs to access lvb.
>
> Without the patch applied,
> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
> o2dlm_lockres         42     42    256   32    2 : tunables    0    0    0 : slabdata      2      2      0
>
> After patch applied,
> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
> o2dlm_lockres         42     42    192   21    1 : tunables    0    0    0 : slabdata      2      2      0
>
> #the result is taken on i686
>    

So the core logic allocates a lvb or not based on the lock name. That
will not work because we support userdlm (not to be confused with
userspace stack that uses fsdlm) that allows the user to specify the name.

A better solution is to make the user pass in a flag to create the lvb.
That's one issue.

The other issue concerns the real savings. While the savings on a per
lockres basis are impressive (will be even more on a 64-bit system), I
am unsure on the overall savings.

To check that, run some workload... like a kernel build (one node should
be sufficient) and gather some numbers below.

# cd /sys/kernel/debug/o2dlm/<domain>
# grep -h "^NAME:" locking_state | sort | cut -c6 | uniq -c

Marcos, Can you also gather this stat when you run metadata heavy
tests.

Thanks
Sunil

[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[MARCOS MATSUNAGA]

  Sunil,

I ran buildkernel and collect some of the stats that you requested.

You can find them at http://oss.oracle.com/~mmatsuna/ocfs2-stats/ and 
the files are:
ocfs2-stat-6850.log 
<http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-6850.log>

ocfs2-stat-6942.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-6942.log>
ocfs2-stat-10059.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-10059.log>
ocfs2-stat-32460.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-32460.log>


I collected samples every 30 seconds on all nodes. Each node starts two 
builds in parallel and the make command is "make -j2 V=1".

On 9/13/2010 1:54 PM, Sunil Mushran wrote:
> On 08/26/2010 06:06 AM, Wengang Wang wrote:
>> This patch tries to dynamically allocate lvb for dlm_lock_resource which needs to access lvb.
>>
>> Without the patch applied,
>> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
>> o2dlm_lockres         42     42    256   32    2 : tunables    0    0    0 : slabdata      2      2      0
>>
>> After patch applied,
>> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
>> o2dlm_lockres         42     42    192   21    1 : tunables    0    0    0 : slabdata      2      2      0
>>
>> #the result is taken on i686
>>
> So the core logic allocates a lvb or not based on the lock name. That
> will not work because we support userdlm (not to be confused with
> userspace stack that uses fsdlm) that allows the user to specify the name.
>
> A better solution is to make the user pass in a flag to create the lvb.
> That's one issue.
>
> The other issue concerns the real savings. While the savings on a per
> lockres basis are impressive (will be even more on a 64-bit system), I
> am unsure on the overall savings.
>
> To check that, run some workload... like a kernel build (one node should
> be sufficient) and gather some numbers below.
>
> # cd /sys/kernel/debug/o2dlm/<domain>
> # grep -h "^NAME:" locking_state | sort | cut -c6 | uniq -c
>
> Marcos, Can you also gather this stat when you run metadata heavy
> tests.
>
> Thanks
> Sunil
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100913/f6861dd3/attachment.html 

[Ocfs2-devel] [PATCH 1/5] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Sunil Mushran]

===== 2010-09-13 : 20:25 UUID:4D5296E659E349F0997FA8997E007D8E =====
   85733 M
   45077 N
   85714 O
       1 P
       2 Q
       1 S
    2249 W
       1 Y


I guess the feature will be useful considering the number of lockres'
with lvb is less than half. Sometimes a third. So significant savings.

Having said that, it should be noted that the test itself is a bit biased
as a kernel build will have more than normal number of open/dentry
locks.

Still, I would vote for the savings.

Wengang,
BTW, we are already passing the lvb flag. Grep on DLM_LKF_VALBLK
and LKM_VALBLK. Much better if we key on that rather than the lock
name.

Sunil

On 09/13/2010 08:48 PM, MARCOS MATSUNAGA wrote:
> Sunil,
>
> I ran buildkernel and collect some of the stats that you requested.
>
> You can find them at http://oss.oracle.com/~mmatsuna/ocfs2-stats/ and 
> the files are:
> ocfs2-stat-6850.log 
> <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-6850.log>
> ocfs2-stat-6942.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-6942.log>
> ocfs2-stat-10059.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-10059.log>
> ocfs2-stat-32460.log  <http://oss.oracle.com/%7Emmatsuna/ocfs2-stats/ocfs2-stat-32460.log>
>
> I collected samples every 30 seconds on all nodes. Each node starts 
> two builds in parallel and the make command is "make -j2 V=1".
>
> On 9/13/2010 1:54 PM, Sunil Mushran wrote:
>> On 08/26/2010 06:06 AM, Wengang Wang wrote:
>>      
>>> This patch tries to dynamically allocate lvb for dlm_lock_resource which needs to access lvb.
>>>
>>> Without the patch applied,
>>> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
>>> o2dlm_lockres         42     42    256   32    2 : tunables    0    0    0 : slabdata      2      2      0
>>>
>>> After patch applied,
>>> [wwg at cool linux-2.6]$ egrep "o2dlm_lockres" /proc/slabinfo
>>> o2dlm_lockres         42     42    192   21    1 : tunables    0    0    0 : slabdata      2      2      0
>>>
>>> #the result is taken on i686
>>>
>>>        
>> So the core logic allocates a lvb or not based on the lock name. That
>> will not work because we support userdlm (not to be confused with
>> userspace stack that uses fsdlm) that allows the user to specify the name.
>>
>> A better solution is to make the user pass in a flag to create the lvb.
>> That's one issue.
>>
>> The other issue concerns the real savings. While the savings on a per
>> lockres basis are impressive (will be even more on a 64-bit system), I
>> am unsure on the overall savings.
>>
>> To check that, run some workload... like a kernel build (one node should
>> be sufficient) and gather some numbers below.
>>
>> # cd /sys/kernel/debug/o2dlm/<domain>
>> # grep -h "^NAME:" locking_state | sort | cut -c6 | uniq -c
>>
>> Marcos, Can you also gather this stat when you run metadata heavy
>> tests.
>>
>> Thanks
>> Sunil
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel at oss.oracle.com
>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>      

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100914/1763125a/attachment.html