ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource
PATCH 1/1 ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource
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/slabinfoo2dlm_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/slabinfoo2dlm_lockres 42 42 192 21 1 : tunables 0 0 0 : slabdata 2 2 0
#that's on i686.
--- 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(-)
fs/ocfs2/dlm/dlmast.c +2 -0
@@ -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
fs/ocfs2/dlm/dlmcommon.h +2 -1
@@ -329,7 +329,7 @@ struct dlm_lock_resourcewait_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) {
fs/ocfs2/dlm/dlmlock.c +6 -0
@@ -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) {
fs/ocfs2/dlm/dlmmaster.c +28 -3
@@ -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:
Signature · 1 line
1.7.2.1
PATCH 1/1 ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource
Quoted 9 lines · Wengang Wang
Not all
dlm_lock_resourceuse lvb. It's a waste of memory for thosedlm_lock_resourcessince lvb is a "char lvb[64];".This patch allocates lvb for
dlm_lock_resourceswho use lvb.Without the patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres"/proc/slabinfoo2dlm_lockres42 42 256 32 2 : tunables 0 0 0 : slabdata 2 2 0
Quoted 6 lines
After patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres"/proc/slabinfoo2dlm_lockres42 42 192 21 1 : tunables 0 0 0 : slabdata 2 2 0#that's on i686.
nice.
Quoted 13 lines
Signed-off-by Wengang Wang<[email protected]>--- 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(-)fs/ocfs2/dlm/dlmast.c +0
-0
fs/ocfs2/dlm/dlmast.c static void dlm_update_lvb(struct dlm_ctxt *dlm, struct dlm_lock_resource *res, · line 198
Quoted 8 lines
fs/ocfs2/dlm/dlmast.c +2
-0@@ -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.
Quoted 27 lines
/* Do nothing for lvb put requests - they should be done in
fs/ocfs2/dlm/dlmcommon.h +2
-1@@ -329,7 +329,7 @@ struct dlm_lock_resourcewait_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) {fs/ocfs2/dlm/dlmlock.c +0
-0
fs/ocfs2/dlm/dlmlock.c static void dlm_init_lock(struct dlm_lock *newlock, int type, · line 433
Quoted 9 lines
fs/ocfs2/dlm/dlmlock.c +5
-0@@ -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.
Quoted 76 lines
fs/ocfs2/dlm/dlmlock.c +1
-0+struct dlm_lock *dlm_new_lock (int type,u8 node,u64 cookie,struct dlm_lockstatus *lksb){fs/ocfs2/dlm/dlmmaster.c +28
-3@@ -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:
PATCH 1/1 ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource
Quoted 26 lines · Sunil Mushran
Quoted 9 lines · Wengang Wang
Not all
dlm_lock_resourceuse lvb. It's a waste of memory for thosedlm_lock_resourcessince lvb is a "char lvb[64];".This patch allocates lvb for
dlm_lock_resourceswho use lvb.Without the patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres"/proc/slabinfoo2dlm_lockres42 42 256 32 2 : tunables 0 0 0 : slabdata 2 2 0Quoted 5 lines
After patch applied,
[wwg at cool linux-2.6]$ egrep "o2dlm_lockres"/proc/slabinfoo2dlm_lockres42 42 192 21 1 : tunables 0 0 0 : slabdata 2 2 0#that's on i686.
nice.
Quoted 8 lines
Signed-off-by Wengang Wang<[email protected]>--- 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(-)
Quoted 15 lines
fs/ocfs2/dlm/dlmast.c static void dlm_update_lvb(struct dlm_ctxt *dlm, struct dlm_lock_resource *res, · line 197
Quoted 12 lines
fs/ocfs2/dlm/dlmast.c +2
-0@@ -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)
Quoted 31 lines
Quoted 31 lines
fs/ocfs2/dlm/dlmast.c +0
-0/* Do nothing for lvb put requests - they should be done in fs/ocfs2/dlm/dlmcommon.h +2
-1@@ -329,7 +329,7 @@ struct dlm_lock_resourcewait_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) {fs/ocfs2/dlm/dlmlock.c +0
-0@@ -425,6 +425,12 @@ static void dlm_init_lock(struct dlm_lock *newlock, int type,kref_init (&newlock->lock_refs); }
Quoted 8 lines
fs/ocfs2/dlm/dlmlock.c static void dlm_init_lock(struct dlm_lock *newlock, int type, · line 432
Quoted 5 lines
fs/ocfs2/dlm/dlmlock.c +5
-0+char *dlm_alloc_lvb (char **lvb) +{ + *lvb =kzalloc (DLM_LVB_LEN ,GFP_NOFS ); +return *lvb; +}Scrap this function. Do the
kzalloc()indlm_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.
PATCH 1/1 ocfs2/dlm: dynamically allocate lvb for dlm_lock_resource
Quoted 8 lines · Wengang Wang
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 indlm_update_lvb()its self. And I think checkingDLM_LKSB_GET_LVBflag 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.
Quoted 8 lines
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 fordlm_lockis 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.