Re: [PATCH] ocfs2: kill osb->system_file_mutex lock

[Tetsuo Handa]

On 2026/05/16 21:27, Heming Zhao wrote:
>>>> In my opinion, the problem with the current code is that the scope of
>>>> mutex_lock(&osb->system_file_mutex) is too broad. This mutex only needs to be
>>>> held prior to calling _ocfs2_get_system_file_inode(). I previously highlighted
>>>> this point in my initial review comment on the patch.
>>
>> If you meant
>>
>>  struct inode *ocfs2_get_system_file_inode(struct ocfs2_super *osb,
>>                                           int type,
>>                                           u32 slot)
>>  {
>>         struct inode *inode = NULL;
>>         struct inode **arr = NULL;
>>
>>         /* avoid the lookup if cached in local system file array */
>>         if (is_global_system_inode(type)) {
>>                 arr = &(osb->global_system_inodes[type]);
>>         } else
>>                 arr = get_local_system_inode(osb, type, slot);
>>
>> -       mutex_lock(&osb->system_file_mutex);
>>         if (arr && ((inode = *arr) != NULL)) {
>>                 /* get a ref in addition to the array ref */
>>                 inode = igrab(inode);
>>                 mutex_unlock(&osb->system_file_mutex);
>>                 BUG_ON(!inode);
>>
>>                 return inode;
>>         }
>>
>> +       mutex_lock(&osb->system_file_mutex);
>> +
>>         /* this gets one ref thru iget */
>>         inode = _ocfs2_get_system_file_inode(osb, type, slot);
>>
>>         /* add one more if putting into array for first time */
>>         if (arr && inode) {
>>                 *arr = igrab(inode);
>>                 BUG_ON(!*arr);
>>         }
>>         mutex_unlock(&osb->system_file_mutex);
>>         return inode;
>>  }
>>
> 
> Yes, this is what I expected.
> 
>> , that brings us back to before commit 43b10a20372d, for two threads can hit
>> the "*arr = igrab(inode)" line.
> 
> I don't think this bring us back, because igrab() uses a spinlock to protect the
> refcount.

You are missing the point. What is wrong with above change is that two threads can
sequentially enter into the "if (arr && inode) { }" block, for we are not checking
whether the slot is already NULL or not. The cmpxchg() is the magic that guarantees
that only one thread can enter into that block.

>>
>> Even if you also change the "if (arr && inode) {" check in order to make sure that
>> the second-reached thread won't again hit the "*arr = igrab(inode)" line when the
>> first-reached thread already hit the "*arr = igrab(inode)" line, the second-reached
>> thread is fully blocked by the first-reached thread. Not serializing two threads
>> allows these threads to return as quick as serializing these threads.
> 
> with the mutex protection removed, and relying on igrab()'s internal spinlock,
> the code logic is already correct for concurrency. 
> 
> there are two kinds of locks involved in this discussion: 
> - inode->i_lock: protects the inode's refcount (used by igrab())
> - osb->system_file_mutex: protects the array slot.

You are missing the point. The lock used inside igrab() is irrelevant.
The cmpxchg() is the lock that protects the array slot (in other words,
serializes two threads) without using the system_file_mutex lock.