ocfs2: fix a potential 'ABBA' deadlock caused by 'l_lock' and 'dentry_attach_lock'
diff mbox

Message ID 5A2925FB.2060908@huawei.com
State New
Headers show

Commit Message

piaojun Dec. 7, 2017, 11:28 a.m. UTC
CPUA                                      CPUB

ocfs2_dentry_convert_worker
get 'l_lock'

                                    get 'dentry_attach_lock'

                                    interruptted by dio_end_io:
                                      dio_end_io
                                        dio_bio_end_aio
                                          dio_complete
                                            dio->end_io
                                              ocfs2_dio_end_io
                                                ocfs2_rw_unlock
                                                ...
                                                try to get 'l_lock'
                                                but CPUA has got it.

try to get 'dentry_attach_lock',
but CPUB has got 'dentry_attach_lock',
and would not release it.

so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
interruptted by softirq.

Signed-off-by: Jun Piao <piaojun@huawei.com>
Reviewed-by: Alex Chen <alex.chen@huawei.com>
---
 fs/ocfs2/dcache.c  | 14 ++++++++------
 fs/ocfs2/dlmglue.c | 14 +++++++-------
 fs/ocfs2/namei.c   |  5 +++--
 3 files changed, 18 insertions(+), 15 deletions(-)

Comments

Changwei Ge Dec. 7, 2017, 11:59 a.m. UTC | #1
Hi Jun,

On 2017/12/7 19:30, piaojun wrote:
>         CPUA                                      CPUB
> 
> ocfs2_dentry_convert_worker
> get 'l_lock'

This lock belongs to ocfs2_dentry_lock::ocfs2_lock_res::l_lock

> 
>                                      get 'dentry_attach_lock'
> 
>                                      interruptted by dio_end_io:
>                                        dio_end_io
>                                          dio_bio_end_aio
>                                            dio_complete
>                                              dio->end_io
>                                                ocfs2_dio_end_io
>                                                  ocfs2_rw_unlock
>                                                  ...
>                                                  try to get 'l_lock'

This lock belongs to ocfs2_lock_res::l_lock though.

So I think they are totally two different locks.
So making spin_lock to interruption safe is pointless.

Thanks,
Changwei

>                                                  but CPUA has got it.
> 
> try to get 'dentry_attach_lock',
> but CPUB has got 'dentry_attach_lock',
> and would not release it.
> 
> so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
> interruptted by softirq.
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> Reviewed-by: Alex Chen <alex.chen@huawei.com>
> ---
>   fs/ocfs2/dcache.c  | 14 ++++++++------
>   fs/ocfs2/dlmglue.c | 14 +++++++-------
>   fs/ocfs2/namei.c   |  5 +++--
>   3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
> index 2903730..6555fbf 100644
> --- a/fs/ocfs2/dcache.c
> +++ b/fs/ocfs2/dcache.c
> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>   	int ret;
>   	struct dentry *alias;
>   	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
> +	unsigned long flags;
> 
>   	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
>   				       (unsigned long long)parent_blkno, dl);
> @@ -309,10 +310,10 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>   	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);
> 
>   out_attach:
> -	spin_lock(&dentry_attach_lock);
> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>   	dentry->d_fsdata = dl;
>   	dl->dl_count++;
> -	spin_unlock(&dentry_attach_lock);
> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
> 
>   	/*
>   	 * This actually gets us our PRMODE level lock. From now on,
> @@ -333,9 +334,9 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>   	if (ret < 0 && !alias) {
>   		ocfs2_lock_res_free(&dl->dl_lockres);
>   		BUG_ON(dl->dl_count != 1);
> -		spin_lock(&dentry_attach_lock);
> +		spin_lock_irqsave(&dentry_attach_lock, flags);
>   		dentry->d_fsdata = NULL;
> -		spin_unlock(&dentry_attach_lock);
> +		spin_unlock_irqrestore(&dentry_attach_lock, flags);
>   		kfree(dl);
>   		iput(inode);
>   	}
> @@ -379,13 +380,14 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
>   			   struct ocfs2_dentry_lock *dl)
>   {
>   	int unlock = 0;
> +	unsigned long flags;
> 
>   	BUG_ON(dl->dl_count == 0);
> 
> -	spin_lock(&dentry_attach_lock);
> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>   	dl->dl_count--;
>   	unlock = !dl->dl_count;
> -	spin_unlock(&dentry_attach_lock);
> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
> 
>   	if (unlock)
>   		ocfs2_drop_dentry_lock(osb, dl);
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index 4689940..9bff3d2 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -3801,7 +3801,7 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>   	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
>   	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
>   	struct dentry *dentry;
> -	unsigned long flags;
> +	unsigned long flags, d_flags;
>   	int extra_ref = 0;
> 
>   	/*
> @@ -3831,13 +3831,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>   	 * flag.
>   	 */
>   	spin_lock_irqsave(&lockres->l_lock, flags);
> -	spin_lock(&dentry_attach_lock);
> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>   	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
>   	    && dl->dl_count) {
>   		dl->dl_count++;
>   		extra_ref = 1;
>   	}
> -	spin_unlock(&dentry_attach_lock);
> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>   	spin_unlock_irqrestore(&lockres->l_lock, flags);
> 
>   	mlog(0, "extra_ref = %d\n", extra_ref);
> @@ -3850,13 +3850,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>   	if (!extra_ref)
>   		return UNBLOCK_CONTINUE;
> 
> -	spin_lock(&dentry_attach_lock);
> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>   	while (1) {
>   		dentry = ocfs2_find_local_alias(dl->dl_inode,
>   						dl->dl_parent_blkno, 1);
>   		if (!dentry)
>   			break;
> -		spin_unlock(&dentry_attach_lock);
> +		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
> 
>   		if (S_ISDIR(dl->dl_inode->i_mode))
>   			shrink_dcache_parent(dentry);
> @@ -3874,9 +3874,9 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>   		d_delete(dentry);
>   		dput(dentry);
> 
> -		spin_lock(&dentry_attach_lock);
> +		spin_lock_irqsave(&dentry_attach_lock, d_flags);
>   	}
> -	spin_unlock(&dentry_attach_lock);
> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
> 
>   	/*
>   	 * If we are the last holder of this dentry lock, there is no
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 3b0a10d..a17454e 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -223,13 +223,14 @@ static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
>   		struct dentry *dentry, struct inode *inode)
>   {
>   	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
> +	unsigned long flags;
> 
>   	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
>   	ocfs2_lock_res_free(&dl->dl_lockres);
>   	BUG_ON(dl->dl_count != 1);
> -	spin_lock(&dentry_attach_lock);
> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>   	dentry->d_fsdata = NULL;
> -	spin_unlock(&dentry_attach_lock);
> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>   	kfree(dl);
>   	iput(inode);
>   }
>
piaojun Dec. 7, 2017, 12:36 p.m. UTC | #2
Hi Changwei,

On 2017/12/7 19:59, Changwei Ge wrote:
> Hi Jun,
> 
> On 2017/12/7 19:30, piaojun wrote:
>>         CPUA                                      CPUB
>>
>> ocfs2_dentry_convert_worker
>> get 'l_lock'
> 
> This lock belongs to ocfs2_dentry_lock::ocfs2_lock_res::l_lock
> 
>>
>>                                      get 'dentry_attach_lock'
>>
>>                                      interruptted by dio_end_io:
>>                                        dio_end_io
>>                                          dio_bio_end_aio
>>                                            dio_complete
>>                                              dio->end_io
>>                                                ocfs2_dio_end_io
>>                                                  ocfs2_rw_unlock
>>                                                  ...
>>                                                  try to get 'l_lock'
> 
> This lock belongs to ocfs2_lock_res::l_lock though.
> 
> So I think they are totally two different locks.
> So making spin_lock to interruption safe is pointless.
> 
> Thanks,
> Changwei
> 

For the same lock, we need use spin_lock_irqsave to ensure irq-safe as
you said. But the scenario described above is another kind of deadlock
caused by two different locks which stuck each other. To avoid this
'ABBA' deadlock we need follow the rule that 'spin_lock_irqsave' should
be called under 'spin_lock_irqsave'.

thanks,
Jun

>>                                                  but CPUA has got it.
>>
>> try to get 'dentry_attach_lock',
>> but CPUB has got 'dentry_attach_lock',
>> and would not release it.
>>
>> so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
>> interruptted by softirq.
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> Reviewed-by: Alex Chen <alex.chen@huawei.com>
>> ---
>>   fs/ocfs2/dcache.c  | 14 ++++++++------
>>   fs/ocfs2/dlmglue.c | 14 +++++++-------
>>   fs/ocfs2/namei.c   |  5 +++--
>>   3 files changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>> index 2903730..6555fbf 100644
>> --- a/fs/ocfs2/dcache.c
>> +++ b/fs/ocfs2/dcache.c
>> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>   	int ret;
>>   	struct dentry *alias;
>>   	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>> +	unsigned long flags;
>>
>>   	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
>>   				       (unsigned long long)parent_blkno, dl);
>> @@ -309,10 +310,10 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>   	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);
>>
>>   out_attach:
>> -	spin_lock(&dentry_attach_lock);
>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>   	dentry->d_fsdata = dl;
>>   	dl->dl_count++;
>> -	spin_unlock(&dentry_attach_lock);
>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>
>>   	/*
>>   	 * This actually gets us our PRMODE level lock. From now on,
>> @@ -333,9 +334,9 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>   	if (ret < 0 && !alias) {
>>   		ocfs2_lock_res_free(&dl->dl_lockres);
>>   		BUG_ON(dl->dl_count != 1);
>> -		spin_lock(&dentry_attach_lock);
>> +		spin_lock_irqsave(&dentry_attach_lock, flags);
>>   		dentry->d_fsdata = NULL;
>> -		spin_unlock(&dentry_attach_lock);
>> +		spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>   		kfree(dl);
>>   		iput(inode);
>>   	}
>> @@ -379,13 +380,14 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
>>   			   struct ocfs2_dentry_lock *dl)
>>   {
>>   	int unlock = 0;
>> +	unsigned long flags;
>>
>>   	BUG_ON(dl->dl_count == 0);
>>
>> -	spin_lock(&dentry_attach_lock);
>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>   	dl->dl_count--;
>>   	unlock = !dl->dl_count;
>> -	spin_unlock(&dentry_attach_lock);
>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>
>>   	if (unlock)
>>   		ocfs2_drop_dentry_lock(osb, dl);
>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>> index 4689940..9bff3d2 100644
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -3801,7 +3801,7 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>   	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
>>   	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
>>   	struct dentry *dentry;
>> -	unsigned long flags;
>> +	unsigned long flags, d_flags;
>>   	int extra_ref = 0;
>>
>>   	/*
>> @@ -3831,13 +3831,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>   	 * flag.
>>   	 */
>>   	spin_lock_irqsave(&lockres->l_lock, flags);
>> -	spin_lock(&dentry_attach_lock);
>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>   	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
>>   	    && dl->dl_count) {
>>   		dl->dl_count++;
>>   		extra_ref = 1;
>>   	}
>> -	spin_unlock(&dentry_attach_lock);
>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>   	spin_unlock_irqrestore(&lockres->l_lock, flags);
>>
>>   	mlog(0, "extra_ref = %d\n", extra_ref);
>> @@ -3850,13 +3850,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>   	if (!extra_ref)
>>   		return UNBLOCK_CONTINUE;
>>
>> -	spin_lock(&dentry_attach_lock);
>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>   	while (1) {
>>   		dentry = ocfs2_find_local_alias(dl->dl_inode,
>>   						dl->dl_parent_blkno, 1);
>>   		if (!dentry)
>>   			break;
>> -		spin_unlock(&dentry_attach_lock);
>> +		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>
>>   		if (S_ISDIR(dl->dl_inode->i_mode))
>>   			shrink_dcache_parent(dentry);
>> @@ -3874,9 +3874,9 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>   		d_delete(dentry);
>>   		dput(dentry);
>>
>> -		spin_lock(&dentry_attach_lock);
>> +		spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>   	}
>> -	spin_unlock(&dentry_attach_lock);
>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>
>>   	/*
>>   	 * If we are the last holder of this dentry lock, there is no
>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>> index 3b0a10d..a17454e 100644
>> --- a/fs/ocfs2/namei.c
>> +++ b/fs/ocfs2/namei.c
>> @@ -223,13 +223,14 @@ static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
>>   		struct dentry *dentry, struct inode *inode)
>>   {
>>   	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>> +	unsigned long flags;
>>
>>   	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
>>   	ocfs2_lock_res_free(&dl->dl_lockres);
>>   	BUG_ON(dl->dl_count != 1);
>> -	spin_lock(&dentry_attach_lock);
>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>   	dentry->d_fsdata = NULL;
>> -	spin_unlock(&dentry_attach_lock);
>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>   	kfree(dl);
>>   	iput(inode);
>>   }
>>
> 
> .
>
Changwei Ge Dec. 8, 2017, 9:09 a.m. UTC | #3
On 2017/12/7 20:37, piaojun wrote:
> Hi Changwei,
> 
> On 2017/12/7 19:59, Changwei Ge wrote:
>> Hi Jun,
>>
>> On 2017/12/7 19:30, piaojun wrote:
>>>          CPUA                                      CPUB
>>>
>>> ocfs2_dentry_convert_worker
>>> get 'l_lock'
>>
>> This lock belongs to ocfs2_dentry_lock::ocfs2_lock_res::l_lock
>>
>>>
>>>                                       get 'dentry_attach_lock'
>>>
>>>                                       interruptted by dio_end_io:
>>>                                         dio_end_io
>>>                                           dio_bio_end_aio
>>>                                             dio_complete
>>>                                               dio->end_io
>>>                                                 ocfs2_dio_end_io
>>>                                                   ocfs2_rw_unlock
>>>                                                   ...
>>>                                                   try to get 'l_lock'
>>
>> This lock belongs to ocfs2_lock_res::l_lock though.
>>
>> So I think they are totally two different locks.
>> So making spin_lock to interruption safe is pointless.
>>
>> Thanks,
>> Changwei
>>
> 
> For the same lock, we need use spin_lock_irqsave to ensure irq-safe as
> you said. But the scenario described above is another kind of deadlock
> caused by two different locks which stuck each other. To avoid this
> 'ABBA' deadlock we need follow the rule that 'spin_lock_irqsave' should
> be called under 'spin_lock_irqsave'.

Hi Jun,
I'm not sure if you understood my concern?
I agree with that we should avoid ABBA dead lock scenario.
But the dead lock scenario your sequence diagram is demonstrating 
doesn't even exist.

Because CPU A is holding lock _X1_ and waiting for _dentry_attach_lock_.
meanwhile CPU B is holding lock _dentry_attach_lock_ and waiting for _X2_.
No ABBA deadlock condition is met, CPU B will acquire lock _X2_ without 
being affected by CPU A.

In a nutshell, there are three locks(lock memory space) involved in your 
diagram rather than two.

Thanks,
Changwei

> 
> thanks,
> Jun
> 
>>>                                                   but CPUA has got it.
>>>
>>> try to get 'dentry_attach_lock',
>>> but CPUB has got 'dentry_attach_lock',
>>> and would not release it.
>>>
>>> so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
>>> interruptted by softirq.
>>>
>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>> Reviewed-by: Alex Chen <alex.chen@huawei.com>
>>> ---
>>>    fs/ocfs2/dcache.c  | 14 ++++++++------
>>>    fs/ocfs2/dlmglue.c | 14 +++++++-------
>>>    fs/ocfs2/namei.c   |  5 +++--
>>>    3 files changed, 18 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>>> index 2903730..6555fbf 100644
>>> --- a/fs/ocfs2/dcache.c
>>> +++ b/fs/ocfs2/dcache.c
>>> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>    	int ret;
>>>    	struct dentry *alias;
>>>    	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>> +	unsigned long flags;
>>>
>>>    	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
>>>    				       (unsigned long long)parent_blkno, dl);
>>> @@ -309,10 +310,10 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>    	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);
>>>
>>>    out_attach:
>>> -	spin_lock(&dentry_attach_lock);
>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>    	dentry->d_fsdata = dl;
>>>    	dl->dl_count++;
>>> -	spin_unlock(&dentry_attach_lock);
>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>
>>>    	/*
>>>    	 * This actually gets us our PRMODE level lock. From now on,
>>> @@ -333,9 +334,9 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>    	if (ret < 0 && !alias) {
>>>    		ocfs2_lock_res_free(&dl->dl_lockres);
>>>    		BUG_ON(dl->dl_count != 1);
>>> -		spin_lock(&dentry_attach_lock);
>>> +		spin_lock_irqsave(&dentry_attach_lock, flags);
>>>    		dentry->d_fsdata = NULL;
>>> -		spin_unlock(&dentry_attach_lock);
>>> +		spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>    		kfree(dl);
>>>    		iput(inode);
>>>    	}
>>> @@ -379,13 +380,14 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
>>>    			   struct ocfs2_dentry_lock *dl)
>>>    {
>>>    	int unlock = 0;
>>> +	unsigned long flags;
>>>
>>>    	BUG_ON(dl->dl_count == 0);
>>>
>>> -	spin_lock(&dentry_attach_lock);
>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>    	dl->dl_count--;
>>>    	unlock = !dl->dl_count;
>>> -	spin_unlock(&dentry_attach_lock);
>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>
>>>    	if (unlock)
>>>    		ocfs2_drop_dentry_lock(osb, dl);
>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>> index 4689940..9bff3d2 100644
>>> --- a/fs/ocfs2/dlmglue.c
>>> +++ b/fs/ocfs2/dlmglue.c
>>> @@ -3801,7 +3801,7 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>    	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
>>>    	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
>>>    	struct dentry *dentry;
>>> -	unsigned long flags;
>>> +	unsigned long flags, d_flags;
>>>    	int extra_ref = 0;
>>>
>>>    	/*
>>> @@ -3831,13 +3831,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>    	 * flag.
>>>    	 */
>>>    	spin_lock_irqsave(&lockres->l_lock, flags);
>>> -	spin_lock(&dentry_attach_lock);
>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>    	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
>>>    	    && dl->dl_count) {
>>>    		dl->dl_count++;
>>>    		extra_ref = 1;
>>>    	}
>>> -	spin_unlock(&dentry_attach_lock);
>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>    	spin_unlock_irqrestore(&lockres->l_lock, flags);
>>>
>>>    	mlog(0, "extra_ref = %d\n", extra_ref);
>>> @@ -3850,13 +3850,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>    	if (!extra_ref)
>>>    		return UNBLOCK_CONTINUE;
>>>
>>> -	spin_lock(&dentry_attach_lock);
>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>    	while (1) {
>>>    		dentry = ocfs2_find_local_alias(dl->dl_inode,
>>>    						dl->dl_parent_blkno, 1);
>>>    		if (!dentry)
>>>    			break;
>>> -		spin_unlock(&dentry_attach_lock);
>>> +		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>
>>>    		if (S_ISDIR(dl->dl_inode->i_mode))
>>>    			shrink_dcache_parent(dentry);
>>> @@ -3874,9 +3874,9 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>    		d_delete(dentry);
>>>    		dput(dentry);
>>>
>>> -		spin_lock(&dentry_attach_lock);
>>> +		spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>    	}
>>> -	spin_unlock(&dentry_attach_lock);
>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>
>>>    	/*
>>>    	 * If we are the last holder of this dentry lock, there is no
>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>> index 3b0a10d..a17454e 100644
>>> --- a/fs/ocfs2/namei.c
>>> +++ b/fs/ocfs2/namei.c
>>> @@ -223,13 +223,14 @@ static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
>>>    		struct dentry *dentry, struct inode *inode)
>>>    {
>>>    	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>> +	unsigned long flags;
>>>
>>>    	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
>>>    	ocfs2_lock_res_free(&dl->dl_lockres);
>>>    	BUG_ON(dl->dl_count != 1);
>>> -	spin_lock(&dentry_attach_lock);
>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>    	dentry->d_fsdata = NULL;
>>> -	spin_unlock(&dentry_attach_lock);
>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>    	kfree(dl);
>>>    	iput(inode);
>>>    }
>>>
>>
>> .
>>
>
piaojun Dec. 8, 2017, 10:16 a.m. UTC | #4
Hi Changwei,

On 2017/12/8 17:09, Changwei Ge wrote:
> On 2017/12/7 20:37, piaojun wrote:
>> Hi Changwei,
>>
>> On 2017/12/7 19:59, Changwei Ge wrote:
>>> Hi Jun,
>>>
>>> On 2017/12/7 19:30, piaojun wrote:
>>>>          CPUA                                      CPUB
>>>>
>>>> ocfs2_dentry_convert_worker
>>>> get 'l_lock'
>>>
>>> This lock belongs to ocfs2_dentry_lock::ocfs2_lock_res::l_lock
>>>
>>>>
>>>>                                       get 'dentry_attach_lock'
>>>>
>>>>                                       interruptted by dio_end_io:
>>>>                                         dio_end_io
>>>>                                           dio_bio_end_aio
>>>>                                             dio_complete
>>>>                                               dio->end_io
>>>>                                                 ocfs2_dio_end_io
>>>>                                                   ocfs2_rw_unlock
>>>>                                                   ...
>>>>                                                   try to get 'l_lock'
>>>
>>> This lock belongs to ocfs2_lock_res::l_lock though.
>>>
>>> So I think they are totally two different locks.
>>> So making spin_lock to interruption safe is pointless.
>>>
>>> Thanks,
>>> Changwei
>>>
>>
>> For the same lock, we need use spin_lock_irqsave to ensure irq-safe as
>> you said. But the scenario described above is another kind of deadlock
>> caused by two different locks which stuck each other. To avoid this
>> 'ABBA' deadlock we need follow the rule that 'spin_lock_irqsave' should
>> be called under 'spin_lock_irqsave'.
> 
> Hi Jun,
> I'm not sure if you understood my concern?
> I agree with that we should avoid ABBA dead lock scenario.
> But the dead lock scenario your sequence diagram is demonstrating 
> doesn't even exist.
> 
> Because CPU A is holding lock _X1_ and waiting for _dentry_attach_lock_.
> meanwhile CPU B is holding lock _dentry_attach_lock_ and waiting for _X2_.
> No ABBA deadlock condition is met, CPU B will acquire lock _X2_ without 
> being affected by CPU A.
> 
> In a nutshell, there are three locks(lock memory space) involved in your 
> diagram rather than two.
> 
> Thanks,
> Changwei
> 

Sorry for misunderstanding your point, and I realize no deadlock would
happen as these two 'l_lock' belong to different lockres. But I still
suggest merging this patch, because this would reduce the risk of
introducing new deadlock probably by programming.

thanks,
Jun

>>
>> thanks,
>> Jun
>>
>>>>                                                   but CPUA has got it.
>>>>
>>>> try to get 'dentry_attach_lock',
>>>> but CPUB has got 'dentry_attach_lock',
>>>> and would not release it.
>>>>
>>>> so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
>>>> interruptted by softirq.
>>>>
>>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>>> Reviewed-by: Alex Chen <alex.chen@huawei.com>
>>>> ---
>>>>    fs/ocfs2/dcache.c  | 14 ++++++++------
>>>>    fs/ocfs2/dlmglue.c | 14 +++++++-------
>>>>    fs/ocfs2/namei.c   |  5 +++--
>>>>    3 files changed, 18 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>>>> index 2903730..6555fbf 100644
>>>> --- a/fs/ocfs2/dcache.c
>>>> +++ b/fs/ocfs2/dcache.c
>>>> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>    	int ret;
>>>>    	struct dentry *alias;
>>>>    	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>>> +	unsigned long flags;
>>>>
>>>>    	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
>>>>    				       (unsigned long long)parent_blkno, dl);
>>>> @@ -309,10 +310,10 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>    	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);
>>>>
>>>>    out_attach:
>>>> -	spin_lock(&dentry_attach_lock);
>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>    	dentry->d_fsdata = dl;
>>>>    	dl->dl_count++;
>>>> -	spin_unlock(&dentry_attach_lock);
>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>
>>>>    	/*
>>>>    	 * This actually gets us our PRMODE level lock. From now on,
>>>> @@ -333,9 +334,9 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>    	if (ret < 0 && !alias) {
>>>>    		ocfs2_lock_res_free(&dl->dl_lockres);
>>>>    		BUG_ON(dl->dl_count != 1);
>>>> -		spin_lock(&dentry_attach_lock);
>>>> +		spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>    		dentry->d_fsdata = NULL;
>>>> -		spin_unlock(&dentry_attach_lock);
>>>> +		spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>    		kfree(dl);
>>>>    		iput(inode);
>>>>    	}
>>>> @@ -379,13 +380,14 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
>>>>    			   struct ocfs2_dentry_lock *dl)
>>>>    {
>>>>    	int unlock = 0;
>>>> +	unsigned long flags;
>>>>
>>>>    	BUG_ON(dl->dl_count == 0);
>>>>
>>>> -	spin_lock(&dentry_attach_lock);
>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>    	dl->dl_count--;
>>>>    	unlock = !dl->dl_count;
>>>> -	spin_unlock(&dentry_attach_lock);
>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>
>>>>    	if (unlock)
>>>>    		ocfs2_drop_dentry_lock(osb, dl);
>>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>>> index 4689940..9bff3d2 100644
>>>> --- a/fs/ocfs2/dlmglue.c
>>>> +++ b/fs/ocfs2/dlmglue.c
>>>> @@ -3801,7 +3801,7 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>    	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
>>>>    	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
>>>>    	struct dentry *dentry;
>>>> -	unsigned long flags;
>>>> +	unsigned long flags, d_flags;
>>>>    	int extra_ref = 0;
>>>>
>>>>    	/*
>>>> @@ -3831,13 +3831,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>    	 * flag.
>>>>    	 */
>>>>    	spin_lock_irqsave(&lockres->l_lock, flags);
>>>> -	spin_lock(&dentry_attach_lock);
>>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>    	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
>>>>    	    && dl->dl_count) {
>>>>    		dl->dl_count++;
>>>>    		extra_ref = 1;
>>>>    	}
>>>> -	spin_unlock(&dentry_attach_lock);
>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>    	spin_unlock_irqrestore(&lockres->l_lock, flags);
>>>>
>>>>    	mlog(0, "extra_ref = %d\n", extra_ref);
>>>> @@ -3850,13 +3850,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>    	if (!extra_ref)
>>>>    		return UNBLOCK_CONTINUE;
>>>>
>>>> -	spin_lock(&dentry_attach_lock);
>>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>    	while (1) {
>>>>    		dentry = ocfs2_find_local_alias(dl->dl_inode,
>>>>    						dl->dl_parent_blkno, 1);
>>>>    		if (!dentry)
>>>>    			break;
>>>> -		spin_unlock(&dentry_attach_lock);
>>>> +		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>
>>>>    		if (S_ISDIR(dl->dl_inode->i_mode))
>>>>    			shrink_dcache_parent(dentry);
>>>> @@ -3874,9 +3874,9 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>    		d_delete(dentry);
>>>>    		dput(dentry);
>>>>
>>>> -		spin_lock(&dentry_attach_lock);
>>>> +		spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>    	}
>>>> -	spin_unlock(&dentry_attach_lock);
>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>
>>>>    	/*
>>>>    	 * If we are the last holder of this dentry lock, there is no
>>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>>> index 3b0a10d..a17454e 100644
>>>> --- a/fs/ocfs2/namei.c
>>>> +++ b/fs/ocfs2/namei.c
>>>> @@ -223,13 +223,14 @@ static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
>>>>    		struct dentry *dentry, struct inode *inode)
>>>>    {
>>>>    	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>>> +	unsigned long flags;
>>>>
>>>>    	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
>>>>    	ocfs2_lock_res_free(&dl->dl_lockres);
>>>>    	BUG_ON(dl->dl_count != 1);
>>>> -	spin_lock(&dentry_attach_lock);
>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>    	dentry->d_fsdata = NULL;
>>>> -	spin_unlock(&dentry_attach_lock);
>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>    	kfree(dl);
>>>>    	iput(inode);
>>>>    }
>>>>
>>>
>>> .
>>>
>>
> 
> .
>
Changwei Ge Dec. 9, 2017, 3:44 a.m. UTC | #5
On 2017/12/8 18:17, piaojun wrote:
> Hi Changwei,
> 
> On 2017/12/8 17:09, Changwei Ge wrote:
>> On 2017/12/7 20:37, piaojun wrote:
>>> Hi Changwei,
>>>
>>> On 2017/12/7 19:59, Changwei Ge wrote:
>>>> Hi Jun,
>>>>
>>>> On 2017/12/7 19:30, piaojun wrote:
>>>>>           CPUA                                      CPUB
>>>>>
>>>>> ocfs2_dentry_convert_worker
>>>>> get 'l_lock'
>>>>
>>>> This lock belongs to ocfs2_dentry_lock::ocfs2_lock_res::l_lock
>>>>
>>>>>
>>>>>                                        get 'dentry_attach_lock'
>>>>>
>>>>>                                        interruptted by dio_end_io:
>>>>>                                          dio_end_io
>>>>>                                            dio_bio_end_aio
>>>>>                                              dio_complete
>>>>>                                                dio->end_io
>>>>>                                                  ocfs2_dio_end_io
>>>>>                                                    ocfs2_rw_unlock
>>>>>                                                    ...
>>>>>                                                    try to get 'l_lock'
>>>>
>>>> This lock belongs to ocfs2_lock_res::l_lock though.
>>>>
>>>> So I think they are totally two different locks.
>>>> So making spin_lock to interruption safe is pointless.
>>>>
>>>> Thanks,
>>>> Changwei
>>>>
>>>
>>> For the same lock, we need use spin_lock_irqsave to ensure irq-safe as
>>> you said. But the scenario described above is another kind of deadlock
>>> caused by two different locks which stuck each other. To avoid this
>>> 'ABBA' deadlock we need follow the rule that 'spin_lock_irqsave' should
>>> be called under 'spin_lock_irqsave'.
>>
>> Hi Jun,
>> I'm not sure if you understood my concern?
>> I agree with that we should avoid ABBA dead lock scenario.
>> But the dead lock scenario your sequence diagram is demonstrating
>> doesn't even exist.
>>
>> Because CPU A is holding lock _X1_ and waiting for _dentry_attach_lock_.
>> meanwhile CPU B is holding lock _dentry_attach_lock_ and waiting for _X2_.
>> No ABBA deadlock condition is met, CPU B will acquire lock _X2_ without
>> being affected by CPU A.
>>
>> In a nutshell, there are three locks(lock memory space) involved in your
>> diagram rather than two.
>>
>> Thanks,
>> Changwei
>>
> 
> Sorry for misunderstanding your point, and I realize no deadlock would
> happen as these two 'l_lock' belong to different lockres. But I still
> suggest merging this patch, because this would reduce the risk of
> introducing new deadlock probably by programming.

Hi Jun,

Um, I don't think so. Moreover, unnecessarily disabling local CPU 
interrupt will introduce overhead like memory copying and also delay 
interrupt handling, which I don't think is a good idea.

Thanks,
Changwei

> 
> thanks,
> Jun
> 
>>>
>>> thanks,
>>> Jun
>>>
>>>>>                                                    but CPUA has got it.
>>>>>
>>>>> try to get 'dentry_attach_lock',
>>>>> but CPUB has got 'dentry_attach_lock',
>>>>> and would not release it.
>>>>>
>>>>> so we need use spin_lock_irqsave for 'dentry_attach_lock' to prevent
>>>>> interruptted by softirq.
>>>>>
>>>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>>>> Reviewed-by: Alex Chen <alex.chen@huawei.com>
>>>>> ---
>>>>>     fs/ocfs2/dcache.c  | 14 ++++++++------
>>>>>     fs/ocfs2/dlmglue.c | 14 +++++++-------
>>>>>     fs/ocfs2/namei.c   |  5 +++--
>>>>>     3 files changed, 18 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
>>>>> index 2903730..6555fbf 100644
>>>>> --- a/fs/ocfs2/dcache.c
>>>>> +++ b/fs/ocfs2/dcache.c
>>>>> @@ -230,6 +230,7 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>>     	int ret;
>>>>>     	struct dentry *alias;
>>>>>     	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>>>> +	unsigned long flags;
>>>>>
>>>>>     	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
>>>>>     				       (unsigned long long)parent_blkno, dl);
>>>>> @@ -309,10 +310,10 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>>     	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);
>>>>>
>>>>>     out_attach:
>>>>> -	spin_lock(&dentry_attach_lock);
>>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>>     	dentry->d_fsdata = dl;
>>>>>     	dl->dl_count++;
>>>>> -	spin_unlock(&dentry_attach_lock);
>>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>>
>>>>>     	/*
>>>>>     	 * This actually gets us our PRMODE level lock. From now on,
>>>>> @@ -333,9 +334,9 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
>>>>>     	if (ret < 0 && !alias) {
>>>>>     		ocfs2_lock_res_free(&dl->dl_lockres);
>>>>>     		BUG_ON(dl->dl_count != 1);
>>>>> -		spin_lock(&dentry_attach_lock);
>>>>> +		spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>>     		dentry->d_fsdata = NULL;
>>>>> -		spin_unlock(&dentry_attach_lock);
>>>>> +		spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>>     		kfree(dl);
>>>>>     		iput(inode);
>>>>>     	}
>>>>> @@ -379,13 +380,14 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
>>>>>     			   struct ocfs2_dentry_lock *dl)
>>>>>     {
>>>>>     	int unlock = 0;
>>>>> +	unsigned long flags;
>>>>>
>>>>>     	BUG_ON(dl->dl_count == 0);
>>>>>
>>>>> -	spin_lock(&dentry_attach_lock);
>>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>>     	dl->dl_count--;
>>>>>     	unlock = !dl->dl_count;
>>>>> -	spin_unlock(&dentry_attach_lock);
>>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>>
>>>>>     	if (unlock)
>>>>>     		ocfs2_drop_dentry_lock(osb, dl);
>>>>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>>>>> index 4689940..9bff3d2 100644
>>>>> --- a/fs/ocfs2/dlmglue.c
>>>>> +++ b/fs/ocfs2/dlmglue.c
>>>>> @@ -3801,7 +3801,7 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>>     	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
>>>>>     	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
>>>>>     	struct dentry *dentry;
>>>>> -	unsigned long flags;
>>>>> +	unsigned long flags, d_flags;
>>>>>     	int extra_ref = 0;
>>>>>
>>>>>     	/*
>>>>> @@ -3831,13 +3831,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>>     	 * flag.
>>>>>     	 */
>>>>>     	spin_lock_irqsave(&lockres->l_lock, flags);
>>>>> -	spin_lock(&dentry_attach_lock);
>>>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>>     	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
>>>>>     	    && dl->dl_count) {
>>>>>     		dl->dl_count++;
>>>>>     		extra_ref = 1;
>>>>>     	}
>>>>> -	spin_unlock(&dentry_attach_lock);
>>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>>     	spin_unlock_irqrestore(&lockres->l_lock, flags);
>>>>>
>>>>>     	mlog(0, "extra_ref = %d\n", extra_ref);
>>>>> @@ -3850,13 +3850,13 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>>     	if (!extra_ref)
>>>>>     		return UNBLOCK_CONTINUE;
>>>>>
>>>>> -	spin_lock(&dentry_attach_lock);
>>>>> +	spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>>     	while (1) {
>>>>>     		dentry = ocfs2_find_local_alias(dl->dl_inode,
>>>>>     						dl->dl_parent_blkno, 1);
>>>>>     		if (!dentry)
>>>>>     			break;
>>>>> -		spin_unlock(&dentry_attach_lock);
>>>>> +		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>>
>>>>>     		if (S_ISDIR(dl->dl_inode->i_mode))
>>>>>     			shrink_dcache_parent(dentry);
>>>>> @@ -3874,9 +3874,9 @@ static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
>>>>>     		d_delete(dentry);
>>>>>     		dput(dentry);
>>>>>
>>>>> -		spin_lock(&dentry_attach_lock);
>>>>> +		spin_lock_irqsave(&dentry_attach_lock, d_flags);
>>>>>     	}
>>>>> -	spin_unlock(&dentry_attach_lock);
>>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
>>>>>
>>>>>     	/*
>>>>>     	 * If we are the last holder of this dentry lock, there is no
>>>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>>>> index 3b0a10d..a17454e 100644
>>>>> --- a/fs/ocfs2/namei.c
>>>>> +++ b/fs/ocfs2/namei.c
>>>>> @@ -223,13 +223,14 @@ static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
>>>>>     		struct dentry *dentry, struct inode *inode)
>>>>>     {
>>>>>     	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
>>>>> +	unsigned long flags;
>>>>>
>>>>>     	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
>>>>>     	ocfs2_lock_res_free(&dl->dl_lockres);
>>>>>     	BUG_ON(dl->dl_count != 1);
>>>>> -	spin_lock(&dentry_attach_lock);
>>>>> +	spin_lock_irqsave(&dentry_attach_lock, flags);
>>>>>     	dentry->d_fsdata = NULL;
>>>>> -	spin_unlock(&dentry_attach_lock);
>>>>> +	spin_unlock_irqrestore(&dentry_attach_lock, flags);
>>>>>     	kfree(dl);
>>>>>     	iput(inode);
>>>>>     }
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>

Patch
diff mbox

diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 2903730..6555fbf 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -230,6 +230,7 @@  int ocfs2_dentry_attach_lock(struct dentry *dentry,
 	int ret;
 	struct dentry *alias;
 	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
+	unsigned long flags;

 	trace_ocfs2_dentry_attach_lock(dentry->d_name.len, dentry->d_name.name,
 				       (unsigned long long)parent_blkno, dl);
@@ -309,10 +310,10 @@  int ocfs2_dentry_attach_lock(struct dentry *dentry,
 	ocfs2_dentry_lock_res_init(dl, parent_blkno, inode);

 out_attach:
-	spin_lock(&dentry_attach_lock);
+	spin_lock_irqsave(&dentry_attach_lock, flags);
 	dentry->d_fsdata = dl;
 	dl->dl_count++;
-	spin_unlock(&dentry_attach_lock);
+	spin_unlock_irqrestore(&dentry_attach_lock, flags);

 	/*
 	 * This actually gets us our PRMODE level lock. From now on,
@@ -333,9 +334,9 @@  int ocfs2_dentry_attach_lock(struct dentry *dentry,
 	if (ret < 0 && !alias) {
 		ocfs2_lock_res_free(&dl->dl_lockres);
 		BUG_ON(dl->dl_count != 1);
-		spin_lock(&dentry_attach_lock);
+		spin_lock_irqsave(&dentry_attach_lock, flags);
 		dentry->d_fsdata = NULL;
-		spin_unlock(&dentry_attach_lock);
+		spin_unlock_irqrestore(&dentry_attach_lock, flags);
 		kfree(dl);
 		iput(inode);
 	}
@@ -379,13 +380,14 @@  void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
 			   struct ocfs2_dentry_lock *dl)
 {
 	int unlock = 0;
+	unsigned long flags;

 	BUG_ON(dl->dl_count == 0);

-	spin_lock(&dentry_attach_lock);
+	spin_lock_irqsave(&dentry_attach_lock, flags);
 	dl->dl_count--;
 	unlock = !dl->dl_count;
-	spin_unlock(&dentry_attach_lock);
+	spin_unlock_irqrestore(&dentry_attach_lock, flags);

 	if (unlock)
 		ocfs2_drop_dentry_lock(osb, dl);
diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 4689940..9bff3d2 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -3801,7 +3801,7 @@  static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
 	struct ocfs2_dentry_lock *dl = ocfs2_lock_res_dl(lockres);
 	struct ocfs2_inode_info *oi = OCFS2_I(dl->dl_inode);
 	struct dentry *dentry;
-	unsigned long flags;
+	unsigned long flags, d_flags;
 	int extra_ref = 0;

 	/*
@@ -3831,13 +3831,13 @@  static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
 	 * flag.
 	 */
 	spin_lock_irqsave(&lockres->l_lock, flags);
-	spin_lock(&dentry_attach_lock);
+	spin_lock_irqsave(&dentry_attach_lock, d_flags);
 	if (!(lockres->l_flags & OCFS2_LOCK_FREEING)
 	    && dl->dl_count) {
 		dl->dl_count++;
 		extra_ref = 1;
 	}
-	spin_unlock(&dentry_attach_lock);
+	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);
 	spin_unlock_irqrestore(&lockres->l_lock, flags);

 	mlog(0, "extra_ref = %d\n", extra_ref);
@@ -3850,13 +3850,13 @@  static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
 	if (!extra_ref)
 		return UNBLOCK_CONTINUE;

-	spin_lock(&dentry_attach_lock);
+	spin_lock_irqsave(&dentry_attach_lock, d_flags);
 	while (1) {
 		dentry = ocfs2_find_local_alias(dl->dl_inode,
 						dl->dl_parent_blkno, 1);
 		if (!dentry)
 			break;
-		spin_unlock(&dentry_attach_lock);
+		spin_unlock_irqrestore(&dentry_attach_lock, d_flags);

 		if (S_ISDIR(dl->dl_inode->i_mode))
 			shrink_dcache_parent(dentry);
@@ -3874,9 +3874,9 @@  static int ocfs2_dentry_convert_worker(struct ocfs2_lock_res *lockres,
 		d_delete(dentry);
 		dput(dentry);

-		spin_lock(&dentry_attach_lock);
+		spin_lock_irqsave(&dentry_attach_lock, d_flags);
 	}
-	spin_unlock(&dentry_attach_lock);
+	spin_unlock_irqrestore(&dentry_attach_lock, d_flags);

 	/*
 	 * If we are the last holder of this dentry lock, there is no
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index 3b0a10d..a17454e 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -223,13 +223,14 @@  static void ocfs2_cleanup_add_entry_failure(struct ocfs2_super *osb,
 		struct dentry *dentry, struct inode *inode)
 {
 	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
+	unsigned long flags;

 	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
 	ocfs2_lock_res_free(&dl->dl_lockres);
 	BUG_ON(dl->dl_count != 1);
-	spin_lock(&dentry_attach_lock);
+	spin_lock_irqsave(&dentry_attach_lock, flags);
 	dentry->d_fsdata = NULL;
-	spin_unlock(&dentry_attach_lock);
+	spin_unlock_irqrestore(&dentry_attach_lock, flags);
 	kfree(dl);
 	iput(inode);
 }