* [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource()
@ 2008-11-28 13:03 Coly Li
2008-12-02 2:43 ` Sunil Mushran
0 siblings, 1 reply; 5+ messages in thread
From: Coly Li @ 2008-11-28 13:03 UTC (permalink / raw)
To: ocfs2-devel
Hi list,
When I read code of dlm_get_lock_resource(), there is something not clear to me.
719 lookup:
720 spin_lock(&dlm->spinlock);
721 tmpres = __dlm_lookup_lockres_full(dlm, lockid, namelen, hash);
722 if (tmpres) {
723 int dropping_ref = 0;
724
725 spin_lock(&tmpres->spinlock);
726 if (tmpres->owner == dlm->node_num) {
727 BUG_ON(tmpres->state & DLM_LOCK_RES_DROPPING_REF);
728 dlm_lockres_grab_inflight_ref(dlm, tmpres);
729 } else if (tmpres->state & DLM_LOCK_RES_DROPPING_REF)
730 dropping_ref = 1;
731 spin_unlock(&tmpres->spinlock);
732 spin_unlock(&dlm->spinlock);
733
734 /* wait until done messaging the master, drop our ref to allow
735 * the lockres to be purged, start over. */
736 if (dropping_ref) {
737 spin_lock(&tmpres->spinlock);
738 __dlm_wait_on_lockres_flags(tmpres, DLM_LOCK_RES_DROPPING_REF);
739 spin_unlock(&tmpres->spinlock);
740 dlm_lockres_put(tmpres);
741 tmpres = NULL;
742 goto lookup;
743 }
744
745 mlog(0, "found in hash!\n");
746 if (res)
747 dlm_lockres_put(res);
748 res = tmpres;
749 goto leave;
750 }
If at line 721 tmpres found from hash, and its state is not DLM_LOCK_RES_DROPPING_REF (dropping_ref
is 0), between line 733 and 748, is it possible to set tmpres->state to DLM_LOCK_RES_DROPPING_REF ?
I don't see any protection for this race, maybe there is something I missed. Can anybody give me a
hint ?
Thanks.
--
Coly Li
SuSE PRC Labs
^ permalink raw reply [flat|nested] 5+ messages in thread* [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource()
2008-11-28 13:03 [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource() Coly Li
@ 2008-12-02 2:43 ` Sunil Mushran
2008-12-02 16:49 ` Coly Li
0 siblings, 1 reply; 5+ messages in thread
From: Sunil Mushran @ 2008-12-02 2:43 UTC (permalink / raw)
To: ocfs2-devel
No. dlm->spinlock prevents it racing the dlm_thread. Secondly, lookup
also does a get.
What scenario are you envisioning?
Coly Li wrote:
> Hi list,
>
> When I read code of dlm_get_lock_resource(), there is something not clear to me.
>
> 719 lookup:
> 720 spin_lock(&dlm->spinlock);
> 721 tmpres = __dlm_lookup_lockres_full(dlm, lockid, namelen, hash);
> 722 if (tmpres) {
> 723 int dropping_ref = 0;
> 724
> 725 spin_lock(&tmpres->spinlock);
> 726 if (tmpres->owner == dlm->node_num) {
> 727 BUG_ON(tmpres->state & DLM_LOCK_RES_DROPPING_REF);
> 728 dlm_lockres_grab_inflight_ref(dlm, tmpres);
> 729 } else if (tmpres->state & DLM_LOCK_RES_DROPPING_REF)
> 730 dropping_ref = 1;
> 731 spin_unlock(&tmpres->spinlock);
> 732 spin_unlock(&dlm->spinlock);
> 733
> 734 /* wait until done messaging the master, drop our ref to allow
> 735 * the lockres to be purged, start over. */
> 736 if (dropping_ref) {
> 737 spin_lock(&tmpres->spinlock);
> 738 __dlm_wait_on_lockres_flags(tmpres, DLM_LOCK_RES_DROPPING_REF);
> 739 spin_unlock(&tmpres->spinlock);
> 740 dlm_lockres_put(tmpres);
> 741 tmpres = NULL;
> 742 goto lookup;
> 743 }
> 744
> 745 mlog(0, "found in hash!\n");
> 746 if (res)
> 747 dlm_lockres_put(res);
> 748 res = tmpres;
> 749 goto leave;
> 750 }
>
> If at line 721 tmpres found from hash, and its state is not DLM_LOCK_RES_DROPPING_REF (dropping_ref
> is 0), between line 733 and 748, is it possible to set tmpres->state to DLM_LOCK_RES_DROPPING_REF ?
>
> I don't see any protection for this race, maybe there is something I missed. Can anybody give me a
> hint ?
>
> Thanks.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource()
2008-12-02 2:43 ` Sunil Mushran
@ 2008-12-02 16:49 ` Coly Li
2008-12-02 19:16 ` Sunil Mushran
0 siblings, 1 reply; 5+ messages in thread
From: Coly Li @ 2008-12-02 16:49 UTC (permalink / raw)
To: ocfs2-devel
Sunil Mushran Wrote:
> No. dlm->spinlock prevents it racing the dlm_thread. Secondly, lookup
> also does a get.
>
> What scenario are you envisioning?
IMHO, locking dlm->spinlock does not mean protecting to tmpres. Even line 727 gets false, how can we
make sure in line 749 res->state is not modified to DLM_LOCK_RES_DROPPING_REF by other process ?
I can not find the magic which make everything work in order, something important must be missed by me.
Thanks.
>
> Coly Li wrote:
>> Hi list,
>>
>> When I read code of dlm_get_lock_resource(), there is something not
>> clear to me.
>>
>> 719 lookup:
>> 720 spin_lock(&dlm->spinlock);
>> 721 tmpres = __dlm_lookup_lockres_full(dlm, lockid, namelen,
>> hash);
>> 722 if (tmpres) {
>> 723 int dropping_ref = 0;
>> 724
>> 725 spin_lock(&tmpres->spinlock);
>> 726 if (tmpres->owner == dlm->node_num) {
>> 727 BUG_ON(tmpres->state &
>> DLM_LOCK_RES_DROPPING_REF);
>> 728 dlm_lockres_grab_inflight_ref(dlm, tmpres);
>> 729 } else if (tmpres->state &
>> DLM_LOCK_RES_DROPPING_REF)
>> 730 dropping_ref = 1;
>> 731 spin_unlock(&tmpres->spinlock);
>> 732 spin_unlock(&dlm->spinlock);
>> 733
>> 734 /* wait until done messaging the master, drop our
>> ref to allow
>> 735 * the lockres to be purged, start over. */
>> 736 if (dropping_ref) {
>> 737 spin_lock(&tmpres->spinlock);
>> 738 __dlm_wait_on_lockres_flags(tmpres,
>> DLM_LOCK_RES_DROPPING_REF);
>> 739 spin_unlock(&tmpres->spinlock);
>> 740 dlm_lockres_put(tmpres);
>> 741 tmpres = NULL;
>> 742 goto lookup;
>> 743 }
>> 744
>> 745 mlog(0, "found in hash!\n");
>> 746 if (res)
>> 747 dlm_lockres_put(res);
>> 748 res = tmpres;
>> 749 goto leave;
>> 750 }
>>
>> If at line 721 tmpres found from hash, and its state is not
>> DLM_LOCK_RES_DROPPING_REF (dropping_ref
>> is 0), between line 733 and 748, is it possible to set tmpres->state
>> to DLM_LOCK_RES_DROPPING_REF ?
>>
>> I don't see any protection for this race, maybe there is something I
>> missed. Can anybody give me a
>> hint ?
--
Coly Li
SuSE PRC Labs
^ permalink raw reply [flat|nested] 5+ messages in thread* [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource()
2008-12-02 16:49 ` Coly Li
@ 2008-12-02 19:16 ` Sunil Mushran
2008-12-02 21:31 ` Sunil Mushran
0 siblings, 1 reply; 5+ messages in thread
From: Sunil Mushran @ 2008-12-02 19:16 UTC (permalink / raw)
To: ocfs2-devel
Read the purge_lockres code, the only place where we set this flag.
Coly Li wrote:
> Sunil Mushran Wrote:
>
>> No. dlm->spinlock prevents it racing the dlm_thread. Secondly, lookup
>> also does a get.
>>
>> What scenario are you envisioning?
>>
> IMHO, locking dlm->spinlock does not mean protecting to tmpres. Even line 727 gets false, how can we
> make sure in line 749 res->state is not modified to DLM_LOCK_RES_DROPPING_REF by other process ?
>
> I can not find the magic which make everything work in order, something important must be missed by me.
>
> Thanks.
>
>
>> Coly Li wrote:
>>
>>> Hi list,
>>>
>>> When I read code of dlm_get_lock_resource(), there is something not
>>> clear to me.
>>>
>>> 719 lookup:
>>> 720 spin_lock(&dlm->spinlock);
>>> 721 tmpres = __dlm_lookup_lockres_full(dlm, lockid, namelen,
>>> hash);
>>> 722 if (tmpres) {
>>> 723 int dropping_ref = 0;
>>> 724
>>> 725 spin_lock(&tmpres->spinlock);
>>> 726 if (tmpres->owner == dlm->node_num) {
>>> 727 BUG_ON(tmpres->state &
>>> DLM_LOCK_RES_DROPPING_REF);
>>> 728 dlm_lockres_grab_inflight_ref(dlm, tmpres);
>>> 729 } else if (tmpres->state &
>>> DLM_LOCK_RES_DROPPING_REF)
>>> 730 dropping_ref = 1;
>>> 731 spin_unlock(&tmpres->spinlock);
>>> 732 spin_unlock(&dlm->spinlock);
>>> 733
>>> 734 /* wait until done messaging the master, drop our
>>> ref to allow
>>> 735 * the lockres to be purged, start over. */
>>> 736 if (dropping_ref) {
>>> 737 spin_lock(&tmpres->spinlock);
>>> 738 __dlm_wait_on_lockres_flags(tmpres,
>>> DLM_LOCK_RES_DROPPING_REF);
>>> 739 spin_unlock(&tmpres->spinlock);
>>> 740 dlm_lockres_put(tmpres);
>>> 741 tmpres = NULL;
>>> 742 goto lookup;
>>> 743 }
>>> 744
>>> 745 mlog(0, "found in hash!\n");
>>> 746 if (res)
>>> 747 dlm_lockres_put(res);
>>> 748 res = tmpres;
>>> 749 goto leave;
>>> 750 }
>>>
>>> If at line 721 tmpres found from hash, and its state is not
>>> DLM_LOCK_RES_DROPPING_REF (dropping_ref
>>> is 0), between line 733 and 748, is it possible to set tmpres->state
>>> to DLM_LOCK_RES_DROPPING_REF ?
>>>
>>> I don't see any protection for this race, maybe there is something I
>>> missed. Can anybody give me a
>>> hint ?
>>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource()
2008-12-02 19:16 ` Sunil Mushran
@ 2008-12-02 21:31 ` Sunil Mushran
0 siblings, 0 replies; 5+ messages in thread
From: Sunil Mushran @ 2008-12-02 21:31 UTC (permalink / raw)
To: ocfs2-devel
On second thoughts, I should elaborate further. I'll do so sometime
later today.
Sunil Mushran wrote:
> Read the purge_lockres code, the only place where we set this flag.
>
> Coly Li wrote:
>
>> Sunil Mushran Wrote:
>>
>>
>>> No. dlm->spinlock prevents it racing the dlm_thread. Secondly, lookup
>>> also does a get.
>>>
>>> What scenario are you envisioning?
>>>
>>>
>> IMHO, locking dlm->spinlock does not mean protecting to tmpres. Even line 727 gets false, how can we
>> make sure in line 749 res->state is not modified to DLM_LOCK_RES_DROPPING_REF by other process ?
>>
>> I can not find the magic which make everything work in order, something important must be missed by me.
>>
>> Thanks.
>>
>>
>>
>>> Coly Li wrote:
>>>
>>>
>>>> Hi list,
>>>>
>>>> When I read code of dlm_get_lock_resource(), there is something not
>>>> clear to me.
>>>>
>>>> 719 lookup:
>>>> 720 spin_lock(&dlm->spinlock);
>>>> 721 tmpres = __dlm_lookup_lockres_full(dlm, lockid, namelen,
>>>> hash);
>>>> 722 if (tmpres) {
>>>> 723 int dropping_ref = 0;
>>>> 724
>>>> 725 spin_lock(&tmpres->spinlock);
>>>> 726 if (tmpres->owner == dlm->node_num) {
>>>> 727 BUG_ON(tmpres->state &
>>>> DLM_LOCK_RES_DROPPING_REF);
>>>> 728 dlm_lockres_grab_inflight_ref(dlm, tmpres);
>>>> 729 } else if (tmpres->state &
>>>> DLM_LOCK_RES_DROPPING_REF)
>>>> 730 dropping_ref = 1;
>>>> 731 spin_unlock(&tmpres->spinlock);
>>>> 732 spin_unlock(&dlm->spinlock);
>>>> 733
>>>> 734 /* wait until done messaging the master, drop our
>>>> ref to allow
>>>> 735 * the lockres to be purged, start over. */
>>>> 736 if (dropping_ref) {
>>>> 737 spin_lock(&tmpres->spinlock);
>>>> 738 __dlm_wait_on_lockres_flags(tmpres,
>>>> DLM_LOCK_RES_DROPPING_REF);
>>>> 739 spin_unlock(&tmpres->spinlock);
>>>> 740 dlm_lockres_put(tmpres);
>>>> 741 tmpres = NULL;
>>>> 742 goto lookup;
>>>> 743 }
>>>> 744
>>>> 745 mlog(0, "found in hash!\n");
>>>> 746 if (res)
>>>> 747 dlm_lockres_put(res);
>>>> 748 res = tmpres;
>>>> 749 goto leave;
>>>> 750 }
>>>>
>>>> If at line 721 tmpres found from hash, and its state is not
>>>> DLM_LOCK_RES_DROPPING_REF (dropping_ref
>>>> is 0), between line 733 and 748, is it possible to set tmpres->state
>>>> to DLM_LOCK_RES_DROPPING_REF ?
>>>>
>>>> I don't see any protection for this race, maybe there is something I
>>>> missed. Can anybody give me a
>>>> hint ?
>>>>
>>>>
>>
>>
>
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-12-02 21:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-11-28 13:03 [Ocfs2-devel] [nov 28] question of dlm_get_lock_resource() Coly Li
2008-12-02 2:43 ` Sunil Mushran
2008-12-02 16:49 ` Coly Li
2008-12-02 19:16 ` Sunil Mushran
2008-12-02 21:31 ` Sunil Mushran
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.