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

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

[Wengang Wang]

Not all dlm_lock_resource use lvb. It's a waste of memory for those
dlm_lock_resources since lvb is a "char lvb[64];".

This patch allocates lvb for dlm_lock_resources who use 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

#that's 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/dlmlock.c   |    6 ++++++
 fs/ocfs2/dlm/dlmmaster.c |   31 ++++++++++++++++++++++++++++---
 4 files changed, 38 insertions(+), 4 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/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] ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource

[Sunil Mushran]

On 08/21/2010 04:13 PM, Wengang Wang wrote:
> Not all dlm_lock_resource use lvb. It's a waste of memory for those
> dlm_lock_resources since lvb is a "char lvb[64];".
>
> This patch allocates lvb for dlm_lock_resources who use 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
>
> #that's on i686.
>    

nice.

> 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/dlmlock.c   |    6 ++++++
>   fs/ocfs2/dlm/dlmmaster.c |   31 ++++++++++++++++++++++++++++---
>   4 files changed, 38 insertions(+), 4 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);
>   		}
>    

Move this to the top of the function. We want to catch this bug as
early as possible.

>   		/* 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/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;
> +}
>    

Scrap this function. Do the kzalloc() in dlm_new_lockres() itself.
This way we do allocs for a lockres in one location.

> +
>   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:
>    

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

[Wengang Wang]

On 10-08-24 11:09, Sunil Mushran wrote:
> On 08/21/2010 04:13 PM, Wengang Wang wrote:
> >Not all dlm_lock_resource use lvb. It's a waste of memory for those
> >dlm_lock_resources since lvb is a "char lvb[64];".
> >
> >This patch allocates lvb for dlm_lock_resources who use 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
> >
> >#that's on i686.
> 
> nice.
> 
> >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/dlmlock.c   |    6 ++++++
> >  fs/ocfs2/dlm/dlmmaster.c |   31 ++++++++++++++++++++++++++++---
> >  4 files changed, 38 insertions(+), 4 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);
> >  		}
> 
> Move this to the top of the function. We want to catch this bug as
> early as possible.

It looks we can't move it to the very top.
The caller doesn't make sure we need to update lvb. In other words, it
treat the lockres' that need lvb and those don't the same. We have to
distinguish them in dlm_update_lvb() its self. And I think checking 
DLM_LKSB_GET_LVB flag is the right place to place the assertion.

Or do you meant we should treat different type (lvb needed or no) of
lockress differently in the callers?(though I don't think you meant this)

> 
> >  		/* 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/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;
> >+}
> 
> Scrap this function. Do the kzalloc() in dlm_new_lockres() itself.
> This way we do allocs for a lockres in one location.

Actually I alreay have another similar patch which dynamically allocates
lvb for dlm_lock. dlm_alloc_lvb() is used in that patch. So allocating
lvb is in a separated function.
Since the patch for dlm_lock is much more complex than this one, I
didn't post it out together before this one gets approved.

Anyway if you persist, I can make it as you wanted.

regards,
wengang.

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

[Sunil Mushran]

  On 8/25/2010 5:31 AM, Wengang Wang wrote:
> It looks we can't move it to the very top.
> The caller doesn't make sure we need to update lvb. In other words, it
> treat the lockres' that need lvb and those don't the same. We have to
> distinguish them in dlm_update_lvb() its self. And I think checking
> DLM_LKSB_GET_LVB flag is the right place to place the assertion.
>
> Or do you meant we should treat different type (lvb needed or no) of
> lockress differently in the callers?(though I don't think you meant this)

ok.

>
> Actually I alreay have another similar patch which dynamically allocates
> lvb for dlm_lock. dlm_alloc_lvb() is used in that patch. So allocating
> lvb is in a separated function.
> Since the patch for dlm_lock is much more complex than this one, I
> didn't post it out together before this one gets approved.
>
> Anyway if you persist, I can make it as you wanted.

Then post both the patches.