ocfs2: move the mlog from the spinlock to outside
diff mbox series

Message ID 1569813459-8601-1-git-send-email-sunny.s.zhang@oracle.com
State New
Headers show
Series
  • ocfs2: move the mlog from the spinlock to outside
Related show

Commit Message

sunnyZhang Sept. 30, 2019, 3:17 a.m. UTC
There is a potential task of deadlock. Because the mask
is 0, the deadlock does not occur now. But There is a
potential task. If someone change the mask of mlog, but
forget to modify the order of the mlog and spin_unlock,
There will be a potential deadlock.So I move the mlog
from the spinlock to outsize.

Signed-off-by: Shuning Zhang <sunny.s.zhang@oracle.com>
---
 fs/ocfs2/dlmglue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Morton Oct. 7, 2019, 10:11 p.m. UTC | #1
On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote:

> There is a potential task of deadlock. Because the mask
> is 0, the deadlock does not occur now. But There is a
> potential task. If someone change the mask of mlog, but
> forget to modify the order of the mlog and spin_unlock,
> There will be a potential deadlock.So I move the mlog
> from the spinlock to outsize.
> 
> ...
>
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode,
>  
>  	spin_lock(&oi->ip_lock);
>  	if (oi->ip_flags & OCFS2_INODE_DELETED) {
> +		spin_unlock(&oi->ip_lock);
>  		mlog(0, "Orphaned inode %llu was deleted while we "
>  		     "were waiting on a lock. ip_flags = 0x%x\n",
>  		     (unsigned long long)oi->ip_blkno, oi->ip_flags);
> -		spin_unlock(&oi->ip_lock);
>  		status = -ENOENT;
>  		goto bail;
>  	}

The patch is obviously OK but I don't see any deadlock.  mlog() doesn't
take any locks?
Joseph Qi Oct. 8, 2019, 1:14 a.m. UTC | #2
On 19/10/8 06:11, Andrew Morton wrote:
> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote:
> 
>> There is a potential task of deadlock. Because the mask
>> is 0, the deadlock does not occur now. But There is a
>> potential task. If someone change the mask of mlog, but
>> forget to modify the order of the mlog and spin_unlock,
>> There will be a potential deadlock.So I move the mlog
>> from the spinlock to outsize.
>>
>> ...
>>
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode,
>>  
>>  	spin_lock(&oi->ip_lock);
>>  	if (oi->ip_flags & OCFS2_INODE_DELETED) {
>> +		spin_unlock(&oi->ip_lock);
>>  		mlog(0, "Orphaned inode %llu was deleted while we "
>>  		     "were waiting on a lock. ip_flags = 0x%x\n",
>>  		     (unsigned long long)oi->ip_blkno, oi->ip_flags);
>> -		spin_unlock(&oi->ip_lock);
>>  		status = -ENOENT;
>>  		goto bail;
>>  	}
> 
> The patch is obviously OK but I don't see any deadlock.  mlog() doesn't
> take any locks?
> 
I guess Shuning refers the calling of printk with spin lock.

> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
sunnyZhang Oct. 8, 2019, 5 a.m. UTC | #3
在 2019/10/8 上午9:14, Joseph Qi 写道:
>
> On 19/10/8 06:11, Andrew Morton wrote:
>> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote:
>>
>>> There is a potential task of deadlock. Because the mask
>>> is 0, the deadlock does not occur now. But There is a
>>> potential task. If someone change the mask of mlog, but
>>> forget to modify the order of the mlog and spin_unlock,
>>> There will be a potential deadlock.So I move the mlog
>>> from the spinlock to outsize.
>>>
>>> ...
>>>
>>> --- a/fs/ocfs2/dlmglue.c
>>> +++ b/fs/ocfs2/dlmglue.c
>>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode,
>>>   
>>>   	spin_lock(&oi->ip_lock);
>>>   	if (oi->ip_flags & OCFS2_INODE_DELETED) {
>>> +		spin_unlock(&oi->ip_lock);
>>>   		mlog(0, "Orphaned inode %llu was deleted while we "
>>>   		     "were waiting on a lock. ip_flags = 0x%x\n",
>>>   		     (unsigned long long)oi->ip_blkno, oi->ip_flags);
>>> -		spin_unlock(&oi->ip_lock);
>>>   		status = -ENOENT;
>>>   		goto bail;
>>>   	}
>> The patch is obviously OK but I don't see any deadlock.  mlog() doesn't
>> take any locks?
>>
> I guess Shuning refers the calling of printk with spin lock.

Yes, It is the calling of printk.

Sorry for the description is not clear enough. :)

>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
Joseph Qi Oct. 8, 2019, 9:51 a.m. UTC | #4
On 19/10/8 13:00, sunnyZhang wrote:
> 
> 在 2019/10/8 上午9:14, Joseph Qi 写道:
>>
>> On 19/10/8 06:11, Andrew Morton wrote:
>>> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote:
>>>
>>>> There is a potential task of deadlock. Because the mask
>>>> is 0, the deadlock does not occur now. But There is a
>>>> potential task. If someone change the mask of mlog, but
>>>> forget to modify the order of the mlog and spin_unlock,
>>>> There will be a potential deadlock.So I move the mlog
>>>> from the spinlock to outsize.
>>>>
>>>> ...
>>>>
>>>> --- a/fs/ocfs2/dlmglue.c
>>>> +++ b/fs/ocfs2/dlmglue.c
>>>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode,
>>>>         spin_lock(&oi->ip_lock);
>>>>       if (oi->ip_flags & OCFS2_INODE_DELETED) {
>>>> +        spin_unlock(&oi->ip_lock);
>>>>           mlog(0, "Orphaned inode %llu was deleted while we "
>>>>                "were waiting on a lock. ip_flags = 0x%x\n",
>>>>                (unsigned long long)oi->ip_blkno, oi->ip_flags);
>>>> -        spin_unlock(&oi->ip_lock);
>>>>           status = -ENOENT;
>>>>           goto bail;
>>>>       }
>>> The patch is obviously OK but I don't see any deadlock.  mlog() doesn't
>>> take any locks?
>>>
>> I guess Shuning refers the calling of printk with spin lock.
> 
> Yes, It is the calling of printk.
> 
> Sorry for the description is not clear enough. :)
> 
IIUC, we can call printk with spinlock, no deadlock will happen.
So I have the same question, where is the "potential deadlock"?

Thanks,
Joseph
sunnyZhang Oct. 9, 2019, 7:39 a.m. UTC | #5
在 2019/10/8 下午5:51, Joseph Qi 写道:
>
> On 19/10/8 13:00, sunnyZhang wrote:
>> 在 2019/10/8 上午9:14, Joseph Qi 写道:
>>> On 19/10/8 06:11, Andrew Morton wrote:
>>>> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote:
>>>>
>>>>> There is a potential task of deadlock. Because the mask
>>>>> is 0, the deadlock does not occur now. But There is a
>>>>> potential task. If someone change the mask of mlog, but
>>>>> forget to modify the order of the mlog and spin_unlock,
>>>>> There will be a potential deadlock.So I move the mlog
>>>>> from the spinlock to outsize.
>>>>>
>>>>> ...
>>>>>
>>>>> --- a/fs/ocfs2/dlmglue.c
>>>>> +++ b/fs/ocfs2/dlmglue.c
>>>>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode,
>>>>>          spin_lock(&oi->ip_lock);
>>>>>        if (oi->ip_flags & OCFS2_INODE_DELETED) {
>>>>> +        spin_unlock(&oi->ip_lock);
>>>>>            mlog(0, "Orphaned inode %llu was deleted while we "
>>>>>                 "were waiting on a lock. ip_flags = 0x%x\n",
>>>>>                 (unsigned long long)oi->ip_blkno, oi->ip_flags);
>>>>> -        spin_unlock(&oi->ip_lock);
>>>>>            status = -ENOENT;
>>>>>            goto bail;
>>>>>        }
>>>> The patch is obviously OK but I don't see any deadlock.  mlog() doesn't
>>>> take any locks?
>>>>
>>> I guess Shuning refers the calling of printk with spin lock.
>> Yes, It is the calling of printk.
>>
>> Sorry for the description is not clear enough. :)
>>
> IIUC, we can call printk with spinlock, no deadlock will happen.
> So I have the same question, where is the "potential deadlock"?

I apology for my stupidity.

I have always had a wrong memory, I think printk() can sleep.

@Andrew Please can you help me to withdraw this patch?


Thanks

> Thanks,
> Joseph
>

Patch
diff mbox series

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 1420723..9960e61 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -2315,10 +2315,10 @@  static int ocfs2_inode_lock_update(struct inode *inode,
 
 	spin_lock(&oi->ip_lock);
 	if (oi->ip_flags & OCFS2_INODE_DELETED) {
+		spin_unlock(&oi->ip_lock);
 		mlog(0, "Orphaned inode %llu was deleted while we "
 		     "were waiting on a lock. ip_flags = 0x%x\n",
 		     (unsigned long long)oi->ip_blkno, oi->ip_flags);
-		spin_unlock(&oi->ip_lock);
 		status = -ENOENT;
 		goto bail;
 	}