ocfs2/dlm: protect 'tracking_list' by 'track_lock'
PATCH 1/2 ocfs2/dlm: protect 'tracking_list' by 'track_lock'
'dlm->tracking_list' need to be protected by 'dlm->track_lock'.
--- fs/ocfs2/dlm/dlmdomain.c | 7 ++++++- fs/ocfs2/dlm/dlmmaster.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +6 -1
@@ -726,12 +726,17 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm) }/* This list should be empty. If not, print remaining lockres */ +spin_lock (&dlm->track_lock);if (!list_empty (&dlm->tracking_list)) {mlog (ML_ERROR ,"Following lockres' are still on the " "tracking list:\n" ); -list_for_each_entry (res, &dlm->tracking_list, tracking) +list_for_each_entry (res, &dlm->tracking_list, tracking) { +spin_unlock (&dlm->track_lock);dlm_print_one_lock_resource (res); +spin_lock (&dlm->track_lock); + } } +spin_unlock (&dlm->track_lock);dlm_mark_domain_leaving (dlm);dlm_leave_domain (dlm);
fs/ocfs2/dlm/dlmmaster.c +2 -2
@@ -589,9 +589,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm, res->last_used =0 ; -spin_lock (&dlm->spinlock); +spin_lock (&dlm->track_lock);list_add_tail (&res->tracking, &dlm->tracking_list); -spin_unlock (&dlm->spinlock); +spin_unlock (&dlm->track_lock);memset (res->lvb,0 ,DLM_LVB_LEN );memset (res->refmap,0 ,sizeof (res->refmap));
PATCH 1/2 ocfs2/dlm: protect 'tracking_list' by 'track_lock'
Quoted 14 lines · piaojun
'
dlm->tracking_list' need to be protected by 'dlm->track_lock'.Signed-off-by Jun Piao <[email protected]>Reviewed-by Alex Chen <[email protected]>--- fs/ocfs2/dlm/dlmdomain.c | 7 ++++++- fs/ocfs2/dlm/dlmmaster.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmdomain.c +0
-0
fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 734
Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +3
-1@@ -726,12 +726,17 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm) }/* This list should be empty. If not, print remaining lockres */ +spin_lock (&dlm->track_lock);if (!list_empty (&dlm->tracking_list)) {mlog (ML_ERROR ,"Following lockres' are still on the " "tracking list:\n" ); -list_for_each_entry (res, &dlm->tracking_list, tracking) +list_for_each_entry (res, &dlm->tracking_list, tracking) { +spin_unlock (&dlm->track_lock);
Um... If we unlock here, the iterator still has chance to be corrupted.
Thanks,
Joseph
Quoted 25 lines
fs/ocfs2/dlm/dlmdomain.c +3
-0dlm_print_one_lock_resource (res); +spin_lock (&dlm->track_lock); + } } +spin_unlock (&dlm->track_lock);dlm_mark_domain_leaving (dlm);dlm_leave_domain (dlm);fs/ocfs2/dlm/dlmmaster.c +2
-2@@ -589,9 +589,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm, res->last_used =0 ; -spin_lock (&dlm->spinlock); +spin_lock (&dlm->track_lock);list_add_tail (&res->tracking, &dlm->tracking_list); -spin_unlock (&dlm->spinlock); +spin_unlock (&dlm->track_lock);memset (res->lvb,0 ,DLM_LVB_LEN );memset (res->refmap,0 ,sizeof (res->refmap));
PATCH 1/2 ocfs2/dlm: protect 'tracking_list' by 'track_lock'
Quoted 30 lines · Joseph Qi
Quoted 14 lines · piaojun
'
dlm->tracking_list' need to be protected by 'dlm->track_lock'.Signed-off-by Jun Piao <[email protected]>Reviewed-by Alex Chen <[email protected]>--- fs/ocfs2/dlm/dlmdomain.c | 7 ++++++- fs/ocfs2/dlm/dlmmaster.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmdomain.c +0
-0fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 734
Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +3
-1@@ -726,12 +726,17 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm) }/* This list should be empty. If not, print remaining lockres */ +spin_lock (&dlm->track_lock);if (!list_empty (&dlm->tracking_list)) {mlog (ML_ERROR ,"Following lockres' are still on the " "tracking list:\n" ); -list_for_each_entry (res, &dlm->tracking_list, tracking) +list_for_each_entry (res, &dlm->tracking_list, tracking) { +spin_unlock (&dlm->track_lock);Um... If we unlock here, the iterator still has chance to be corrupted.
Quoted 3 lines
Thanks,
Joseph
we don't need care much about the corrupted 'tracking_list' because we have already picked up 'res' from 'tracking_list'. then we will get 'track_lock' again to prevent 'tracking_list' from being corrupted. but I'd better make sure that 'res' is not NULL before printing, just like:
list_for_each_entry(res, &dlm->tracking_list, tracking) {
spin_unlock(&dlm->track_lock);
if (res)
dlm_print_one_lock_resource(res);
spin_lock(&dlm->track_lock);
}Thanks
Jun
Quoted 27 lines
Quoted 15 lines
fs/ocfs2/dlm/dlmdomain.c +3
-0dlm_print_one_lock_resource (res); +spin_lock (&dlm->track_lock); + } } +spin_unlock (&dlm->track_lock);dlm_mark_domain_leaving (dlm);dlm_leave_domain (dlm);fs/ocfs2/dlm/dlmmaster.c +0
-0@@ -589,9 +589,9 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,fs/ocfs2/dlm/dlmmaster.c static void dlm_init_lockres(struct dlm_ctxt *dlm, · line 597
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +2
-2res->last_used =0 ; -spin_lock (&dlm->spinlock); +spin_lock (&dlm->track_lock);list_add_tail (&res->tracking, &dlm->tracking_list); -spin_unlock (&dlm->spinlock); +spin_unlock (&dlm->track_lock);memset (res->lvb,0 ,DLM_LVB_LEN );memset (res->refmap,0 ,sizeof (res->refmap));.
PATCH 1/2 ocfs2/dlm: protect 'tracking_list' by 'track_lock'
Quoted 49 lines · piaojun
Quoted 30 lines · Joseph Qi
Quoted 14 lines · piaojun
'
dlm->tracking_list' need to be protected by 'dlm->track_lock'.Signed-off-by Jun Piao <[email protected]>Reviewed-by Alex Chen <[email protected]>--- fs/ocfs2/dlm/dlmdomain.c | 7 ++++++- fs/ocfs2/dlm/dlmmaster.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmdomain.c +0
-0fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 734
Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +3
-1@@ -726,12 +726,17 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm) }/* This list should be empty. If not, print remaining lockres */ +spin_lock (&dlm->track_lock);if (!list_empty (&dlm->tracking_list)) {mlog (ML_ERROR ,"Following lockres' are still on the " "tracking list:\n" ); -list_for_each_entry (res, &dlm->tracking_list, tracking) +list_for_each_entry (res, &dlm->tracking_list, tracking) { +spin_unlock (&dlm->track_lock);Um... If we unlock here, the iterator still has chance to be corrupted.
Quoted 3 lines
Thanks,
Josephwe don't need care much about the corrupted '
tracking_list' because we have already picked up 'res' from 'tracking_list'. then we will get 'track_lock' again to prevent 'tracking_list' from being corrupted. but I'd better make sure that 'res' is not NULL before printing, just like:list_for_each_entry(res, &dlm->tracking_list, tracking) { spin_unlock(&dlm->track_lock); if (res) dlm_print_one_lock_resource(res); spin_lock(&dlm->track_lock); }
Quoted 2 lines
Thanks
Jun
IIUC, your intent to add track lock here is to protect tracking list when iterate the loop, right? I am saying that if unlock track lock here, the loop is still unsafe.
Checking res here is meaningless. Maybe list_for_each_entry_safe could work here.
BTW, how this race case happens? The above code is during umount, what is the other flow?
Thanks,
Joseph
PATCH 1/2 ocfs2/dlm: protect 'tracking_list' by 'track_lock'
Quoted 63 lines · Joseph Qi
Quoted 49 lines · piaojun
Quoted 30 lines · Joseph Qi
Quoted 14 lines · piaojun
'
dlm->tracking_list' need to be protected by 'dlm->track_lock'.Signed-off-by Jun Piao <[email protected]>Reviewed-by Alex Chen <[email protected]>--- fs/ocfs2/dlm/dlmdomain.c | 7 ++++++- fs/ocfs2/dlm/dlmmaster.c | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmdomain.c +0
-0fs/ocfs2/dlm/dlmdomain.c void dlm_unregister_domain(struct dlm_ctxt *dlm) · line 734
Quoted 10 lines
fs/ocfs2/dlm/dlmdomain.c +3
-1@@ -726,12 +726,17 @@ void dlm_unregister_domain(struct dlm_ctxt *dlm) }/* This list should be empty. If not, print remaining lockres */ +spin_lock (&dlm->track_lock);if (!list_empty (&dlm->tracking_list)) {mlog (ML_ERROR ,"Following lockres' are still on the " "tracking list:\n" ); -list_for_each_entry (res, &dlm->tracking_list, tracking) +list_for_each_entry (res, &dlm->tracking_list, tracking) { +spin_unlock (&dlm->track_lock);Um... If we unlock here, the iterator still has chance to be corrupted.
Quoted 3 lines
Thanks,
Josephwe don't need care much about the corrupted '
tracking_list' because we have already picked up 'res' from 'tracking_list'. then we will get 'track_lock' again to prevent 'tracking_list' from being corrupted. but I'd better make sure that 'res' is not NULL before printing, just like:list_for_each_entry(res, &dlm->tracking_list, tracking) { spin_unlock(&dlm->track_lock); if (res) dlm_print_one_lock_resource(res); spin_lock(&dlm->track_lock); }Quoted 2 lines
Thanks
JunIIUC, your intent to add track lock here is to protect tracking list when iterate the loop, right? I am saying that if unlock track lock here, the loop is still unsafe.
Checking res here is meaningless. Maybelist_for_each_entry_safecould work here.
BTW, how this race case happens? The above code is during umount, what is the other flow?
Quoted 4 lines
Thanks,
Joseph
.
I have not caught the race case yet, and the code rarely enter this branch, because 'tracking list' is always empty here. the key problem is we try to protect 'tracking list' under 'res->spinlock' and 'dlm->track_lock', but we could not get 'res->spinlock' before iterating 'tracking list'. I have to figured it out further.
thanks
Jun